Skip to content

Commit 13c149b

Browse files
committed
Fix a security issue in radius_get_vendor_attr().
The underlying rad_get_vendor_attr() function assumed that it would always be given valid VSA data. Indeed, the buffer length wasn't even passed in; the assumption was that the length field within the VSA structure would be valid. This could result in denial of service by providing a length that would be beyond the memory limit, or potential arbitrary memory access by providing a length greater than the actual data given. rad_get_vendor_attr() has been changed to require the raw data length be provided, and this is then used to check that the VSA is valid. Conflicts: radlib_vs.h
1 parent 71b2139 commit 13c149b

File tree

3 files changed

+19
-10
lines changed

3 files changed

+19
-10
lines changed

Diff for: radius.c

+7-7
Original file line numberDiff line numberDiff line change
@@ -541,24 +541,24 @@ PHP_FUNCTION(radius_get_attr)
541541
/* {{{ proto string radius_get_vendor_attr(data) */
542542
PHP_FUNCTION(radius_get_vendor_attr)
543543
{
544-
int res;
545-
const void *data;
544+
const void *data, *raw;
546545
int len;
547546
u_int32_t vendor;
547+
unsigned char type;
548+
size_t data_len;
548549

549-
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &data, &len) == FAILURE) {
550+
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &raw, &len) == FAILURE) {
550551
return;
551552
}
552553

553-
res = rad_get_vendor_attr(&vendor, &data, (size_t *) &len);
554-
if (res == -1) {
554+
if (rad_get_vendor_attr(&vendor, &type, &data, &data_len, raw, len) == -1) {
555555
RETURN_FALSE;
556556
} else {
557557

558558
array_init(return_value);
559-
add_assoc_long(return_value, "attr", res);
559+
add_assoc_long(return_value, "attr", type);
560560
add_assoc_long(return_value, "vendor", vendor);
561-
add_assoc_stringl(return_value, "data", (char *) data, len, 1);
561+
add_assoc_stringl(return_value, "data", (char *) data, data_len, 1);
562562
return;
563563
}
564564
}

Diff for: radlib.c

+11-2
Original file line numberDiff line numberDiff line change
@@ -898,15 +898,24 @@ split(char *str, char *fields[], int maxfields, char *msg, size_t msglen)
898898
}
899899

900900
int
901-
rad_get_vendor_attr(u_int32_t *vendor, const void **data, size_t *len)
901+
rad_get_vendor_attr(u_int32_t *vendor, unsigned char *type, const void **data, size_t *len, const void *raw, size_t raw_len)
902902
{
903903
struct vendor_attribute *attr;
904904

905-
attr = (struct vendor_attribute *)*data;
905+
if (raw_len < sizeof(struct vendor_attribute)) {
906+
return -1;
907+
}
908+
909+
attr = (struct vendor_attribute *) raw;
906910
*vendor = ntohl(attr->vendor_value);
911+
*type = attr->attrib_type;
907912
*data = attr->attrib_data;
908913
*len = attr->attrib_len - 2;
909914

915+
if ((attr->attrib_len + 4) > raw_len) {
916+
return -1;
917+
}
918+
910919
return (attr->attrib_type);
911920
}
912921

Diff for: radlib_vs.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@
7474

7575
struct rad_handle;
7676

77-
int rad_get_vendor_attr(u_int32_t *, const void **, size_t *);
77+
int rad_get_vendor_attr(u_int32_t *, unsigned char *, const void **, size_t *, const void *, size_t);
7878
int rad_put_vendor_addr(struct rad_handle *, int, int, struct in_addr);
7979
int rad_put_vendor_attr(struct rad_handle *, int, int, const void *,
8080
size_t);

0 commit comments

Comments
 (0)