radius_xlat chops embedded NULs in cisco-av-pair
Hi, there is a problem under freeradius 2.1.12 when incoming radius packet has embedded NUL bytes in the middle of the string attribute cisco-av-pair in debug packet output it is printed as Cisco-AVPair = "release-source=\000\000\000\002" but when we use %{Cisco-AVPair[*]} in sql accounting query it is expanded as "release-source=" in previous version 1.x the bahaviour was different - xlat-ed string in query was shown as in debug output according to RFC 2869 section 5 it is said: "Servers and servers and clients MUST be able to deal with embedded nulls. RADIUS implementers using C are cautioned not to use strcpy() when handling strings." and radius_xlat function uses strlcpy(out, vp->vp_strvalue, outlen) in vp_prints_value, so not the full string vp->vp_strvalue is copied what should be done in this case: fix the radius source, or fix the NAS?
On 31 Mar 2014, at 07:00, Kiril <kyrmail@gmail.com> wrote:
Hi, there is a problem under freeradius 2.1.12 when incoming radius packet has embedded NUL bytes in the middle of the string attribute cisco-av-pair in debug packet output it is printed as Cisco-AVPair = "release-source=\000\000\000\002" but when we use %{Cisco-AVPair[*]} in sql accounting query it is expanded as "release-source=" in previous version 1.x the bahaviour was different - xlat-ed string in query was shown as in debug output
according to RFC 2869 section 5 it is said: "Servers and servers and clients MUST be able to deal with embedded nulls. RADIUS implementers using C are cautioned not to use strcpy() when handling strings."
You might want to fully read the RFC before quoting it. RFC 2865 states: Note that none of the types in RADIUS terminate with a NUL (hex 00). In particular, types "text" and "string" in RADIUS do not terminate with a NUL (hex 00). The Attribute has a length field and does not use a terminator. Text contains UTF-8 encoded 10646 [7] characters and String contains 8-bit binary data. Servers and servers and clients MUST be able to deal with embedded nulls. RADIUS implementers using C are cautioned not to use strcpy() when handling strings. The format of the value field is one of five data types. Note that type "text" is a subset of type "string". text 1-253 octets containing UTF-8 encoded 10646 [7] characters. Text of length zero (0) MUST NOT be sent; omit the entire attribute instead. string 1-253 octets containing binary data (values 0 through 255 decimal, inclusive). Strings of length zero (0) MUST NOT be sent; omit the entire attribute instead. In RFC2865 speak a string is binary data, which FreeRADIUS deals with fine. strcpy and variants will never be used on attributes of type octets (which is the FR type which maps to 'string'). I'll grant you that "Servers and servers and clients MUST be able to deal with embedded nulls." is ambiguous as to whether it's referring to strings or text or both, but this is later clarified in the same paragraph with "RADIUS implementers using C are cautioned not to use strcpy() when handling strings.
and radius_xlat function uses strlcpy(out, vp->vp_strvalue, outlen) in vp_prints_value, so not the full string vp->vp_strvalue is copied
But that's not the problem. The string (RFC type 'text') should be escaped before being added to the concatenation buffer. I'll take a look and see if it's fixed in later versions. It'll almost certainly work fine in 3.0.2. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2
On 31 Mar 2014, at 12:42, Arran Cudbard-Bell <a.cudbardb@freeradius.org> wrote:
On 31 Mar 2014, at 07:00, Kiril <kyrmail@gmail.com> wrote:
Hi, there is a problem under freeradius 2.1.12 when incoming radius packet has embedded NUL bytes in the middle of the string attribute cisco-av-pair in debug packet output it is printed as Cisco-AVPair = "release-source=\000\000\000\002" but when we use %{Cisco-AVPair[*]} in sql accounting query it is expanded as "release-source=" in previous version 1.x the bahaviour was different - xlat-ed string in query was shown as in debug output
according to RFC 2869 section 5 it is said: "Servers and servers and clients MUST be able to deal with embedded nulls. RADIUS implementers using C are cautioned not to use strcpy() when handling strings."
You might want to fully read the RFC before quoting it.
RFC 2865 states:
Note that none of the types in RADIUS terminate with a NUL (hex 00). In particular, types "text" and "string" in RADIUS do not terminate with a NUL (hex 00). The Attribute has a length field and does not use a terminator. Text contains UTF-8 encoded 10646 [7] characters and String contains 8-bit binary data. Servers and servers and clients MUST be able to deal with embedded nulls. RADIUS implementers using C are cautioned not to use strcpy() when handling strings.
The format of the value field is one of five data types. Note that type "text" is a subset of type "string".
text 1-253 octets containing UTF-8 encoded 10646 [7] characters. Text of length zero (0) MUST NOT be sent; omit the entire attribute instead.
string 1-253 octets containing binary data (values 0 through 255 decimal, inclusive). Strings of length zero (0) MUST NOT be sent; omit the entire attribute instead.
In RFC2865 speak a string is binary data, which FreeRADIUS deals with fine. strcpy and variants will never be used on attributes of type octets (which is the FR type which maps to 'string').
I'll grant you that "Servers and servers and clients MUST be able to deal with embedded nulls." is ambiguous as to whether it's referring to strings or text or both, but this is later clarified in the same paragraph with "RADIUS implementers using C are cautioned not to use strcpy() when handling strings.
and radius_xlat function uses strlcpy(out, vp->vp_strvalue, outlen) in vp_prints_value, so not the full string vp->vp_strvalue is copied
But that's not the problem. The string (RFC type 'text') should be escaped before being added to the concatenation buffer. I'll take a look and see if it's fixed in later versions. It'll almost certainly work fine in 3.0.2.
Oh dear, big "FIXME" case PW_TYPE_STRING: /* * FIXME: deal with \r\n" ?? */ p = talloc_strdup(ctx, vp->vp_strvalue); break; I guess i'll fix that too then... Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2
participants (2)
-
Arran Cudbard-Bell -
Kiril