make_passwd(), rlm_perl and double quotes in the password
Hi, we're seeing failed logins when proxying authentication requests through to another RADIUS server if the password contains a double quote ("). I've debugged the issue a bit, and what happens is that when the password does a roundtrip through the rlm_perl module, the value pair structure ends up with extra bytes at the end so that strlen(vp->data.strvalue) > vp->length. These extra bytes are the result of escaping the double quote before entering the Perl code and then unescaping it afterwards. For instance, the password abcdef" is passed as abcdef\" to the Perl code, and afterwards it becomes abcdef"" with length 7 (so the extra byte is not supposed to hurt AIUI.) Unfortunately make_passwd() rounds 'inlen' up to a multiple of 16 before copying the input string, so the extra bytes end up in the message authenticator of the proxied request. It seems to me that the escaping and unescaping done in rlm_perl via perl_store_vps() -> vp_prints_value() -> librad_safeprint() and get_hv_content() -> pairadd_sv() -> pairmake() -> pairparsevalue() is a bit unnecessary, and that the unescaping could zero out the extra bytes, but the main bug is in make_passwd() IMO. I have reproduced this with a Debian 2.0.4 freeradius package, a Fedora 2.1.3 package, and a current git.freeradius.org checkout (commit ef2a7db5e). I'm attaching a proposed patch to make_passwd(). Please let me know if you need more information, I can easily produce stack traces and the like. Thanks for your work on FreeRADIUS, -- Niko Tyni ntyni@cc.helsinki.fi
Niko Tyni wrote:
I'm attaching a proposed patch to make_passwd(). Please let me know if you need more information, I can easily produce stack traces and the like.
No, it looks like a real issue. I've committed the patch. The rlm_perl code could arguable be updated, too. But that's less of a priority. Alan DeKok.
On Wed, 20 May 2009 14:10:07 +0200, Alan DeKok wrote:
Niko Tyni wrote:
It seems to me that the escaping and unescaping done in rlm_perl via perl_store_vps() -> vp_prints_value() -> librad_safeprint() and get_hv_content() -> pairadd_sv() -> pairmake() -> pairparsevalue() is a bit unnecessary,
I agree; there doesn't seem to be a reason to quote double-quotes (or anything else that fr_print_string() guards against) in rlm_perl.
The rlm_perl code could arguable be updated, too. But that's less of a priority.
Alan, would you accept a patch to get rid of the vp_prints_value() in perl_store_vps? john -- John Morrissey _o /\ ---- __o jwm@horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
On Wed, Sep 02, 2009 at 07:30:37PM -0400, John Morrissey wrote:
On Wed, 20 May 2009 14:10:07 +0200, Alan DeKok wrote:
Niko Tyni wrote:
It seems to me that the escaping and unescaping done in rlm_perl via perl_store_vps() -> vp_prints_value() -> librad_safeprint() and get_hv_content() -> pairadd_sv() -> pairmake() -> pairparsevalue() is a bit unnecessary,
I agree; there doesn't seem to be a reason to quote double-quotes (or anything else that fr_print_string() guards against) in rlm_perl.
The rlm_perl code could arguable be updated, too. But that's less of a priority.
For the benefit of the archives, we're working around this with: if (defined $RAD_REQUEST{'User-Password'}) { $RAD_REQUEST{'User-Password'} =~ s/\\"/"/g; } in our rlm_perl authorize handler. john -- John Morrissey _o /\ ---- __o jwm@horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
On Wed, Sep 02, 2009 at 07:30:37PM -0400, John Morrissey wrote:
On Wed, 20 May 2009 14:10:07 +0200, Alan DeKok wrote:
Niko Tyni wrote:
It seems to me that the escaping and unescaping done in rlm_perl via perl_store_vps() -> vp_prints_value() -> librad_safeprint() and get_hv_content() -> pairadd_sv() -> pairmake() -> pairparsevalue() is a bit unnecessary,
I agree; there doesn't seem to be a reason to quote double-quotes (or anything else that fr_print_string() guards against) in rlm_perl.
The rlm_perl code could arguable be updated, too. But that's less of a priority.
Alan, would you accept a patch to get rid of the vp_prints_value() in perl_store_vps?
Getting rid of vp_prints_value() in perl_store_vps() won't be possible without other backwards-compatibility-breaking changes. Namely, fr_print_string() encodes non-printing characters as octal values (e.g., ASCII 29 -> '\035'). rlm_perl modules may be expecting this behavior. How should this be handled? john -- John Morrissey _o /\ ---- __o jwm@horde.net _-< \_ / \ ---- < \, www.horde.net/ __(_)/_(_)________/ \_______(_) /_(_)__
John Morrissey wrote:
Getting rid of vp_prints_value() in perl_store_vps() won't be possible without other backwards-compatibility-breaking changes.
It looks like the issue is in pairparsevalue(). ALL of the callers to it do escaping of \\. BUT it also does escaping, which is bad.
Namely, fr_print_string() encodes non-printing characters as octal values (e.g., ASCII 29 -> '\035'). rlm_perl modules may be expecting this behavior.
How should this be handled?
I think simply deleting the bad code in pairparsevalue() should be fine. Alan DeKok.
participants (3)
-
Alan DeKok -
John Morrissey -
Niko Tyni