Summary

Fix security flaw and general design of the LDAP authentication backend (repost)

Review Request #610 — Created Oct. 24, 2008 and discarded

Information

Owner

mrb

Repository

Review Board SVN (deprecated)

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

(This is a correct repost of http://reviews.review-board.org/r/608/ which I incorrectly uploaded.)


There is a serious bug in the current LDAP auth backend: it is possible to authenticate as any user by entering an empty password !

This is because the password is used in a bind request, but in LDAP a bind request with an empty password is equivalent to an anonymous bind request, and most LDAP servers allow anonymous bind requests.

But more generally speaking, the design of the LDAP backend is flawed and inflexible. For example with a standard Windows domain controller, a typical DN might look like:
  CN=John Smith,OU=engineering,DC=company,DC=com

So if the following User Mask is specified in Review Board's LDAP settings:
  CN=%s,OU=engineering,DC=company,DC=com

Endusers are forced to type their full name ("John Smith") as their username, instead of the more practical SAM Account Name (which could be "jsmith" -- Windows domain controllers store it in the sAMAccountName attribute).

The right way to implement LDAP authentication is to do what lighttpd's mod_auth does for example (see http://redmine.lighttpd.net/wiki/lighttpd/Docs:ModAuth).  Basically when an enduser enters <enduser-login> and <enduser-passwd>, Review Board should:

(1) Immediately fail the authentication if <enduser-passwd> is empty.
(2) Bind anonymously, or with a given DN/password (<bind-DN> and <bind-passwd>).
(3) Find the enduser's DN <enduser-DN> by doing a SUBTREE search of a <base-DN> (eg. "OU=engineering,DC=company,DC=com") with a filter <filter> built from <enduser-login> (eg. "sAMAccountName=<enduser-login>").
(4) Retrieve <enduser-DN> from the SUBTREE search.
(5) Attempt to bind with <enduser-DN> and <enduser-passwd> to verify the validity of the auth credentials.

So a Review Board administrator needs to specify up to 4 LDAP parameters in addition to the LDAP server:

  <bind-DN>     (optional)
  <bind-passwd> (optional)
  <base-DN>     (typically something like OU=foo,DC=company,DC=com)
  <filter>      (typically sAMAccountName=%s for Windows-based domain controllers)

I have written a patch to implement all of this correctly. I have repurposed the 3 existing parameters as follow:

  Anonymous User Mask:     <bind-DN>
  Anonymous User Password: <bind-passwd>
  User Mask:               <filter>

I don't know Django that well so I have hard-coded <base-DN> in my patch, but ultimately it should be added to the LDAP configuration page /admin/settings/general/. And the description of these settings needs to be updated as well.

Let me know what you think.

-marc

Testing Done

I use this patch on a Review Board server (SVN r1551), the LDAP server is a Windows 2003 DC, and it seems to work well.

mrb Oct. 27, 2008, 4:16 a.m.

Thanks for the review, all your comments are valid, unfortunately it will be a while before I can take the time to completely finish that work...

chipx86 Sept. 10, 2009, 7:48 p.m.

Hi mrb,

It's been a while since there's been activity on this. Just wanted to check in, see if you've had time to finish this. We don't have a good infrastructure for testing this, and the LDAP code has changed since this diff.

mrb Sept. 13, 2009, 7:40 p.m.

I have not taken the time to update the patch, and I don't think I can. Sorry. I still use the original version of my patch with hardcoded values in production... Someone would have to take it over to finish it up.

/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Should just be:

  if not password:
```
/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Blank line before/after blocks.
```

/trunk/reviewboard/accounts/backends.py (Diff revision 1)

Params should be indented to the same column.

/trunk/reviewboard/accounts/backends.py (Diff revision 1)

I'm unsure about this. Shouldn't we be using what the admin provided here? Will this string even work?

mrb Oct. 27, 2008, 4:13 a.m.

No it won't work (on my Review Board server, I hardcoded it to the value I needed). As I explained in the description, this patch is work in progress, base_dn  needs to be configurable from the UI.

/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Should be aligned.
```

/trunk/reviewboard/accounts/backends.py (Diff revision 1)

passwd[0][0] can go in simple_bind_s directly.

/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Same question as above.
```
/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Alignment.
```

I would also suggest that when using an LDAP back-end, the mapping from username -> email needs to be more sophisticated. Simply doing shortusername@domain will not work in many environments. You should have an option to retrieve each user's email address from the directory using LDAP. For example, Active Directory has an additional attribute called "mail" which stores the email address.

cloder Dec. 2, 2008, 1:37 a.m.

This email feature request has been submitted as a separate diff by somebody else: http://reviews.review-board.org/r/652/