Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 785470 (CVE-2012-3981)

[SECURITY] Missing escaping of the username can lead to LDAP injection

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
User Accounts
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Frédéric Buclin, Assigned: reed)

Tracking

(Blocks: 1 bug)

2.12
Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +
blocking4.4 +
approval4.2 +
blocking4.2.3 +
approval4.0 +
blocking4.0.8 +
approval3.6 +
blocking3.6.11 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
Flags: blocking4.4+
Flags: blocking4.2.3+
Flags: blocking4.0.8+
Flags: blocking3.6.11+
(Assignee)

Comment 1

5 years ago
Created attachment 655094 [details] [diff] [review]
patch - v1 (untested)

something like this? :)
Attachment #655094 - Flags: review?(LpSolit)
(Reporter)

Comment 2

5 years ago
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).
Attachment #655094 - Flags: review?(LpSolit)
Attachment #655094 - Flags: review?
Attachment #655094 - Flags: review+
(Reporter)

Updated

5 years ago
Assignee: user-accounts → reed
Status: NEW → ASSIGNED
(Reporter)

Comment 3

5 years ago
CC'ing our LDAP guru. :) manu, could you look at the patch, please?

Comment 4

5 years ago
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.
(Reporter)

Comment 5

5 years ago
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! :)
Attachment #655094 - Flags: review?
(Reporter)

Comment 6

5 years ago
reed, we need a CVE number for this one.
Flags: approval?
Flags: approval4.2?
Flags: approval4.0?
Flags: approval3.6?
Summary: [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
(Assignee)

Updated

5 years ago
Alias: CVE-2012-3981
Summary: (CVE-2012-3981)[SECURITY] Missing escaping of the username can lead to LDAP injection → [SECURITY] Missing escaping of the username can lead to LDAP injection
(Assignee)

Comment 7

5 years ago
Created attachment 656030 [details] [diff] [review]
patch - v2

For check-in.
Attachment #655094 - Attachment is obsolete: true
Attachment #656030 - Flags: review?(LpSolit)
(Assignee)

Updated

5 years ago
Attachment #656030 - Attachment is patch: true

Comment 8

5 years ago
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.
Alias: CVE-2012-3981

Updated

5 years ago
Alias: CVE-2012-3981
(Reporter)

Comment 9

5 years ago
Comment on attachment 656030 [details] [diff] [review]
patch - v2

r=LpSolit
Attachment #656030 - Flags: review?(LpSolit) → review+
(Reporter)

Updated

5 years ago
Blocks: 786364
(Reporter)

Updated

5 years ago
Flags: approval?
Flags: approval4.2?
Flags: approval4.2+
Flags: approval4.0?
Flags: approval4.0+
Flags: approval3.6?
Flags: approval3.6+
Flags: approval+
(Reporter)

Comment 10

5 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.