CVE-2013-2220: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: Buffer overflow in the radius_get_vendor_attr function in the Radius extension before 1.2.7 for PHP allows remote attackers to cause a denial of service (crash) and possibly execute arbitrary code via a large Vendor Specific Attributes (VSA) length value.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
Published: 2013-07-31T13:20Z
Repository: https://github.com/LawnGnome/php-radius
Language: C
Files changed: radius.c, radlib.c, radlib_vs.h
Commit date: 2013-06-27 14:06:16 -0700
Commit message: 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

Diff: diff --git a/radius.c b/radius.c
index 210b935..45b31c2 100644
--- a/radius.c
+++ b/radius.c
@@ -539,28 +539,28 @@ PHP_FUNCTION(radius_get_attr)
 /* }}} */
 
 /* {{{ proto string radius_get_vendor_attr(data) */
 PHP_FUNCTION(radius_get_vendor_attr)
 {
-	int res;
-	const void *data;
+	const void *data, *raw;
 	int len;
 	u_int32_t vendor;
+	unsigned char type;
+	size_t data_len;
 
-	if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &data, &len) == FAILURE) {
+	if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &raw, &len) == FAILURE) {
 		return;
 	}
 
-	res = rad_get_vendor_attr(&vendor, &data, (size_t *) &len);
-	if (res == -1) {
+	if (rad_get_vendor_attr(&vendor, &type, &data, &data_len, raw, len) == -1) {
 		RETURN_FALSE;
 	} else {
 
 		array_init(return_value);
-		add_assoc_long(return_value, "attr", res);
+		add_assoc_long(return_value, "attr", type);
 		add_assoc_long(return_value, "vendor", vendor);
-		add_assoc_stringl(return_value, "data", (char *) data, len, 1);
+		add_assoc_stringl(return_value, "data", (char *) data, data_len, 1);
 		return;
 	}
 }
 /* }}} */
 
diff --git a/radlib.c b/radlib.c
index 3592cde..3e163d7 100644
--- a/radlib.c
+++ b/radlib.c
@@ -896,19 +896,28 @@ split(char *str, char *fields[], int maxfields, char *msg, size_t msglen)
 	}
 	return i;
 }
 
 int
-rad_get_vendor_attr(u_int32_t *vendor, const void **data, size_t *len)
+rad_get_vendor_attr(u_int32_t *vendor, unsigned char *type, const void **data, size_t *len, const void *raw, size_t raw_len)
 {
 	struct vendor_attribute *attr;
 
-	attr = (struct vendor_attribute *)*data;
+	if (raw_len < sizeof(struct vendor_attribute)) {
+		return -1;
+	}
+
+	attr = (struct vendor_attribute *) raw;
 	*vendor = ntohl(attr->vendor_value);
+	*type = attr->attrib_type;
 	*data = attr->attrib_data;
 	*len = attr->attrib_len - 2;
 
+	if ((attr->attrib_len + 4) > raw_len) {
+		return -1;
+	}
+
 	return (attr->attrib_type);
 }
 
 int
 rad_put_vendor_addr(struct rad_handle *h, int vendor, int type,
diff --git a/radlib_vs.h b/radlib_vs.h
index 1c448a6..889a454 100644
--- a/radlib_vs.h
+++ b/radlib_vs.h
@@ -72,11 +72,11 @@
 
 #define SALT_LEN    2
 
 struct rad_handle;
 
-int	rad_get_vendor_attr(u_int32_t *, const void **, size_t *);
+int	rad_get_vendor_attr(u_int32_t *, unsigned char *, const void **, size_t *, const void *, size_t);
 int	rad_put_vendor_addr(struct rad_handle *, int, int, struct in_addr);
 int	rad_put_vendor_attr(struct rad_handle *, int, int, const void *,
 	    size_t);
 int	rad_put_vendor_int(struct rad_handle *, int, int, u_int32_t);
 int	rad_put_vendor_string(struct rad_handle *, int, int, const char *);

Vulnerable code: 	int res;
	const void *data;
	if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &data, &len) == FAILURE) {
	res = rad_get_vendor_attr(&vendor, &data, (size_t *) &len);
	if (res == -1) {
		add_assoc_long(return_value, "attr", res);
		add_assoc_stringl(return_value, "data", (char *) data, len, 1);
rad_get_vendor_attr(u_int32_t *vendor, const void **data, size_t *len)
	attr = (struct vendor_attribute *)*data;
int	rad_get_vendor_attr(u_int32_t *, const void **, size_t *);
Fixed code: 	const void *data, *raw;
	unsigned char type;
	size_t data_len;
	if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &raw, &len) == FAILURE) {
	if (rad_get_vendor_attr(&vendor, &type, &data, &data_len, raw, len) == -1) {
		add_assoc_long(return_value, "attr", type);
		add_assoc_stringl(return_value, "data", (char *) data, data_len, 1);
rad_get_vendor_attr(u_int32_t *vendor, unsigned char *type, const void **data, size_t *len, const void *raw, size_t raw_len)
	if (raw_len < sizeof(struct vendor_attribute)) {
		return -1;
	}

	attr = (struct vendor_attribute *) raw;
	*type = attr->attrib_type;
	if ((attr->attrib_len + 4) > raw_len) {
		return -1;
	}

int	rad_get_vendor_attr(u_int32_t *, unsigned char *, const void **, size_t *, const void *, size_t);