Suppress X509{,_REQ}_get1_email fallback to subject for malformed SAN

This has no impact on cert validation (a malformed SAN would already
cause validation to fail), but we might as well suppress the incorrect
result.

Bug: 491158075
Change-Id: I51fe41775d61e1143a553530c630c1646a6a6964
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/101027
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Lily Chen <chlily@google.com>

crypto/x509/v3_utl.cc

diff --git a/crypto/x509/v3_utl.cc b/crypto/x509/v3_utl.cc
index 765dfa6..324e149 100644
--- a/crypto/x509/v3_utl.cc
+++ b/crypto/x509/v3_utl.cc
@@ -498,8 +498,12 @@
 }
 
 STACK_OF(OPENSSL_STRING) *X509_get1_email(const X509 *x) {
+  int critical;
   UniquePtr<GENERAL_NAMES> gens(reinterpret_cast<GENERAL_NAMES *>(
-      X509_get_ext_d2i(x, NID_subject_alt_name, nullptr, nullptr)));
+      X509_get_ext_d2i(x, NID_subject_alt_name, &critical, nullptr)));
+  if (!gens && critical != -1) {
+    return nullptr;
+  }
   return get_email(X509_get_subject_name(x), gens.get()).release();
 }
 
@@ -526,8 +530,12 @@
 
 STACK_OF(OPENSSL_STRING) *X509_REQ_get1_email(const X509_REQ *x) {
   UniquePtr<STACK_OF(X509_EXTENSION)> exts(X509_REQ_get_extensions(x));
+  int critical;
   UniquePtr<GENERAL_NAMES> gens(reinterpret_cast<GENERAL_NAMES *>(
-      X509V3_get_d2i(exts.get(), NID_subject_alt_name, nullptr, nullptr)));
+      X509V3_get_d2i(exts.get(), NID_subject_alt_name, &critical, nullptr)));
+  if (!gens && critical != -1) {
+    return nullptr;
+  }
   return get_email(X509_REQ_get_subject_name(x), gens.get()).release();
 }
 

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index daaf20e..06a4a16 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -10485,6 +10485,41 @@
     std::sort(actual.begin(), actual.end());
     EXPECT_EQ(actual, expected);
   }
+
+  // An invalid SAN extension should cause X509_get1_email and
+  // X509_REQ_get1_email to fail.
+  {
+    cert = MakeTestCert("Issuer", "Subject", p256.get(), /*is_ca=*/false);
+    ASSERT_TRUE(cert);
+    ASSERT_TRUE(X509_set_subject_name(cert.get(), subject.get()));
+    UniquePtr<X509_EXTENSION> ext(X509_EXTENSION_new());
+    ASSERT_TRUE(ext);
+    // Set an invalid (empty) SAN.
+    ASSERT_TRUE(X509_EXTENSION_set_object(ext.get(),
+                                          OBJ_nid2obj(NID_subject_alt_name)));
+    ASSERT_TRUE(X509_add_ext(cert.get(), ext.get(), -1));
+    ASSERT_TRUE(X509_sign(cert.get(), p256.get(), EVP_sha256()));
+
+    EXPECT_EQ(nullptr, X509_get1_email(cert.get()));
+  }
+  {
+    req.reset(X509_REQ_new());
+    ASSERT_TRUE(req);
+    ASSERT_TRUE(X509_REQ_set_subject_name(req.get(), subject.get()));
+    ASSERT_TRUE(X509_REQ_set_pubkey(req.get(), p256.get()));
+    UniquePtr<X509_EXTENSION> ext(X509_EXTENSION_new());
+    ASSERT_TRUE(ext);
+    ASSERT_TRUE(X509_EXTENSION_set_object(ext.get(),
+                                          OBJ_nid2obj(NID_subject_alt_name)));
+    STACK_OF(X509_EXTENSION) *exts_raw = sk_X509_EXTENSION_new_null();
+    ASSERT_TRUE(exts_raw);
+    UniquePtr<STACK_OF(X509_EXTENSION)> exts(exts_raw);
+    ASSERT_TRUE(PushToStack(exts.get(), std::move(ext)));
+    ASSERT_TRUE(X509_REQ_add_extensions(req.get(), exts.get()));
+    ASSERT_TRUE(X509_REQ_sign(req.get(), p256.get(), EVP_sha256()));
+
+    EXPECT_EQ(nullptr, X509_REQ_get1_email(req.get()));
+  }
 }
 
 TEST(X509Test, GetOCSP) {