Address second round of PR feedback

aws · Sep 5, 2024 · 402538c · 402538c
1 parent 9f5500d
commit 402538c
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 6 deletions.
diff --git a/crypto/pkcs7/pkcs7.c b/crypto/pkcs7/pkcs7.c
@@ -209,6 +209,7 @@ int PKCS7_set_type(PKCS7 *p7, int type) {
     switch (type) {
     case NID_pkcs7_signed:
         p7->type = obj;
+        PKCS7_SIGNED_free(p7->d.sign);
         p7->d.sign = PKCS7_SIGNED_new();
         if (p7->d.sign == NULL) {
             return 0;
@@ -221,6 +222,7 @@ int PKCS7_set_type(PKCS7 *p7, int type) {
         break;
     case NID_pkcs7_digest:
         p7->type = obj;
+        PKCS7_DIGEST_free(p7->d.digest);
         p7->d.digest = PKCS7_DIGEST_new();
         if (p7->d.digest == NULL) {
             return 0;
@@ -233,13 +235,15 @@ int PKCS7_set_type(PKCS7 *p7, int type) {
         break;
     case NID_pkcs7_data:
         p7->type = obj;
+        ASN1_OCTET_STRING_free(p7->d.data);
         p7->d.data = ASN1_OCTET_STRING_new();
         if (p7->d.data == NULL) {
             return 0;
         }
         break;
     case NID_pkcs7_signedAndEnveloped:
         p7->type = obj;
+        PKCS7_SIGN_ENVELOPE_free(p7->d.signed_and_enveloped);
         p7->d.signed_and_enveloped = PKCS7_SIGN_ENVELOPE_new();
         if (p7->d.signed_and_enveloped == NULL) {
             return 0;
@@ -253,6 +257,7 @@ int PKCS7_set_type(PKCS7 *p7, int type) {
         break;
     case NID_pkcs7_enveloped:
         p7->type = obj;
+        PKCS7_ENVELOPE_free(p7->d.enveloped);
         p7->d.enveloped = PKCS7_ENVELOPE_new();
         if (p7->d.enveloped == NULL) {
             return 0;
@@ -266,6 +271,7 @@ int PKCS7_set_type(PKCS7 *p7, int type) {
         break;
     case NID_pkcs7_encrypted:
         p7->type = obj;
+        PKCS7_ENCRYPT_free(p7->d.encrypted);
         p7->d.encrypted = PKCS7_ENCRYPT_new();
         if (p7->d.encrypted == NULL) {
             return 0;
@@ -479,7 +485,7 @@ STACK_OF(PKCS7_SIGNER_INFO) *PKCS7_get_signer_info(PKCS7 *p7) {
 int PKCS7_SIGNER_INFO_set(PKCS7_SIGNER_INFO *p7i, X509 *x509, EVP_PKEY *pkey,
                           const EVP_MD *dgst) {
     /* We now need to add another PKCS7_SIGNER_INFO entry */
-    if (!p7i || !dgst || !pkey || !dgst) {
+    if (!p7i || !x509 || !pkey || !dgst) {
         OPENSSL_PUT_ERROR(PKCS7, ERR_R_PASSED_NULL_PARAMETER);
         return 0;
     } else if (!ASN1_INTEGER_set(p7i->version, 1)) {

diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc
@@ -1141,6 +1141,9 @@ TEST(PKCS7Test, GettersSetters) {
     p7.reset(PKCS7_new());
     ASSERT_TRUE(p7);
     EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_signed));
+    // set type redundantly to ensure we're properly freeing up existing
+    // resources on subsequent set.
+    EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_signed));
     EXPECT_TRUE(PKCS7_type_is_signed(p7.get()));
     EXPECT_TRUE(PKCS7_content_new(p7.get(), NID_pkcs7_signed));
     EXPECT_FALSE(PKCS7_set_cipher(p7.get(), EVP_aes_128_gcm()));
@@ -1150,6 +1153,9 @@ TEST(PKCS7Test, GettersSetters) {
     p7.reset(PKCS7_new());
     ASSERT_TRUE(p7);
     EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_digest));
+    // set type redundantly to ensure we're properly freeing up existing
+    // resources on subsequent set.
+    EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_digest));
     EXPECT_TRUE(PKCS7_type_is_digest(p7.get()));
     EXPECT_TRUE(PKCS7_content_new(p7.get(), NID_pkcs7_digest));
     EXPECT_FALSE(PKCS7_add_certificate(p7.get(), nullptr));
@@ -1160,29 +1166,43 @@ TEST(PKCS7Test, GettersSetters) {
     p7.reset(PKCS7_new());
     ASSERT_TRUE(p7.get());
     EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_data));
+    // set type redundantly to ensure we're properly freeing up existing
+    // resources on subsequent set.
+    EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_data));
     EXPECT_TRUE(PKCS7_type_is_data(p7.get()));
     EXPECT_FALSE(PKCS7_set_content(p7.get(), p7.get()));
 
     p7.reset(PKCS7_new());
     ASSERT_TRUE(p7.get());
     EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_signedAndEnveloped));
+    // set type redundantly to ensure we're properly freeing up existing
+    // resources on subsequent set.
+    EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_signedAndEnveloped));
     EXPECT_TRUE(PKCS7_type_is_signedAndEnveloped(p7.get()));
     EXPECT_TRUE(PKCS7_set_cipher(p7.get(), EVP_aes_128_gcm()));
     EXPECT_FALSE(PKCS7_set_content(p7.get(), p7.get()));
 
     p7.reset(PKCS7_new());
     ASSERT_TRUE(p7.get());
     EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_enveloped));
