Convert X509_NAME_get_text_by_[NID|OBJ] to return UTF-8

Callers to these functions are usually using them to grab
subject name components and universally use the result as a C
string, whereas the OpenSSL versions return raw ASN1_STRING
bytes and ignore the encoding, which "usually works" in a
"hold my beer here are some bytes" sort of way until the
object is not encoded as you hoped.

Make this safer for the callers by making the returned
result be at least "text" and a C string. This converts
the ASN1_STRING bytes to UTF-8, and will introduce new
failure cases for this function if either memory allocation
fails for the UTF-8 conversion, or if the resulting UTF-8
contains a 0 codepoint and would produce an artificially
truncated C string. Additionally if the provided buffer
is not NULL but is too small to hold the output, we fail
rather than returning a truncated output.

Fixed: 436
Change-Id: I487c10a5ff5188e94df520ef4c8982e593c680d7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58445
Reviewed-by: David Benjamin <[email protected]>
Commit-Queue: Bob Beck <[email protected]>
(cherry picked from commit 3763efb56b5282cf92d71c259576352555c1a8f8)

Author: Bob Beck

crypto/x509/x509_test.cc

@@ -6931,3 +6931,68 @@ TEST(X509Test, NameAttributeValues) {
     EXPECT_FALSE(name);
   }
 }
+
+TEST(X509Test, GetTextByOBJ) {
+  struct OBJTestCase {
+    const char *content;
+    int content_type;
+    int len;
+    int expected_result;
+    const char *expected_string;
+  } kTests[] = {
+      {"", V_ASN1_UTF8STRING, 0, 0, ""},
+      {"derp", V_ASN1_UTF8STRING, 4, 4, "derp"},
+      {"\x30\x00",  // Empty sequence can not be converted to UTF-8
+       V_ASN1_SEQUENCE, 2, -1, ""},
+      {
+          "der\0p",
+          V_ASN1_TELETEXSTRING,
+          5,
+          -1,
+          "",
+      },
+      {
+          "0123456789ABCDEF",
+          V_ASN1_IA5STRING,
+          16,
+          16,
+          "0123456789ABCDEF",
+      },
+      {
+          "\x07\xff",
+          V_ASN1_BMPSTRING,
+          2,
+          2,
+          "\xdf\xbf",
+      },
+      {
+          "\x00\xc3\x00\xaf",
+          V_ASN1_BMPSTRING,
+          4,
+          4,
+          "\xc3\x83\xc2\xaf",
+      },
+  };
+  for (const auto &test : kTests) {
+    bssl::UniquePtr<X509_NAME> name(X509_NAME_new());
+    ASSERT_TRUE(name);
+    ASSERT_TRUE(X509_NAME_add_entry_by_NID(
+        name.get(), NID_commonName, test.content_type,
+        reinterpret_cast<const uint8_t *>(test.content), test.len, /*loc=*/-1,
+        /*set=*/0));
+    char text[256] = {};
+    EXPECT_EQ(test.expected_result,
+              X509_NAME_get_text_by_NID(name.get(), NID_commonName, text,
+                                        sizeof(text)));
+    EXPECT_STREQ(text, test.expected_string);
+    if (test.expected_result > 0) {
+      // Test truncation. The function writes a trailing NUL byte so the
+      // buffer needs to be one bigger than the expected result.
+      char small[2] = "a";
+      EXPECT_EQ(
+          -1, X509_NAME_get_text_by_NID(name.get(), NID_commonName, small, 1));
+      // The buffer should be unmodified by truncation failure.
+      EXPECT_STREQ(small, "a");
+    }
+  }
+}

crypto/x509/x509name.c

@@ -57,6 +57,7 @@
 #include <string.h>
 
 #include <openssl/asn1.h>
+#include <openssl/bytestring.h>
 #include <openssl/err.h>
 #include <openssl/evp.h>
 #include <openssl/obj.h>
@@ -86,13 +87,34 @@ int X509_NAME_get_text_by_OBJ(const X509_NAME *name, const ASN1_OBJECT *obj,
   }
   const ASN1_STRING *data =
       X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, i));
-  i = (data->length > (len - 1)) ? (len - 1) : data->length;
-  if (buf == NULL) {
-    return data->length;
+  unsigned char *text = NULL;
+  int ret = -1;
+  int text_len = ASN1_STRING_to_UTF8(&text, data);
+  // Fail if we could not encode as UTF-8.
+  if (text_len < 0) {
+    goto out;
+  }
+  CBS cbs;
+  CBS_init(&cbs, text, text_len);
+  // Fail if the UTF-8 encoding constains a 0 byte because this is
+  // returned as a C string and callers very often do not check.
+  if (CBS_contains_zero_byte(&cbs)) {
+    goto out;
+  }
+  // We still support the "pass NULL to find out how much" API
+  if (buf != NULL) {
+    if (text_len >= len || len <= 0 ||
+        !CBS_copy_bytes(&cbs, (uint8_t *)buf, text_len)) {
+      goto out;
+    }
+    // It must be a C string
+    buf[text_len] = '\0';
   }
-  OPENSSL_memcpy(buf, data->data, i);
-  buf[i] = '\0';
-  return i;
+  ret = text_len;
+
+out:
+  OPENSSL_free(text);
+  return ret;
 }
 
 int X509_NAME_entry_count(const X509_NAME *name) {

include/openssl/x509.h

@@ -2105,20 +2105,22 @@ OPENSSL_EXPORT ASN1_TIME *X509_CRL_get_nextUpdate(X509_CRL *crl);
 OPENSSL_EXPORT ASN1_INTEGER *X509_get_serialNumber(X509 *x509);
 
 // X509_NAME_get_text_by_OBJ finds the first attribute with type |obj| in
-// |name|. If found, it ignores the value's ASN.1 type, writes the raw
-// |ASN1_STRING| representation to |buf|, followed by a NUL byte, and
-// returns the number of bytes in output, excluding the NUL byte.
-//
-// This function writes at most |len| bytes, including the NUL byte. If |len| is
-// not large enough, it silently truncates the output to fit. If |buf| is NULL,
-// it instead writes enough and returns the number of bytes in the output,
-// excluding the NUL byte.
-//
-// WARNING: Do not use this function. It does not return enough information for
-// the caller to correctly interpret its output. The attribute value may be of
-// any type, including one of several ASN.1 string encodings, but this function
-// only outputs the raw |ASN1_STRING| representation. See
-// https://crbug.com/boringssl/436.
+// |name|. If found, it writes the value's UTF-8 representation to |buf|.
+// followed by a NUL byte, and returns the number of bytes in the output,
+// excluding the NUL byte. This is unlike OpenSSL which returns the raw
+// ASN1_STRING data. The UTF-8 encoding of the |ASN1_STRING| may not contain a 0
+// codepoint.
+//
+// This function writes at most |len| bytes, including the NUL byte.  If |buf|
+// is NULL, it writes nothing and returns the number of bytes in the
+// output, excluding the NUL byte that would be required for the full UTF-8
+// output.
+//
+// This function may return -1 if an error occurs for any reason, including the
+// value not being a recognized string type, |len| being of insufficient size to
+// hold the full UTF-8 encoding and NUL byte, memory allocation failures, an
+// object with type |obj| not existing in |name|, or if the UTF-8 encoding of
+// the string contains a zero byte.
 OPENSSL_EXPORT int X509_NAME_get_text_by_OBJ(const X509_NAME *name,
                                              const ASN1_OBJECT *obj, char *buf,
                                              int len);