Implement PKCS7_dataInit and PKCS7_dataFinal #1816

WillChilds-Klein · 2024-08-29T21:22:02Z

Notes

This PR adds a few new functions, most notably PKCS7_dataInit and PKCS7_dataFinal as they're used by ruby to prepare/finalize BIO chains for reading from or writing to a PKCS7 object. Their usage is documented here in pkcs7.h.

Testing

new unit tests
more comprehensive testing exercising dataInit/dataFinal for encryption/signing operations can be found here. those tests will be added in the next 2 PRs.

Coverage in this PR itself is a bit low due to the cipher-related initialization in PKCS7_dataInit and signature-related finalization in PKCS7_dataFinal. Coverage for these sections of code will be added in the next 2 PRs.

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 · 2024-08-29T21:43:20Z

Codecov Report

Attention: Patch coverage is 74.64387% with 89 lines in your changes missing coverage. Please review.

Project coverage is 78.88%. Comparing base (c9d48a6) to head (40696f9).
Report is 26 commits behind head on main.

Files with missing lines	Patch %	Lines
crypto/pkcs7/pkcs7.c	70.23%	89 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1816      +/-   ##
==========================================
+ Coverage   78.79%   78.88%   +0.09%     
==========================================
  Files         590      595       +5     
  Lines      101506   102430     +924     
  Branches    14401    14514     +113     
==========================================
+ Hits        79979    80805     +826     
- Misses      20891    20974      +83     
- Partials      636      651      +15

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

crypto/pkcs7/pkcs7.c

crypto/pkcs7/bio/cipher.c

samuel40791765 · 2024-11-13T22:50:03Z

crypto/pkcs7/pkcs7_test.cc

@@ -274,6 +274,215 @@ static const uint8_t kPKCS7NSS[] = {
    0x00, 0x00, 0x00,
 };

