Unsupported Vendor Specific Attribute results in incorrect behavior of rc_avpair_gen()
Hi, 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 dictionary file on client does not have entry for Vendor-Specific attribute value “14817” so this is an unsupported vendor-specific attribute I’ve found that if an unsupported Vendor-Specific attribute is last in the list of attributes then rc_avpair_gen function (avpair.c) returns NULL below: Sendserver.c: rc_send_server: line 367 if (length > 0) { data->receive_pairs = rc_avpair_gen(rh, NULL, recv_auth->data, ←- data->receive_pairs is NULL length, 0); } else { data->receive_pairs = NULL; } The problem lies in rc_avpair_gen function. As Vendor-Specific is last and unsupported so “pair” will be NULL before below fragment returns if (attribute == PW_VENDOR_SPECIFIC) { if (attrlen < 4) { rc_log(LOG_ERR, "rc_avpair_gen: received VSA " "attribute with invalid length"); goto shithappens; } memcpy(&lvalue, ptr, 4); 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 " "attribute with unknown Vendor-Id %d", vendorpec); return pair; ←- This will be NULL } /* Process recursively */ return rc_avpair_gen(rh, pair, ptr + 4, attrlen - 4, vendorpec); } 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; } 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? Thanks Amol Lad Software Architect 4RF Limited DDI: +64 4 498 7024 Tel: +64 4 499 6000 Mobile: +64 21 257 6610 Fax: +64 4 473 4447 Web: www.4rf.com Email: amol.lad@4rf.com The information in this email communication (inclusive of attachments) is confidential to 4RF Limited and the intended recipient(s). If you are not the intended recipient(s), please note that any use, disclosure, distribution or copying of this information or any part thereof is strictly prohibited and that the author accepts no liability for the consequences of any action taken on the basis of the information provided. If you have received this email in error, please notify the sender immediately by return email and then delete all instances of this email from your system. 4RF Limited will not accept responsibility for any consequences associated with the use of this email (including, but not limited to, damages sustained as a result of any viruses and/or any action or lack of action taken in reliance on it).
On Jan 26, 2015, at 9:43 PM, Amol Lad <Amol.Lad@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.
I've changed "Vendor-Specific = 4RF" but still same problem. Please note that, radius server shows vendor-specific=0x345246 (running with -X) It showed the same value when I was using "14817" -----Original Message----- From: freeradius-users-bounces+amol.lad=4rf.com@lists.freeradius.org [mailto:freeradius-users-bounces+amol.lad=4rf.com@lists.freeradius.org] On Behalf Of Alan DeKok Sent: Tuesday, 27 January 2015 3:56 p.m. To: FreeRadius users mailing list Subject: Re: Unsupported Vendor Specific Attribute results in incorrect behavior of rc_avpair_gen() On Jan 26, 2015, at 9:43 PM, Amol Lad <Amol.Lad@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. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/users.html The information in this email communication (inclusive of attachments) is confidential to 4RF Limited and the intended recipient(s). If you are not the intended recipient(s), please note that any use, disclosure, distribution or copying of this information or any part thereof is strictly prohibited and that the author accepts no liability for the consequences of any action taken on the basis of the information provided. If you have received this email in error, please notify the sender immediately by return email and then delete all instances of this email from your system. 4RF Limited will not accept responsibility for any consequences associated with the use of this email (including, but not limited to, damages sustained as a result of any viruses and/or any action or lack of action taken in reliance on it).
On Jan 26, 2015, at 10:24 PM, Amol Lad <Amol.Lad@4rf.com> wrote:
I've changed "Vendor-Specific = 4RF" but still same problem. Please note that, radius server shows vendor-specific=0x345246 (running with -X)
Yes… 0x344246 is ASCII for “4RF”. I think you’re not clear on what “Vendor-Specific” is. You NEVER specify a value for it directly. Instead, you specify a VSA via the name of the VSA. e.g. Cisco-AVPair. The code will figure out it’s a VSA, and do the correct encoding. Do NOT put ASCII values into a Vendor-Specific. It’s *always* wrong. Alan DeKok.
Ohh.. Thanks for pointing that out. Now the "users" file is as below: abcd Cleartext-Password := "xyz" Service-Type = Framed-Management, Management-Privilege-level = 4, Cisco-AVPair = test The problem is still there; as my embedded client does not recognize Cisco-AVPair and it is last in the list, rc_avpair_gen() returns NULL. Why do you think return NULL from below is not wrong? Why should caller return NULL if rc_avpair_gen returns NULL? if (length != attrlen) { pair = rc_avpair_gen(rh, pair, ptr + attrlen, length - attrlen, vendorpec); if (pair == NULL) return NULL; } Thanks -----Original Message----- From: freeradius-users-bounces+amol.lad=4rf.com@lists.freeradius.org [mailto:freeradius-users-bounces+amol.lad=4rf.com@lists.freeradius.org] On Behalf Of Alan DeKok Sent: Tuesday, 27 January 2015 4:32 p.m. To: FreeRadius users mailing list Subject: Re: Unsupported Vendor Specific Attribute results in incorrect behavior of rc_avpair_gen() On Jan 26, 2015, at 10:24 PM, Amol Lad <Amol.Lad@4rf.com> wrote:
I've changed "Vendor-Specific = 4RF" but still same problem. Please note that, radius server shows vendor-specific=0x345246 (running with -X)
Yes... 0x344246 is ASCII for "4RF". I think you're not clear on what "Vendor-Specific" is. You NEVER specify a value for it directly. Instead, you specify a VSA via the name of the VSA. e.g. Cisco-AVPair. The code will figure out it's a VSA, and do the correct encoding. Do NOT put ASCII values into a Vendor-Specific. It's *always* wrong. Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/users.html The information in this email communication (inclusive of attachments) is confidential to 4RF Limited and the intended recipient(s). If you are not the intended recipient(s), please note that any use, disclosure, distribution or copying of this information or any part thereof is strictly prohibited and that the author accepts no liability for the consequences of any action taken on the basis of the information provided. If you have received this email in error, please notify the sender immediately by return email and then delete all instances of this email from your system. 4RF Limited will not accept responsibility for any consequences associated with the use of this email (including, but not limited to, damages sustained as a result of any viruses and/or any action or lack of action taken in reliance on it).
On Jan 26, 2015, at 10:52 PM, Amol Lad <Amol.Lad@4rf.com> wrote:
The problem is still there; as my embedded client does not recognize Cisco-AVPair and it is last in the list, rc_avpair_gen() returns NULL.
Then add Cisco-AVPair to the dictionaries.
Why do you think return NULL from below is not wrong? Why should caller return NULL if rc_avpair_gen returns NULL?
For a host of reasons. In an embedded system, you want to produce the packet you expect, with all of the attributes you want. Silently discarding an attribute means that the client does unexpected, and unknown things. In an embedded system, you control which attributes get created. There’s no editable configuration file that an administrator can screw up by mistyping an attribute. So if your code is doesn’t generate the correct attribute… it’s a bug. Fix your code, rather than trying to push the library into working around it. If the library did as you wanted, you’d still be posting here. “Hi, I asked the library to create 4 attributes, and it only created 4. That’s a bug. Why does it do that?” Fix your code to create *valid* attributes. It’s as simple as that. Alan DeKok.
I'm not sure if I fully understand you. It's _not_ possible to add every possible VSA in the client dictionary. It's possible that "this" radius client is in a network with many other devices all authenticated by a common RADIUS server. Other devices might support VSAs that this device will not understand. I'm only trying to implement below recommendation in RFC 5080: A configuration flag such as "treat unknown attributes as reject" can be exposed to the system administrator. If the flag is set to true, then Access-Accepts containing unknown attributes are treated as Access-Rejects. If the flag is set to false, then unknown attributes in Access-Accepts are silently ignored. Unfortunately with current library "bug" of discarding all known attributes if any unknown VSA is present in the end of the list, I cannot support "silently ignore" portion of above statement Amol -----Original Message----- From: freeradius-users-bounces+amol.lad=4rf.com@lists.freeradius.org [mailto:freeradius-users-bounces+amol.lad=4rf.com@lists.freeradius.org] On Behalf Of Alan DeKok Sent: Wednesday, 28 January 2015 2:57 a.m. To: FreeRadius users mailing list Subject: Re: Unsupported Vendor Specific Attribute results in incorrect behavior of rc_avpair_gen() On Jan 26, 2015, at 10:52 PM, Amol Lad <Amol.Lad@4rf.com> wrote:
The problem is still there; as my embedded client does not recognize Cisco-AVPair and it is last in the list, rc_avpair_gen() returns NULL.
Then add Cisco-AVPair to the dictionaries.
Why do you think return NULL from below is not wrong? Why should caller return NULL if rc_avpair_gen returns NULL?
For a host of reasons. In an embedded system, you want to produce the packet you expect, with all of the attributes you want. Silently discarding an attribute means that the client does unexpected, and unknown things. In an embedded system, you control which attributes get created. There's no editable configuration file that an administrator can screw up by mistyping an attribute. So if your code is doesn't generate the correct attribute... it's a bug. Fix your code, rather than trying to push the library into working around it. If the library did as you wanted, you'd still be posting here. "Hi, I asked the library to create 4 attributes, and it only created 4. That's a bug. Why does it do that?" Fix your code to create *valid* attributes. It's as simple as that. Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/users.html The information in this email communication (inclusive of attachments) is confidential to 4RF Limited and the intended recipient(s). If you are not the intended recipient(s), please note that any use, disclosure, distribution or copying of this information or any part thereof is strictly prohibited and that the author accepts no liability for the consequences of any action taken on the basis of the information provided. If you have received this email in error, please notify the sender immediately by return email and then delete all instances of this email from your system. 4RF Limited will not accept responsibility for any consequences associated with the use of this email (including, but not limited to, damages sustained as a result of any viruses and/or any action or lack of action taken in reliance on it).
On Jan 27, 2015, at 4:33 PM, Amol Lad <Amol.Lad@4rf.com> wrote:]
I'm not sure if I fully understand you. It's _not_ possible to add every possible VSA in the client dictionary. It's possible that "this" radius client is in a network with many other devices all authenticated by a common RADIUS server. Other devices might support VSAs that this device will not understand.
Yes.
I'm only trying to implement below recommendation in RFC 5080:
A configuration flag such as "treat unknown attributes as reject" can be exposed to the system administrator. If the flag is set to true, then Access-Accepts containing unknown attributes are treated as Access-Rejects. If the flag is set to false, then unknown attributes in Access-Accepts are silently ignored.
That recommendation is for packets received from the network. It is NOT for attributes read from a configuration file.
Unfortunately with current library "bug" of discarding all known attributes if any unknown VSA is present in the end of the list, I cannot support "silently ignore" portion of above statement
Patches are welcome. The problem is that your previous messages used examples of attributes read from configuration files. So it’s natural to assume that’s what you meant. So… my recommendations were for attributes read from configuration files. If you’re now talking about attributes read from a packet… that’s different. And my recommendations are different. Alan DeKok.
participants (2)
-
Alan DeKok -
Amol Lad