Spring Security
  1. Spring Security
  2. SEC-1419

BasePasswordEncoder facilities are buggy for password containing '{' and null/empty salt

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 3.0.2
    • Fix Version/s: 3.1.0.M1
    • Component/s: Core
    • Labels:
      None

      Description

      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 "

      {<salt>}

      " to the password even if salt is null.
      Note that demergePasswordAndSalt() should also check that the last char of mergedPasswordSalt is '}'.

        Activity

        Hide
        Luke Taylor added a comment - - edited

        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.

        Show
        Luke Taylor added a comment - - edited 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.
        Hide
        Cédrik LIME added a comment -

        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?

        Show
        Cédrik LIME added a comment - 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?
        Hide
        Luke Taylor added a comment -

        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.

        Show
        Luke Taylor added a comment - 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.
        Hide
        Cédrik LIME added a comment -

        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.

        Show
        Cédrik LIME added a comment - 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.
        Hide
        Luke Taylor added a comment -

        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.

        Show
        Luke Taylor added a comment - 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.
        Hide
        Luke Taylor added a comment - - edited

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

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

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Cédrik LIME
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: