Make setting an X509_NAME to itself work

Although a pointless no-op,
X509_set_issuer_name(cert, X509_get_issuer_name(cert)) used to work.
https://boringssl-review.googlesource.com/c/boringssl/+/81894 broke
this.

Change-Id: I67c13ff9d4fe668f58fcf692d12131e269f3ab58
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82207
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Lily Chen <chlily@google.com>
Reviewed-by: Lily Chen <chlily@google.com>

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 5133d3c..445dc51 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -1089,6 +1089,14 @@
   return stack;
 }
 
+static bssl::Span<const uint8_t> ASN1StringAsBytes(const ASN1_STRING *str) {
+  return bssl::Span(ASN1_STRING_get0_data(str), ASN1_STRING_length(str));
+}
+
+static std::string_view ASN1StringAsView(const ASN1_STRING *str) {
+  return bssl::BytesAsStringView(ASN1StringAsBytes(str));
+}
+
 // CRLsToStack converts a vector of |X509_CRL*| to an OpenSSL
 // STACK_OF(X509_CRL), bumping the reference counts for each CRL in question.
 static bssl::UniquePtr<STACK_OF(X509_CRL)> CRLsToStack(
@@ -4358,8 +4366,7 @@
       ASSERT_TRUE(value);
       EXPECT_EQ(V_ASN1_BMPSTRING, value->type);
       EXPECT_EQ(Bytes(kTest1),
-                Bytes(ASN1_STRING_get0_data(value->value.bmpstring),
-                      ASN1_STRING_length(value->value.bmpstring)));
+                Bytes(ASN1StringAsBytes(value->value.bmpstring)));
 
       // |X509_ATTRIBUTE_get0_data| requires the type match.
       EXPECT_FALSE(
@@ -4367,8 +4374,7 @@
       const ASN1_BMPSTRING *bmpstring = static_cast<const ASN1_BMPSTRING *>(
           X509_ATTRIBUTE_get0_data(attr, idx, V_ASN1_BMPSTRING, nullptr));
       ASSERT_TRUE(bmpstring);
-      EXPECT_EQ(Bytes(kTest1), Bytes(ASN1_STRING_get0_data(bmpstring),
-                                     ASN1_STRING_length(bmpstring)));
+      EXPECT_EQ(Bytes(kTest1), Bytes(ASN1StringAsBytes(bmpstring)));
       idx++;
     }
 
@@ -4377,8 +4383,7 @@
       ASSERT_TRUE(value);
       EXPECT_EQ(V_ASN1_BMPSTRING, value->type);
       EXPECT_EQ(Bytes(kTest2),
-                Bytes(ASN1_STRING_get0_data(value->value.bmpstring),
-                      ASN1_STRING_length(value->value.bmpstring)));
+                Bytes(ASN1StringAsBytes(value->value.bmpstring)));
       idx++;
     }
 
@@ -5734,8 +5739,7 @@
       EXPECT_EQ(OBJ_obj2nid(X509_EXTENSION_get_object(ext)), exts[i].nid);
       EXPECT_EQ(X509_EXTENSION_get_critical(ext), exts[i].critical ? 1 : 0);
       const ASN1_OCTET_STRING *data = X509_EXTENSION_get_data(ext);
-      EXPECT_EQ(Bytes(ASN1_STRING_get0_data(data), ASN1_STRING_length(data)),
-                Bytes(exts[i].data));
+      EXPECT_EQ(Bytes(ASN1StringAsBytes(data)), Bytes(exts[i].data));
     }
   };
 
@@ -7454,8 +7458,7 @@
     EXPECT_EQ(Bytes(OBJ_get0_data(obj), OBJ_length(obj)), Bytes(kOID));
     const ASN1_STRING *value = X509_NAME_ENTRY_get_data(entry);
     EXPECT_EQ(ASN1_STRING_type(value), t.str_type);
-    EXPECT_EQ(Bytes(ASN1_STRING_get0_data(value), ASN1_STRING_length(value)),
-              Bytes(t.str_contents));
+    EXPECT_EQ(Bytes(ASN1StringAsBytes(value)), Bytes(t.str_contents));
 
     // The name should re-encode with the same input.
     uint8_t *der = nullptr;
@@ -8876,8 +8879,7 @@
       EXPECT_FALSE(oct);
     } else {
       ASSERT_TRUE(oct);
-      EXPECT_EQ(Bytes(t.out), Bytes(ASN1_STRING_get0_data(oct.get()),
-                                    ASN1_STRING_length(oct.get())));
+      EXPECT_EQ(Bytes(t.out), Bytes(ASN1StringAsBytes(oct.get())));
     }
   }
 }
@@ -9134,4 +9136,30 @@
                    /*intermediates=*/{}, /*crls=*/{}));
 }
 
+// Test that |X509_set_subject_name| on an |X509_NAME| that was already the
+// subject did not break.
+TEST(X509Test, SelfSetSubjectAndIssuer) {
+  bssl::UniquePtr<EVP_PKEY> key = PrivateKeyFromPEM(kP256Key);
+  ASSERT_TRUE(key);
+  bssl::UniquePtr<X509> cert =
+      MakeTestCert("Issuer", "Subject", key.get(), /*is_ca=*/true);
+
+  EXPECT_TRUE(
+      X509_set_issuer_name(cert.get(), X509_get_issuer_name(cert.get())));
+  EXPECT_TRUE(
+      X509_set_subject_name(cert.get(), X509_get_subject_name(cert.get())));
+
+  const X509_NAME *issuer = X509_get_issuer_name(cert.get());
+  EXPECT_EQ(X509_NAME_entry_count(issuer), 1);
+  const X509_NAME_ENTRY *entry = X509_NAME_get_entry(issuer, 0);
+  EXPECT_EQ(OBJ_obj2nid(X509_NAME_ENTRY_get_object(entry)), NID_commonName);
+  EXPECT_EQ("Issuer", ASN1StringAsView(X509_NAME_ENTRY_get_data(entry)));
+
+  const X509_NAME *subject = X509_get_subject_name(cert.get());
+  EXPECT_EQ(X509_NAME_entry_count(subject), 1);
+  entry = X509_NAME_get_entry(subject, 0);
+  EXPECT_EQ(OBJ_obj2nid(X509_NAME_ENTRY_get_object(entry)), NID_commonName);
+  EXPECT_EQ("Subject", ASN1StringAsView(X509_NAME_ENTRY_get_data(entry)));
+}
+
 }  // namespace

crypto/x509/x_name.cc

diff --git a/crypto/x509/x_name.cc b/crypto/x509/x_name.cc
index 54493cb..6b10219 100644
--- a/crypto/x509/x_name.cc
+++ b/crypto/x509/x_name.cc
@@ -268,6 +268,13 @@
   if (cache == nullptr) {
     return 0;
   }
+  // Callers sometimes try to set a name back to itself. We check this after
+  // |x509_name_get_cache| because, if |src| was so broken that it could not be
+  // serialized, we used to return an error. (It's not clear if this codepath is
+  // even possible.)
+  if (dst == src) {
+    return 1;
+  }
   CBS cbs;
   CBS_init(&cbs, cache->der, cache->der_len);
   if (!x509_parse_name(&cbs, dst)) {