Last Comment Bug 785470 - (CVE-2012-3981) [SECURITY] Missing escaping of the username can lead to LDAP injection
(CVE-2012-3981)
: [SECURITY] Missing escaping of the username can lead to LDAP injection
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: User Accounts (show other bugs)
: 2.12
: All All
: -- critical (vote)
: Bugzilla 3.6
Assigned To: Reed Loden [:reed] (use needinfo?)
: default-qa
Mentors:
Depends on:
Blocks: 785112 786364
  Show dependency treegraph
 
Reported: 2012-08-24 12:22 PDT by Frédéric Buclin
Modified: 2012-08-30 13:48 PDT (History)
5 users (show)
LpSolit: approval+
LpSolit: blocking4.4+
LpSolit: approval4.2+
LpSolit: blocking4.2.3+
LpSolit: approval4.0+
LpSolit: blocking4.0.8+
LpSolit: approval3.6+
LpSolit: blocking3.6.11+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch - v1 (untested) (719 bytes, patch)
2012-08-24 12:26 PDT, Reed Loden [:reed] (use needinfo?)
LpSolit: review+
Details | Diff | Review
patch - v2 (643 bytes, patch)
2012-08-28 09:07 PDT, Reed Loden [:reed] (use needinfo?)
LpSolit: review+
Details | Diff | Review

Description Frédéric Buclin 2012-08-24 12:22:32 PDT
When the user enters his credentials, no sanity check is done and his username is passed as is to create the filter which will be passed to $self->ldap->search(), see Bugzilla::Auth::Verify::LDAP::_bz_search_params(). This can lead to LDAP injection, as exploited successfully in bug 785112.

The username must be escaped, e.g. by using Net::LDAP::Util::escape_filter_value().

This problem exists since LDAP authentication has been implemented in Bugzilla 2.12, see bug 51185.
Comment 1 Reed Loden [:reed] (use needinfo?) 2012-08-24 12:26:55 PDT
Created attachment 655094 [details] [diff] [review]
patch - v1 (untested)

something like this? :)
Comment 2 Frédéric Buclin 2012-08-24 12:32:53 PDT
Comment on attachment 655094 [details] [diff] [review]
patch - v1 (untested)

>-                      . "=$username)"
>+                      . '=' . escape_filter_value($username) . ')'

Nit: instead of this, we could write:

    $username = escape_filter_value($username);

before building the filter. The code would be easier to parse.


Anyway, I had the same fix in mind, so r=LpSolit from a code point of view, but I'm totally unable to test this patch as I have no LDAP installation to play with. So before being checked in, it needs to be tested (we cannot afford to break LDAP authentication on branches).
Comment 3 Frédéric Buclin 2012-08-27 10:55:45 PDT
CC'ing our LDAP guru. :) manu, could you look at the patch, please?
Comment 4 Emmanuel Seyman 2012-08-28 08:15:37 PDT
I've tested the patch and it doesn't break LDAP authentification (mine, at least).
Like LpSolit, I'm far more partiel to adding:

    $username = escape_filter_value($username);

before the return call. It's much more readable that way.
Comment 5 Frédéric Buclin 2012-08-28 08:25:41 PDT
Comment on attachment 655094 [details] [diff] [review]
patch - v1 (untested)

OK, manu's testing is enough for me. No need for another review. Thanks, manu! :)
Comment 6 Frédéric Buclin 2012-08-28 08:27:26 PDT
reed, we need a CVE number for this one.
Comment 7 Reed Loden [:reed] (use needinfo?) 2012-08-28 09:07:39 PDT
Created attachment 656030 [details] [diff] [review]
patch - v2

For check-in.
Comment 8 Emmanuel Seyman 2012-08-28 09:11:35 PDT
I've discussed this a bit with LpSolit and I don't how this allows you to inject data into the LDAP directory.
At best, you can probably invalid the filter that the Bugzilla administrator put in place but that's a stretch and you'll still need the password of an account to login.
Comment 9 Frédéric Buclin 2012-08-28 10:00:35 PDT
Comment on attachment 656030 [details] [diff] [review]
patch - v2

r=LpSolit
Comment 10 Frédéric Buclin 2012-08-30 11:30:58 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Auth/Verify/LDAP.pm
Committed revision 8370.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/Auth/Verify/LDAP.pm
Committed revision 8132.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Auth/Verify/LDAP.pm
Committed revision 7721.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/Auth/Verify/LDAP.pm
Committed revision 7297.

Note You need to log in before you can comment on or make changes to this bug.