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 36c329394c44e816d87018252aabaad0be1da8dd..2270e63d51d4eacca2f60167d0d554cc535b30e9 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;
     }
@@ -277,9 +284,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))
@@ -347,6 +374,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 81b45e54e553cc38c5d43337f4b1178e31701073..4e58da2b755ce2e68a573ac82b31ba396a611b44 100644 (file)
--- a/test/asn1_internal_test.c
+++ b/test/asn1_internal_test.c
@@ -274,6 +274,22 @@ static int test_obj_nid_undef(void)
     return 1;
 }
 
+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);
@@ -283,5 +299,6 @@ int setup_tests(void)
     ADD_TEST(test_invalid_utf8);
     ADD_TEST(test_obj_create);
     ADD_TEST(test_obj_nid_undef);
+    ADD_TEST(test_mbstring_ncopy);
     return 1;
 }