Harden ASN1_mbstring_ncopy

Reported by Murali Aniruddhan

Signed-off-by: Norbert Pocs <norbertp@openssl.org>
Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/29376)

diff --git a/crypto/asn1/a_mbstr.c b/crypto/asn1/a_mbstr.c
index c290824d17152bc9dfa4c14fbe8bc1f1d8a63779..531e01d33257a165537b044df14a24f8c06b5c2f 100644 (file)
--- a/crypto/asn1/a_mbstr.c
+++ b/crypto/asn1/a_mbstr.c
@@ -114,7 +114,10 @@ int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len,
         return -1;
     }
 
-    /* Now work out output format and string type */
+    /*
+     * Now work out output format and string type.
+     * These checks should be in sync with the checks in type_str.
+     */
     outform = MBSTRING_ASC;
     if (mask & B_ASN1_NUMERICSTRING)
         str_type = V_ASN1_NUMERICSTRING;
@@ -182,7 +185,11 @@ int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len,
 
     case MBSTRING_UTF8:
         outlen = 0;
-        traverse_string(in, len, inform, out_utf8, &outlen);
+        ret = traverse_string(in, len, inform, out_utf8, &outlen);
+        if (ret < 0) {
+            ERR_raise(ERR_LIB_ASN1, ASN1_R_INVALID_UTF8STRING);
+            return -1;
+        }
         cpyfunc = cpy_utf8;
         break;
     }
@@ -278,9 +285,29 @@ static int out_utf8(unsigned long value, void *arg)
 
 static int type_str(unsigned long value, void *arg)
 {
-    unsigned long types = *((unsigned long *)arg);
+    unsigned long usable_types = *((unsigned long *)arg);
+    unsigned long types = usable_types;
     const int native = value > INT_MAX ? INT_MAX : ossl_fromascii(value);
 
+    /*
+     * Clear out all the types which are not checked later. If any of those
+     * is present in the mask, then the UTF8 type will be added and checked
+     * below.
+     */
+    types &= B_ASN1_NUMERICSTRING | B_ASN1_PRINTABLESTRING
+        | B_ASN1_IA5STRING | B_ASN1_T61STRING | B_ASN1_BMPSTRING
+        | B_ASN1_UNIVERSALSTRING | B_ASN1_UTF8STRING;
+
+    /*
+     * If any other types were in the input mask, they're effectively treated
+     * as UTF8
+     */
+    if (types != usable_types)
+        types |= B_ASN1_UTF8STRING;
+
+    /*
+     * These checks should be in sync with ASN1_mbstring_ncopy.
+     */
     if ((types & B_ASN1_NUMERICSTRING) && !(ossl_isdigit(native) || native == ' '))
         types &= ~B_ASN1_NUMERICSTRING;
     if ((types & B_ASN1_PRINTABLESTRING) && !ossl_isasn1print(native))
@@ -348,6 +375,8 @@ static int cpy_utf8(unsigned long value, void *arg)
     p = arg;
     /* We already know there is enough room so pass 0xff as the length */
     ret = UTF8_putc(*p, 0xff, value);
+    if (ret < 0)
+        return ret;
     *p += ret;
     return 1;
 }

diff --git a/test/asn1_internal_test.c b/test/asn1_internal_test.c
index 97f4b69ac44f2d3cbd33a316dc73847e64cd00e6..919c7aaf335e11bea2f237de10944f608825c66f 100644 (file)
--- a/test/asn1_internal_test.c
+++ b/test/asn1_internal_test.c
@@ -190,11 +190,28 @@ static int test_unicode_range(void)
     return ok;
 }
 
+static int test_mbstring_ncopy(void)
+{
+    ASN1_STRING *str = NULL;
+    const unsigned char in[] = { 0xFF, 0xFE, 0xFF, 0xFE };
+    int inlen = 4;
+    int inform = MBSTRING_UNIV;
+
+    if (!TEST_int_eq(ASN1_mbstring_ncopy(&str, in, inlen, inform, B_ASN1_GENERALSTRING, 0, 0), -1)
+        || !TEST_int_eq(ASN1_mbstring_ncopy(&str, in, inlen, inform, B_ASN1_VISIBLESTRING, 0, 0), -1)
+        || !TEST_int_eq(ASN1_mbstring_ncopy(&str, in, inlen, inform, B_ASN1_VIDEOTEXSTRING, 0, 0), -1)
+        || !TEST_int_eq(ASN1_mbstring_ncopy(&str, in, inlen, inform, B_ASN1_GENERALIZEDTIME, 0, 0), -1))
+        return 0;
+
+    return 1;
+}
+
 int setup_tests(void)
 {
     ADD_TEST(test_tbl_standard);
     ADD_TEST(test_standard_methods);
     ADD_TEST(test_empty_nonoptional_content);
     ADD_TEST(test_unicode_range);
+    ADD_TEST(test_mbstring_ncopy);
     return 1;
 }