commit c9b75aff28a9dac52bc92987f7c4288fbf17dc33
author David Benjamin <davidben@google.com> Mon Sep 06 17:44:37 2021 -0400
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Sep 09 19:42:17 2021 +0000
tree 10c52291f592ebb01f99117af2e33de9b177049d
parent 6e70be0f872a3a6c090a29311f17abad698d4233

Reject -1 types in ASN1_TYPE and MSTRINGs when encoding.

See https://github.com/openssl/openssl/issues/16538

Update-Note: A default-constructed object with a required ANY or
string-like CHOICE field cannot be encoded until the field is specified.
Note this affects i2d_X509: notBefore and notAfter are string-like
CHOICEs in OpenSSL.

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

crypto/asn1/asn1_test.cc

diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index e1c9cbd..5cd17af 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -1096,6 +1096,29 @@
   EXPECT_EQ(-1, i2d_X509_ALGOR(alg.get(), nullptr));
 }
 
+// Encoding invalid |ASN1_TYPE|s should fail. |ASN1_TYPE|s are
+// default-initialized to an invalid type.
+TEST(ASN1Test, InvalidASN1Type) {
+  bssl::UniquePtr<ASN1_TYPE> obj(ASN1_TYPE_new());
+  ASSERT_TRUE(obj);
+  EXPECT_EQ(-1, obj->type);
+  EXPECT_EQ(-1, i2d_ASN1_TYPE(obj.get(), nullptr));
+}
+
+// Encoding invalid MSTRING types should fail. An MSTRING is a CHOICE of
+// string-like types. They are initialized to an invalid type.
+TEST(ASN1Test, InvalidMSTRING) {
+  bssl::UniquePtr<ASN1_STRING> obj(ASN1_TIME_new());
+  ASSERT_TRUE(obj);
+  EXPECT_EQ(-1, obj->type);
+  EXPECT_EQ(-1, i2d_ASN1_TIME(obj.get(), nullptr));
+
+  obj.reset(DIRECTORYSTRING_new());
+  ASSERT_TRUE(obj);
+  EXPECT_EQ(-1, obj->type);
+  EXPECT_EQ(-1, i2d_DIRECTORYSTRING(obj.get(), nullptr));
+}
+
 // 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/tasn_enc.c

diff --git a/crypto/asn1/tasn_enc.c b/crypto/asn1/tasn_enc.c
index 49b79d2..7e79da2 100644
--- a/crypto/asn1/tasn_enc.c
+++ b/crypto/asn1/tasn_enc.c
@@ -554,6 +554,11 @@
         /* If MSTRING type set the underlying type */
         strtmp = (ASN1_STRING *)*pval;
         utype = strtmp->type;
+        if (utype < 0 && utype != V_ASN1_OTHER) {
+            /* MSTRINGs can have type -1 when default-constructed. */
+            OPENSSL_PUT_ERROR(ASN1, ASN1_R_WRONG_TYPE);
+            return -1;
+        }
         /* Negative INTEGER and ENUMERATED values use |ASN1_STRING| type values
          * that do not match their corresponding utype values. INTEGERs cannot
          * participate in MSTRING types, but ENUMERATEDs can.
@@ -574,6 +579,11 @@
         ASN1_TYPE *typ;
         typ = (ASN1_TYPE *)*pval;
         utype = typ->type;
+        if (utype < 0 && utype != V_ASN1_OTHER) {
+            /* |ASN1_TYPE|s can have type -1 when default-constructed. */
+            OPENSSL_PUT_ERROR(ASN1, ASN1_R_WRONG_TYPE);
+            return -1;
+        }
         *putype = utype;
         pval = &typ->value.asn1_value;
     } else