reply attributes not always sent (radius_paircreate() problem seen in rlm_ippool)

[John Dennis]

-- 
John
-------------- next part --------------
This analysis and debugging was done on the 3.0.1 version. There seems
to be a serious bug with reply attribute pairs not being sent to a client.

rlm_ippool does not send the the Framed-IP-Address nor the
Framed-IP-Netmask attributes to the client despite seeing the
following output during server debug:

Sending Access-Accept of id 93 from 127.0.0.1 port 1812 to 127.0.0.1 port 42598
	Framed-IP-Address = 192.168.1.3
	Framed-IP-Netmask = 255.255.255.0

The above message is actually a lie. rad_send() emits the message via
debug_pair() because it iterates over the packet->vps valuepair
list. Unfortunately rad_encode() never put the contents of packet->vps
valuepair list into the outgoing data buffer. Why?

It's because the length returned by rad_vp2attr() on line 1842 is
zero. A few lines below fr_strerror_printf() is used to emit "WARNING:
Skipping zero-length attribute" but that message is never actually
written out :-( Why?

Why is vp->length zero? It's because it's never initialized (other
than talloc_zero in pairalloc()). However the dictionary attribute
which is set by radius_paircreate() -> paircreate() ->
dict_attrbyvalue() is correct. The vp->da->length is correctly set to 4
because it's an IP4 addr. But the vp->da->length is never copied into
vp->length. 

Thus rad_encode() never updates the output buffer because the
vp->length is zero and never actually emits the warning message it
generated concerning the error and it lies about the attributes being
sent in the debug output :-(

But we know sending the Framed-IP-Address attribute in other contexts
works. Why? What's different? It works when the attribute is parsed
(i.e. from a file), in the case the call chain is userparse() ->
pairmake() -> pairparsevalue() and pairparsevalue() sees the
vp->da->type is PW_TYPE_IPADDR and sets the vp->length to 4.

But when mod_post_auth() in rlm_ippool.c calls radius_paircreate()
nothing in the call chain sets the vp->length value, it remains at
zero. The message stating which attriubtes are being emitted comes
from debug_pair() which looks at the vp->da fields, but rad_encode()
call chain only looks at the vp->length field (zero).

I don't understand the responsibility for who does what in each of the
many functions which deal with pair handling to understand who is
dropping the ball and not updating the vp->length. But I do know that
if I break in the debugger and manually set vp->length to 4 in
mod_post_auth() after radius_paircreate() is called the attriubte is
actually transmitted and received by the client.

A quick look through the code suggests there are very few places after
calling radius_paircreate where the vp->length is explicitly set, it
seems to assume the radius_paircreate code will do it.

I don't know where else this shows up but I'm concerned it could be a
serious bug, what other places where radius_paircreate is invoked will
have their attributes dropped?