54901 - [security] LDAP Authentication should fail for empty passwords

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[David L. Crow]

When using LDAP and no password is specified in the login screen, the bind
is successful even if the user has a password.  This is because and LDAP bind
with no password (regardless of the bindn value) will be treated as an anonymous
bind.

The Netscape SDK documentation at
<http://docs.iplanet.com/docs/manuals/dirsdk/csdk30/writing.htm>, states

    Note that if you specify a DN but no password, your client will
    bind to the server anonymously. If you want a NULL password to
    be rejected as an incorrect password, you need to write code to
    perform the check before you call the ldap_simple_bind() or
   ldap_simple_bind_s() function.

The following diff shows such a change to CGI.pl:

===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/CGI.pl,v
retrieving revision 1.73
diff -u -r1.73 CGI.pl
--- CGI.pl      2000/09/18 21:29:44     1.73
+++ CGI.pl      2000/10/02 16:18:27
@@ -719,6 +719,21 @@
          exit;
        }
 
+       # if no password was provided, then fail the authentication
+       # while it may be valid to not have an LDAP password, when you
+       # bind without a password (regardless of the binddn value), you
+       # will get an anonymous bind.  I do not know of a way to determine
+       # whether a bind is anonymous or not without making changes to the
+       # LDAP access control settings
+       if ( ! $::FORM{"LDAP_password"} ) {
+         print "Content-type: text/html\n\n";
+         PutHeader("Login Failed");
+        print "You did not provide a password.\n";
+         print "Please click <b>Back</b> and try again.\n";
+         PutFooter();
+         exit;
+       }
+
        # We've got our anonymous bind;  let's look up this user.
        my $dnEntry =
$LDAPconn->search(Param("LDAPBaseDN"),"subtree","uid=".$::FORM{"LDAP_login"});
        if(!$dnEntry) {

Comment 1

[Andreas Franke (gone)]

-> Bugzilla product, General component (LDAP, CGI.pl), reassigning.

Comment 2

[Andreas Franke (gone)]

Oops. Really changing product now.

Comment 3

[Gervase Markham [:gerv]]

Reassigning to LDAP author. Please set this to 2.16 if you feel the patch is
useful and ready to go.

Gerv

Comment 4

[Joe Robins]

Sorry for the delay in reviewing this.  The Job That Pays Me has been eating up 
my time.

This looks good to me.  I've tested it on my dev instance, and it works fine.  
You can go ahead and put it in 2.16.

Comment 5

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 6

[Joe Robins]

This should probably be marked as higher priority than it is...  It's actually a 
major security issue.  If you're using LDAP, this bug allows anyone to log into 
your Bugzilla instance as any user solely by knowing the username, and not the 
password.  Personally, I consider this to be a giant gaping flaw in the 
security, that should be dealt with as quickly as possible.

The patch below is simple, straightforward, and I've tested it on my instance.  
It remedies this problem with minimal changes, so it should have no effects on 
any other functionality.  (One caveat: I've tested it on my system, but applied 
the changes manually instead of using patch, because I've made a lot of other 
changes to my version.  I'm just assuming that the diff is legitimate w.r.t line 
numbers, etc.)

I know it's late in the game to be pushing for this to be in 2.16, so if it's 
too late, it's too late, but if it can get in there, it'd probably be a good 
thing.

Comment 7

[Dave Miller [:justdave]]

Attachment: David Crow's patch

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 59251 [details] [diff] [review]
David Crow's patch

marking first-review on behalf of jmrobins

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 59251 [details] [diff] [review]
David Crow's patch

doesn't break the existing login stuff.

r= justdave

Comment 10

[Dave Miller [:justdave]]

Checked in on the trunk:

/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.125; previous revision: 1.124

checked in on the 2.14.1 branch:

/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.100.2.1; previous revision: 1.100

Comment 11

[Dave Miller [:justdave]]

Hmm, it seems the bulk change thinks I'm not changing anything if all I do is
add names to the CC list, so I guess I have to make a comment.  Anyhow, adding
the representatives from the organizations we know of that support Bugzilla
distributions so they're aware of our upcoming security release

Comment 12

[James A. Laska]

This code is now unnecesary.  Simple change the userbind statement to the
following (see bug#105504):  

       # Now we attempt to bind as the specified user.
       my $retcode = $LDAPconn->simpleAuth($userDN,$::FORM{"LDAP_password"});
       if ( ($::FORM{"LDAP_password"} eq "") ||
            ($userDN eq "") ||
            ($retcode == 0)  )
       {
         $LDAPconn->close();
         print "Content-type: text/html\n\n";
         PutHeader("Login Failed");
         print "The username or password for <font
color=\"blue\">".$::FORM{"LDAP_login"}."</font> was not found in the IBM <a
href=\"http://w3.ibm.com/bluepages/\">bluepages</a> directory.\n";
         print "<br>Please click <b>Back</b> and try again.\n";
         PutFooter();
         exit;
       }

This comes straight from the perLDAP examples.  This will properly fail a login
attempt for a bad passwd/no passwd etc...