search

FreeRADIUS / freeradius-client

A BSD licenced RADIUS client library
http://freeradius.org/freeradius-client/
Other
143 stars 100 forks source link

BUG: Dictionary attributes compared against UINT8_MAX #102

Closed KoffKoff closed 4 years ago

KoffKoff commented 4 years ago

The value comparison for dictionary attributes are done against UINT8_MAX, the variable that holds the values (value) is int32_t and the field in the dict_attr struct are uint32_t. The affected function is rc_read_dictionary in lib/dict.c.

There are a multiple of issues with this, first of since value can hold negative values the comparison is somehwat wrong. second if the dict_attr struct holds uint32_t it makes no sense to have an upper bound of uint8_max (unless I'm missing something).

I wouldn't mind doing a pull request for this, but since I don't know which of these types and values are proper I don't know what to change.

alandekok commented 4 years ago

The attribute numbers in RADIUS are 8-bit unsigned integers, so UINT8_MAX is OK.

Some vendor-specific attributes can be 16-bit, but those aren't needed by this library.

KoffKoff commented 4 years ago

But the type of value variable in the function is still int32_t which could cause an underflow (in bad dictionaries).

KoffKoff commented 4 years ago

I'm closing this since it seems as though I've made incorrect assumptions and the bug report is inaccurate, however you still seem to have a potential problem when an OID value is negative, I'm not sure if that is something you want to handle though.