Spring Security
  1. Spring Security
  2. SEC-1890

BCryptPasswordEncoder throws IllegalArgumentException: Encoded password cannot be null or empty if password is empty (i.e. not encoded)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Complete
    • Affects Version/s: 3.1.0
    • Fix Version/s: 3.1.1
    • Component/s: Crypto
    • Labels:
      None

      Description

      Example stacktrace:

      java.lang.IllegalArgumentException: Encoded password cannot be null or empty
      org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder.matches(BCryptPasswordEncoder.java:77) 
      org.springframework.security.authentication.dao.DaoAuthenticationProvider$1.isPasswordValid(DaoAuthenticationProvider.java:148) 
      org.springframework.security.authentication.dao.DaoAuthenticationProvider.additionalAuthenticationChecks(DaoAuthenticationProvider.java:84) 
      org.springframework.security.authentication.dao.AbstractUserDetailsAuthenticationProvider.authenticate(AbstractUserDetailsAuthenticationProvider.java:149) 
      org.springframework.security.authentication.ProviderManager.authenticate(ProviderManager.java:156) 
      org.springframework.security.authentication.ProviderManager.authenticate(ProviderManager.java:174) 
      org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter.attemptAuthentication(UsernamePasswordAuthenticationFilter.java:94) 
      org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.doFilter(AbstractAuthenticationProcessingFilter.java:195) 
      

        Activity

        Hide
        Dave Syer added a comment -

        Actually, I'm not sure what should happen here. The error from BCrypt is unfriendly so we can fix that I suppose. But should a password encoder ever be passed an empty (hence from the encoder's point of view unencoded) password to check? The use case comes from Spring Security OAuth where it is legal (but not advisable) for clients to have no secret, but to use the DaoAuthenticationProvider it has to supply a User, which isn't allowed to have a null password.

        Show
        Dave Syer added a comment - Actually, I'm not sure what should happen here. The error from BCrypt is unfriendly so we can fix that I suppose. But should a password encoder ever be passed an empty (hence from the encoder's point of view unencoded) password to check? The use case comes from Spring Security OAuth where it is legal (but not advisable) for clients to have no secret, but to use the DaoAuthenticationProvider it has to supply a User, which isn't allowed to have a null password.
        Hide
        Dave Syer added a comment -

        I can't edit the summary field in this issue but it's not just about empty passwords - anything unencrypted will cause an IllegalArgumentException in hashpw() which isn't caught and dealt with by the security filters. Probably it should be caught by the PasswordEncoder and translated into a BadCredentialsException or something (but with a warning that probably there is a problem with the server because an unencoded password should not be presented for comparison).

        Show
        Dave Syer added a comment - I can't edit the summary field in this issue but it's not just about empty passwords - anything unencrypted will cause an IllegalArgumentException in hashpw() which isn't caught and dealt with by the security filters. Probably it should be caught by the PasswordEncoder and translated into a BadCredentialsException or something (but with a warning that probably there is a problem with the server because an unencoded password should not be presented for comparison).
        Hide
        Luke Taylor added a comment -

        I would say the User object shouldn't be loaded with an empty password for use in DaoAuthenticationProvider. If a DaoAuthenticationProvider is configured with a particular type of PasswordEncoder, then the assumption is that the data it loads will be compatible with the encoder. If that assumption doesn't hold, then the AuthenticationProvider should probably be customized to deal with the situation where it doesn't actually need to authenticate the client. I think it would be preferable to rely on something other than an empty password field to indicate that a client doesn't need to authenticate though.

        Show
        Luke Taylor added a comment - I would say the User object shouldn't be loaded with an empty password for use in DaoAuthenticationProvider. If a DaoAuthenticationProvider is configured with a particular type of PasswordEncoder, then the assumption is that the data it loads will be compatible with the encoder. If that assumption doesn't hold, then the AuthenticationProvider should probably be customized to deal with the situation where it doesn't actually need to authenticate the client. I think it would be preferable to rely on something other than an empty password field to indicate that a client doesn't need to authenticate though.
        Hide
        Dave Syer added a comment -

        Agree (and we can work with the empty password somehow). But the BCryptPasswordEncoder should still probably catch IllegalArgumentException and translate it into something more helpful for the security components higher up the stack (like returning false from isPasswordValid())?

        Show
        Dave Syer added a comment - Agree (and we can work with the empty password somehow). But the BCryptPasswordEncoder should still probably catch IllegalArgumentException and translate it into something more helpful for the security components higher up the stack (like returning false from isPasswordValid())?
        Hide
        Luke Taylor added a comment -

        I dunno. It is an illegal argument, since it should only be passed bcrypt-encoded data. Returning false from isPasswordValid() is supposed to indicate that the user-supplied password is incorrect, not that there is invalid data stored on the server. It would usually indicate that they should retry with the correct password. To me this makes more sense as a server error, since they can never enter the correct value.

        Show
        Luke Taylor added a comment - I dunno. It is an illegal argument, since it should only be passed bcrypt-encoded data. Returning false from isPasswordValid() is supposed to indicate that the user-supplied password is incorrect, not that there is invalid data stored on the server. It would usually indicate that they should retry with the correct password. To me this makes more sense as a server error, since they can never enter the correct value.
        Hide
        Dave Syer added a comment -

        True. At a minimum we shoudl fix the BCrypt then so that it throws exceptions with better messages - e.g. it makes a lot of assumptions about the length of the salt in hashpw(), so the IllegalArgumentException is often never seen - just a confusing ArrayIndexOutOfBounds or something.

        Show
        Dave Syer added a comment - True. At a minimum we shoudl fix the BCrypt then so that it throws exceptions with better messages - e.g. it makes a lot of assumptions about the length of the salt in hashpw(), so the IllegalArgumentException is often never seen - just a confusing ArrayIndexOutOfBounds or something.
        Hide
        Luke Taylor added a comment -

        I'd prefer not to modify the BCrypt.java file, since it is a drop in copy of jBCrypt which we may want to upgrade. To get to a situation where you got an ArrayIndexOutOfBounds, it looks like the string would have to have a valid BCrypt header, otherwise you would get an IllegalArgumentException ("Invalid salt version"), which seems unlikely unless the BCrypt string is truncated.

        I've added a Pattern to the BCryptPasswordEncoder, which it will check to make sure the stored value actually is a BCrypt string, so it won't accept anything without the correct headers and length.

        Show
        Luke Taylor added a comment - I'd prefer not to modify the BCrypt.java file, since it is a drop in copy of jBCrypt which we may want to upgrade. To get to a situation where you got an ArrayIndexOutOfBounds, it looks like the string would have to have a valid BCrypt header, otherwise you would get an IllegalArgumentException ("Invalid salt version"), which seems unlikely unless the BCrypt string is truncated. I've added a Pattern to the BCryptPasswordEncoder, which it will check to make sure the stored value actually is a BCrypt string, so it won't accept anything without the correct headers and length.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Dave Syer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: