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 Change-Id: I363e01ebfde621c8865101f5bcbd5f323fb59e79 Reviewed-on: https://boringssl-review.googlesource.com/7238 Reviewed-by: Adam Langley <agl@google.com>

diff --git a/crypto/asn1/CMakeLists.txt b/crypto/asn1/CMakeLists.txt
index 41e3122..df48e26 100644
--- a/crypto/asn1/CMakeLists.txt
+++ b/crypto/asn1/CMakeLists.txt

@@ -43,3 +43,14 @@
   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)

diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c
index 08fdbac..0025a67 100644
--- a/crypto/asn1/asn1_lib.c
+++ b/crypto/asn1/asn1_lib.c

@@ -160,6 +160,11 @@
         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))

diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
new file mode 100644
index 0000000..1044f7b
--- /dev/null
+++ b/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;
+}

diff --git a/crypto/test/scoped_types.h b/crypto/test/scoped_types.h
index d666c71..b5ae324 100644
--- a/crypto/test/scoped_types.h
+++ b/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 @@
   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>;

diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 63bde18..e27164e 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h

@@ -85,6 +85,9 @@
 #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

diff --git a/util/all_tests.json b/util/all_tests.json
index d67255f..bacee7a 100644
--- a/util/all_tests.json
+++ b/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"],