lib/filters.c returning incorrect length filters
Hi freeRADIUS developers, Firstly, a disclaimer/confession. The analysis below is based on freeRADIUS version 1.0.1, however I can't see any significant differences between this and CVS HEAD in relation to this issue. The patch I have attached is based on CVS HEAD, but is unfortunately untested. Please feel free to tell me its not an issue in CVS HEAD any more due to other changes, or to go away and test it first, but with my current work load I'm not sure when I will be able to do that. While testing Ascend Data Filters (IP filters) it became apparent that freeRADIUS was sending 32 byte long Ascend-Data-Filter attributes. In our case our NAS's expect 24 byte IP filters (while some newer NAS's apparently expect 26 bytes?) and complains when its gets something longer. Taking a look at lib/filters.c it became apparent that the code makes no distinction between the various filter types when assigning the length of the attribute. The attached patch helps address this issue by assigning the length of the string (or vp_filter) attribute based on the actual size of the filter type (and corresponding changes for printing Ascend Data Filter attributes). A bit of reading on the web revealed that some other RADIUS servers allow the fill size at the end of an IP filter to be configured (to allow for 24 or 26 byte filters more easily). This looks a bit difficult in freeRADIUS due to the code being in libradius rather than the core. It is also something that would need to be configured on a per client basis, which is rather nasty. I believe there is also a bug in the print_abinary() function. It was comparing the vp->length to sizeof(filter), but filter is now a pointer. Also, it is not possible to do a "!=" comparison here, as different filter types have different lengths. At worst we can do a ">" comparison, a better solution would be to do the comparison based on the actual filter type further down the code. My patch compares the length of the attribute against the filter type. regards, Mike
"Mitchell, Michael J" <Michael.Mitchell@team.telstra.com> wrote:
While testing Ascend Data Filters (IP filters) it became apparent that freeRADIUS was sending 32 byte long Ascend-Data-Filter attributes. In our case our NAS's expect 24 byte IP filters (while some newer NAS's apparently expect 26 bytes?) and complains when its gets something longer.
That's the first I've heard of the problem. The code has been in the server (and in Cistron before that) for over 8 years.
Taking a look at lib/filters.c it became apparent that the code makes no distinction between the various filter types when assigning the length of the attribute.
From everything I've seen, that's correct, and should work.
A bit of reading on the web revealed that some other RADIUS servers allow the fill size at the end of an IP filter to be configured (to allow for 24 or 26 byte filters more easily). This looks a bit difficult in freeRADIUS due to the code being in libradius rather than the core. It is also something that would need to be configured on a per client basis, which is rather nasty.
A module could do this without too much trouble.
I believe there is also a bug in the print_abinary() function. It was comparing the vp->length to sizeof(filter), but filter is now a pointer.
No... it's comparing vp->length to SIZEOF_RADFILTER, which is a macro defined to be 32. See earlier code.
Also, it is not possible to do a "!=" comparison here, as different filter types have different lengths. At worst we can do a ">" comparison, a better solution would be to do the comparison based on the actual filter type further down the code. My patch compares the length of the attribute against the filter type.
I'm really surprised that this code is necessary. I'd like to know a lot more before making any changes. Alan DeKok.
Hi Alan, Thanks for the response. See comments below.
"Mitchell, Michael J" <Michael.Mitchell@team.telstra.com> wrote:
While testing Ascend Data Filters (IP filters) it became apparent that freeRADIUS was sending 32 byte long Ascend-Data-Filter attributes. In our case our NAS's expect 24 byte IP filters (while some newer NAS's apparently expect 26 bytes?) and complains when its gets something longer.
That's the first I've heard of the problem. The code has been in the server (and in Cistron before that) for over 8 years.
I'll try to set up a clean install and retest, but I'm pretty sure the behaviour I have observed is as stated. By analysis: sizeof(ascend_ip_filter_t) = 22 bytes sizeof(ascend_ipx_filter_t) = 26 bytes sizeof(ascend_generic_filter_t) = 22 bytes and, according to: typedef struct ascend_filter_t { uint8_t type; uint8_t forward; uint8_t direction; uint8_t fill; union { ascend_ip_filter_t ip; ascend_ipx_filter_t ipx; ascend_generic_filter_t generic; uint8_t data[28]; /* ensure it's 32 bytes */ } u; } ascend_filter_t; sizeof(ascend_filter_t) = 32 bytes (4 + size of the union) In ascend_parse_filter(): pair->length is set to sizeof(filter), and sizeof(filter) bytes are copied to pair->vp_filter where filter is of type ascend_filter_t. So, maybe the actual problem is grouping ascend_ipx_filter_t with ip and generic filters, which are a different format/size?
I believe there is also a bug in the print_abinary() function. It was comparing the vp->length to sizeof(filter), but filter is now a pointer.
No... it's comparing vp->length to SIZEOF_RADFILTER, which is a macro defined to be 32. See earlier code.
In earlier versions of the file, yes I agree. But in CVS HEAD (version 1.45) it compares sizeof(filter). Unless my winCVS has gone wacky. It changed going from 1.42 to 1.43.
I'm really surprised that this code is necessary. I'd like to know a lot more before making any changes.
Understood! The change may not be 100% necessary. In our case, the filters are still applied (Juniper ERX), but produces warning messages indicating that the format may not be correct. So it may be a matter of "correctness" rather than being 100% necessary. regards, Mike
"Mike Mitchell" <mitchell.michael@bigpond.com> wrote:
I'll try to set up a clean install and retest, but I'm pretty sure the behaviour I have observed is as stated. By analysis:
Yes, I understand. But I don't think the *server* has a problem, other than the sizeof(filter) in the CVS head.
In earlier versions of the file, yes I agree. But in CVS HEAD (version 1.45) it compares sizeof(filter). Unless my winCVS has gone wacky. It changed going from 1.42 to 1.43.
I'll fix that.
Understood! The change may not be 100% necessary. In our case, the filters are still applied (Juniper ERX)
Ah... then it's a bug in the Juniper code. Ascend created these things, and the Ascend boxes support 32-byte filters. If Juniper has re-implemented support, they probably are more restrictive than Ascend. In any case, a module could hack the attributes to have the correct length, and do so for specific clients. Alan DeKok.
"Alan DeKok" <aland@nitros9.org> writes:
"Mike Mitchell" <mitchell.michael@bigpond.com> wrote:
Understood! The change may not be 100% necessary. In our case, the filters are still applied (Juniper ERX)
Ah... then it's a bug in the Juniper code.
Ascend created these things, and the Ascend boxes support 32-byte filters. If Juniper has re-implemented support, they probably are more restrictive than Ascend.
I don't know anything about Ascend's implementation, but this is how Juniper documents using the Ascend-Data-Filter attribute on the ERX: http://www.juniper.net/techpubs/software/erx/junose71/swconfig-policy/html/p... which I add up to a variable length from 28 to 108 bytes. Bjørn
=?iso-8859-1?Q?Bj=F8rn_Mork?= <bjorn@mork.no> wrote:
I don't know anything about Ascend's implementation, but this is how Juniper documents using the Ascend-Data-Filter attribute on the ERX:
http://www.juniper.net/techpubs/software/erx/junose71/swconfig-policy/html/p...
OK... after a bit of typing, 1.1.2 should support this. You'll have to use Ascend-Data-Filter = 0x01020304..., but it will work. Alan DeKok.
"Mitchell, Michael J" <Michael.Mitchell@team.telstra.com> wrote... See: http://www.iruntheinter.net/files/cistron/radiusd-cistron-1.6-ascend_filter.... The file is dated 2001, but the copyright is 1994. It's originally from Ascend, and BSD licensed. And it does: ... pair->length = sizeof( RadFilter ); ... which is a fixed size of 32 bytes. So I'm unsure what to think. Alan DeKok.
participants (4)
-
Alan DeKok -
Bjørn Mork -
Mike Mitchell -
Mitchell, Michael J