Check for invalid CHOICE selectors in i2d functions.
This handles normal CHOICE types. A follow-up CL will handle MSTRING and
ANY types.
Update-Note: An invalid CHOICE object (e.g. GENERAL_NAME) will now fail
when encoded, rather than be silently omitted. In particular, CHOICE
objects are default-initialized by tasn_new.c in an empty -1 state.
Structures containing a required CHOICE field can no longer be encoded
without filling in the CHOICE.
Bug: 429
Change-Id: I7011deadf518ddc344a56b07a0e268ceaae17fe0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49347
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 28a5998..d93e704 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -1067,6 +1067,22 @@
}
}
+// Encoding a CHOICE type with an invalid selector should fail.
+TEST(ASN1Test, InvalidChoice) {
+ bssl::UniquePtr<GENERAL_NAME> name(GENERAL_NAME_new());
+ ASSERT_TRUE(name);
+ // CHOICE types are initialized with an invalid selector.
+ EXPECT_EQ(-1, name->type);
+ // |name| should fail to encode.
+ EXPECT_EQ(-1, i2d_GENERAL_NAME(name.get(), nullptr));
+
+ // The error should be propagated through types containing |name|.
+ bssl::UniquePtr<GENERAL_NAMES> names(GENERAL_NAMES_new());
+ ASSERT_TRUE(names);
+ EXPECT_TRUE(bssl::PushToStack(names.get(), std::move(name)));
+ EXPECT_EQ(-1, i2d_GENERAL_NAMES(names.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)
diff --git a/crypto/asn1/tasn_enc.c b/crypto/asn1/tasn_enc.c
index 51a860d..6f35820 100644
--- a/crypto/asn1/tasn_enc.c
+++ b/crypto/asn1/tasn_enc.c
@@ -145,7 +145,7 @@
}
return asn1_i2d_ex_primitive(pval, out, it, -1, aclass);
- case ASN1_ITYPE_CHOICE:
+ case ASN1_ITYPE_CHOICE: {
/*
* It never makes sense for CHOICE types to have implicit tagging, so if
* tag != -1, then this looks like an error in the template.
@@ -157,19 +157,19 @@
if (asn1_cb && !asn1_cb(ASN1_OP_I2D_PRE, pval, it, NULL))
return -1;
i = asn1_get_choice_selector(pval, it);
- if ((i >= 0) && (i < it->tcount)) {
- ASN1_VALUE **pchval;
- const ASN1_TEMPLATE *chtt;
- chtt = it->templates + i;
- pchval = asn1_get_field_ptr(pval, chtt);
- return asn1_template_ex_i2d(pchval, out, chtt, -1, aclass);
- }
- /* Fixme: error condition if selector out of range. Additionally,
- * |asn1_cb| is currently called only on error when it should be called
- * on success. */
- if (asn1_cb && !asn1_cb(ASN1_OP_I2D_POST, pval, it, NULL))
+ if (i < 0 || i >= it->tcount) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_NO_MATCHING_CHOICE_TYPE);
return -1;
- return 0;
+ }
+ const ASN1_TEMPLATE *chtt = it->templates + i;
+ ASN1_VALUE **pchval = asn1_get_field_ptr(pval, chtt);
+ int ret = asn1_template_ex_i2d(pchval, out, chtt, -1, aclass);
+ if (ret < 0 ||
+ (asn1_cb && !asn1_cb(ASN1_OP_I2D_POST, pval, it, NULL))) {
+ return -1;
+ }
+ return ret;
+ }
case ASN1_ITYPE_EXTERN:
/* If new style i2d it does all the work */
