PBKDF2 updates to conform to SP800-132 #8868

slontis · 2019-05-03T04:04:14Z

Checklist

documentation is added or updated
tests are added or updated

crypto/kdf/scrypt.c

doc/man7/EVP_KDF_PBKDF2.pod

test/evp_kdf_test.c

doc/man7/EVP_KDF_PBKDF2.pod

slontis · 2019-05-15T03:07:49Z

Updated commit message and rebased.

mspncp · 2019-05-27T15:23:37Z

Would you mind using present tense in the commit message of 1827b7119195? See #8763 (comment) for an explanation.

PBKDF2: conform to SP800-132

The existing code used PKCS5 specifications.
SP800-132 adds the following additional constraints:
  - Check the range of the key length.
  - Minimum iteration count (1000 recommended).
  - Randomly-generated portion of the salt shall be at least 128 bits.
There are cases where the constraints will cause errors (in scrypt, and
some PKCS5 related test vectors) so a ctrl "pkcs5" is added to disable
these additional constraints.

slontis · 2019-05-28T11:24:09Z

Updated the commit message and rebased. (This present tense stuff hurts my brain.)

mspncp · 2019-05-28T11:26:39Z

Updated the commit message and rebased. (This present tense stuff hurts my brain.)

Sorry that it hit you so often. I did not mean to stalk you ;-)

mattcaswell · 2019-05-29T09:39:31Z

Looks like you've picked up @t8m's commit in this PR (I guess you rebased on top of github master in the window while 6acf605 was there, before it got force pushed away - see the discussion in #9015 (comment)). Can you rebase this?

slontis · 2019-05-29T09:54:06Z

Ahh that would explain the merge commit I had earlier in the day.. sigh.

mattcaswell

My suspicion is that this is going to break stuff. It will probably have an impact on "enc" command line usage - e.g. for files that have previously been encrypted with bad iteration counts it might no longer be possible to decrypt them. Probably the breaking aspects of this need wider discussion. At the very least we would need a CHANGES entry. Probably some changes to "enc".

mattcaswell · 2019-05-29T09:42:07Z

doc/man7/EVP_KDF_PBKDF2.pod

+A minimum iteration count of 1000 and length of the salt being at least
+128 bits.
+
+The default value is 0. Use 1 to disable the checks.


So, this sounds like having the default like this is a breaking change?

enc uses the PKCS5_PBKDF2_HMAC() method which sets this control to be 1, as does scrypt.

Added note to CHANGES

mattcaswell · 2019-05-29T09:45:17Z

crypto/kdf/pbkdf2.c

+
+    /* Check that 112 bits <= keylen < (2^32 - 1) * mdlen */
+    if (((keylen * 8) < KDF_PBKDF2_MIN_KEY_LEN_BITS)
+        || ((keylen / mdlen) >= KDF_PBKDF2_MAX_KEY_LEN_DIGEST_RATIO))


Should we be also checking the value of range_checks here?

I did ponder it - But I think not..
If it goes above that range the counter overflows which is very bad.

If it goes above that range the counter overflows which is very bad.

Perhaps a comment stating that would be helpful

Is that also true for the KDF_PBKDF2_MIN_KEY_LEN_BITS check?

Not sure - I assume that is similar to the salt check so maybe that one goes in the check - in that case I might rename the range_checks variable :)

Comment added and the lower bound check has been moved.

doc/man7/EVP_KDF_PBKDF2.pod

crypto/kdf/pbkdf2.c

slontis · 2019-06-04T21:49:44Z

ping @mattcaswell

mattcaswell · 2019-06-05T10:23:26Z

This looks ok, but since this is a breaking change I'd like a wider viewpoint from @openssl/omc.

slontis · 2019-06-05T10:28:15Z

Note that the minimum iteration count is only recommended (it is not a SHALL) so that particular test could be relaxed (but it is probably a better idea to still force it to at least the recommended value)

levitte · 2019-06-05T11:58:47Z

Whether this is a breaking change or not depends on your point of view. EVP_KDF is an unreleased API, so there's nothing that can be called a breaking change there. However, for KDFs implemented via EVP_PKEY, this is effectively a breaking change.

To resolve that issue, the use via EVP_PKEY could be relaxed so as not to pose a problem.

kroeckx · 2019-06-05T12:22:50Z

I think we should support reading old data from before this change.

slontis · 2019-06-06T00:38:44Z

To resolve that issue, the use via EVP_PKEY could be relaxed so as not to pose a problem.

Ok so that would be a fairly ugly check. The best solution would probably be to have it default to how it used to work, but in FIPS_MODE it does the extra checks by default. The flag can still be enabled or disabled in either provider.
Does that sound like a better solution?

slontis · 2019-06-06T01:53:26Z

Updated the default value to disable the checks by default in the default provider.

mattcaswell · 2019-06-06T08:42:45Z

Updated the default value to disable the checks by default in the default provider.

Sounds like a reasonable approach.

levitte · 2019-06-08T13:12:46Z

I agree with @mattcaswell. Good thinking!

slontis · 2019-06-10T07:42:59Z

ping @mattcaswell or @levitte : Code was updated to set default check for FIPS_MODE only.

slontis · 2019-06-11T01:01:23Z

rebased to fix CHANGES conflict.

mattcaswell

Reconfirmed. Although CHANGES is conflicting again :-). I don't think there is a need to seek reapproval where a conflict is trivial. Just go ahead and commit this.

The existing code used PKCS5 specifications. SP800-132 adds the following additional constraints for: - the range of the key length. - the minimum iteration count (1000 recommended). - salt length (at least 128 bits). These additional constraints may cause errors (in scrypt, and some PKCS5 related test vectors). To disable the new constraints use the new ctrl string "pkcs5". For backwards compatability, the checks are only enabled by default for fips mode.

The existing code used PKCS5 specifications. SP800-132 adds the following additional constraints for: - the range of the key length. - the minimum iteration count (1000 recommended). - salt length (at least 128 bits). These additional constraints may cause errors (in scrypt, and some PKCS5 related test vectors). To disable the new constraints use the new ctrl string "pkcs5". For backwards compatability, the checks are only enabled by default for fips mode. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #8868)

paulidale reviewed May 3, 2019

View reviewed changes

crypto/kdf/scrypt.c Outdated Show resolved Hide resolved

doc/man7/EVP_KDF_PBKDF2.pod Outdated Show resolved Hide resolved

doc/man7/EVP_KDF_PBKDF2.pod Outdated Show resolved Hide resolved

test/evp_kdf_test.c Outdated Show resolved Hide resolved

test/evp_kdf_test.c Show resolved Hide resolved

kaduk reviewed May 3, 2019

View reviewed changes

doc/man7/EVP_KDF_PBKDF2.pod Outdated Show resolved Hide resolved

slontis force-pushed the pbkdf2_updates branch from d4dab2d to 30da5f3 Compare May 6, 2019 21:28

slontis force-pushed the pbkdf2_updates branch from b11a1fe to 1827b71 Compare May 15, 2019 03:07

slontis force-pushed the pbkdf2_updates branch from 1827b71 to de97873 Compare May 28, 2019 11:22

levitte force-pushed the master branch from 6acf605 to 3b43740 Compare May 28, 2019 12:54

mattcaswell requested changes May 29, 2019

View reviewed changes

slontis force-pushed the pbkdf2_updates branch from de97873 to 8838dbf Compare May 29, 2019 09:57

slontis changed the title ~~PBKDF2 changed to conform to SP800-132~~ PBKDF2 updates to conform to SP800-132 May 29, 2019

mattcaswell approved these changes Jun 10, 2019

View reviewed changes

mattcaswell added the approval: done This pull request has the required number of approvals label Jun 10, 2019

slontis added this to In progress in 3.0 New Core + FIPS via automation Jun 11, 2019

slontis force-pushed the pbkdf2_updates branch from f747a8b to 97bf3c5 Compare June 11, 2019 00:59

slontis added the branch: master Merge to master branch label Jun 11, 2019

mattcaswell approved these changes Jun 11, 2019

View reviewed changes

3.0 New Core + FIPS automation moved this from In progress to Reviewer approved Jun 11, 2019

slontis force-pushed the pbkdf2_updates branch from 97bf3c5 to 71007a0 Compare June 11, 2019 10:41

slontis closed this Jun 11, 2019

3.0 New Core + FIPS automation moved this from Reviewer approved to Done Jun 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PBKDF2 updates to conform to SP800-132 #8868

PBKDF2 updates to conform to SP800-132 #8868

slontis commented May 3, 2019

slontis commented May 15, 2019

mspncp commented May 27, 2019

slontis commented May 28, 2019

mspncp commented May 28, 2019

mattcaswell commented May 29, 2019 •

edited by mspncp

slontis commented May 29, 2019

mattcaswell left a comment

mattcaswell May 29, 2019

slontis Jun 2, 2019

slontis Jun 2, 2019

mattcaswell May 29, 2019

slontis May 29, 2019

mattcaswell May 29, 2019

slontis May 29, 2019

slontis Jun 2, 2019

slontis commented Jun 4, 2019

mattcaswell commented Jun 5, 2019

slontis commented Jun 5, 2019

levitte commented Jun 5, 2019

kroeckx commented Jun 5, 2019

slontis commented Jun 6, 2019

slontis commented Jun 6, 2019

mattcaswell commented Jun 6, 2019

levitte commented Jun 8, 2019

slontis commented Jun 10, 2019

slontis commented Jun 11, 2019

mattcaswell left a comment

PBKDF2 updates to conform to SP800-132 #8868

PBKDF2 updates to conform to SP800-132 #8868

Conversation

slontis commented May 3, 2019

Checklist

slontis commented May 15, 2019

mspncp commented May 27, 2019

slontis commented May 28, 2019

mspncp commented May 28, 2019

mattcaswell commented May 29, 2019 • edited by mspncp

slontis commented May 29, 2019

mattcaswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slontis commented Jun 4, 2019

mattcaswell commented Jun 5, 2019

slontis commented Jun 5, 2019

levitte commented Jun 5, 2019

kroeckx commented Jun 5, 2019

slontis commented Jun 6, 2019

slontis commented Jun 6, 2019

mattcaswell commented Jun 6, 2019

levitte commented Jun 8, 2019

slontis commented Jun 10, 2019

slontis commented Jun 11, 2019

mattcaswell left a comment

Choose a reason for hiding this comment

mattcaswell commented May 29, 2019 •

edited by mspncp