ASN1_get_object should not accept large universal tags.

The high bits of the type get used for the V_ASN1_NEG bit, so when used with
ASN1_ANY/ASN1_TYPE, universal tags become ambiguous. This allows one to create
a negative zero, which should be impossible. Impose an upper bound on universal
tags accepted by crypto/asn1 and add a test.

BUG=590615

(Cherry-picked from fb2c6f8c8565e1e2d85c24408050c96521acbcdc.)

Change-Id: Ia988acf73fd11807869510a2b3825637a8d98853
Reviewed-on: https://boringssl-review.googlesource.com/7298
Reviewed-by: Adam Langley <agl@google.com>

crypto/asn1/CMakeLists.txt

@@ -43,3 +43,14 @@ add_library(
   x_bignum.c
   x_long.c
 )
+
+add_executable(
+  asn1_test
+
+  asn1_test.cc
+
+  $<TARGET_OBJECTS:test_support>
+)
+
+target_link_libraries(asn1_test crypto)
+add_dependencies(all_tests asn1_test)

crypto/asn1/a_int.c

@@ -125,6 +125,8 @@ int i2c_ASN1_INTEGER(ASN1_INTEGER *a, unsigned char **pp)
 		{
 		ret=a->length;
 		i=a->data[0];
+		if (ret == 1 && i == 0)
+			neg = 0;
 		if (!neg && (i > 127)) {
 			pad=1;
 			pb=0;
@@ -158,7 +160,7 @@ int i2c_ASN1_INTEGER(ASN1_INTEGER *a, unsigned char **pp)
 		p += a->length - 1;
 		i = a->length;
 		/* Copy zeros to destination as long as source is zero */
-		while(!*n) {
+		while(!*n && i > 1) {
 			*(p--) = 0;
 			n--;
 			i--;

crypto/asn1/asn1_lib.c

@@ -161,6 +161,11 @@ int ASN1_get_object(const unsigned char **pp, long *plength, int *ptag,
 		p++;
 		if (--max == 0) goto err;
 		}
+
+	/* To avoid ambiguity with V_ASN1_NEG, impose a limit on universal tags. */
+	if (xclass == V_ASN1_UNIVERSAL && tag > V_ASN1_MAX_UNIVERSAL)
+		goto err;
+
 	*ptag=tag;
 	*pclass=xclass;
 	if (!asn1_get_length(&p,&inf,plength,(int)max)) goto err;

crypto/asn1/asn1_test.cc

@@ -0,0 +1,51 @@
+/* Copyright (c) 2016, Google Inc.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
+ * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
+ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
+
+#include <stdio.h>
+
+#include <openssl/asn1.h>
+#include <openssl/crypto.h>
+#include <openssl/err.h>
+
+#include "../test/scoped_types.h"
+
+
+// kTag258 is an ASN.1 structure with a universal tag with number 258.
+static const uint8_t kTag258[] = {
+    0x1f, 0x82, 0x02, 0x01, 0x00,
+};
+
+static_assert(V_ASN1_NEG_INTEGER == 258,
+              "V_ASN1_NEG_INTEGER changed. Update kTag258 to collide with it.");
+
+bool TestLargeTags() {
+  const uint8_t *p = kTag258;
+  ScopedASN1_TYPE obj(d2i_ASN1_TYPE(NULL, &p, sizeof(kTag258)));
+  if (obj) {
+    fprintf(stderr, "Parsed value with illegal tag (type = %d).\n", obj->type);
+    return false;
+  }
+  return true;
+}
+
+int main() {
+  CRYPTO_library_init();
+
+  if (!TestLargeTags()) {
+    return 1;
+  }
+
+  printf("PASS\n");
+  return 0;
+}

crypto/evp/p_dsa_asn1.c

@@ -184,64 +184,20 @@ static int dsa_priv_decode(EVP_PKEY *pkey, PKCS8_PRIV_KEY_INFO *p8) {
   /* In PKCS#8 DSA: you just get a private key integer and parameters in the
    * AlgorithmIdentifier the pubkey must be recalculated. */
 
-  STACK_OF(ASN1_TYPE) *ndsa = NULL;
   DSA *dsa = NULL;
 
   if (!PKCS8_pkey_get0(NULL, &p, &pklen, &palg, p8)) {
     return 0;
   }
-  X509_ALGOR_get0(NULL, &ptype, &pval, palg);
-
-  /* Check for broken DSA PKCS#8, UGH! */
-  if (*p == (V_ASN1_SEQUENCE | V_ASN1_CONSTRUCTED)) {
-    ASN1_TYPE *t1, *t2;
-    ndsa = d2i_ASN1_SEQUENCE_ANY(NULL, &p, pklen);
-    if (ndsa == NULL) {
-      goto decerr;
-    }
-    if (sk_ASN1_TYPE_num(ndsa) != 2) {
-      goto decerr;
-    }
-
-    /* Handle Two broken types:
-     * SEQUENCE {parameters, priv_key}
-     * SEQUENCE {pub_key, priv_key}. */
-
-    t1 = sk_ASN1_TYPE_value(ndsa, 0);
-    t2 = sk_ASN1_TYPE_value(ndsa, 1);
-    if (t1->type == V_ASN1_SEQUENCE) {
-      p8->broken = PKCS8_EMBEDDED_PARAM;
-      pval = t1->value.ptr;
-    } else if (ptype == V_ASN1_SEQUENCE) {
-      p8->broken = PKCS8_NS_DB;
-    } else {
-      goto decerr;
-    }
-
-    if (t2->type != V_ASN1_INTEGER) {
-      goto decerr;
-    }
-
-    privkey = t2->value.integer;
-  } else {
-    const uint8_t *q = p;
-    privkey = d2i_ASN1_INTEGER(NULL, &p, pklen);
-    if (privkey == NULL) {
-      goto decerr;
-    }
-    if (privkey->type == V_ASN1_NEG_INTEGER) {
-      p8->broken = PKCS8_NEG_PRIVKEY;
-      ASN1_INTEGER_free(privkey);
-      privkey = d2i_ASN1_UINTEGER(NULL, &q, pklen);
-      if (privkey == NULL) {
-        goto decerr;
-      }
-    }
-    if (ptype != V_ASN1_SEQUENCE) {
-      goto decerr;
-    }
+  privkey = d2i_ASN1_INTEGER(NULL, &p, pklen);
+  if (privkey == NULL || privkey->type == V_ASN1_NEG_INTEGER) {
+    goto decerr;
   }
 
+  X509_ALGOR_get0(NULL, &ptype, &pval, palg);
+  if (ptype != V_ASN1_SEQUENCE) {
+    goto decerr;
+  }
   pstr = pval;
   pm = pstr->data;
   pmlen = pstr->length;
@@ -274,7 +230,6 @@ static int dsa_priv_decode(EVP_PKEY *pkey, PKCS8_PRIV_KEY_INFO *p8) {
 
   EVP_PKEY_assign_DSA(pkey, dsa);
   BN_CTX_free(ctx);
-  sk_ASN1_TYPE_pop_free(ndsa, ASN1_TYPE_free);
   ASN1_INTEGER_free(privkey);
 
   return 1;
@@ -285,7 +240,6 @@ decerr:
 dsaerr:
   BN_CTX_free(ctx);
   ASN1_INTEGER_free(privkey);
-  sk_ASN1_TYPE_pop_free(ndsa, ASN1_TYPE_free);
   DSA_free(dsa);
   return 0;
 }

crypto/test/scoped_types.h

@@ -21,6 +21,7 @@
 #include <memory>
 
 #include <openssl/aead.h>
+#include <openssl/asn1.h>
 #include <openssl/bio.h>
 #include <openssl/bn.h>
 #include <openssl/cmac.h>
@@ -95,6 +96,7 @@ class ScopedOpenSSLContext {
   T ctx_;
 };
 
+using ScopedASN1_TYPE = ScopedOpenSSLType<ASN1_TYPE, ASN1_TYPE_free>;
 using ScopedBIO = ScopedOpenSSLType<BIO, BIO_vfree>;
 using ScopedBIGNUM = ScopedOpenSSLType<BIGNUM, BN_free>;
 using ScopedBN_CTX = ScopedOpenSSLType<BN_CTX, BN_CTX_free>;

include/openssl/asn1.h

@@ -85,6 +85,9 @@ extern "C" {
 #define V_ASN1_ANY			-4	/* used in ASN1 template code */
 
 #define V_ASN1_NEG			0x100	/* negative flag */
+/* No supported universal tags may exceed this value, to avoid ambiguity with
+ * V_ASN1_NEG. */
+#define V_ASN1_MAX_UNIVERSAL		0xff
 
 #define V_ASN1_UNDEF			-1
 #define V_ASN1_EOC			0

util/all_tests.json

@@ -1,5 +1,6 @@
 [
 	["crypto/aes/aes_test"],
+	["crypto/asn1/asn1_test"],
 	["crypto/base64/base64_test"],
 	["crypto/bio/bio_test"],
 	["crypto/bn/bn_test"],