Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

SEC-1419: BasePasswordEncoder facilities are buggy for password containing '{' and null/empty salt #1662

spring-issuemaster opened this Issue Feb 24, 2010 · 6 comments


None yet
1 participant

Cédrik LIME (Migrated from SEC-1419) said:

BasePasswordEncoder#demergePasswordAndSalt() fails when salt is null/empty and password contains '{'.

See enclosed JUnit 4 test case.

A proposed workaround would be for mergePasswordAndSalt() to always append "{}" to the password even if salt is null.
Note that demergePasswordAndSalt() should also check that the last char of mergedPasswordSalt is '}'.

Luke Taylor said:

Out of curiosity, when do you actually need to use demergePasswordAndSalt? I was looking at it recently and it seemed like a strong candidate for deprecation/removal. With password encoding, you are often using a one-way hash (in which case you can't decode the original String) and, even with something like symmetric encryption, you can re-encrypt the submitted password and salt (which by definition is something you know in advance) in order to perform the validation.

Cédrik LIME said:

While hashing (1-way) is better from a security POV, some of our clients want to encrypt (2-way) their credentials.
demergePasswordAndSalt() is used in the decryption step:
encryption: crypt(mergePasswordAndSalt(clearTextPassword, salt))
decryption: demergePasswordAndSalt(decrypt(dbPasswordSalt))

If anyone is interested, I can upload the full source code of our EncryptPasswordEncoder. Luke, do you think this would be a valuable addition to SpringSecurity?

Luke Taylor said:

The PasswordEncoder is an authentication plugin, so when it is used, you have the username and salt available. So I'm still not quite clear why you need a decryption operation. If you are using encryption, then the salt requirements are different - the salt is only required to stop someone from determining that two passwords in the database are the same. It's not there to prevent attacks. As such a better approach would be just to write an encoder that adds a fixed-length sequence of random characters to the end of each password before encrypting it and strips that number of characters from the decrypted string in order to obtain the password. That way there are no issues with allowed/disallowed characters.

Cédrik LIME said:

Right, we use PasswordEncoder's in a non-standard way: we handle authentication and password management ourself, but find Spring Security classes very convenient. :-)

We are perfectly aware that using a salt only prevents encrypted password comparison. Our salt is a function of the user name.
While we could do as you suggested (add a fixed-length random string to the password), re-using facilities in a base class that do exactly what we need is too compelling to pass!

Nonetheless, mergePasswordAndSalt() and demergePasswordAndSalt() are not symmetric, when they should be. I believe my proposed fix would work quite well, without impacting existing applications.

Luke Taylor said:

I can see that. My problem is that I don't really think the existing hierarchy is optimal, partly because it causes the kind of issue you've raised here. Also I think your proposed fix would break all existing hash implementations by changing the value prior to hashing (when the salt is null).

If we are going to change the implementations in a future version, I would prefer to separate the hashing implementations completely and just simply add the salt directly to the digest. An encryption based implementation would be simpler if it used an approach like the one I suggested above.

Luke Taylor said:

I think this has to be a "won't fix", for the reason given - it would break usage of the existing password encoder implementations.

@spring-issuemaster spring-issuemaster added this to the 3.1.0.M1 milestone Feb 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment