Increase default salt from 8 to 16 bytes for PKCS#8 & PKCS#12

[xnox]

Currently PKCS8_marshal_encrypted_private_key function uses salt
length of 8 bytes / 64 bits when no salt length is specified. Increase
this fallback default to 16 bytes / 128 bits, as recommended by NIST
SP 800-132.

Note this is for interoperability purposes. Some FIPS implementations
enforce minimum salt length of 16 bytes. Examples of such FIPS
implementations are Bouncycastle FIPS Java API and Chainguard FIPS
Provider for OpenSSL. Also future v3.6 release of OpenSSL will also
increase the default salt length to 16 bytes. A similar change has
also been submitted to BoringSSL and LibreSSL.

Keep legacy EVP_BytesToKey and PKCS5_SALT_LEN defines at 8 bytes. Add
a new internal only define PKCS12_SALT_LEN set to 16 bytes. Use
PKCS12_SALT_LEN as the fallback salt length in PKCS#8 and PKCS#12
codepaths.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

[github-actions]

clang-tidy made some suggestions

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.87%. Comparing base (40ac425) to head (1e3d0e1).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2409      +/-   ##
==========================================
- Coverage   78.89%   78.87%   -0.03%     
==========================================
  Files         621      621              
  Lines      108670   108670              
  Branches    15420    15419       -1     
==========================================
- Hits        85738    85716      -22     
- Misses      22262    22283      +21     
- Partials      670      671       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[justsmth]

This change appears to break our Ruby integration test:

  1) Error:
OpenSSL::TestCipher#test_pkcs5_keyivgen:
OpenSSL::Cipher::CipherError: salt must be an 8-octet string
    /codebuild/output/src3499518791/src/github.com/aws/aws-lc/RUBY_BUILD_ROOT/ruby-src/master/test/openssl/test_cipher.rb:36:in 'OpenSSL::Cipher#pkcs5_keyivgen'
    /codebuild/output/src3499518791/src/github.com/aws/aws-lc/RUBY_BUILD_ROOT/ruby-src/master/test/openssl/test_cipher.rb:36:in 'OpenSSL::TestCipher#test_pkcs5_keyivgen'

Finished tests in 38.317931s, 13.9621 tests/s, 128.6604 assertions/s.

[xnox]

This change appears to break our Ruby integration test:

  1) Error:
OpenSSL::TestCipher#test_pkcs5_keyivgen:
OpenSSL::Cipher::CipherError: salt must be an 8-octet string
    /codebuild/output/src3499518791/src/github.com/aws/aws-lc/RUBY_BUILD_ROOT/ruby-src/master/test/openssl/test_cipher.rb:36:in 'OpenSSL::Cipher#pkcs5_keyivgen'
    /codebuild/output/src3499518791/src/github.com/aws/aws-lc/RUBY_BUILD_ROOT/ruby-src/master/test/openssl/test_cipher.rb:36:in 'OpenSSL::TestCipher#test_pkcs5_keyivgen'

Finished tests in 38.317931s, 13.9621 tests/s, 128.6604 assertions/s.

it likely means that it is also broken against OpenSSL 3.6 master branch. Will poke it to fix their test suite.

[xnox]

This change appears to break our Ruby integration test:

  1) Error:
OpenSSL::TestCipher#test_pkcs5_keyivgen:
OpenSSL::Cipher::CipherError: salt must be an 8-octet string
    /codebuild/output/src3499518791/src/github.com/aws/aws-lc/RUBY_BUILD_ROOT/ruby-src/master/test/openssl/test_cipher.rb:36:in 'OpenSSL::Cipher#pkcs5_keyivgen'
    /codebuild/output/src3499518791/src/github.com/aws/aws-lc/RUBY_BUILD_ROOT/ruby-src/master/test/openssl/test_cipher.rb:36:in 'OpenSSL::TestCipher#test_pkcs5_keyivgen'

Finished tests in 38.317931s, 13.9621 tests/s, 128.6604 assertions/s.

https://github.com/ruby/openssl/blob/080b21d3a2a413b74c36a1ebd821153bdd15c02a/ext/openssl/ossl_cipher.c#L315

The code there is a bit incoherent. Salt can be of any length, 8 is suggested minimum, and raising the constant didn't change the testcase inputs. Will propose to fix up that unit test to not use such comparison.

[rhenium]

FYI OpenSSL's openssl enc utility uses PKCS5_SALT_LEN to generate the salt to be passed to EVP_BytesToKey(), which is often the reference for API consumers when documentation is lacking: https://github.com/openssl/openssl/blob/3818f7779ef4bf4d4ccacd13506ec92885e45553/apps/enc.c#L348-L349

LibreSSL's man page for EVP_BytesToKey() says PKCS5_SALT_LEN is 8: https://man.openbsd.org/EVP_BytesToKey.3

OpenSSL avoided changing PKCS5_SALT_LEN due to a compatibility concern: openssl/openssl#20783

I'm afraid there may be other examples in the wild that assume PKCS5_SALT_LEN to not change from 8, not just ruby/openssl.

[xnox]

FYI OpenSSL's openssl enc utility uses PKCS5_SALT_LEN to generate the salt to be passed to EVP_BytesToKey(), which is often the reference for API consumers when documentation is lacking: https://github.com/openssl/openssl/blob/3818f7779ef4bf4d4ccacd13506ec92885e45553/apps/enc.c#L348-L349

LibreSSL's man page for EVP_BytesToKey() says PKCS5_SALT_LEN is 8: https://man.openbsd.org/EVP_BytesToKey.3

