3.0.x - string attributes copied from perl to FreeRADIUS may be truncated (also in unlang)

[nchaigne]

String attributes copied from perl to FreeRADIUS may be truncated (also in unlang).

If the value of a "string" attribute contains a '\0', then the VP value copied to FreeRADIUS is truncated. Value is fetched from perl using SvPV_nolen, then put into a VP using pairstrcpy, which does a "talloc_strdup" (hence stopping at the first '\0' in the string).

In function pairadd_sv, instead of:

val = SvPV_nolen(sv);

We should do the following to get the string length:

STRLEN len;
val = SvPV(sv, len);

Then instead of:

pairstrcpy(vp, val);

Do something else, for example:

pairmemcpy(vp, val, len);

This fix seems to work, but I'm not sure pairmemcpy should be called (its purpose is to handle an "octet" type, not a "string").

Maybe, pairstrcpy should take a "length" argument, and do a "talloc_memdup" instead of "talloc_strdup". In its current form, more things won't work besides perl. For instance, in unlang, manipulating VP strings with '\0' inside will truncate them.

There are quite a few calls to pairstrcpy, though, so it's not a minor fix...

[arr2036]

There's now a pair strncpy, which is a slight misnomer because it won't stop on \0s like the actual strncpy would, but it's what should be used here. Traditionally the server hasn't been \0 safe for string type attributes.

[arr2036]

and you're right, pairmemcpy isn't the right one to use here as it doesn't allocate the extra space for the terminating \0.

[arr2036]

I tend to stay away from rlm_perl, I view it as a hell mouth where all the bad things in server pour forth from, but you're welcome to send a pull request.

[nchaigne]

Thanks.

Can you look into the issue about unlang ?

[nchaigne]

Damn, I butchered my previous pull request... :'( https://github.com/FreeRADIUS/freeradius-server/pull/615

Oh well. If you need the code, I still have it.

[arr2036]

What issue's that, all the nul unsafe code? I honestly don't have time :( If you can pick off the easy ones where length is available and were just using pairstrcpy anyway that would be a big help. Regarding the radeapclient code, we want to create a single utility which is why it hasn't been merged yet. It'll probably a month or two.

[nchaigne]

Well it's not that simple... looks like "pairparsevalue" also has the same issue: it receives a char* value without length, and calls pairstrcpy or talloc_typed_strdup for a "string"...

Function "pairparsevalue" is called by "radius_xlat_do". The problem may be here for unlang. But "pairparsevalue" is also called at other places (about 20 times). If the signature is changed, every call would have to be fixed also.

I don't want to break things. I'd prefer that you look into this (when you have the time, I understand that you don't now, no worries... I'm not in a hurry, but it's definitively something for which I'll need a fix in the future. If I can't use unlang, I can still build a custom module to handle these vexing string values myself, but I'd rather not do that if I can avoid it !).

[arr2036]

I have to say i'm not entirely sure why embedded null safety is a priority. If you need to pass around non printable chars, do it in an octets type attribute which are guaranteed to be embedded null safe. You can still use "%{string:}" which will print the escape sequences for the embedded nulls...

[nchaigne]

In fact this is a standard attribute of type "string" which is in a reply from a home server. They can put whatever they want inside, including '\0'. And they do.

True, we can probably update our local dictionary to change this attribute type to octets. That would be another workaround.

Since I have workarounds, we can consider that this problem is not of high priority :)

[arr2036]

The terms in RFC 2865 are confusing

RFC 2865  |  FR Type
text      |  string
string    |  octets

[arr2036]

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.

I've discussed this with Alan before and apparently that string refers exclusively to octet types. So actually if they're sending rfc2865:text attributes, they shouldn't contain embedded NULs, if they're sending rfc2865:string attributes, then they can, but the code that processes them should be NUL safe.

[nchaigne]

I've read the RFC also, I agree this is confusing...

I read (RFC 2865):

  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.

My attribute is "string" (FRD), which means "text" (RFC), hence not octets but UTF-8 text. But a '\0' is a valid UTF-8 character... from RFC 3629:

  Character numbers from U+0000 to U+007F (US-ASCII repertoire)
  correspond to octets 00 to 7F (7 bit US-ASCII values).  A direct
  consequence is that a plain ASCII string is also a valid UTF-8
  string.

[alandekok]

sending a zero byte in a printable string is stupid. We'll take a look at fixing it, but it's not a high priority.

The server hasn't handled it well for the last 15 years, and there have been few complaints.

I'll also note there's no use-case, example debug output, or information about what the NAS is.

[nchaigne]

OK no problem. Maybe FRD should fix the dictionary instead ?

The attribute is "Chargeable-User-Identity", it is defined in RFC 4372, and is labelled as "string" (which, if the RFC is consistent with RFC 2865, means "octets" for FreeRADIUS)

In dictionary.rfc4372 we currently have:

ATTRIBUTE Chargeable-User-Identity 89 string

[arr2036]

Yes, you're quite correct, this should be octets.

[nchaigne]

Aww. I just assumed the dictionary was correct, I should have started here :'(

Oh well. Thanks for your time :)

[arr2036]

Fixed. @alanbuxey should probably check whether everything still works though. The identifiers will appear as hexits in queries now.

[spaetow]

So CUI was the wrong attribute type (i.e. string instead of octet)? Yikes!

[arr2036]

Looks like there might be more horrors lurking somewhere as the CUI encoding test now fails.

[arr2036]

Not that many horrors actually, should be ok now.