Possible Bug in valuepair / rlm_file etc etc.
I have found a potential bug in the way rlm_file or valuepair does the xlat of %{var} items. These are supposedly dynamic, being based on a per-packet replacement. The following example can be done a different way but its the simplest example I can think of ;-) Take the following from a users file (X-MyCLI filled in from some db in an earlier module). DEFAULT X-MyCLI != "%{Calling-Station-Id}", Auth-Type:="Reject" The db of valid CLI's might contain. user cli --------------- one 12345 two 54321 If you do two queries that should auth one thats sending: username=one, callingid=12345; the other username=two, callingid=54321. Only the first works the second failing because the in memory config for the check will have changed to read X-MyCLI != 12345 ...!!! Looking at the code this appears to be due to paircmp expanding the %{} on the structure passed to it (a potentially bad idea for so many reasons). The best solution, I suspect, is to copy the data sent to paircmp and work on the copy and not the original. The copy can be done on a per-pair way and if deemed expensive only for things that need xlat. .............................................. In sudo code: for (check; check->next ; ..... memcpy(&blah, check, sizeof(check)); Do the comparison using blah instead of check. memcpy(check, &blah, sizeof(check)); ............................................... I don't have a detailed patch yet as I have only done half the work required to avoid trashing the config but thus far it does fix my case. PS: Although I have only looked at 1.0.2 this appears to hold true of the CVS head as well. Am I missing something here and if not I'll be back soon with a complete patch (Currently I just save the old check_item and put it back which is fine in a single thread but not so clever in a threaded system). -- Alister Winfield
Alister Winfield <alister.winfield@uk.easynet.net> wrote:
I have found a potential bug in the way rlm_file or valuepair does the xlat of %{var} items. These are supposedly dynamic, being based on a per-packet replacement. The following example can be done a different way but its the simplest example I can think of ;-)
Yeah, it's a bug. The "users" file originally didn't have variable expansion, and once we added it, things like this happen.
Am I missing something here and if not I'll be back soon with a complete patch (Currently I just save the old check_item and put it back which is fine in a single thread but not so clever in a threaded system).
Rather, if flags.xlat is set, do: VALUE_PAIR *new_check = rad_malloc(sizeof(*new_check)); memcpy(new_check, check, sizeof(*new_check)); new_check->next = NULL paircmp(... new_check...) pairfree(&new_check); Alan DeKok.
On Fri, 2005-10-21 at 13:51 -0400, Alan DeKok wrote:
Alister Winfield <alister.winfield@uk.easynet.net> wrote:
I have found a potential bug in the way rlm_file or valuepair does the xlat of %{var} items. These are supposedly dynamic, being based on a per-packet replacement. The following example can be done a different way but its the simplest example I can think of ;-)
Yeah, it's a bug. The "users" file originally didn't have variable expansion, and once we added it, things like this happen.
Am I missing something here and if not I'll be back soon with a complete patch (Currently I just save the old check_item and put it back which is fine in a single thread but not so clever in a threaded system).
Rather, if flags.xlat is set, do:
VALUE_PAIR *new_check = rad_malloc(sizeof(*new_check)); memcpy(new_check, check, sizeof(*new_check)); new_check->next = NULL paircmp(... new_check...) pairfree(&new_check);
Alan DeKok.
Are you suggesting this in valuepair.c inside the loop that walks the check items, or in rlm_file which I suspect would require the full check items list to be copied to keep everything sane? I suspect you mean in valuepair.c but thought it wise to ask ;). Anyway just noticed another unfortunate feature in rlm_file.c. The != and !~ tests only notice if the very first matching attribute doesn't match the value / regex but there is no way of asking the question does this attribute value pair NOT exist anywhere in the packet. I have a fix for this which involves effectively doing a positive match then inverting the response if required. -- Alister Winfield.
Alister Winfield wrote:
Anyway just noticed another unfortunate feature in rlm_file.c.
function? Do you mean in src/main/valuepair.c paircmp() (cvs renamed to paircompare() )?
The != and !~ tests only notice if the very first matching attribute doesn't match the value / regex but there is no way of asking the question does this attribute value pair NOT exist anywhere in the packet. I have a fix for this which involves effectively doing a positive match then inverting the response if required.
paircmp() loops every av. Perphaps if you posted your fix simple schmoes like me can take a look. It wont actually buy you anything if I do, but perhaps some others might be interested.
-- Alister Winfield.
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
Alister Winfield <alister.winfield@uk.easynet.net> wrote:
Are you suggesting this in valuepair.c inside the loop that walks the check items, or in rlm_file which I suspect would require the full check items list to be copied to keep everything sane? I suspect you mean in valuepair.c but thought it wise to ask ;).
valuepair.c. That code is *also* used by other modules in FreeRADIUS, so editing rlm_files wouldn't be correct.
Anyway just noticed another unfortunate feature in rlm_file.c. The != and !~ tests only notice if the very first matching attribute doesn't match the value / regex but there is no way of asking the question does this attribute value pair NOT exist anywhere in the packet.
Huh? !* and =* Alan DeKok.
Sent this before but I had a mixed up valuepair.c which ment I couldn't just get a diff ;-(. Now I have found time to unpick the changes to make a sane copy of valuepair and, therefore, a good looking diff. Re-iterating the problem... When trying to get a not equals match the existing code, up to 1.0.5 and probably in the later CVS HEAD, only notices the first value, which, if it doesn't match returns a true response. Should you actually have many values for the same attribute then you may want the test to only return true if none of the values of that attribute are a match. To get the effect of finding that a value doesn't appear at all for a given attribute this patch avoids negative tests and instead does a positive match and inverts the response at the exit points in the function as required. BTW. Hopefully this doesn't touch the current behaviour for anything else certainly my tests suggest its fine. This patch looks good for the latest released version 1.0.5. -- Alister Winfield.
Alister Winfield wrote:
Sent this before but I had a mixed up valuepair.c which ment I couldn't just get a diff ;-(. Now I have found time to unpick the changes to make a sane copy of valuepair and, therefore, a good looking diff.
Re-iterating the problem... When trying to get a not equals match the existing code, up to 1.0.5 and probably in the later CVS HEAD, only notices the first value, which, if it doesn't match returns a true response. Should you actually have many values for the same attribute then you may want the test to only return true if none of the values of that attribute are a match.
To get the effect of finding that a value doesn't appear at all for a given attribute this patch avoids negative tests and instead does a positive match and inverts the response at the exit points in the function as required. BTW. Hopefully this doesn't touch the current behaviour for anything else certainly my tests suggest its fine.
This patch looks good for the latest released version 1.0.5.
--
Not certain I understand what you mean, Tell me if the attached patch looks like what you want and I will test it. --- valuepair.c.orig 2005-11-04 15:27:46.000000000 -0500 +++ valuepair.c.JM 2005-11-04 15:34:08.000000000 -0500 @@ -284,7 +284,7 @@ * to not find it, then we succeeded. */ if (check_item->operator == T_OP_CMP_FALSE) - return 0; + continue; else return -1; } @@ -295,7 +295,10 @@ */ if (check_item->operator == T_OP_CMP_FALSE) return -1; - + + /* no comparison needed if it exists */ + if (check_item->operator == T_OP_CMP_TRUE) + continue; /* * We've got to xlat the string before doing @@ -327,14 +330,14 @@ radlog(L_ERR, "Invalid operator for item %s: " "reverting to '=='", check_item->name); /*FALLTHRU*/ - case T_OP_CMP_TRUE: /* compare always == 0 */ - case T_OP_CMP_FALSE: /* compare always == 1 */ case T_OP_CMP_EQ: if (compare != 0) result = -1; break; case T_OP_NE: - if (compare == 0) result = -1; + /* != only succeeds if NO reply values match*/ + if (compare == 0) return -1; + result = -1; /*check the rest for no match */ break; case T_OP_LT: @@ -428,7 +431,9 @@ compare = regexec(®, (char *)auth_item->strvalue, 0, NULL, 0); regfree(®); - if (compare == 0) result = -1; + /* !~ only succeeds if NO matches are found in reply pair */ + if (compare == 0) return -1; + result = -1; /*check the rest for no match */ break; #endif
participants (3)
-
Alan DeKok -
Alister Winfield -
Joe Maimon