+    // set type redundantly to ensure we're properly freeing up existing
+    // resources on subsequent set.
+    EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_enveloped));
     EXPECT_TRUE(PKCS7_type_is_enveloped(p7.get()));
     EXPECT_TRUE(PKCS7_set_cipher(p7.get(), EVP_aes_128_gcm()));
     EXPECT_FALSE(PKCS7_set_content(p7.get(), p7.get()));
 
     p7.reset(PKCS7_new());
     ASSERT_TRUE(p7.get());
     EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_encrypted));
+    // set type redundantly to ensure we're properly freeing up existing
+    // resources on subsequent set.
+    EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_encrypted));
     EXPECT_TRUE(PKCS7_type_is_encrypted(p7.get()));
     EXPECT_FALSE(PKCS7_set_content(p7.get(), p7.get()));
 
+    // |d2i_*| functions advance the input reference by number of bytes parsed,
+    // so save off a local reference and reset it for each test case.
     const uint8_t *p7_der = kPKCS7SignedWithSignerInfo;
     const size_t p7_der_len = sizeof(kPKCS7SignedWithSignerInfo);
     p7.reset(d2i_PKCS7(nullptr, &p7_der, p7_der_len));

diff --git a/crypto/pkcs7/pkcs7_x509.c b/crypto/pkcs7/pkcs7_x509.c
@@ -459,8 +459,7 @@ int PKCS7_add_certificate(PKCS7 *p7, X509 *x509)
         return 0;
     }
 
-    if (!sk_X509_insert(*sk, x509, 0)) {
-        X509_free(x509);
+    if (!sk_X509_push(*sk, x509)) {
         return 0;
     }
     X509_up_ref(x509);
@@ -498,7 +497,6 @@ int PKCS7_add_crl(PKCS7 *p7, X509_CRL *crl)
     }
 
     if (!sk_X509_CRL_push(*sk, crl)) {
-        X509_CRL_free(crl);
         return 0;
     }
     X509_CRL_up_ref(crl);

diff --git a/include/openssl/pkcs7.h b/include/openssl/pkcs7.h
@@ -233,8 +233,8 @@ OPENSSL_EXPORT int PKCS7_content_new(PKCS7 *p7, int nid);
 OPENSSL_EXPORT int PKCS7_set_cipher(PKCS7 *p7, const EVP_CIPHER *cipher);
 
 // PKCS7_set_content sets |p7_data| as content on |p7| for applicable types of
-// |p7|: signedData and digestData. It frees any existing content on |p7|,
-// returning 1 on success and 0 on failure.
+// |p7|: signedData and digestData. |p7_data| may be NULL. It frees any
+// existing content on |p7|, returning 1 on success and 0 on failure.
 OPENSSL_EXPORT int PKCS7_set_content(PKCS7 *p7, PKCS7 *p7_data);
 
 // PKCS7_set_type instantiates |p7| as type |type|. It returns 1 on success and