Fix negative ENUMERATED values in multi-strings.
I noticed this while I was reading through the encoder. OpenSSL's ASN.1
library is very sloppy when it comes to reusing enums. It has...
- Universal tag numbers. These are just tag numbers from ASN.1
- utype. These are used in the ASN1_TYPE type field, as well as the
ASN1_ITEM utype fields They are the same as universal tag numbers,
except non-universal types map to V_ASN1_OTHER. I believe ASN1_TYPE
types and ASN1_ITEM utypes are the same, but I am not positive.
- ASN1_STRING types. These are the same as utypes, except V_ASN1_OTHER
appears to only be possible when embedded inside ASN1_TYPE, and
negative INTEGER and ENUMERATED values get mapped to
V_ASN1_NEG_INTEGER and V_ASN1_NEG_ENUMERATED. Additionally, some
values like V_ASN1_OBJECT are possible in a utype but not possible in
an ASN1_STRING (and will cause lots of problems if ever placed in
one).
- Sometimes one of these enums is augmented with V_ASN1_UNDEF and/or
V_ASN1_APP_CHOOSE for extra behaviors.
- Probably others I'm missing.
These get mixed up all the time. asn1_ex_i2c's MSTRING path converts
from ASN1_STRING type to utype and forgets to normalize V_ASN1_NEG_*.
This means that negative INTEGERs and ENUMERATEDs in MSTRINGs do not get
encoded right.
The negative INTEGER case is unreachable (unless the caller passes
the wrong ASN1_STRING to an MSTRING i2d function, but mismatching i2d
functions generally does wrong things), but the negative ENUMERATED case
is reachable. Fix this and add a test.
Change-Id: I762d482e72ebf03fd64bba291e751ab0b51af2a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48805
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 6fa3f95..e6847c8 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -1023,6 +1023,18 @@
}
}
+// Test that multi-string types correctly encode negative ENUMERATED.
+// Multi-string types cannot contain INTEGER, so we only test ENUMERATED.
+TEST(ASN1Test, NegativeEnumeratedMultistring) {
+ static const uint8_t kMinusOne[] = {0x0a, 0x01, 0xff}; // ENUMERATED { -1 }
+ // |ASN1_PRINTABLE| is a multi-string type that allows ENUMERATED.
+ const uint8_t *p = kMinusOne;
+ bssl::UniquePtr<ASN1_STRING> str(
+ d2i_ASN1_PRINTABLE(nullptr, &p, sizeof(kMinusOne)));
+ ASSERT_TRUE(str);
+ TestSerialize(str.get(), i2d_ASN1_PRINTABLE, kMinusOne);
+}
+
// 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 95d8611..142de6d 100644
--- a/crypto/asn1/tasn_enc.c
+++ b/crypto/asn1/tasn_enc.c
@@ -525,6 +525,20 @@
/* If MSTRING type set the underlying type */
strtmp = (ASN1_STRING *)*pval;
utype = strtmp->type;
+ /* 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.
+ *
+ * TODO(davidben): Is this a bug? Although arguably one of the MSTRING
+ * types should contain more values, rather than less. See
+ * https://crbug.com/boringssl/412. But it is not possible to fit all
+ * possible ANY values into an |ASN1_STRING|, so matching the spec here
+ * is somewhat hopeless. */
+ if (utype == V_ASN1_NEG_INTEGER) {
+ utype = V_ASN1_INTEGER;
+ } else if (utype == V_ASN1_NEG_ENUMERATED) {
+ utype = V_ASN1_ENUMERATED;
+ }
*putype = utype;
} else if (it->utype == V_ASN1_ANY) {
/* If ANY set type and pointer to value */