OpenSSL avoided changing PKCS5_SALT_LEN due to a compatibility concern: openssl/openssl#20783

I'm afraid there may be other examples in the wild that assume PKCS5_SALT_LEN to not change from 8, not just ruby/openssl.

there are two types of PKCS5_SALT_LEN.

And some implementations use this same define; in different headers/files; but are actually distinct - e.g. BoringSSL.

Please carefully look at:

• openssl/openssl@995e948
• https://marc.info/?l=openbsd-tech&m=174689919725477&w=2
• https://marc.info/?l=openbsd-tech&m=174690438827143&w=2
• Increase default salt from 8 to 16 bytes for PKCS#8 & PKCS#12 #2409
• https://boringssl-review.googlesource.com/c/boringssl/+/79267

And observe that in all cases EVP_BytesToKey API remains using "salt[8]" input argument.

Some are used for EVP_BytesToKey, others are used to create pkcs12 stores and what not.

In boringssl, libressl and openssl the EVP_BytesToKey API remains the same, at 8 bytes. The PKCS12 one however is upgrading. I ensured to do these patches consistent.

But yes, there are many places that use EVP_BytesToKey api that uses "salt[8]" input type.

Note here, in this patch, I too keep EVP_BytesToKey to continue using salt[8], but change the pkcs12 one to 16.

[xnox]

Possibly this define is used as a type-hint of the EVP_BytesToKeys() api (which has salt, but no salt length). And thus maybe alternative approach is needed of keeping this define as is; and stop using it altogether.

[rhenium]

Possibly this define is used as a type-hint of the EVP_BytesToKeys() api (which has salt, but no salt length). And thus maybe alternative approach is needed of keeping this define as is; and stop using it altogether.

I think it's the case, PKCS5_SALT_LEN is unfortunately baked into the public API at this point, even if it might not be the original intent. It was moved to a public header file in 1999.
openssl/openssl@8e21c14

Just a data point, OpenSSL's man page for the legacy PEM encryption (which uses EVP_BytesToKey()) references PKCS5_SALT_LEN.
https://docs.openssl.org/master/man3/PEM_read_bio_PrivateKey/#pem-encryption-format

[xnox]

Ok, let me go back and figure out how to fix things - such that this public API remains in place, and is from now on the salt length for the Evo bytestokeys. And do something else for pkcs8/12 which can have arbitrary salt length to define a different private constant there.

[xnox]

@rhenium @justsmth reimplemented the change that matters (ensuring that approved pkcs#8 and pkcs#12 use approved salt length) without changing in any way the legacy/deprecated EVP_BytesToKeys() api, which should be somehow deprecated across everywhere. Also left a comment such that nobody else attempts to bump that public constant which must remain frozen.

[xnox]

Please retry https://github.com/aws/aws-lc/actions/runs/15020513928/job/42210011957?pr=2409 i attempted to edit the description correctly again (it didn't like newline separation)

[justsmth]

Ruby "main" integration is still failing, but it might be unrelated.

• One of the required check is failing due to the documentation not matching expectations. It can be fixed by applying the following diff.

diff --git a/include/openssl/evp.h b/include/openssl/evp.h
index f2eeb64d9..79f11b675 100644
--- a/include/openssl/evp.h
+++ b/include/openssl/evp.h
@@ -511,8 +511,8 @@ OPENSSL_EXPORT int EVP_PKEY_print_params(BIO *out, const EVP_PKEY *pkey,
 // function that results in a key suitable for use in symmetric
 // cryptography.

-// Deprecated constant as used by deprecated EVP_BytesToKey() which
-// cannot change
+// PKCS5_SALT_LEN is a deprecated constant as used by deprecated EVP_BytesToKey()
+// which cannot change.
 #define PKCS5_SALT_LEN 8

 // PKCS5_PBKDF2_HMAC computes |iterations| iterations of PBKDF2 of |password|

• Our required license check CI job is failing b/c we require all PRs to have a description including the following text: " By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license."

If you agree, would you update the PR description to contain the required text? Thanks!

[xnox]

done & done

sticking to 80 char column mode as i don't see any other comment or code go above it. And redid description without new line in the license statement.

[xnox]

The equivalent change has now landed in the boringssl fork, and is being integrated throughout all the chromium trees and the rest of the mono repo.

Is there anything outstanding from me needed to land this?

Is my formatting still incorrect for lint / formatters?

Do I need to grant maintainers edit access on this?

Edit: I see there was a release, thus rebasing, hopefully that will make ABI diff pass

[justsmth]

The equivalent change has now landed in the boringssl fork, and is being integrated throughout all the chromium trees and the rest of the mono repo.

For reference: google/boringssl@6295694

Is there anything outstanding from me needed to land this?

I don't see any other blocking issues. We just need one more approval for a merge. I'll try to poke people for a review.

[nebeid]

I was wondering whether it's better to put this new constant in evp.h? Instead of just leaving there a deprecated constant we'd also have the one recommended by NIST
SP 800-132.

[xnox]

@nebeid as a data point all other libraries have internalised such a constant without making it public, and only document the default salt length when otherwise not specified in their manpages and API docs. This is what boringssl, libressl, openssl have done.

Note that in openssl case it is also ifndef'ed such that one can override it to something else at compile time (to downgrade back to 8).

Hence my preference is for API users to explicitly set it themselves, or to read it back after encrypt has happened out of the data structure that encrypt has emitted.