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>

crypto/asn1/asn1_test.cc

@@ -1023,6 +1023,18 @@ TEST(ASN1Test, MBString) {
   }
 }
 
+// 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)

crypto/asn1/tasn_enc.c

@@ -525,6 +525,20 @@ static int asn1_ex_i2c(ASN1_VALUE **pval, unsigned char *cout, int *putype,
         /* 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 */