Make the ASN1_TYPE-level type take precedence over the ASN1_STRING one
It is not very difficult to get an ASN1_TYPE that is self-inconsistent,
where the ASN1_TYPE-level type says one thing and the ASN1_STRING-level
type says another.
In particular, a not entirely unreasonable caller pattern used in
Android does this when filling in X509_ALGOR parameters. The old
ASN1_TYPE encoder took the ASN1_TYPE type, but the new one takes the
ASN1_STRING type.
Match the old behavior. We probably should also make ASN1_TYPE_set0
internally fix the type of the ASN1_STRING, but I haven't done that
here. I think, either way, we should restore the old behavior of using
the outer type. With the mess of exposed types and ambiguous APIs in
OpenSSL, it's not too hard to manually construct something invalid here.
Bug: 446993031
Change-Id: Idd5c0356769d8ff9a034222a9ffc107ce7c6f02e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82187
Reviewed-by: Lily Chen <chlily@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/asn1/a_type.cc b/crypto/asn1/a_type.cc
index b714340..26d8e26 100644
--- a/crypto/asn1/a_type.cc
+++ b/crypto/asn1/a_type.cc
@@ -346,6 +346,9 @@
}
}
+static int asn1_marshal_string_with_type(CBB *out, const ASN1_STRING *in,
+ int type);
+
int asn1_marshal_any(CBB *out, const ASN1_TYPE *in) {
switch (in->type) {
case V_ASN1_OBJECT:
@@ -374,7 +377,10 @@
case V_ASN1_SEQUENCE:
case V_ASN1_SET:
case V_ASN1_OTHER:
- return asn1_marshal_any_string(out, in->value.asn1_string);
+ // If |in->type| and the underlying |ASN1_STRING| type don't match, use
+ // |in->type|. See b/446993031.
+ return asn1_marshal_string_with_type(out, in->value.asn1_string,
+ in->type);
default:
// |ASN1_TYPE|s can have type -1 when default-constructed.
OPENSSL_PUT_ERROR(ASN1, ASN1_R_WRONG_TYPE);
@@ -382,8 +388,9 @@
}
}
-int asn1_marshal_any_string(CBB *out, const ASN1_STRING *in) {
- switch (in->type) {
+static int asn1_marshal_string_with_type(CBB *out, const ASN1_STRING *in,
+ int type) {
+ switch (type) {
case V_ASN1_INTEGER:
case V_ASN1_NEG_INTEGER:
return asn1_marshal_integer(out, in, CBS_ASN1_INTEGER);
@@ -420,3 +427,7 @@
return 0;
}
}
+
+int asn1_marshal_any_string(CBB *out, const ASN1_STRING *in) {
+ return asn1_marshal_string_with_type(out, in, in->type);
+}
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index be865ce..9136c0e 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -2076,6 +2076,47 @@
EXPECT_EQ(-1, i2d_DIRECTORYSTRING(obj.get(), nullptr));
}
+TEST(ASN1Test, TypeMismatch) {
+ // Pack PSS parameters into an |ASN1_STRING|. This makes an OCTET STRING.
+ bssl::UniquePtr<RSA_PSS_PARAMS> pss(RSA_PSS_PARAMS_new());
+ ASSERT_TRUE(pss);
+ bssl::UniquePtr<ASN1_STRING> str(
+ ASN1_item_pack(pss.get(), ASN1_ITEM_rptr(RSA_PSS_PARAMS), nullptr));
+ ASSERT_TRUE(str);
+ EXPECT_EQ(ASN1_STRING_type(str.get()), V_ASN1_OCTET_STRING);
+
+ // Pass this to |X509_ALGOR_set0| as a |V_ASN1_SEQUENCE|, which uses the
+ // |ASN1_TYPE_set0| calling convention. This leads to an ambiguous state:
+ // whether this should be a SEQUENCE or OCTET STRING value.
+ bssl::UniquePtr<X509_ALGOR> alg(X509_ALGOR_new());
+ ASSERT_TRUE(alg);
+ ASSERT_TRUE(X509_ALGOR_set0(alg.get(), OBJ_nid2obj(NID_rsassaPss),
+ V_ASN1_SEQUENCE, str.release()));
+
+ // It should be interpreted as a SEQUENCE value and serialize in this way.
+ //
+ // SEQUENCE {
+ // # rsassa-pss
+ // OBJECT_IDENTIFIER { 1.2.840.113549.1.1.10 }
+ // SEQUENCE {}
+ // }
+ //
+ // TODO(davidben): This currently works because |asn1_marshal_any| is careful
+ // to use the |ASN1_TYPE|-level type rather than the |ASN1_STRING|-level type.
+ // Should we also make |ASN1_TYPE_set0| fix the |ASN1_STRING| so fewer
+ // |ASN1_TYPE|s break invariants?
+ static const uint8_t kExpected[] = {0x30, 0x0d, 0x06, 0x09, 0x2a,
+ 0x86, 0x48, 0x86, 0xf7, 0x0d,
+ 0x01, 0x01, 0x0a, 0x30, 0x00};
+ TestSerialize(alg.get(), i2d_X509_ALGOR, kExpected);
+
+ // Even if |X509_ALGOR_set0| is fixed to maintain invariants, enough types are
+ // publicly exposed that we should make sure we handle the invalid state. This
+ // test is currently a no-op, but will not be if the above TODO is resolved.
+ alg->parameter->value.asn1_string->type = V_ASN1_OCTET_STRING;
+ TestSerialize(alg.get(), i2d_X509_ALGOR, kExpected);
+}
+
TEST(ASN1Test, StringTableSorted) {
const ASN1_STRING_TABLE *table;
size_t table_len;
@@ -2130,6 +2171,7 @@
bssl::UniquePtr<ASN1_STRING> str(
ASN1_item_pack(val.get(), ASN1_ITEM_rptr(BASIC_CONSTRAINTS), nullptr));
ASSERT_TRUE(str);
+ EXPECT_EQ(ASN1_STRING_type(str.get()), V_ASN1_OCTET_STRING);
EXPECT_EQ(
Bytes(ASN1_STRING_get0_data(str.get()), ASN1_STRING_length(str.get())),
Bytes(kExpected));
@@ -2138,6 +2180,7 @@
str.reset(ASN1_item_pack(val.get(), ASN1_ITEM_rptr(BASIC_CONSTRAINTS), &raw));
ASSERT_TRUE(str);
EXPECT_EQ(raw, str.get());
+ EXPECT_EQ(ASN1_STRING_type(str.get()), V_ASN1_OCTET_STRING);
EXPECT_EQ(
Bytes(ASN1_STRING_get0_data(str.get()), ASN1_STRING_length(str.get())),
Bytes(kExpected));
@@ -2148,6 +2191,7 @@
EXPECT_TRUE(
ASN1_item_pack(val.get(), ASN1_ITEM_rptr(BASIC_CONSTRAINTS), &raw));
EXPECT_EQ(raw, str.get());
+ EXPECT_EQ(ASN1_STRING_type(str.get()), V_ASN1_OCTET_STRING);
EXPECT_EQ(
Bytes(ASN1_STRING_get0_data(str.get()), ASN1_STRING_length(str.get())),
Bytes(kExpected));
