Derby
  1. Derby
  2. DERBY-5539

Harden password hashing in the builtin authentication service

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.9.1.0
    • Fix Version/s: 10.9.1.0
    • Component/s: Services
    • Labels:
      None
    • Bug behavior facts:
      Security

      Description

      The Open Web Application Security Project has some suggestions on how to make it harder for an attacker to crack hashed passwords: https://www.owasp.org/index.php/Hashing_Java

      The builtin authentication service doesn't follow all the suggestions. In particular, it doesn't add a random salt, and it only performs the hash operation once.

      I propose that we add two new properties that makes it possible to configure builtin to use a random salt and run multiple iterations of the hash operation:

      • derby.authentication.builtin.saltLength - the length of the random salt to add (in bytes)
      • derby.authentication.builtin.iterations - the number of times to perform the hash operation

      I'd also suggest that we set the defaults so that random salt and multiple iterations are used by default. The OWASP page mentions 64 bits of salt (8 bytes) and a minimum of 1000 iterations. I consulted a security expert who thought that these recommendations sounded OK, but he believed the recommended salt length was likely to be revised and suggested 16 bytes instead. The only price we pay by going from 8 to 16 bytes, is that we'll need to store 8 bytes extra per user in the database, so I don't see any reason not to set the default for derby.authentication.builtin.saltLength as high as 16. Setting the default for derby.authentication.builtin.iterations to 1000 will make authentication of a user somewhat slower (which is the point, really), but experiments on my machine suggest that running our default hash function (SHA-256) 1000 times takes around 1 ms. Since authentication only happens when establishing a new connection to the database, that would be a negligible cost, I think.

      If saltLength is set to 0 and iterations is set to 1, the hashing will be done in the exact same way as in previous versions.

      Both of the properties should only be respected when the data dictionary version is 10.9 or higher, so that users in soft-upgraded databases can still log in after a downgrade.

      1. d5539-1a.diff
        34 kB
        Knut Anders Hatlen
      2. d5539-1b.diff
        34 kB
        Knut Anders Hatlen
      3. d5539-2a.diff
        4 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          Attaching patch d5539-1a.diff, which adds the two proposed properties (with the defaults suggested in the issue description) and makes the configurable hash scheme in AuthenticationServiceBase use them.

          When using the existing algorithm, we store the credentials as a token with the following components:

          • a format identifier
          • the hashed password
          • the name of the hash function

          With the new algorithm, the format identifier is bumped, and the random salt and the number of iterations are added to the token.

          If the data dictionary version is less than 10.9, the two properties are ignored. This is done to ensure that it is safe to set passwords in soft upgrade mode. The patch also adds upgrade tests to verify this (more precisely, it moved the upgrade tests for DERBY-4483 from Changes10_6 to Changes10_9, so that they'll also test upgrade from 10.6, 10.7 and 10.8, and updated them to check the stored tokens against the new format). AuthenticationTest was also updated to test the new format.

          The regression tests ran cleanly. Patch is ready for review.

          Show
          Knut Anders Hatlen added a comment - Attaching patch d5539-1a.diff, which adds the two proposed properties (with the defaults suggested in the issue description) and makes the configurable hash scheme in AuthenticationServiceBase use them. When using the existing algorithm, we store the credentials as a token with the following components: a format identifier the hashed password the name of the hash function With the new algorithm, the format identifier is bumped, and the random salt and the number of iterations are added to the token. If the data dictionary version is less than 10.9, the two properties are ignored. This is done to ensure that it is safe to set passwords in soft upgrade mode. The patch also adds upgrade tests to verify this (more precisely, it moved the upgrade tests for DERBY-4483 from Changes10_6 to Changes10_9, so that they'll also test upgrade from 10.6, 10.7 and 10.8, and updated them to check the stored tokens against the new format). AuthenticationTest was also updated to test the new format. The regression tests ran cleanly. Patch is ready for review.
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Knut. I read through the patch, and it looks good to me. +1

          Nits: Do you need the upper bound in AuthenticationServiceBase#getIntProperty?
          The Javadoc for Changes10_9#testBuiltinAuthenticationWithConfigurableHash is a little ambiguous in its use of "only".
          Could you test with the salt in the upgrade test by reading the property first?

          Show
          Dag H. Wanvik added a comment - Thanks, Knut. I read through the patch, and it looks good to me. +1 Nits: Do you need the upper bound in AuthenticationServiceBase#getIntProperty? The Javadoc for Changes10_9#testBuiltinAuthenticationWithConfigurableHash is a little ambiguous in its use of "only". Could you test with the salt in the upgrade test by reading the property first?
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Dag! I'm uploading an updated patch (d5539-1b.diff) which clarifies the javadoc comment. Committed with revision 1221666. Further comments can be addressed in followup patches.

          > Nits: Do you need the upper bound in AuthenticationServiceBase#getIntProperty?

          You're right, we don't, since both of the current callers pass in Integer.MAX_VALUE. I guess it just felt more natural to specify the accepted range rather than just the lower bound, especially since the method is a general one. It shouldn't cause too much bloat, but I can remove that parameter if you think that would be better.

          (On a related note, I started wondering about what would happen if the salt length was set so high that the token wouldn't fit in a varchar, and if we'd need an upper bound. That turned out not to be an issue, as the db properties conglomerate doesn't store the values in varchars. It stores them in UserType columns. I tested with salt length 40000, which results in a token with more than 80000 characters, and that worked fine.)

          > Could you test with the salt in the upgrade test by reading the property first?

          Yes and no. With non-zero length salts, the stored token will vary between test runs, so in that case we'd need to duplicate the hashing algorithm in the test in order to verify that we get the correct token. But the main purpose of the test case is to verify that the token is stored in the expected format (for which we just need to check the format identifier in the first four characters) and that we're still able to connect after upgrade/downgrade, so we might also run the test case with default salt length and disable the full checks of the stored tokens. Those two alternatives should be more or less equivalent with respect to what we want to test in this test.

          Show
          Knut Anders Hatlen added a comment - Thanks, Dag! I'm uploading an updated patch (d5539-1b.diff) which clarifies the javadoc comment. Committed with revision 1221666. Further comments can be addressed in followup patches. > Nits: Do you need the upper bound in AuthenticationServiceBase#getIntProperty? You're right, we don't, since both of the current callers pass in Integer.MAX_VALUE. I guess it just felt more natural to specify the accepted range rather than just the lower bound, especially since the method is a general one. It shouldn't cause too much bloat, but I can remove that parameter if you think that would be better. (On a related note, I started wondering about what would happen if the salt length was set so high that the token wouldn't fit in a varchar, and if we'd need an upper bound. That turned out not to be an issue, as the db properties conglomerate doesn't store the values in varchars. It stores them in UserType columns. I tested with salt length 40000, which results in a token with more than 80000 characters, and that worked fine.) > Could you test with the salt in the upgrade test by reading the property first? Yes and no. With non-zero length salts, the stored token will vary between test runs, so in that case we'd need to duplicate the hashing algorithm in the test in order to verify that we get the correct token. But the main purpose of the test case is to verify that the token is stored in the expected format (for which we just need to check the format identifier in the first four characters) and that we're still able to connect after upgrade/downgrade, so we might also run the test case with default salt length and disable the full checks of the stored tokens. Those two alternatives should be more or less equivalent with respect to what we want to test in this test.
          Hide
          Knut Anders Hatlen added a comment -

          There's one more thing I'd like to change before I mark this issue as resolved. The OWASP page doesn't mention it explicitly, but the code example has a comment saying:

          // TIME RESISTANT ATTACK (Even if the user does not exist the
          // Computation time is equal to the time needed for a legitimate user

          I think the point here is that we should always generate a hashed token, also if the user does not exist, so that attackers cannot use the difference in computation time to determine whether or not a user exists.

          The attached patch (d5539-2a.diff) makes BasicAuthenticationServiceImpl.authenticateUser() generate a hashed token based on the supplied credentials if it couldn't find a user with the specified name at the database level. The token is thrown away once it's generated, since its only purpose is to make the authentication failure as slow for non-existing users as it is for existing users.

          Without the patch, 10000 unsuccessful attempts to connect to a database using a non-existing user name took 40 seconds in my environment, whereas 10000 unsuccessful connection attempts with an existing user name took 9 seconds. With the patch, 10000 unsuccessful connection attempts took 40 seconds in both scenarios.

          All the regression tests ran cleanly with the patch.

          Show
          Knut Anders Hatlen added a comment - There's one more thing I'd like to change before I mark this issue as resolved. The OWASP page doesn't mention it explicitly, but the code example has a comment saying: // TIME RESISTANT ATTACK (Even if the user does not exist the // Computation time is equal to the time needed for a legitimate user I think the point here is that we should always generate a hashed token, also if the user does not exist, so that attackers cannot use the difference in computation time to determine whether or not a user exists. The attached patch (d5539-2a.diff) makes BasicAuthenticationServiceImpl.authenticateUser() generate a hashed token based on the supplied credentials if it couldn't find a user with the specified name at the database level. The token is thrown away once it's generated, since its only purpose is to make the authentication failure as slow for non-existing users as it is for existing users. Without the patch, 10000 unsuccessful attempts to connect to a database using a non-existing user name took 40 seconds in my environment, whereas 10000 unsuccessful connection attempts with an existing user name took 9 seconds. With the patch, 10000 unsuccessful connection attempts took 40 seconds in both scenarios. All the regression tests ran cleanly with the patch.
          Hide
          Knut Anders Hatlen added a comment -

          > Without the patch, 10000 unsuccessful attempts to connect to a
          > database using a non-existing user name took 40 seconds in my
          > environment, whereas 10000 unsuccessful connection attempts with an
          > existing user name took 9 seconds.

          Oops. I was too quick when I wrote the above comment. It was the other
          way around. It was faster with non-existing users than with existing
          users.

          But the main point is still that the time it took to refuse a
          connection was different for existing and non-existing users before
          the patch, and it takes (approximately) the same time for existing and
          non-existing users when the patch is applied.

          Show
          Knut Anders Hatlen added a comment - > Without the patch, 10000 unsuccessful attempts to connect to a > database using a non-existing user name took 40 seconds in my > environment, whereas 10000 unsuccessful connection attempts with an > existing user name took 9 seconds. Oops. I was too quick when I wrote the above comment. It was the other way around. It was faster with non-existing users than with existing users. But the main point is still that the time it took to refuse a connection was different for existing and non-existing users before the patch, and it takes (approximately) the same time for existing and non-existing users when the patch is applied.
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 1292704.
          Marking the issue as resolved since there's no more planned work.

          Show
          Knut Anders Hatlen added a comment - Committed revision 1292704. Marking the issue as resolved since there's no more planned work.

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Knut Anders Hatlen
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development