Rewrite ASN1_PRINTABLE_type and add tests.

The old loop read one byte past the length. It also stopped the loop
too early on interior NUL. See also upstream's
https://github.com/openssl/openssl/pull/16433, though I've opted to
rewrite the function entirely rather than use their fix.

Also deduplicate the PrintableString check.

Change-Id: Ia8bd282047c2a2ed1d5e71a68a3947c7c108df95
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49066
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

crypto/asn1/a_mbstr.c

@@ -66,8 +66,6 @@
 #include "internal.h"
 #include "../bytestring/internal.h"
 
-static int is_printable(uint32_t value);
-
 /*
  * These functions take a string in UTF8, ASCII or multibyte form and a mask
  * of permissible ASN1 string types. It then works out the minimal type
@@ -153,7 +151,7 @@ int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len,
         }
 
         /* Update which output formats are still possible. */
-        if ((mask & B_ASN1_PRINTABLESTRING) && !is_printable(c)) {
+        if ((mask & B_ASN1_PRINTABLESTRING) && !asn1_is_printable(c)) {
             mask &= ~B_ASN1_PRINTABLESTRING;
         }
         if ((mask & B_ASN1_IA5STRING) && (c > 127)) {
@@ -285,8 +283,7 @@ int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len,
     return -1;
 }
 
-/* Return 1 if the character is permitted in a PrintableString */
+int asn1_is_printable(uint32_t value)
-static int is_printable(uint32_t value)
 {
     if (value > 0x7f) {
         return 0;

crypto/asn1/a_print.c

@@ -56,38 +56,28 @@
 
 #include <openssl/asn1.h>
 
-#include <openssl/err.h>
+#include <string.h>
-#include <openssl/mem.h>
+
+#include "internal.h"
+
 
 int ASN1_PRINTABLE_type(const unsigned char *s, int len)
 {
-    int c;
+    if (len < 0) {
-    int ia5 = 0;
+        len = strlen((const char *)s);
-    int t61 = 0;
+    }
-
-    if (len <= 0)
-        len = -1;
-    if (s == NULL)
-        return (V_ASN1_PRINTABLESTRING);
 
-    while ((*s) && (len-- != 0)) {
+    int printable = 1;
-        c = *(s++);
+    for (int i = 0; i < len; i++) {
-        if (!(((c >= 'a') && (c <= 'z')) ||
+        unsigned char c = s[i];
-              ((c >= 'A') && (c <= 'Z')) ||
+        if (c & 0x80) {
-              (c == ' ') ||
+            /* No need to continue iterating. */
-              ((c >= '0') && (c <= '9')) ||
+            return V_ASN1_T61STRING;
-              (c == ' ') || (c == '\'') ||
-              (c == '(') || (c == ')') ||
-              (c == '+') || (c == ',') ||
-              (c == '-') || (c == '.') ||
-              (c == '/') || (c == ':') || (c == '=') || (c == '?')))
-            ia5 = 1;
-        if (c & 0x80)
-            t61 = 1;
         }
-    if (t61)
+        if (!asn1_is_printable(c)) {
-        return (V_ASN1_T61STRING);
+            printable = 0;
-    if (ia5)
+        }
-        return (V_ASN1_IA5STRING);
+    }
-    return (V_ASN1_PRINTABLESTRING);
+
+    return printable ? V_ASN1_PRINTABLESTRING : V_ASN1_IA5STRING;
 }

crypto/asn1/asn1_test.cc

@@ -1046,6 +1046,27 @@ TEST(ASN1Test, NegativeEnumeratedMultistring) {
   TestSerialize(str.get(), i2d_ASN1_PRINTABLE, kMinusOne);
 }
 
+TEST(ASN1Test, PrintableType) {
+  const struct {
+    std::vector<uint8_t> in;
+    int result;
+  } kTests[] = {
+      {{}, V_ASN1_PRINTABLESTRING},
+      {{'a', 'A', '0', '\'', '(', ')', '+', ',', '-', '.', '/', ':', '=', '?'},
+       V_ASN1_PRINTABLESTRING},
+      {{'*'}, V_ASN1_IA5STRING},
+      {{'\0'}, V_ASN1_IA5STRING},
+      {{'\0', 'a'}, V_ASN1_IA5STRING},
+      {{0, 1, 2, 3, 125, 126, 127}, V_ASN1_IA5STRING},
+      {{0, 1, 2, 3, 125, 126, 127, 128}, V_ASN1_T61STRING},
+      {{128, 0, 1, 2, 3, 125, 126, 127}, V_ASN1_T61STRING},
+  };
+  for (const auto &t : kTests) {
+    SCOPED_TRACE(Bytes(t.in));
+    EXPECT_EQ(t.result, ASN1_PRINTABLE_type(t.in.data(), t.in.size()));
+  }
+}
+
 // The ASN.1 macros do not work on Windows shared library builds, where usage of
 // |OPENSSL_EXPORT| is a bit stricter.
 #if !defined(OPENSSL_WINDOWS) || !defined(BORINGSSL_SHARED_LIBRARY)

crypto/asn1/internal.h

@@ -150,6 +150,10 @@ int asn1_enc_save(ASN1_VALUE **pval, const unsigned char *in, int inlen,
  * a pointer. */
 const void *asn1_type_value_as_pointer(const ASN1_TYPE *a);
 
+/* asn1_is_printable returns one if |value| is a valid Unicode codepoint for an
+ * ASN.1 PrintableString, and zero otherwise. */
+int asn1_is_printable(uint32_t value);
+
 
 #if defined(__cplusplus)
 }  /* extern C */

include/openssl/asn1.h

@@ -1118,9 +1118,11 @@ OPENSSL_EXPORT ASN1_OBJECT *ASN1_OBJECT_create(int nid,
                                                int len, const char *sn,
                                                const char *ln);
 
-// General
+// ASN1_PRINTABLE_type interprets |len| bytes from |s| as a Latin-1 string. It
-// given a string, return the correct type, max is the maximum length
+// returns the first of |V_ASN1_PRINTABLESTRING|, |V_ASN1_IA5STRING|, or
-OPENSSL_EXPORT int ASN1_PRINTABLE_type(const unsigned char *s, int max);
+// |V_ASN1_T61STRING| that can represent every character. If |len| is negative,
+// |strlen(s)| is used instead.
+OPENSSL_EXPORT int ASN1_PRINTABLE_type(const unsigned char *s, int len);
 
 OPENSSL_EXPORT unsigned long ASN1_tag2bit(int tag);