Unsupported Vendor Specific Attribute results in incorrect behavior of rc_avpair_gen()

[Alan DeKok]

On Jan 26, 2015, at 9:43 PM, Amol Lad <Amol.Lad at 4rf.com> wrote:
> I’m using freeradius client library 1.1.6 with radius server version 2.2.6.  The “users” file on server has an entry for user “abcd” as below:
> 
> abcd  Cleartext-Password := "xyz"
>     Service-Type = Framed-Management,
>    Management-Privilege-level  = 4,
>    Vendor-Specific            = “14817"

  The Vendor-Specific attribute isn’t ASCII text.

> The dictionary file on client does not have entry for Vendor-Specific attribute value “14817” so this is an unsupported vendor-specific attribute

  No, the attribute is malformed.  It shouldn’t be allowed.

> As a result, below fragment will return NULL as well. Thus recursion unwinds by returning “pair” NULL even if valid attributes are also present
> 
>                /* 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)  ←-- Recursion unwinds and all invocations return NULL
>                                                return NULL;
>                }

  That check is likely not needed… but it’s probably not *wrong*.

> Please advise how it can be addressed? Why is check [if (pair == NULL) return NULL;] needed above? Can’t we just continue with the function and return from the end of the function?

  Probably.  Patches are welcome.

  However… the freeradius-client code is intended for use in embedded environments.  i.e. where the attributes are known, and their values are known (or at least well-formed).

  If you’re creating attributes with garbage values, the simple solution is “don’t do that”.

  Alan DeKok.