radius_xlat chops embedded NULs in cisco-av-pair

[Arran Cudbard-Bell]

On 31 Mar 2014, at 12:42, Arran Cudbard-Bell <a.cudbardb at freeradius.org> wrote:

> 
> On 31 Mar 2014, at 07:00, Kiril <kyrmail at 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 at freeradius.org>
FreeRADIUS Development Team

FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 881 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.freeradius.org/mailman/private/freeradius-users/attachments/20140331/664bee46/attachment.pgp>