Dave Syer (Migrated from SEC-1890) said:
java.lang.IllegalArgumentException: Encoded password cannot be null or empty
Dave Syer said:
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.
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).
Luke Taylor said:
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.
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())?
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.
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.
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.