Hello List, I believe I found a bug in token.c, more precisely in static FR_TOKEN getthing(...) The exact point is the part handling backslahes: --- cut --- if (quote && (*p == '\\')) { p++; switch(*p) { case 'r': *s++ = '\r'; break; case 'n': *s++ = '\n'; break; case 't': *s++ = '\t'; break; case '\0': *s++ = '\\'; p--; /* force EOS */ break; default: if (*p >= '0' && *p <= '9' && sscanf(p, "%3o", &x) == 1) { *s++ = x; p += 2; } else *s++ = *p; break; } p++; continue; } --- cut --- This piece of code removes all backslashes from the input string, which gives incorrect results when parsing strings for regular expressions with excaped characters. At a first glance, the fix is easy: --- cut --- default: if (*p >= '0' && *p <= '9' && sscanf(p, "%3o", &x) == 1) { *s++ = x; p += 2; } else { *s++ = '\\'; /* copy the '\' too */ *s++ = *p; } break; --- cut --- which solves the problem. On the other hand, I'm not sure if it breaks anything else. Is there a reason to remove backslashes on purpose? Or is this really a bug? Regards, Oliver Schröder
Oliver Schröder wrote:
This piece of code removes all backslashes from the input string, which gives incorrect results when parsing strings for regular expressions with excaped characters.
*Why*? Do you have an example?
On the other hand, I'm not sure if it breaks anything else. Is there a reason to remove backslashes on purpose? Or is this really a bug?
The function gets a token from a string. The token can is *parsed*, which means interpreting backslashes. They're not "removed", they're parsed and understood. So yes, there's a reason to do this. Alan DeKok.
I'm currently evaluating if we are able to replace our comercial radius with freeradius. Our user base is very large and usernames are composed in quite different schemes. Some usernames have a realm, some have no realm. Some are authenticated against ldap, some against a user file, others get proxied or tunneled etc. Some usernames have the form somthing**somethingelse**other etc. My current approach is to make heavy use of the "hints" file, in which I attach different attributes to the requests, which in turn get evaluated via unlang. In most cases the distinction of the different cases is based on a username/nas combination and to correctly handle the different usernames I use regexps in the hints file. One simple example is this: DEFAULT User-Name =~ "(.*\.de\.de$)" Hint := "Blacklist" It is aimed to match User-Names like "x@whatever.de.de". In this case the quoted string should not be interpreted but given to regcomp as it is. regards, Oliver Schröder Am 25.07.2011 13:53, schrieb Alan DeKok:
Oliver Schröder wrote:
This piece of code removes all backslashes from the input string, which gives incorrect results when parsing strings for regular expressions with excaped characters.
*Why*?
Do you have an example?
On the other hand, I'm not sure if it breaks anything else. Is there a reason to remove backslashes on purpose? Or is this really a bug?
The function gets a token from a string. The token can is *parsed*, which means interpreting backslashes. They're not "removed", they're parsed and understood.
So yes, there's a reason to do this.
Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
On 07/26/2011 07:14 AM, Oliver Schröder wrote:
DEFAULT User-Name =~ "(.*\.de\.de$)" Hint := "Blacklist"
It is aimed to match User-Names like "x@whatever.de.de". In this case the quoted string should not be interpreted but given to regcomp as it is.
As has been explained to you, that isn't how FreeRADIUS config parser works. The files are parsed and tokenised before syntax is applied; there's no way for the parser to "know" it's a regexp. You need to write: DEFAULT User-Name =~ "(.*\\.de\\.de$)" ...as I also mentioned, this is somewhat common; Exim for example requires that you do this in it's config file. This is not unusual.
Oliver Schröder wrote:
I'm currently evaluating if we are able to replace our comercial radius with freeradius.
The answer is, of course, "yes". FreeRADIUS does more than any of the commecial servers, and is easier to use.
My current approach is to make heavy use of the "hints" file, in which I attach different attributes to the requests, which in turn get evaluated via unlang. In most cases the distinction of the different cases is based on a username/nas combination and to correctly handle the different usernames I use regexps in the hints file. One simple example is this:
DEFAULT User-Name =~ "(.*\.de\.de$)" Hint := "Blacklist"
It is aimed to match User-Names like "x@whatever.de.de". In this case the quoted string should not be interpreted but given to regcomp as it is.
Well, no. Look at what you've typed: It's a double-quoted string, not a regex. The parser reads the double-quoted string, removes the backslashes to get what you want (including \"), and then passes the rest to the regex engine. So you need two backslashes. Your patch to token.c would break *all* interpretation of double-quoted strings. The explanation is clear, even if the resulting string looks weird. Alan DeKok.
On 25/07/11 09:45, Oliver Schröder wrote:
This piece of code removes all backslashes from the input string, which gives incorrect results when parsing strings for regular expressions with excaped characters. At a first glance, the fix is easy:
No. You have to double-escape everything. This is common (e.g. Exim does the same thing with it's config file) if tedious. If you want the following regexp: \d+\. ...you need to write: \\d+\\. ...in the FreeRADIUS config. From what I can see, you're suggesting that: \X ...where X is not specifically handled should leave the "\" unchanged. Maybe, maybe not - I've seen string parsers that do both. There is no "right" answer. FreeRADIUS expects backslashes to precede known escapes or be themselves backslashed.
participants (3)
-
Alan DeKok -
Oliver Schröder -
Phil Mayers