Work around broken Estonian smart cards. Again.

Estonian IDs issued between September 2014 to September 2015 are broken and use
negative moduli. They last five years and are common enough that we need to
work around this bug.

Add parallel "buggy" versions of BN_cbs2unsigned and RSA_parse_public_key which
tolerate this mistake, to align with OpenSSL's previous behavior. This code is
currently hooked up to rsa_pub_decode in RSA_ASN1_METHOD so that d2i_X509 is
tolerant. (This isn't a huge deal as the rest of that stack still uses the
legacy ASN.1 code which is overly lenient in many other ways.)

In future, when Chromium isn't using crypto/x509 and has more unified
certificate handling code, we can put client certificates under a slightly
different codepath, so this needn't hold for all certificates forever. Then in
September 2019, when the broken Estonian certificates all expire, we can purge
this codepath altogether.

BUG=532048

Change-Id: Iadb245048c71dba2eec45dd066c4a6e077140751
Reviewed-on: https://boringssl-review.googlesource.com/5894
Reviewed-by: Adam Langley <agl@google.com>
(cherry picked from commit 231cb8214511ea5784dd94a59f3e5f5fb7d39f8e)
diff --git a/crypto/bn/bn_asn1.c b/crypto/bn/bn_asn1.c
index 5c47a06..a148afc 100644
--- a/crypto/bn/bn_asn1.c
+++ b/crypto/bn/bn_asn1.c
@@ -25,10 +25,12 @@
     OPENSSL_PUT_ERROR(BN, BN_R_BAD_ENCODING);
     return 0;
   }
+
   if (CBS_data(&child)[0] & 0x80) {
     OPENSSL_PUT_ERROR(BN, BN_R_NEGATIVE_NUMBER);
     return 0;
   }
+
   /* INTEGERs must be minimal. */
   if (CBS_data(&child)[0] == 0x00 &&
       CBS_len(&child) > 1 &&
@@ -36,6 +38,33 @@
     OPENSSL_PUT_ERROR(BN, BN_R_BAD_ENCODING);
     return 0;
   }
+
+  return BN_bin2bn(CBS_data(&child), CBS_len(&child), ret) != NULL;
+}
+
+int BN_cbs2unsigned_buggy(CBS *cbs, BIGNUM *ret) {
+  CBS child;
+  if (!CBS_get_asn1(cbs, &child, CBS_ASN1_INTEGER) ||
+      CBS_len(&child) == 0) {
+    OPENSSL_PUT_ERROR(BN, BN_R_BAD_ENCODING);
+    return 0;
+  }
+
+  /* INTEGERs must be minimal. */
+  if (CBS_data(&child)[0] == 0x00 &&
+      CBS_len(&child) > 1 &&
+      !(CBS_data(&child)[1] & 0x80)) {
+    OPENSSL_PUT_ERROR(BN, BN_R_BAD_ENCODING);
+    return 0;
+  }
+
+  /* This function intentionally does not reject negative numbers. Estonian IDs
+   * issued between September 2014 to September 2015 are broken and use negative
+   * moduli. They last five years and are common enough that we need to work
+   * around this bug. See https://crbug.com/532048.
+   *
+   * TODO(davidben): Remove this code and callers in September 2019 when all the
+   * bad certificates have expired. */
   return BN_bin2bn(CBS_data(&child), CBS_len(&child), ret) != NULL;
 }
 
diff --git a/crypto/bn/bn_test.cc b/crypto/bn/bn_test.cc
index 74299c5..0e52119 100644
--- a/crypto/bn/bn_test.cc
+++ b/crypto/bn/bn_test.cc
@@ -1714,12 +1714,17 @@
     {"\x03\x01\x00", 3},
     // Empty contents.
     {"\x02\x00", 2},
-    // Negative number.
-    {"\x02\x01\x80", 3},
-    // Leading zeros.
+    // Unnecessary leading zeros.
     {"\x02\x02\x00\x01", 4},
 };
 
+// kASN1NegativeTests are encodings of negative numbers and how
+// |BN_cbs2unsigned_buggy| should interpret them.
+static const ASN1Test kASN1NegativeTests[] = {
+    {"128", "\x02\x01\x80", 3},
+    {"255", "\x02\x01\xff", 3},
+};
+
 static bool test_asn1() {
   for (const ASN1Test &test : kASN1Tests) {
     ScopedBIGNUM bn = ASCIIToBIGNUM(test.value_ascii);
@@ -1760,6 +1765,17 @@
       fprintf(stderr, "Bad serialization.\n");
       return false;
     }
+
+    // |BN_cbs2unsigned_buggy| parses all valid input.
+    CBS_init(&cbs, reinterpret_cast<const uint8_t*>(test.der), test.der_len);
+    if (!BN_cbs2unsigned_buggy(&cbs, bn2.get()) || CBS_len(&cbs) != 0) {
+      fprintf(stderr, "Parsing ASN.1 INTEGER failed.\n");
+      return false;
+    }
+    if (BN_cmp(bn.get(), bn2.get()) != 0) {
+      fprintf(stderr, "Bad parse.\n");
+      return false;
+    }
   }
 
   for (const ASN1InvalidTest &test : kASN1InvalidTests) {
@@ -1774,6 +1790,48 @@
       return false;
     }
     ERR_clear_error();
