Re: radius_xlat chops embedded NULs in cisco-av-pair
I'll take a look and see if it's fixed in later versions. It'll almost certainly work fine in 3.0.2.
I looked into the current git v3 master sources - the main thing remais the same in vp_prints_value: print.c, line 258
/* xlat.c - need to copy raw value verbatim */ else if (quote < 0) { return strlcpy(out, vp->vp_strvalue, outlen);
in other cases fr_print_string is used and our company is not ready to move on freeradius v3, so we need a solution on v2 branch
On 31 Mar 2014, at 13:23, Kiril <kyrmail@gmail.com> wrote:
I'll take a look and see if it's fixed in later versions. It'll almost certainly work fine in 3.0.2.
I looked into the current git v3 master sources - the main thing remais the same in vp_prints_value:
No, it doesn't.
print.c, line 258
/* xlat.c - need to copy raw value verbatim */ else if (quote < 0) { return strlcpy(out, vp->vp_strvalue, outlen);
in other cases fr_print_string is used
If you look at the calls to fr_print_string, you'd see quote < 0 is only passed in two places, valuepair.c:85 and valuepair.c:114, both in the radius_compare_vps function, where the output is passed to the regular expression comparison functions. Arguably if were doing comparisons on the raw attribute it should be a memcpy, especially as regcomp appears to take the length of the input string so it can probably deal with embedded NULLs But, this isn't related to your issue, if you read the concat code in 3.x.x xlat.c:1911 it's calling vp_aprint, which at the moment just does a talloc_strdup, which is as bad as strlcpy. I'm fixing it so it does: case PW_TYPE_STRING: { size_t len; /* Gets us the size of the buffer we need to alloc */ len = fr_print_string_len(vp->vp_strvalue, vp->length); p = talloc_array(ctx, char, len + 1); /* +1 for '\0' */ if (!p) return NULL; if (fr_print_string(vp->vp_strvalue, vp->length, p, len + 1) != len) { talloc_free(p); fr_strerror_printf("Incorrect size of buffer allocated to hold escaped string"); return NULL; } break; } Where fr_print_string_len is a new function which calculates the buffer required to hold an escaped version of the string. After i've fixed that, and written a regression test for 3.x.x I will take a look at 2.x.x. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2
Arran Cudbard-Bell wrote:
if (fr_print_string(vp->vp_strvalue, vp->length, p, len + 1) != len) { talloc_free(p); fr_strerror_printf("Incorrect size of buffer allocated to hold escaped string"); return NULL;
Except that an embedded zero will end up as 4 characters of "\000". It will *always* hit that error. The xlat code doesn't ask for strings to be escaped because it assumes that the escaping is done elsewhere. That assumption is arguably wrong for embedded NULs. I think that the xlat code should always escape those characters. I'll take a look through the git history to see why xlat behaves this way. The underlying assumptions may no longer be valid. Alan DeKok.
On 31 Mar 2014, at 15:37, Alan DeKok <aland@deployingradius.com> wrote:
Arran Cudbard-Bell wrote:
if (fr_print_string(vp->vp_strvalue, vp->length, p, len + 1) != len) { talloc_free(p); fr_strerror_printf("Incorrect size of buffer allocated to hold escaped string"); return NULL;
Except that an embedded zero will end up as 4 characters of "\000". It will *always* hit that error.
The fr_print_string_len function takes that into account, it's like the double pass stuff with aprintf, it's just calculating the length of the buffer required to hold the escaped version of the string.
The xlat code doesn't ask for strings to be escaped because it assumes that the escaping is done elsewhere. That assumption is arguably wrong for embedded NULs.
Agreed.
I think that the xlat code should always escape those characters.
Or ask for those chars to be escaped? Yes.
I'll take a look through the git history to see why xlat behaves this way. The underlying assumptions may no longer be valid.
Well, in 3.x.x there was actually another place where -1 was passed to vp_prints_value, but it was just for getting the value for foreach loops, so i've changed that. For 2.x.x I agree the best fix is just to get fr_prints_value to do the escaping. For 3.x.x I tried modifying the vp_prints_value to do a null terminated memcpy, for quote < 0, but it triggered an assert somewhere deep in the template/ paircmp code. I'll add a note to the issue tracker and deal with it later. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team FD31 3077 42EC 7FCD 32FE 5EE2 56CF 27F9 30A8 CAA2
participants (3)
-
Alan DeKok -
Arran Cudbard-Bell -
Kiril