Package: libfreeradius-client2
Version: 1.1.6-7
Severity: normal
Tags: patch
Cc: freeradius-de...@lists.freeradius.org
I have noticed that the attribute list is empty for radius packets like:
Radius Protocol
Code: Access-Accept (2)
Packet identifier: 0xe4 (228)
Length: 137
Authenticator: c43ca3c2765cfab8545e6e4a7f83ba9a
[This is a response to a request in frame 142]
[Time from request: 0.004763000 seconds]
Attribute Value Pairs
AVP: l=11 t=Vendor-Specific(26) v=Reserved(0)
VSA: l=5 t=Unknown-Attribute(27): 363030
Unknown-Attribute: 363030
AVP: l=12 t=Vendor-Specific(26) v=Wireless Broadband Alliance Ltd
(previous was 'Wi-Fi Alliance')(14122)
VSA: l=6 t=WISPr-Bandwidth-Max-Down(8): 124008
WISPr-Bandwidth-Max-Down: 124008
AVP: l=12 t=Vendor-Specific(26) v=Wireless Broadband Alliance Ltd
(previous was 'Wi-Fi Alliance')(14122)
VSA: l=6 t=WISPr-Bandwidth-Max-Up(7): 124007
WISPr-Bandwidth-Max-Up: 124007
AVP: l=6 t=Framed-Protocol(7): PPP(1)
Framed-Protocol: PPP (1)
AVP: l=6 t=Service-Type(6): Framed(2)
Service-Type: Framed (2)
AVP: l=46 t=Class(25):
8fdc089a00000137000102000a010a2d00000000d5a02756...
Class: 8fdc089a00000137000102000a010a2d00000000d5a02756...
AVP: l=12 t=Vendor-Specific(26) v=Microsoft(311)
VSA: l=6 t=MS-Link-Utilization-Threshold(14): 50
MS-Link-Utilization-Threshold: 50
AVP: l=12 t=Vendor-Specific(26) v=Microsoft(311)
VSA: l=6 t=MS-Link-Drop-Time-Limit(15): 120
MS-Link-Drop-Time-Limit: 120
The problem is that the vendor microsoft in my version was unknown.
The recursive attribute parser uses NULL as information for the caller that
its part of the parsing failed. But NULL is also returned when the last
attribute was from an unknown vendor. Instead it should only be skipped as
it is documented inside the function.
A proof of concept patch is attached which uses a special parameter which
is used to inform the caller about an error. The returned value is only
used as handler for the list.
--- System information. ---
Architecture: amd64
Kernel: Linux 4.0.0-2-amd64
Debian Release: stretch/sid
500 unstable httpredir.debian.org
From: Sven Eckelmann <s...@open-mesh.com>
Date: Wed, 22 Jul 2015 12:24:47 +0200
Subject: [PATCH] radiusclient-ng: Don't drop attribute list when last attribute is from unknown vendor
The recursive attribute parser uses NULL as information for the caller that
its part of the parsing failed. But NULL is also returned when the last
attribute was from an unknown vendor. Instead it should only be skipped as
it is documented inside the function.
Introduce an extra parameter which is only used to store the error state
and leave the return value for the list item.
---
lib/avpair.c | 44 +++++++++++++++++++++++++++++---------------
1 file changed, 29 insertions(+), 15 deletions(-)
diff --git a/lib/avpair.c b/lib/avpair.c
index 979c0d9..7b215d2 100644
--- a/lib/avpair.c
+++ b/lib/avpair.c
@@ -154,6 +154,10 @@ VALUE_PAIR *rc_avpair_new (rc_handle *rh, int attrid, void *pval, int len, int v
return vp;
}
+static VALUE_PAIR *
+rc_avpair_gen_rec(rc_handle *rh, VALUE_PAIR *pair, unsigned char *ptr,
+ int length, int vendorpec, int *recursive_error);
+
/*
*
* Function: rc_avpair_gen
@@ -168,6 +172,15 @@ VALUE_PAIR *
rc_avpair_gen(rc_handle *rh, VALUE_PAIR *pair, unsigned char *ptr,
int length, int vendorpec)
{
+ int recursive_error = 0;
+
+ return rc_avpair_gen_rec(rh, pair,ptr, length, vendorpec, &recursive_error);
+}
+
+static VALUE_PAIR *
+rc_avpair_gen_rec(rc_handle *rh, VALUE_PAIR *pair, unsigned char *ptr,
+ int length, int vendorpec, int *recursive_error)
+{
int attribute, attrlen, x_len;
unsigned char *x_ptr;
UINT4 lvalue;
@@ -178,22 +191,22 @@ rc_avpair_gen(rc_handle *rh, VALUE_PAIR *pair, unsigned char *ptr,
char hex[3];
if (length < 2) {
- rc_log(LOG_ERR, "rc_avpair_gen: received attribute with "
+ rc_log(LOG_ERR, "rc_avpair_gen_rec: received attribute with "
"invalid length");
goto shithappens;
}
attrlen = ptr[1];
if (length < attrlen || attrlen < 2) {
- rc_log(LOG_ERR, "rc_avpair_gen: received attribute with "
+ rc_log(LOG_ERR, "rc_avpair_gen_rec: received attribute with "
"invalid length");
goto shithappens;
}
/* Advance to the next attribute and process recursively */
if (length != attrlen) {
- pair = rc_avpair_gen(rh, pair, ptr + attrlen, length - attrlen,
- vendorpec);
- if (pair == NULL)
+ pair = rc_avpair_gen_rec(rh, pair, ptr + attrlen, length - attrlen,
+ vendorpec, recursive_error);
+ if (*recursive_error)
return NULL;
}
@@ -205,7 +218,7 @@ rc_avpair_gen(rc_handle *rh, VALUE_PAIR *pair, unsigned char *ptr,
/* VSA */
if (attribute == PW_VENDOR_SPECIFIC) {
if (attrlen < 4) {
- rc_log(LOG_ERR, "rc_avpair_gen: received VSA "
+ rc_log(LOG_ERR, "rc_avpair_gen_rec: received VSA "
"attribute with invalid length");
goto shithappens;
}
@@ -213,13 +226,13 @@ rc_avpair_gen(rc_handle *rh, VALUE_PAIR *pair, unsigned char *ptr,
vendorpec = ntohl(lvalue);
if (rc_dict_getvend(rh, vendorpec) == NULL) {
/* Warn and skip over the unknown VSA */
- rc_log(LOG_WARNING, "rc_avpair_gen: received VSA "
+ rc_log(LOG_WARNING, "rc_avpair_gen_rec: received VSA "
"attribute with unknown Vendor-Id %d", vendorpec);
return pair;
}
/* Process recursively */
- return rc_avpair_gen(rh, pair, ptr + 4, attrlen - 4,
- vendorpec);
+ return rc_avpair_gen_rec(rh, pair, ptr + 4, attrlen - 4,
+ vendorpec, recursive_error);
}
/* Normal */
@@ -232,11 +245,11 @@ rc_avpair_gen(rc_handle *rh, VALUE_PAIR *pair, unsigned char *ptr,
strcat(buffer, hex);
}
if (vendorpec == 0) {
- rc_log(LOG_WARNING, "rc_avpair_gen: received "
+ rc_log(LOG_WARNING, "rc_avpair_gen_rec: received "
"unknown attribute %d of length %d: 0x%s",
attribute, attrlen + 2, buffer);
} else {
- rc_log(LOG_WARNING, "rc_avpair_gen: received "
+ rc_log(LOG_WARNING, "rc_avpair_gen_rec: received "
"unknown VSA attribute %d, vendor %d of "
"length %d: 0x%s", attribute & 0xffff,
VENDOR(attribute), attrlen + 2, buffer);
@@ -246,7 +259,7 @@ rc_avpair_gen(rc_handle *rh, VALUE_PAIR *pair, unsigned char *ptr,
rpair = malloc(sizeof(*rpair));
if (rpair == NULL) {
- rc_log(LOG_CRIT, "rc_avpair_gen: out of memory");
+ rc_log(LOG_CRIT, "rc_avpair_gen_rec: out of memory");
goto shithappens;
}
memset(rpair, '\0', sizeof(*rpair));
@@ -267,13 +280,13 @@ rc_avpair_gen(rc_handle *rh, VALUE_PAIR *pair, unsigned char *ptr,
case PW_TYPE_INTEGER:
if (attrlen != 4) {
- rc_log(LOG_ERR, "rc_avpair_gen: received INT "
+ rc_log(LOG_ERR, "rc_avpair_gen_rec: received INT "
"attribute with invalid length");
goto shithappens;
}
case PW_TYPE_IPADDR:
if (attrlen != 4) {
- rc_log(LOG_ERR, "rc_avpair_gen: received IPADDR"
+ rc_log(LOG_ERR, "rc_avpair_gen_rec: received IPADDR"
" attribute with invalid length");
goto shithappens;
}
@@ -282,7 +295,7 @@ rc_avpair_gen(rc_handle *rh, VALUE_PAIR *pair, unsigned char *ptr,
break;
default:
- rc_log(LOG_WARNING, "rc_avpair_gen: %s has unknown type",
+ rc_log(LOG_WARNING, "rc_avpair_gen_rec: %s has unknown type",
attr->name);
goto shithappens;
}
@@ -294,6 +307,7 @@ shithappens:
free(pair);
pair = rpair;
}
+ recursive_error = 1;
return NULL;
}