+
+    // All tests in kASN1InvalidTests are also rejected by
+    // |BN_cbs2unsigned_buggy|.
+    CBS_init(&cbs, reinterpret_cast<const uint8_t*>(test.der), test.der_len);
+    if (BN_cbs2unsigned_buggy(&cbs, bn.get())) {
+      fprintf(stderr, "Parsed invalid input.\n");
+      return false;
+    }
+    ERR_clear_error();
+  }
+
+  for (const ASN1Test &test : kASN1NegativeTests) {
+    // Negative numbers are rejected by |BN_cbs2unsigned|.
+    ScopedBIGNUM bn(BN_new());
+    if (!bn) {
+      return false;
+    }
+
+    CBS cbs;
+    CBS_init(&cbs, reinterpret_cast<const uint8_t*>(test.der), test.der_len);
+    if (BN_cbs2unsigned(&cbs, bn.get())) {
+      fprintf(stderr, "Parsed invalid input.\n");
+      return false;
+    }
+    ERR_clear_error();
+
+    // However |BN_cbs2unsigned_buggy| accepts them.
+    ScopedBIGNUM bn2 = ASCIIToBIGNUM(test.value_ascii);
+    if (!bn2) {
+      return false;
+    }
+
+    CBS_init(&cbs, reinterpret_cast<const uint8_t*>(test.der), test.der_len);
+    if (!BN_cbs2unsigned_buggy(&cbs, bn.get()) || CBS_len(&cbs) != 0) {
+      fprintf(stderr, "Parsing (invalid) ASN.1 INTEGER failed.\n");
+      return false;
+    }
+
+    if (BN_cmp(bn.get(), bn2.get()) != 0) {
+      fprintf(stderr, "\"Bad\" parse.\n");
+      return false;
+    }
   }
 
   // Serializing negative numbers is not supported.
diff --git a/crypto/evp/p_rsa_asn1.c b/crypto/evp/p_rsa_asn1.c
index 544ff1b..94a967a 100644
--- a/crypto/evp/p_rsa_asn1.c
+++ b/crypto/evp/p_rsa_asn1.c
@@ -57,6 +57,7 @@
 
 #include <openssl/asn1.h>
 #include <openssl/asn1t.h>
+#include <openssl/bytestring.h>
 #include <openssl/digest.h>
 #include <openssl/err.h>
 #include <openssl/mem.h>
