From 6b7525a9fab38f408651bf5c39804c65103969f6 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 26 Aug 2021 12:00:28 -0400 Subject: [PATCH] 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 Reviewed-by: Adam Langley --- crypto/asn1/a_mbstr.c | 7 ++---- crypto/asn1/a_print.c | 48 ++++++++++++++++------------------------ crypto/asn1/asn1_test.cc | 21 ++++++++++++++++++ crypto/asn1/internal.h | 4 ++++ include/openssl/asn1.h | 8 ++++--- 5 files changed, 51 insertions(+), 37 deletions(-) diff --git a/crypto/asn1/a_mbstr.c b/crypto/asn1/a_mbstr.c index b0046651b..5853b7235 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 @@ 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 */ -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 2104521e6..c7efede8a 100644 --- a/crypto/asn1/a_print.c +++ b/crypto/asn1/a_print.c @@ -56,38 +56,28 @@ #include -#include -#include +#include + +#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); + if (len < 0) { + len = strlen((const char *)s); + } - 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; + 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; + } } - if (t61) - return (V_ASN1_T61STRING); - if (ia5) - return (V_ASN1_IA5STRING); - return (V_ASN1_PRINTABLESTRING); + + return printable ? V_ASN1_PRINTABLESTRING : V_ASN1_IA5STRING; } diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 3e835db88..28a59985d 100644 --- a/crypto/asn1/asn1_test.cc +++ b/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 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 4e42c7053..f40aa860f 100644 --- a/crypto/asn1/internal.h +++ b/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 */ diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index 45a97942f..497ad7c29 100644 --- a/include/openssl/asn1.h +++ b/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 -// 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);