Fix X509_ALGOR_set_md()

This API allocates internally and can leave a corrupted |alg| behind. Change it to return an int so that callers can check for an error. Also fix its only caller in rsa_md_to_algor(). This is an ABI change but will not break any callers. Also add a small regress test for this API. Change-Id: I7a5d1729dcd4c7726c3d4ead3740d478231f3611 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67187 Commit-Queue: Theo Buehler <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Reviewed-by: David Benjamin <[email protected]> (cherry picked from commit c99364a313795b2baaa40bd0683a05ae2e1cd993)
aws · Dec 18, 2024 · c40aa2f · c40aa2f
1 parent e3a9ed5
commit c40aa2f
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 5 deletions.
diff --git a/crypto/x509/rsa_pss.c b/crypto/x509/rsa_pss.c
@@ -127,7 +127,11 @@ static int rsa_md_to_algor(X509_ALGOR **palg, const EVP_MD *md) {
   if (*palg == NULL) {
     return 0;
   }
-  X509_ALGOR_set_md(*palg, md);
+  if (!X509_ALGOR_set_md(*palg, md)) {
+    X509_ALGOR_free(*palg);
+    *palg = NULL;
+    return 0;
+  }
   return 1;
 }
 

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
@@ -3245,6 +3245,25 @@ TEST(X509Test, PrettyPrintIntegers) {
   }
 }
 
+TEST(X509Test, X509AlgorSetMd) {
+  bssl::UniquePtr<X509_ALGOR> alg(X509_ALGOR_new());
+  ASSERT_TRUE(alg);
+  EXPECT_TRUE(X509_ALGOR_set_md(alg.get(), EVP_sha256()));
+  const ASN1_OBJECT *obj;
+  const void *pval;
+  int ptype = 0;
+  X509_ALGOR_get0(&obj, &ptype, &pval, alg.get());
+  EXPECT_TRUE(obj);
+  EXPECT_EQ(OBJ_obj2nid(obj), NID_sha256);
+  EXPECT_EQ(ptype, V_ASN1_NULL); // OpenSSL has V_ASN1_UNDEF
+  EXPECT_EQ(pval, nullptr);
+  EXPECT_TRUE(X509_ALGOR_set_md(alg.get(), EVP_md5()));
+  X509_ALGOR_get0(&obj, &ptype, &pval, alg.get());
+  EXPECT_EQ(OBJ_obj2nid(obj), NID_md5);
+  EXPECT_EQ(ptype, V_ASN1_NULL);
+  EXPECT_EQ(pval, nullptr);
+}
+
 TEST(X509Test, X509NameSet) {
   bssl::UniquePtr<X509_NAME> name(X509_NAME_new());
   ASSERT_TRUE(name);

diff --git a/crypto/x509/x_algor.c b/crypto/x509/x_algor.c
@@ -123,7 +123,7 @@ void X509_ALGOR_get0(const ASN1_OBJECT **out_obj, int *out_param_type,
 
 // Set up an X509_ALGOR DigestAlgorithmIdentifier from an EVP_MD
 
-void X509_ALGOR_set_md(X509_ALGOR *alg, const EVP_MD *md) {
+int X509_ALGOR_set_md(X509_ALGOR *alg, const EVP_MD *md) {
   int param_type;
 
   if (EVP_MD_flags(md) & EVP_MD_FLAG_DIGALGID_ABSENT) {
@@ -132,7 +132,7 @@ void X509_ALGOR_set_md(X509_ALGOR *alg, const EVP_MD *md) {
     param_type = V_ASN1_NULL;
   }
 
-  X509_ALGOR_set0(alg, OBJ_nid2obj(EVP_MD_type(md)), param_type, NULL);
+  return X509_ALGOR_set0(alg, OBJ_nid2obj(EVP_MD_type(md)), param_type, NULL);
 }
 
 // X509_ALGOR_cmp returns 0 if |a| and |b| are equal and non-zero otherwise.

diff --git a/include/openssl/x509.h b/include/openssl/x509.h
@@ -2305,7 +2305,7 @@ OPENSSL_EXPORT void X509_ALGOR_get0(const ASN1_OBJECT **out_obj,
 
 // X509_ALGOR_set_md sets |alg| to the hash function |md|. Note this
 // AlgorithmIdentifier represents the hash function itself, not a signature
-// algorithm that uses |md|.
+// algorithm that uses |md|. It returns one on success and zero on error.
 //
 // Due to historical specification mistakes (see Section 2.1 of RFC 4055), the
 // parameters field is sometimes omitted and sometimes a NULL value. When used
@@ -2316,7 +2316,7 @@ OPENSSL_EXPORT void X509_ALGOR_get0(const ASN1_OBJECT **out_obj,
 //
 // TODO(davidben): Rename this function, or perhaps just add a bespoke API for
 // constructing PSS and move on.
-OPENSSL_EXPORT void X509_ALGOR_set_md(X509_ALGOR *alg, const EVP_MD *md);
+OPENSSL_EXPORT int X509_ALGOR_set_md(X509_ALGOR *alg, const EVP_MD *md);
 
 // X509_ALGOR_cmp returns zero if |a| and |b| are equal, and some non-zero value
 // otherwise. Note this function can only be used for equality checks, not an