@@ -87,16 +88,26 @@
 static int rsa_pub_decode(EVP_PKEY *pkey, X509_PUBKEY *pubkey) {
   const uint8_t *p;
   int pklen;
-  RSA *rsa;
-
   if (!X509_PUBKEY_get0_param(NULL, &p, &pklen, NULL, pubkey)) {
     return 0;
   }
-  rsa = RSA_public_key_from_bytes(p, pklen);
-  if (rsa == NULL) {
-    OPENSSL_PUT_ERROR(EVP, ERR_R_RSA_LIB);
+
+  /* Estonian IDs issued between September 2014 to September 2015 are broken and
+   * use negative moduli. They last five years and are common enough that we
+   * need to work around this bug. See https://crbug.com/532048.
+   *
+   * TODO(davidben): Switch this to the strict version in September 2019 or when
+   * Chromium can force client certificates down a different codepath, whichever
+   * comes first. */
+  CBS cbs;
+  CBS_init(&cbs, p, pklen);
+  RSA *rsa = RSA_parse_public_key_buggy(&cbs);
+  if (rsa == NULL || CBS_len(&cbs) != 0) {
+    OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
+    RSA_free(rsa);
     return 0;
   }
+
   EVP_PKEY_assign_RSA(pkey, rsa);
   return 1;
 }
diff --git a/crypto/rsa/rsa_asn1.c b/crypto/rsa/rsa_asn1.c
index c62bcf0..3f20acb 100644
--- a/crypto/rsa/rsa_asn1.c
+++ b/crypto/rsa/rsa_asn1.c
@@ -69,15 +69,22 @@
 #include "internal.h"
 
 
-static int parse_integer(CBS *cbs, BIGNUM **out) {
+static int parse_integer_buggy(CBS *cbs, BIGNUM **out, int buggy) {
   assert(*out == NULL);
   *out = BN_new();
   if (*out == NULL) {
     return 0;
   }
+  if (buggy) {
+    return BN_cbs2unsigned_buggy(cbs, *out);
+  }
   return BN_cbs2unsigned(cbs, *out);
 }
 
+static int parse_integer(CBS *cbs, BIGNUM **out) {
+  return parse_integer_buggy(cbs, out, 0 /* not buggy */);
+}
+
 static int marshal_integer(CBB *cbb, BIGNUM *bn) {
   if (bn == NULL) {
     /* An RSA object may be missing some components. */
@@ -87,14 +94,14 @@
   return BN_bn2cbb(cbb, bn);
 }
 
-RSA *RSA_parse_public_key(CBS *cbs) {
+static RSA *parse_public_key(CBS *cbs, int buggy) {
   RSA *ret = RSA_new();
   if (ret == NULL) {
     return NULL;
   }
   CBS child;
   if (!CBS_get_asn1(cbs, &child, CBS_ASN1_SEQUENCE) ||
-      !parse_integer(&child, &ret->n) ||
+      !parse_integer_buggy(&child, &ret->n, buggy) ||
       !parse_integer(&child, &ret->e) ||
       CBS_len(&child) != 0) {
     OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_ENCODING);
@@ -104,6 +111,20 @@
   return ret;
 }
 
+RSA *RSA_parse_public_key(CBS *cbs) {
+  return parse_public_key(cbs, 0 /* not buggy */);
+}
+
+RSA *RSA_parse_public_key_buggy(CBS *cbs) {
+  /* Estonian IDs issued between September 2014 to September 2015 are broken and
+   * use negative moduli. They last five years and are common enough that we
+   * need to work around this bug. See https://crbug.com/532048.
+   *
+   * TODO(davidben): Remove this code and callers in September 2019 when all the
+   * bad certificates have expired. */
+  return parse_public_key(cbs, 1 /* buggy */);
+}
+
 RSA *RSA_public_key_from_bytes(const uint8_t *in, size_t in_len) {
   CBS cbs;
   CBS_init(&cbs, in, in_len);
diff --git a/crypto/rsa/rsa_test.cc b/crypto/rsa/rsa_test.cc
index 9d44e2c..d52b78b 100644
--- a/crypto/rsa/rsa_test.cc
+++ b/crypto/rsa/rsa_test.cc
@@ -60,6 +60,7 @@
 #include <string.h>
 
 #include <openssl/bn.h>
+#include <openssl/bytestring.h>
 #include <openssl/crypto.h>
 #include <openssl/err.h>
 #include <openssl/obj.h>
@@ -466,6 +467,34 @@
     0x34, 0x25, 0x11, 0x14,
 };
 
+// kEstonianRSAKey is an RSAPublicKey encoded with a negative modulus. See
+// https://crbug.com/532048.
+static const uint8_t kEstonianRSAKey[] = {
+    0x30, 0x82, 0x01, 0x09, 0x02, 0x82, 0x01, 0x00, 0x96, 0xa6, 0x2e, 0x9c,
+    0x4e, 0x6a, 0xc3, 0xcc, 0xcd, 0x8f, 0x70, 0xc3, 0x55, 0xbf, 0x5e, 0x9c,
+    0xd4, 0xf3, 0x17, 0xc3, 0x97, 0x70, 0xae, 0xdf, 0x12, 0x5c, 0x15, 0x80,
+    0x03, 0xef, 0x2b, 0x18, 0x9d, 0x6a, 0xcb, 0x52, 0x22, 0xc1, 0x81, 0xb8,
+    0x7e, 0x61, 0xe8, 0x0f, 0x79, 0x24, 0x0f, 0x82, 0x70, 0x24, 0x4e, 0x29,
+    0x20, 0x05, 0x54, 0xeb, 0xd4, 0xa9, 0x65, 0x59, 0xb6, 0x3c, 0x75, 0x95,
+    0x2f, 0x4c, 0xf6, 0x9d, 0xd1, 0xaf, 0x5f, 0x14, 0x14, 0xe7, 0x25, 0xea,
+    0xa5, 0x47, 0x5d, 0xc6, 0x3e, 0x28, 0x8d, 0xdc, 0x54, 0x87, 0x2a, 0x7c,
+    0x10, 0xe9, 0xc6, 0x76, 0x2d, 0xe7, 0x79, 0xd8, 0x0e, 0xbb, 0xa9, 0xac,
+    0xb5, 0x18, 0x98, 0xd6, 0x47, 0x6e, 0x06, 0x70, 0xbf, 0x9e, 0x82, 0x25,
+    0x95, 0x4e, 0xfd, 0x70, 0xd7, 0x73, 0x45, 0x2e, 0xc1, 0x1f, 0x7a, 0x9a,
+    0x9d, 0x60, 0xc0, 0x1f, 0x67, 0x06, 0x2a, 0x4e, 0x87, 0x3f, 0x19, 0x88,
+    0x69, 0x64, 0x4d, 0x9f, 0x75, 0xf5, 0xd3, 0x1a, 0x41, 0x3d, 0x35, 0x17,
+    0xb6, 0xd1, 0x44, 0x0d, 0x25, 0x8b, 0xe7, 0x94, 0x39, 0xb0, 0x7c, 0xaf,
+    0x3e, 0x6a, 0xfa, 0x8d, 0x90, 0x21, 0x0f, 0x8a, 0x43, 0x94, 0x37, 0x7c,
+    0x2a, 0x15, 0x4c, 0xa0, 0xfa, 0xa9, 0x2f, 0x21, 0xa6, 0x6f, 0x8e, 0x2f,
+    0x89, 0xbc, 0xbb, 0x33, 0xf8, 0x31, 0xfc, 0xdf, 0xcd, 0x68, 0x9a, 0xbc,
+    0x75, 0x06, 0x95, 0xf1, 0x3d, 0xef, 0xca, 0x76, 0x27, 0xd2, 0xba, 0x8e,
+    0x0e, 0x1c, 0x43, 0xd7, 0x70, 0xb9, 0xc6, 0x15, 0xca, 0xd5, 0x4d, 0x87,
+    0xb9, 0xd1, 0xae, 0xde, 0x69, 0x73, 0x00, 0x2a, 0x97, 0x51, 0x4b, 0x30,
+    0x01, 0xc2, 0x85, 0xd0, 0x05, 0xcc, 0x2e, 0xe8, 0xc7, 0x42, 0xe7, 0x94,
+    0x51, 0xe3, 0xf5, 0x19, 0x35, 0xdc, 0x57, 0x96, 0xe7, 0xd9, 0xb4, 0x49,
+    0x02, 0x03, 0x01, 0x00, 0x01,
+};
+
 static bool TestRSA(const uint8_t *der, size_t der_len,
                     const uint8_t *oaep_ciphertext,
                     size_t oaep_ciphertext_len) {
@@ -790,6 +819,22 @@
   }
   ERR_clear_error();
 
+  // Public keys with negative moduli are invalid.
+  rsa.reset(RSA_public_key_from_bytes(kEstonianRSAKey,
+                                      sizeof(kEstonianRSAKey)));
+  if (rsa) {
+    return false;
+  }
+  ERR_clear_error();
+
+  // But |RSA_parse_public_key_buggy| will accept it.
+  CBS cbs;
+  CBS_init(&cbs, kEstonianRSAKey, sizeof(kEstonianRSAKey));
+  rsa.reset(RSA_parse_public_key_buggy(&cbs));
+  if (!rsa || CBS_len(&cbs) != 0) {
+    return false;
+  }
+
   return true;
 }
 
diff --git a/include/openssl/bn.h b/include/openssl/bn.h
index 7edb778..deacee0 100644
--- a/include/openssl/bn.h
+++ b/include/openssl/bn.h
@@ -304,6 +304,11 @@
  * result to |ret|. It returns one on success and zero on failure. */
 OPENSSL_EXPORT int BN_cbs2unsigned(CBS *cbs, BIGNUM *ret);
 
+/* BN_cbs2unsigned_buggy acts like |BN_cbs2unsigned| but tolerates negative
+ * INTEGERs. This is to work around buggy encoders which drop the 00 padding
+ * byte. Do not use this function. */
+OPENSSL_EXPORT int BN_cbs2unsigned_buggy(CBS *cbs, BIGNUM *ret);
+
 /* BN_bn2cbb marshals |bn| as a non-negative DER INTEGER and appends the result
  * to |cbb|. It returns one on success and zero on failure. */
 OPENSSL_EXPORT int BN_bn2cbb(CBB *cbb, const BIGNUM *bn);
diff --git a/include/openssl/rsa.h b/include/openssl/rsa.h
index 0d87f27..e4ec8f2 100644
--- a/include/openssl/rsa.h
+++ b/include/openssl/rsa.h
@@ -338,6 +338,10 @@
  * error. */
 OPENSSL_EXPORT RSA *RSA_parse_public_key(CBS *cbs);
 
+/* RSA_parse_public_key_buggy behaves like |RSA_parse_public_key|, but it
+ * tolerates negative moduli. Do not use this function. */
+OPENSSL_EXPORT RSA *RSA_parse_public_key_buggy(CBS *cbs);
+
 /* RSA_public_key_from_bytes parses |in| as a DER-encoded RSAPublicKey structure
  * (RFC 3447). It returns a newly-allocated |RSA| or NULL on error. */
 OPENSSL_EXPORT RSA *RSA_public_key_from_bytes(const uint8_t *in, size_t in_len);