[security] LDAP Authentication should fail for empty passwords

RESOLVED FIXED in Bugzilla 2.16

Status

()

Bugzilla
Bugzilla-General
P1
blocker
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: David L. Crow, Assigned: Joe Robins)

Tracking

unspecified
Bugzilla 2.16
x86
Linux

Details

Attachments

(1 attachment)

(Reporter)

Description

17 years ago
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) {
Keywords: patch, review
Target Milestone: --- → Bugzilla 2.16

Comment 1

16 years ago
-> Bugzilla product, General component (LDAP, CGI.pl), reassigning.
Assignee: tara → justdave

Comment 2

16 years ago
Oops. Really changing product now.
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: other → unspecified
Reassigning to LDAP author. Please set this to 2.16 if you feel the patch is
useful and ready to go.

Gerv
Assignee: justdave → jmrobins
Target Milestone: Bugzilla 2.16 → Future
Keywords: patch, review
(Assignee)

Comment 4

16 years ago
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.
Target Milestone: Future → Bugzilla 2.16
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.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
(Assignee)

Comment 6

16 years ago
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.
Severity: normal → blocker
Priority: P3 → P1
Summary: LDAP Authentication should fail for empty passwords → [security] LDAP Authentication should fail for empty passwords
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
Created attachment 59251 [details] [diff] [review]
David Crow's patch
Comment on attachment 59251 [details] [diff] [review]
David Crow's patch

marking first-review on behalf of jmrobins
Attachment #59251 - Attachment description: David Crowe's patch → David Crow's patch
Attachment #59251 - Flags: review+
Comment on attachment 59251 [details] [diff] [review]
David Crow's patch

doesn't break the existing login stuff.

r= justdave
Attachment #59251 - Flags: review+
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
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
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

16 years ago
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...
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.