Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement PKCS7_encrypt and PKC7_decrypt #1996

Merged
merged 13 commits into from
Nov 25, 2024

Conversation

WillChilds-Klein
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein commented Nov 15, 2024

Issues:

Addresses CryptoAlg-2494

Description of changes:

This PR adds 2 new functions to encrypt/decrypt BIO contents into/out of "enveloped"-type PKCS7 objects.

Call-outs:

Like OpenSSL, this implementation of PKCS7_decrypt contains mitigations against the "Million Message Attack" (MMA) as prescribed in RFC 3218. A more detailed description is given in source comments.

Testing:

  • new unit tests. further coverage to be added in PR 1993 and beyond

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.

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2024

Codecov Report

Attention: Patch coverage is 76.57993% with 63 lines in your changes missing coverage. Please review.

Project coverage is 78.91%. Comparing base (85f58da) to head (8b49c3f).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crypto/pkcs7/pkcs7.c 71.35% 59 Missing ⚠️
crypto/pkcs7/pkcs7_test.cc 93.44% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1996      +/-   ##
==========================================
+ Coverage   78.89%   78.91%   +0.01%     
==========================================
  Files         595      594       -1     
  Lines      102451   102679     +228     
  Branches    14525    14578      +53     
==========================================
+ Hits        80832    81032     +200     
- Misses      20969    20996      +27     
- Partials      650      651       +1     

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


🚨 Try these New Features:

@WillChilds-Klein WillChilds-Klein changed the title [DRAFT] Add PKCS7 encrypt/decrypt BIOs [DRAFT] Implement PKCS7_encrypt and PKC7_decrypt Nov 18, 2024
@WillChilds-Klein WillChilds-Klein marked this pull request as ready for review November 18, 2024 20:56
@WillChilds-Klein WillChilds-Klein requested a review from a team as a code owner November 18, 2024 20:56
@WillChilds-Klein WillChilds-Klein changed the title [DRAFT] Implement PKCS7_encrypt and PKC7_decrypt Implement PKCS7_encrypt and PKC7_decrypt Nov 18, 2024
@WillChilds-Klein
Copy link
Contributor Author

WillChilds-Klein commented Nov 19, 2024

Interesting... aws-lc-ci-linux-x86's ubuntu2004_clang8x_x86_64 job (and only that job) has failed with the following error:

...
[ RUN      ] PKCS7Test.TestEnveloped
../crypto/pkcs7/pkcs7_test.cc:1813: Failure
Expected equality of these values:
  max_decrypt - 1
    Which is: 79
  decrypted_len
    Which is: 78
...

I suspect this is due to a different flavor of the same MMA defense edge case accounted for on L1812 of the test -- random occurrence of valid PKCS#7 ciphertext padding (note that this is about padding for symmetrically encrypted content, not the asymmetric key encryption attacked by MMA). Originally, we accounted for one byte of randomly valid padding (i.e. 0x01) occurring with probability $\frac{1}{16} = 6.25$ percent of runs for AES (16 bytes is AES block size). Two bytes of randomly valid padding would be 0x02 0x02 occurring with probability $\frac{1}{16^2} \approx 0.4$ percent of runs -- not common, but not rare. I'll think about how we can better account for this in our MMA countermeasure tests...

crypto/pkcs7/pkcs7.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
Comment on lines +1135 to +1132
for (size_t i = 0; i < sk_X509_num(certs); i++) {
x509 = sk_X509_value(certs, i);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenSSL's documentation makes the following statement:

Only RSA keys are supported in PKCS#7 and envelopedData so the recipient certificates supplied to this function must all contain RSA public keys, though they do not have to be signed using the RSA algorithm.

Does that apply to our implementation as well?

Copy link
Contributor Author

@WillChilds-Klein WillChilds-Klein Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Of all the EVP_PKEY_METHODSs, RSA is the only one where encrypt/decrypt apply. KEMs are similar, but they have distinct encaps/decaps functions and don't define encrypt/decrypt.

crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
include/openssl/pkcs7.h Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
crypto/pkcs7/pkcs7.c Show resolved Hide resolved
if (!pkcs7_decrypt_rinfo(&tmp_cek, ri, pkey)) {
goto err;
}
// Set |cek| to the first successful decryption and keep going
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, OpenSSL seems not to be doing that, isn't that a behavioral difference between the two libraries?

Copy link
Contributor Author

@WillChilds-Klein WillChilds-Klein Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, they use the last successfully decrypted key not the first. we should probably conform to OpenSSL's behavior.

but it looks like they might have a memory leak in that case. we'll avoid that.

crypto/pkcs7/pkcs7.c Show resolved Hide resolved
dkostic
dkostic previously approved these changes Nov 22, 2024
Comment on lines +226 to +229
// BIO_get_cipher_status returns 1 if the cipher is in a healthy state or 0
// otherwise. Unhealthy state could indicate decryption failure or other
// abnormalities. Data read from an unhealthy cipher should not be considered
// authentic.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit just in case we misuse this in the future, but we can include this in the next PR.

Suggested change
// BIO_get_cipher_status returns 1 if the cipher is in a healthy state or 0
// otherwise. Unhealthy state could indicate decryption failure or other
// abnormalities. Data read from an unhealthy cipher should not be considered
// authentic.
// BIO_get_cipher_status returns 1 if the cipher is in a healthy state or 0
// otherwise. A negative value could be returned if |b| is in an uninitialized
// state. Unhealthy state could indicate decryption failure or other
// abnormalities. Data read from an unhealthy cipher should not be considered
// authentic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. will incorporate in next PR.

@WillChilds-Klein WillChilds-Klein merged commit f14cc9a into aws:main Nov 25, 2024
114 of 117 checks passed
@WillChilds-Klein WillChilds-Klein deleted the pkcs7-bio-encrypt branch November 25, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants