possible security bug: empty password accepted for LDAP login

 






 

 


  Review Request #1720
  —
  Created July 23, 2010 and submitted

 


















 

  

   Information

  





 
 

  bugfood
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This is a copy of the text I sent to reviewboard-dev.

Hello,

Let me preface this by saying that I'm not particularly knowledgeable
with regard to ReviewBoard, Python, or LDAP. My diagnosis may be
incorrect, but I thought I should report this in case it is of any use.

I noticed this behavior on ReviewBoard-1.0.5.1-2.1.el5.

http://download.fedora.redhat.com/pub/epel/5/x86_64/repoview/ReviewBoard.html

I tried to confirm the problem with 1.5rc1, but I had trouble installing
it on my workstation and didn't want to spend too much time going down
that path; I did examine the relevant code, though, and it doesn't seem
to have changed at all between 1.0.5 and 1.5rc1.


The problem:

When I set up ReviewBoard to use LDAP authentication, and then try to
log in with a valid LDAP user and an empty password, the login succeeds
(using an invalid user or incorrect password fails as expected). I
looked at some packet dumps and determined that ReviewBoard was sending
an LDAP BindRequest with simple authentication and an empty password.
The LDAP server was returning success, which seemed odd, but appears to
be part of the specification:

http://tools.ietf.org/html/rfc4513#section-5.1.2

-----------------------------------------------------------------------
   An LDAP client may use the unauthenticated authentication mechanism
   of the simple Bind method to establish an anonymous authorization
   state by sending a Bind request with a name value (a distinguished
   name in LDAP string form [RFC4514] of non-zero length) and specifying
   the simple authentication choice containing a password value of zero
   length.

   The distinguished name value provided by the client is intended to be
   used for trace (e.g., logging) purposes only.  The value is not to be
   authenticated or otherwise validated (including verification that the
   DN refers to an existing directory object).  The value is not to be
   used (directly or indirectly) for authorization purposes.

   Unauthenticated Bind operations can have significant security issues
   (see Section 6.3.1).  In particular, users intending to perform
   Name/Password Authentication may inadvertently provide an empty
   password and thus cause poorly implemented clients to request
   Unauthenticated access.  Clients SHOULD be implemented to require
   user selection of the Unauthenticated Authentication Mechanism by
   means other than user input of an empty password.  Clients SHOULD
   disallow an empty password input to a Name/Password Authentication
   user interface.  Additionally, Servers SHOULD by default fail
   Unauthenticated Bind requests with a resultCode of
   unwillingToPerform.
-----------------------------------------------------------------------


I have attached a patch which seems to solve the problem.

Thanks,
Corey


 








  


 
 

  
Tested with ReviewBoard-1.0.5.1-2.1.el5 (I would test current git, had trouble setting it up).

 







 









 





  








 









 






  

 








 














 





 

  

   




mike_conley




   

   





  

 



 

  


   

   

    
 
mike_conley


    
   


  

  

  



    




  1. 
 
    
  
    
   
    The diff is broken, so I can't review.  :/

I worked with LDAP a few years ago and ran into the same problem.  It's actually part of the LDAP spec:

   "An LDAP client MAY also choose to explicitly bind anonymously. A 
   client that wishes to do so MUST choose the simple authentication 
   option in the Bind Request (see section 4.1) and set the password to
   be of zero length. (This is often done by LDAPv2 clients.) Typically
   the name is also of zero length."

   From:  http://tools.ietf.org/html/draft-ietf-ldapbis-authmeth-19

    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Wow - I didn't notice that you'd already included stuff about the spec in your review request description.  My bad.  :)
      
 
      

      2. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   





 BU





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  









  

   

    Change Summary:
   

   

    
Here's a diff of the same change using --full-index (Christian Hammond helped me out on reviewboard-dev).

   

  








  

   

    Diff:
   

   

    
 Revision 2 (+8)
Show changes
 
    reviewboard/accounts/backends.py 
 

   

  












  

 
















 





 

  

   




mike_conley




   

   







  

 



 

  


   

   

    
 
mike_conley


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Looks OK to me.
    
  
    
 
    

 
    
  



 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Looks good. Committed as rc85b0ac. Thanks.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3.