Work around even more Estonian ID card misissuances.

Not content with signing negative RSA moduli, still other Estonian IDs have too
many leading zeros. Work around those too.

This workaround will be removed in six months.

BUG=534766

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

crypto/bn/bn_asn1.c

@@ -25,10 +25,12 @@ int BN_cbs2unsigned(CBS *cbs, BIGNUM *ret) {
     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,23 @@ int BN_cbs2unsigned(CBS *cbs, BIGNUM *ret) {
     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;
+  }
+
+  /* This function intentionally does not reject negative numbers or non-minimal
+   * encodings. Estonian IDs issued between September 2014 to September 2015 are
+   * broken. See https://crbug.com/532048 and https://crbug.com/534766.
+   *
+   * TODO(davidben): Remove this code and callers in March 2016. */
   return BN_bin2bn(CBS_data(&child), CBS_len(&child), ret) != NULL;
 }
 

crypto/bn/bn_test.cc

@@ -1714,10 +1714,16 @@ static const ASN1InvalidTest kASN1InvalidTests[] = {
     {"\x03\x01\x00", 3},
     // Empty contents.
     {"\x02\x00", 2},
-    // Negative number.
-    {"\x02\x01\x80", 3},
-    // Leading zeros.
-    {"\x02\x02\x00\x01", 4},
+};
+
+// kASN1BuggyTests are incorrect encodings and how |BN_cbs2unsigned_buggy|
+// should interpret them.
+static const ASN1Test kASN1BuggyTests[] = {
+    // Negative numbers.
+    {"128", "\x02\x01\x80", 3},
+    {"255", "\x02\x01\xff", 3},
+    // Unnecessary leading zeros.
+    {"1", "\x02\x02\x00\x01", 4},
 };
 
 static bool test_asn1() {
@@ -1760,6 +1766,17 @@ static bool test_asn1() {
       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 +1791,48 @@ static bool test_asn1() {
       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 : kASN1BuggyTests) {
+    // These broken encodings 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.

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,25 @@ static int rsa_pub_encode(X509_PUBKEY *pk, const EVP_PKEY *pkey) {
 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. See https://crbug.com/532048 and https://crbug.com/534766.
+   *
+   * TODO(davidben): Switch this to the strict version in March 2016 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;
 }

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 @@ static int marshal_integer(CBB *cbb, BIGNUM *bn) {
   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,18 @@ RSA *RSA_parse_public_key(CBS *cbs) {
   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. See https://crbug.com/532048 and https://crbug.com/534766.
+   *
+   * TODO(davidben): Remove this code and callers in March 2016. */
+  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);

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 @@ static const uint8_t kSixPrimeEncryptedMessage[] = {
     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 @@ static bool TestASN1() {
   }
   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;
 }
 

include/openssl/bn.h

@@ -304,6 +304,10 @@ OPENSSL_EXPORT BN_ULONG BN_get_word(const BIGNUM *bn);
  * 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 some invalid
+ * encodings. 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);

include/openssl/rsa.h

@@ -338,6 +338,10 @@ OPENSSL_EXPORT int RSA_add_pkcs1_prefix(uint8_t **out_msg, size_t *out_msg_len,
  * error. */
 OPENSSL_EXPORT RSA *RSA_parse_public_key(CBS *cbs);
 
+/* RSA_parse_public_key_buggy behaves like |RSA_parse_public_key|, but it
+ * tolerates some invalid encodings. 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);