Date: Sat, 31 Aug 1996 15:12:27 -0700 From: Pius Fischer <pius@iago.ienet.com> To: scrain@shore.net Cc: ascend-users@bungi.com, freebsd-isp@FreeBSD.ORG Subject: Re: (ASCEND) 2 bugs in Ascend's RADIUS server Message-ID: <199608312212.PAA12635@iago.ienet.com>
next in thread | raw e-mail | index | archive | help
On Sat, 31 Aug 1996, scrain@shore.net wrote:
>> VALUE_PAIR *
>> cut_attribute(list, attributeId, attribute)
>> VALUE_PAIR *list;
>> int attributeId;
>> VALUE_PAIR **attribute;
>> {
>> VALUE_PAIR *head = (VALUE_PAIR *)0;
>>
>> *attribute = (VALUE_PAIR *)0;
>> if( !list ) {
>> head = list;
>> }
>> else if( list->attribute == attributeId ) {
>> *attribute = list;
>> head = list->next;
>Insert:
> list->next=(VALUE_PAIR *)NULL;
>> }
>> else {
>> /* ... */
>> }
>> return head;
>> }
>
>IMHO, the bug is actually in cut_attribute. When the found item is deep
>in the list it returns a list without the item in it and the item. When
>the item is first in the list it doesn't properly NULL out the cut
>attribute, so pairfree frees the whole list instead of just the one cut
>out item.
Yes, I agree, a fix to cut_attribute strikes closer to the root of the
problem, but you're still left with at least the problem of the two calls
to pairfree in authCleanup:
void
authCleanup(authInfo)
AuthInfo *authInfo;
{
pairfree(authInfo->cutList);
authInfo->cutList = (VALUE_PAIR *)0;
pairfree(authInfo->authreq->request);
memset(authInfo->authreq, 0, sizeof(AUTH_REQ));
free(authInfo->authreq);
authInfo->authreq = (AUTH_REQ *)0;
}
With the above change to cut_attribute, both authInfo->cutList and
authInfo->authreq->request still end up pointing to the same place
which is now just one element and not the entire attribute list. So,
authCleanup will free the first element twice and all the other
elements won't be freed at all.
One possible fix would be to add another line to rad_authenticate:
/* Get the username from the request */
authList = cut_attribute(authreq->request, PW_USER_NAME, &authName);
+ authreq->request = authList;
if((authName == (VALUE_PAIR *)NULL) ||
((int)strlen(authName->strvalue) <= 0)) {
log_err("Authenticate: from %s - No User Name\n",
ip_hostname(authreq->ipaddr));
authCleanup(&authInfo);
return -1;
}
And then again, there's the possibility of other problems being introduced
because of cut_attribute's new behavior. I simply figured that the original
patch I supplied would fix the problem without the possibility of introducing
any new problems. Yet I agree that fixing cut_attribute would probably be the
more elegant solution, but it would require a little more time.
Maybe cut_attribute's first argument could be a (VALUE_PAIR **) instead of
(VALUE_PAIR *) and it wouldn't have to return anything. That way the pointer
to the head of the original list could be changed to truly point to "a list
of all the attributes in the original list EXCEPT the first occurence of one
matching the attribute id" (quoted from the comments for cut_attribute).
Regards,
Pius
--
I N T E R N E T Pius Fischer, Chief Engineer
E X P R E S S 611 W. Sixth St., Suite 3201
N E T W O R K Los Angeles, CA 90017
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199608312212.PAA12635>