+static const uint8_t kPKCS7EnvelopedData[] = {


Just curious how this file was generated?

Great question. I snagged this from BouncyCastle. I'll drop a comment.

crypto/pkcs7/pkcs7_test.cc

samuel40791765 · 2024-11-13T23:21:26Z

crypto/pkcs7/pkcs7_test.cc

+  ASSERT_TRUE(PKCS7_get_PEM_certificates(certs.get(), bio.get()));
+  ASSERT_EQ(1U, sk_X509_num(certs.get()));
+  rsa_x509.reset(sk_X509_pop(certs.get()));
+  ASSERT_TRUE(X509_set_pubkey(rsa_x509.get(), rsa_pkey.get()));


When I parse kPEMCert, it seems like the cert already has a public key in it? Or are both fields actually referring to something different

... Subject Public Key Info: Public Key Algorithm: id-ecPublicKey Public-Key: (256 bit) pub: 04:ec:c7:40:2e:60:a4:71:14:5f:fe:dc:d0:6b:c7: ae:dc:9e:d2:e4:24:d0:6b:99:ec:d1:17:85:f6:4b:

it's the same field, but we don't have the corresponding private key for the pre-existing public key. i'd gotten in the habit of replacing the cert's public key with that of a freshly generated keypair to simplify sign/verify testing.

include/openssl/pkcs7.h

crypto/pkcs7/pkcs7.c

justsmth · 2024-11-15T17:32:58Z

crypto/pkcs7/pkcs7.c

+    OPENSSL_BEGIN_ALLOW_DEPRECATED
+    if (!PKCS7_is_detached(p7) && content && content->length > 0) {
+      OPENSSL_END_ALLOW_DEPRECATED


NP: Here and several other places -- Since the OPENSSL_XXX_ALLOW_DEPRECATED are not part of the logic, I'd not have them indented; just keep them at the start of their lines.

yeah, unfortunately this is clang-format's preference not mine. also unfortunately clang-format doesn't support next-line-only disabling so i've added off/on directives above/below these macro invocations.

justsmth · 2024-11-15T17:39:21Z

crypto/pkcs7/pkcs7.c

+err:
+  BIO_free_all(out);
+  BIO_free_all(btmp);
+  return NULL;


Do we have any tests that force an error in PKCS7_dataInit?

no, but we should. i've added a few and will add more when we address sign/verify + detached signatures.

crypto/pkcs7/pkcs7.c

justsmth · 2024-11-15T17:51:06Z

crypto/pkcs7/pkcs7.c

+      int sign_nid = OBJ_obj2nid(si->digest_alg->algorithm);
+      bio_tmp = PKCS7_find_digest(&md_ctx, bio_tmp, sign_nid);
+      if (bio_tmp == NULL) {
+        goto err;
+      }
+      // We now have the EVP_MD_CTX, lets do the signing.
+      if (!EVP_MD_CTX_copy_ex(md_ctx_tmp, md_ctx)) {
+        goto err;
+      }
+      // We don't currently support signed attributes
+      if (sk_X509_ATTRIBUTE_num(si->auth_attr) > 0) {
+        OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_PKCS7_DATASIGN);
+        goto err;
+      }
+      unsigned char *abuf = NULL;
+      unsigned int abuflen = EVP_PKEY_size(si->pkey);
+      if (abuflen == 0 || (abuf = OPENSSL_malloc(abuflen)) == NULL) {
+        goto err;
+      }
+      if (!EVP_SignFinal(md_ctx_tmp, abuf, &abuflen, si->pkey)) {
+        OPENSSL_free(abuf);
+        OPENSSL_PUT_ERROR(PKCS7, ERR_R_EVP_LIB);
+        goto err;
+      }
+      ASN1_STRING_set0(si->enc_digest, abuf, abuflen);


There's apparently no test coverage of the latter part of this loop.

Correct. that will come once we add PKCS7 verification and expand PKCS7 signing. that will happen 2 PRs from now. In the meantime, I have a "parent" PR here that combines all changes across the PR series (pre-cleanup for each individual PR, so larger and messier). The coverage report shows it's covered there, but we should drop a TODO here to cover (I generally avoid TODO's across commits, but there are currently none in the crypto/pkcs7/ directory, so it won't get lost in any noise).

crypto/pkcs7/pkcs7.c

WillChilds-Klein force-pushed the pkcs7-bio-data-init-final branch 3 times, most recently from 23e4d39 to 1b0c84d Compare September 5, 2024 19:57

WillChilds-Klein force-pushed the pkcs7-bio-data-init-final branch from 1b0c84d to b7ffbde Compare September 6, 2024 16:17

WillChilds-Klein force-pushed the pkcs7-bio-data-init-final branch from b7ffbde to 69cb0fd Compare October 22, 2024 14:31

WillChilds-Klein force-pushed the pkcs7-bio-data-init-final branch 5 times, most recently from a4a9d39 to f7c09cf Compare November 11, 2024 17:40

WillChilds-Klein changed the title ~~[DRAFT] Implement PKCS7_dataInit and PKCS7_dataFinal~~ Implement PKCS7_dataInit and PKCS7_dataFinal Nov 11, 2024

WillChilds-Klein added 6 commits November 11, 2024 17:47

Import code and tests from pkcs7-ruby branch

9d0aea3

Down to 1 memory leak

5e8e80d

Fix remaining memory leaks

e2811f4

Convert macros to concrete functions

bafb345

Clean up code style, formatting, etc.

02f113e

Add doc comments to pkcs7.h

aa9a47c

WillChilds-Klein force-pushed the pkcs7-bio-data-init-final branch from f7c09cf to aa9a47c Compare November 11, 2024 17:47

WillChilds-Klein marked this pull request as ready for review November 11, 2024 17:55

WillChilds-Klein requested a review from a team as a code owner November 11, 2024 17:55

WillChilds-Klein requested review from samuel40791765 and justsmth November 11, 2024 18:10

samuel40791765 reviewed Nov 14, 2024

View reviewed changes

PR feedback

82a0094

justsmth reviewed Nov 15, 2024

View reviewed changes

WillChilds-Klein requested a review from samuel40791765 November 15, 2024 19:35

Next round of PR comments

c1d46a8

WillChilds-Klein requested a review from justsmth November 15, 2024 19:55

Some compilers didnt like the inline declaration?

40696f9

justsmth approved these changes Nov 18, 2024

View reviewed changes

crypto/pkcs7/pkcs7.c Show resolved Hide resolved

crypto/pkcs7/pkcs7.c Show resolved Hide resolved

crypto/pkcs7/pkcs7.c Show resolved Hide resolved

samuel40791765 approved these changes Nov 18, 2024

View reviewed changes

WillChilds-Klein merged commit 318413c into aws:main Nov 18, 2024
116 of 117 checks passed

WillChilds-Klein deleted the pkcs7-bio-data-init-final branch November 18, 2024 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PKCS7_dataInit and PKCS7_dataFinal #1816

Implement PKCS7_dataInit and PKCS7_dataFinal #1816

WillChilds-Klein commented Aug 29, 2024 •

edited

Loading

codecov-commenter commented Aug 29, 2024 •

edited

Loading

samuel40791765 Nov 13, 2024

WillChilds-Klein Nov 15, 2024 •

edited

Loading

samuel40791765 Nov 13, 2024

WillChilds-Klein Nov 15, 2024

justsmth Nov 15, 2024

WillChilds-Klein Nov 15, 2024

justsmth Nov 15, 2024

WillChilds-Klein Nov 15, 2024

justsmth Nov 15, 2024

WillChilds-Klein Nov 15, 2024

Implement PKCS7_dataInit and PKCS7_dataFinal #1816

Implement PKCS7_dataInit and PKCS7_dataFinal #1816

Conversation

WillChilds-Klein commented Aug 29, 2024 • edited Loading

Notes

Testing

codecov-commenter commented Aug 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

WillChilds-Klein Nov 15, 2024 • edited Loading

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

WillChilds-Klein commented Aug 29, 2024 •

edited

Loading

codecov-commenter commented Aug 29, 2024 •

edited

Loading

WillChilds-Klein Nov 15, 2024 •

edited

Loading