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>

4 files changed, 69 insertions, 0 deletions

diff --git a/crypto/asn1/CMakeLists.txt b/crypto/asn1/CMakeLists.txt
index 41e31224..df48e26d 100644
--- a/crypto/asn1/CMakeLists.txt
+++ b/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)
diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c
index 08fdbac5..0025a676 100644
--- a/crypto/asn1/asn1_lib.c
+++ b/crypto/asn1/asn1_lib.c
@@ -160,6 +160,11 @@ int ASN1_get_object(const unsigned char **pp, long *plength, int *ptag,
         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 00000000..1044f7b2
--- /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 d666c710..b5ae324d 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 @@ 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>;