We've recently moved our servers to FreeRadius 2, and we've been having reports of some problems with our wireless service. I investigated, and it seems the problem is the following stanza from the "inner-tunnel" config, which I just copied over from the default configs: post-auth { update outer.reply { User-Name = "%{request:User-Name}" } } ...specifically, it seems that the xlat of this string is treating embedded "\" as escape characters. For example; I was unable to stop the radius server, but a tshark capture of a failing user shows me (C: is radius client i.e. the NAS, S: is packet from the radius server); note the "\\" are tshark escaping the data, the actual username is "IC\nyshir" C: access-request IC\\nyshir S: access-challenge C: access-request IC\\nyshir S: access-challenge C: access-request IC\\nyshir S: access-challenge C: access-request IC\\nyshir S: access-challenge C: access-request IC\\nyshir S: access-challenge C: access-request IC\\nyshir S: access-challenge C: access-request IC\\nyshir S: access-challenge C: access-request IC\\nyshir S: access-challenge C: access-request IC\\nyshir S: access-challenge IC\x0ayshir C: access-request IC\x0ayshir S: access-reject Note that the final "access-challenge" sends back IC<newline>ayshir; the AP then carries this over to the next request, which fails. I've solved that problem by removing the "update outer.reply", which leads me to a 2nd question - given that the "eap" module does this anyway (and since it uses a dumb "memcpy", correctly) why is that unlang statement there in the sample configs? Is it necessary? One final thing; can I suggest the attached patch (though it should probably escape the data, since it comes from the user)
Phil Mayers wrote:
We've recently moved our servers to FreeRadius 2, and we've been having reports of some problems with our wireless service. ... ...specifically, it seems that the xlat of this string is treating embedded "\" as escape characters.
That needs to be fixed. I'll take a look next week, as I'm away at a conference right now.
I've solved that problem by removing the "update outer.reply", which leads me to a 2nd question - given that the "eap" module does this anyway (and since it uses a dumb "memcpy", correctly) why is that unlang statement there in the sample configs? Is it necessary?
No.
One final thing; can I suggest the attached patch (though it should probably escape the data, since it comes from the user)
Sure. Send it via git-format-patch, and it can be committed with your name on it. Alan DeKok.
This is a simple version of the patch, just logging the identities --- src/modules/rlm_eap/eap.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/modules/rlm_eap/eap.c b/src/modules/rlm_eap/eap.c index e9094c4..e947844 100644 --- a/src/modules/rlm_eap/eap.c +++ b/src/modules/rlm_eap/eap.c @@ -1025,7 +1025,7 @@ EAP_HANDLER *eap_handler(rlm_eap_t *inst, eap_packet_t **eap_packet_p, */ if (strncmp(handler->identity, vp->vp_strvalue, MAX_STRING_LEN) != 0) { - radlog(L_ERR, "rlm_eap: Identity does not match User-Name. Authentication failed."); + radlog(L_ERR, "rlm_eap: Identity %s does not match User-Name %s. Authentication failed.", handler->identity, vp->vp_strvalue); free(*eap_packet_p); *eap_packet_p = NULL; return NULL; -- 1.5.4.1
A more complex version replacing the previous version; this logs the escaped username, possibly useful if it contains various binary nonsense etc. --- src/modules/rlm_eap/eap.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/modules/rlm_eap/eap.c b/src/modules/rlm_eap/eap.c index e947844..6e2367e 100644 --- a/src/modules/rlm_eap/eap.c +++ b/src/modules/rlm_eap/eap.c @@ -953,6 +953,8 @@ EAP_HANDLER *eap_handler(rlm_eap_t *inst, eap_packet_t **eap_packet_p, eap_packet_t *eap_packet = *eap_packet_p; VALUE_PAIR *vp; + char ident_safe[MAX_STRING_LEN+1], username_safe[MAX_STRING_LEN+1]; + /* * Ensure it's a valid EAP-Request, or EAP-Response. */ @@ -1025,7 +1027,10 @@ EAP_HANDLER *eap_handler(rlm_eap_t *inst, eap_packet_t **eap_packet_p, */ if (strncmp(handler->identity, vp->vp_strvalue, MAX_STRING_LEN) != 0) { - radlog(L_ERR, "rlm_eap: Identity %s does not match User-Name %s. Authentication failed.", handler->identity, vp->vp_strvalue); + librad_safeprint(handler->identity, strlen(handler->identity), ident_safe, MAX_STRING_LEN); + librad_safeprint(vp->vp_strvalue, strlen(vp->vp_strvalue), username_safe, MAX_STRING_LEN); + + radlog(L_ERR, "rlm_eap: Identity %s does not match User-Name %s. Authentication failed.", ident_safe, username_safe); free(*eap_packet_p); *eap_packet_p = NULL; return NULL; @@ -1081,7 +1086,10 @@ EAP_HANDLER *eap_handler(rlm_eap_t *inst, eap_packet_t **eap_packet_p, */ if (strncmp(handler->identity, vp->vp_strvalue, MAX_STRING_LEN) != 0) { - radlog(L_ERR, "rlm_eap: Identity does not match User-Name, setting from EAP Identity."); + librad_safeprint(handler->identity, strlen(handler->identity), ident_safe, MAX_STRING_LEN); + librad_safeprint(vp->vp_strvalue, strlen(vp->vp_strvalue), username_safe, MAX_STRING_LEN); + + radlog(L_ERR, "rlm_eap: Identity %s does not match User-Name %s. Authentication failed.", ident_safe, username_safe); free(*eap_packet_p); *eap_packet_p = NULL; eap_handler_free(handler); -- 1.5.4.1
Alan DeKok wrote:
Phil Mayers wrote:
We've recently moved our servers to FreeRadius 2, and we've been having reports of some problems with our wireless service. ... ...specifically, it seems that the xlat of this string is treating embedded "\" as escape characters.
That needs to be fixed. I'll take a look next week, as I'm away at a conference right now.
I've solved that problem by removing the "update outer.reply", which leads me to a 2nd question - given that the "eap" module does this anyway (and since it uses a dumb "memcpy", correctly) why is that unlang statement there in the sample configs? Is it necessary?
No.
One final thing; can I suggest the attached patch (though it should probably escape the data, since it comes from the user)
Sure. Send it via git-format-patch, and it can be committed with your name on it.
Hmm. I sort-of tried that; getting it to integrate with Thunderbird didn't look easy, I could send it with "mutt -H" but I needed to extract the message-id to get the threading right. Ah well - something to play with.
Alan DeKok wrote:
Phil Mayers wrote:
We've recently moved our servers to FreeRadius 2, and we've been having reports of some problems with our wireless service. ... ...specifically, it seems that the xlat of this string is treating embedded "\" as escape characters.
That needs to be fixed. I'll take a look next week, as I'm away at a conference right now.
I'm also seeing this problem with rlm_sql_log and rlm_sqlippool it seems, though I'm amazed I haven't seen it before. I've had a dig in the guts of radius_xlat (yikes, complex) and done some testing with "user\random" e.g.: update reply { # works My-Attr-1 = "%{User-Name}" # works My-Attr-1 = "%u" # works My-Attr-1 = "%{Stripped-User-Name:-%{User-Name:-none}}" # fails, "user\random" turns to "user<cr>andom" My-Attr-1 = "%{SQL-User-Name}" } ...so I think this means the issue must be in lib/valuepair.c, specifically the "pairparsevalue" function (called from pairmake, called from sql_set_user)? Similarly I presume setting the "User-Name" in the reply items from unlang calls pairmake and the same code path. I'm not sure what the best way to proceed with regards fixing it - most of the ideas I had were hacks. If you have any pointers I'm happy to take a look.
Phil Mayers wrote:
I've had a dig in the guts of radius_xlat (yikes, complex) and done some testing with "user\random" e.g.:
update reply { # works My-Attr-1 = "%{User-Name}"
How is that different from %{request:User-Name}, which didn't seem to work?
# fails, "user\random" turns to "user<cr>andom" My-Attr-1 = "%{SQL-User-Name}"
I'd like to see the debug output... The SQL-User-Name should be set to *exactly* "user\random". MAYBE the problem is that instead, it's set to "user<cr>andom". Knowing *where* the problem occurs would be useful. i.e. I don't think it's in the expansion of "%{foo}", and assignment to My-Attr-1. If it was, then "%{User-Name}" wouldn't work, either. Alan DeKok.
Alan DeKok wrote:
Phil Mayers wrote:
I've had a dig in the guts of radius_xlat (yikes, complex) and done some testing with "user\random" e.g.:
update reply { # works My-Attr-1 = "%{User-Name}"
How is that different from %{request:User-Name}, which didn't seem to work?
Hmm. That's a good point.
# fails, "user\random" turns to "user<cr>andom" My-Attr-1 = "%{SQL-User-Name}"
I'd like to see the debug output...
Sure: Ready to process requests. rad_recv: Access-Request packet from host 127.0.0.1 port 32938, id=252, length=41 User-Name = "IC\\rmtw" NAS-Port = 5060 NAS-IP-Address = 127.0.0.1 +- entering group authorize expand: %{NAS-IP-Address} -> 127.0.0.1 expand: %{NAS-IP-Address} -> 127.0.0.1 hints: Matched DEFAULT at 35 expand: %{Client-IP-Address} -> 127.0.0.1 expand: %{Client-IP-Address} -> 127.0.0.1 expand: %{Client-IP-Address} -> 127.0.0.1 expand: %{Client-IP-Address} -> 127.0.0.1 expand: %{Client-IP-Address} -> 127.0.0.1 ++[preprocess] returns ok expand: %{User-Name} -> IC\rmtw WARNING: Deprecated conditional expansion ":-". See "man unlang" for details WARNING: Deprecated conditional expansion ":-". See "man unlang" for details expand: %{Stripped-User-Name:-%{User-Name:-none}} -> IC\rmtw rlm_sql (sql): - sql_xlat WARNING: Deprecated conditional expansion ":-". See "man unlang" for details WARNING: Deprecated conditional expansion ":-". See "man unlang" for details expand: %{Stripped-User-Name:-%{User-Name:-none}} -> IC\rmtw rlm_sql (sql): sql_set_user escaped user --> 'IC\rmtw' expand: select '%{SQL-User-Name}' -> select 'IC=0Dmtww' rlm_sql (sql): Reserving sql socket id: 31 rlm_sql_postgresql: Status: PGRES_TUPLES_OK rlm_sql_postgresql: query affected rows = 1 , fields = 1 rlm_sql (sql): - sql_xlat finished rlm_sql (sql): Released sql socket id: 31 expand: %{sql:select '%{SQL-User-Name}'} -> IC=0Dmtww ++[reply] returns ok ++[control] returns ok rad_check_password: Found Auth-Type Accept rad_check_password: Auth-Type = Accept, accepting the user Sending Access-Accept of id 252 to 127.0.0.1 port 32938 ICzone = "IC\rmtw" ICbuilding = "IC\rmtw" ICgroup = "IC=0Dmtww" Finished request 0. Going to the next request Waking up in 4.9 seconds. Cleaning up request 0 ID 252 with timestamp +0 Ready to process requests. Ok, so I'm wrong - looking closer, the radius_xlat inside "sql_set_user" is definitely succeeding: rlm_sql (sql): sql_set_user escaped user --> 'IC\rmtw' ...but then the final query which is passed into SQL is wrong: expand: select '%{SQL-User-Name}' -> select 'IC=0Dmtww' ...so maybe it's the bit that handles xlatname:string?
Phil Mayers wrote: ...
expand: %{Stripped-User-Name:-%{User-Name:-none}} -> IC\rmtw rlm_sql (sql): sql_set_user escaped user --> 'IC\rmtw' expand: select '%{SQL-User-Name}' -> select 'IC=0Dmtww'
...
...so maybe it's the bit that handles xlatname:string?
I think it's the "sql_set_user" function. It should create the SQL-User-Name attribute, and then copy the text string *verbatim*. It's the extra layer of parsing through pairmake() that's turning \r into a CR. Alan DeKok.
Alan DeKok wrote:
Phil Mayers wrote: ...
expand: %{Stripped-User-Name:-%{User-Name:-none}} -> IC\rmtw rlm_sql (sql): sql_set_user escaped user --> 'IC\rmtw' expand: select '%{SQL-User-Name}' -> select 'IC=0Dmtww'
...
...so maybe it's the bit that handles xlatname:string?
I think it's the "sql_set_user" function. It should create the SQL-User-Name attribute, and then copy the text string *verbatim*. It's the extra layer of parsing through pairmake() that's turning \r into a CR.
It's also appending a 2nd "w" on the end, almost as if something is re-using the original string buffer: "IC\rmtw" ...and writing "IC<cr>mtw" into it, giving: "IC<cr>mtww" I can work up a patch for the sql_set_user functions in rlm_sql and rlm_sql_log; I take it then this is un-related to the unlang issue?
Phil Mayers wrote:
It's also appending a 2nd "w" on the end, almost as if something is re-using the original string buffer:
"IC\rmtw"
...and writing "IC<cr>mtw" into it, giving:
"IC<cr>mtww"
That looks like a separate bug. Try the "valuepair.diff" patch first.
I can work up a patch for the sql_set_user functions in rlm_sql and rlm_sql_log; I take it then this is un-related to the unlang issue?
Yes. Try the sql.diff file for the \r bug. Alan DeKok.
participants (2)
-
Alan DeKok -
Phil Mayers