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>
diff --git a/crypto/asn1/a_mbstr.c b/crypto/asn1/a_mbstr.c
index b004665..5853b72 100644
--- a/crypto/asn1/a_mbstr.c
+++ b/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 @@
}
/* 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 @@
return -1;
}
-/* Return 1 if the character is permitted in a PrintableString */
-static int is_printable(uint32_t value)
+int asn1_is_printable(uint32_t value)
{
if (value > 0x7f) {
return 0;
diff --git a/crypto/asn1/a_print.c b/crypto/asn1/a_print.c
index 2104521..c7efede 100644
--- a/crypto/asn1/a_print.c
+++ b/crypto/asn1/a_print.c
@@ -56,38 +56,28 @@
#include <openssl/asn1.h>
-#include <openssl/err.h>
-#include <openssl/mem.h>
+#include <string.h>
+
+#include "internal.h"
+
int ASN1_PRINTABLE_type(const unsigned char *s, int len)
{
- int c;
- int ia5 = 0;
- int t61 = 0;
-
- if (len <= 0)
- len = -1;
- if (s == NULL)
- return (V_ASN1_PRINTABLESTRING);
-
- while ((*s) && (len-- != 0)) {
- c = *(s++);
- if (!(((c >= 'a') && (c <= 'z')) ||
- ((c >= 'A') && (c <= 'Z')) ||
- (c == ' ') ||
- ((c >= '0') && (c <= '9')) ||
- (c == ' ') || (c == '\'') ||
- (c == '(') || (c == ')') ||
- (c == '+') || (c == ',') ||
- (c == '-') || (c == '.') ||
- (c == '/') || (c == ':') || (c == '=') || (c == '?')))
- ia5 = 1;
- if (c & 0x80)
- t61 = 1;
+ if (len < 0) {
+ len = strlen((const char *)s);
}
- if (t61)
- return (V_ASN1_T61STRING);
- if (ia5)
- return (V_ASN1_IA5STRING);
- return (V_ASN1_PRINTABLESTRING);
+
+ int printable = 1;
+ for (int i = 0; i < len; i++) {
+ unsigned char c = s[i];
+ if (c & 0x80) {
+ /* No need to continue iterating. */
+ return V_ASN1_T61STRING;
+ }
+ if (!asn1_is_printable(c)) {
+ printable = 0;
+ }
+ }
+
+ return printable ? V_ASN1_PRINTABLESTRING : V_ASN1_IA5STRING;
}
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 3e835db..28a5998 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -1046,6 +1046,27 @@
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)
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h
index 4e42c70..f40aa86 100644
--- a/crypto/asn1/internal.h
+++ b/crypto/asn1/internal.h
@@ -150,6 +150,10 @@
* 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 */
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 45a9794..497ad7c 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -1118,9 +1118,11 @@
int len, const char *sn,
const char *ln);
-// General
-// given a string, return the correct type, max is the maximum length
-OPENSSL_EXPORT int ASN1_PRINTABLE_type(const unsigned char *s, int max);
+// ASN1_PRINTABLE_type interprets |len| bytes from |s| as a Latin-1 string. It
+// returns the first of |V_ASN1_PRINTABLESTRING|, |V_ASN1_IA5STRING|, or
+// |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);
