From owner-freebsd-isp  Sat Aug 31 15:13:32 1996
Return-Path: owner-isp
Received: (from root@localhost)
          by freefall.freebsd.org (8.7.5/8.7.3) id PAA20231
          for isp-outgoing; Sat, 31 Aug 1996 15:13:32 -0700 (PDT)
Received: from iago.ienet.com (iago.ienet.com [207.78.32.53])
          by freefall.freebsd.org (8.7.5/8.7.3) with ESMTP id PAA20213
          for <freebsd-isp@FreeBSD.ORG>; Sat, 31 Aug 1996 15:13:24 -0700 (PDT)
Received: from ienet.com (localhost.ienet.com [127.0.0.1]) by iago.ienet.com (8.7.5/8.7.3) with ESMTP id PAA12635; Sat, 31 Aug 1996 15:12:27 -0700 (PDT)
Message-Id: <199608312212.PAA12635@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 
Date: Sat, 31 Aug 1996 15:12:27 -0700
From: Pius Fischer <pius@iago.ienet.com>
Sender: owner-isp@FreeBSD.ORG
X-Loop: FreeBSD.org
Precedence: bulk


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