Remove the buggy RSA parser.
I've left EVP_set_buggy_rsa_parser as a no-op stub for now, but it
shouldn't need to last very long. (Just waiting for a CL to land in a
consumer.)
Bug: chromium:735616
Change-Id: I6426588f84dd0803661a79c6636a0414f4e98855
Reviewed-on: https://boringssl-review.googlesource.com/22124
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/crypto/bn_extra/bn_asn1.c b/crypto/bn_extra/bn_asn1.c
index 8b939da..0d96573 100644
--- a/crypto/bn_extra/bn_asn1.c
+++ b/crypto/bn_extra/bn_asn1.c
@@ -42,22 +42,6 @@
return BN_bin2bn(CBS_data(&child), CBS_len(&child), ret) != NULL;
}
-int BN_parse_asn1_unsigned_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;
-}
-
int BN_marshal_asn1(CBB *cbb, const BIGNUM *bn) {
// Negative numbers are unsupported.
if (BN_is_negative(bn)) {
diff --git a/crypto/evp/p_rsa_asn1.c b/crypto/evp/p_rsa_asn1.c
index 5157a89..5dfe70a 100644
--- a/crypto/evp/p_rsa_asn1.c
+++ b/crypto/evp/p_rsa_asn1.c
@@ -63,18 +63,10 @@
#include <openssl/rsa.h>
#include "../fipsmodule/rsa/internal.h"
-#include "../internal.h"
#include "internal.h"
-static struct CRYPTO_STATIC_MUTEX g_buggy_lock = CRYPTO_STATIC_MUTEX_INIT;
-static int g_buggy = 0;
-
-void EVP_set_buggy_rsa_parser(int buggy) {
- CRYPTO_STATIC_MUTEX_lock_write(&g_buggy_lock);
- g_buggy = buggy;
- CRYPTO_STATIC_MUTEX_unlock_write(&g_buggy_lock);
-}
+void EVP_set_buggy_rsa_parser(int buggy) {}
static int rsa_pub_encode(CBB *out, const EVP_PKEY *key) {
// See RFC 3279, section 2.3.1.
@@ -96,11 +88,6 @@
}
static int rsa_pub_decode(EVP_PKEY *out, CBS *params, CBS *key) {
- int buggy;
- CRYPTO_STATIC_MUTEX_lock_read(&g_buggy_lock);
- buggy = g_buggy;
- CRYPTO_STATIC_MUTEX_unlock_read(&g_buggy_lock);
-
// See RFC 3279, section 2.3.1.
// The parameters must be NULL.
@@ -112,13 +99,7 @@
return 0;
}
- // 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.
- RSA *rsa = buggy ? RSA_parse_public_key_buggy(key) : RSA_parse_public_key(key);
+ RSA *rsa = RSA_parse_public_key(key);
if (rsa == NULL || CBS_len(key) != 0) {
OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
RSA_free(rsa);
diff --git a/crypto/fipsmodule/bn/bn_test.cc b/crypto/fipsmodule/bn/bn_test.cc
index fe03e5f..6362d52 100644
--- a/crypto/fipsmodule/bn/bn_test.cc
+++ b/crypto/fipsmodule/bn/bn_test.cc
@@ -1000,16 +1000,11 @@
{"\x03\x01\x00", 3},
// Empty contents.
{"\x02\x00", 2},
-};
-
-// kASN1BuggyTests contains incorrect encodings and the corresponding, expected
-// results of |BN_parse_asn1_unsigned_buggy| given that input.
-static const ASN1Test kASN1BuggyTests[] = {
// Negative numbers.
- {"128", "\x02\x01\x80", 3},
- {"255", "\x02\x01\xff", 3},
+ {"\x02\x01\x80", 3},
+ {"\x02\x01\xff", 3},
// Unnecessary leading zeros.
- {"1", "\x02\x02\x00\x01", 4},
+ {"\x02\x02\x00\x01", 4},
};
TEST_F(BNTest, ASN1) {
@@ -1036,12 +1031,6 @@
ASSERT_TRUE(CBB_finish(cbb.get(), &der, &der_len));
bssl::UniquePtr<uint8_t> delete_der(der);
EXPECT_EQ(Bytes(test.der, test.der_len), Bytes(der, der_len));
-
- // |BN_parse_asn1_unsigned_buggy| parses all valid input.
- CBS_init(&cbs, reinterpret_cast<const uint8_t*>(test.der), test.der_len);
- ASSERT_TRUE(BN_parse_asn1_unsigned_buggy(&cbs, bn2.get()));
- EXPECT_EQ(0u, CBS_len(&cbs));
- EXPECT_BIGNUMS_EQUAL("decode ASN.1 buggy", bn.get(), bn2.get());
}
for (const ASN1InvalidTest &test : kASN1InvalidTests) {
@@ -1053,35 +1042,6 @@
EXPECT_FALSE(BN_parse_asn1_unsigned(&cbs, bn.get()))
<< "Parsed invalid input.";
ERR_clear_error();
-
- // All tests in kASN1InvalidTests are also rejected by
- // |BN_parse_asn1_unsigned_buggy|.
- CBS_init(&cbs, reinterpret_cast<const uint8_t*>(test.der), test.der_len);
- EXPECT_FALSE(BN_parse_asn1_unsigned_buggy(&cbs, bn.get()))
- << "Parsed invalid input.";
- ERR_clear_error();
- }
-
- for (const ASN1Test &test : kASN1BuggyTests) {
- SCOPED_TRACE(Bytes(test.der, test.der_len));
-
- // These broken encodings are rejected by |BN_parse_asn1_unsigned|.
- bssl::UniquePtr<BIGNUM> bn(BN_new());
- ASSERT_TRUE(bn);
- CBS cbs;
- CBS_init(&cbs, reinterpret_cast<const uint8_t*>(test.der), test.der_len);
- EXPECT_FALSE(BN_parse_asn1_unsigned(&cbs, bn.get()))
- << "Parsed invalid input.";
- ERR_clear_error();
-
- // However |BN_parse_asn1_unsigned_buggy| accepts them.
- bssl::UniquePtr<BIGNUM> bn2 = ASCIIToBIGNUM(test.value_ascii);
- ASSERT_TRUE(bn2);
-
- CBS_init(&cbs, reinterpret_cast<const uint8_t*>(test.der), test.der_len);
- ASSERT_TRUE(BN_parse_asn1_unsigned_buggy(&cbs, bn.get()));
- EXPECT_EQ(0u, CBS_len(&cbs));
- EXPECT_BIGNUMS_EQUAL("decode ASN.1 buggy", bn2.get(), bn.get());
}
// Serializing negative numbers is not supported.
diff --git a/crypto/rsa_extra/rsa_asn1.c b/crypto/rsa_extra/rsa_asn1.c
index 23c91bd..3cc6a9c 100644
--- a/crypto/rsa_extra/rsa_asn1.c
+++ b/crypto/rsa_extra/rsa_asn1.c
@@ -69,22 +69,15 @@
#include "../internal.h"
-static int parse_integer_buggy(CBS *cbs, BIGNUM **out, int buggy) {
+static int parse_integer(CBS *cbs, BIGNUM **out) {
assert(*out == NULL);
*out = BN_new();
if (*out == NULL) {
return 0;
}
- if (buggy) {
- return BN_parse_asn1_unsigned_buggy(cbs, *out);
- }
return BN_parse_asn1_unsigned(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.
@@ -94,14 +87,14 @@
return BN_marshal_asn1(cbb, bn);
}
-static RSA *parse_public_key(CBS *cbs, int buggy) {
+RSA *RSA_parse_public_key(CBS *cbs) {
RSA *ret = RSA_new();
if (ret == NULL) {
return NULL;
}
CBS child;
if (!CBS_get_asn1(cbs, &child, CBS_ASN1_SEQUENCE) ||
- !parse_integer_buggy(&child, &ret->n, buggy) ||
+ !parse_integer(&child, &ret->n) ||
!parse_integer(&child, &ret->e) ||
CBS_len(&child) != 0) {
OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_ENCODING);
@@ -119,18 +112,6 @@
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);
diff --git a/crypto/rsa_extra/rsa_test.cc b/crypto/rsa_extra/rsa_test.cc
index 1cc71ea..23e8c67 100644
--- a/crypto/rsa_extra/rsa_test.cc
+++ b/crypto/rsa_extra/rsa_test.cc
@@ -622,13 +622,6 @@
sizeof(kEstonianRSAKey)));
EXPECT_FALSE(rsa);
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));
- EXPECT_TRUE(rsa);
- EXPECT_EQ(0u, CBS_len(&cbs));
}
TEST(RSATest, BadExponent) {
diff --git a/include/openssl/bn.h b/include/openssl/bn.h
index 52333ac..9960b75 100644
--- a/include/openssl/bn.h
+++ b/include/openssl/bn.h
@@ -317,10 +317,6 @@
// the result to |ret|. It returns one on success and zero on failure.
OPENSSL_EXPORT int BN_parse_asn1_unsigned(CBS *cbs, BIGNUM *ret);
-// BN_parse_asn1_unsigned_buggy acts like |BN_parse_asn1_unsigned| but tolerates
-// some invalid encodings. Do not use this function.
-OPENSSL_EXPORT int BN_parse_asn1_unsigned_buggy(CBS *cbs, BIGNUM *ret);
-
// BN_marshal_asn1 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_marshal_asn1(CBB *cbb, const BIGNUM *bn);
diff --git a/include/openssl/evp.h b/include/openssl/evp.h
index c57d7c7..29369ce 100644
--- a/include/openssl/evp.h
+++ b/include/openssl/evp.h
@@ -225,10 +225,6 @@
// success and zero on error.
OPENSSL_EXPORT int EVP_marshal_private_key(CBB *cbb, const EVP_PKEY *key);
-// EVP_set_buggy_rsa_parser configures whether |RSA_parse_public_key_buggy| is
-// used by |EVP_parse_public_key|. By default, it is not used.
-OPENSSL_EXPORT void EVP_set_buggy_rsa_parser(int buggy);
-
// Signing
@@ -800,6 +796,9 @@
// EVP_PKEY_get0_DH returns NULL.
OPENSSL_EXPORT DH *EVP_PKEY_get0_DH(EVP_PKEY *pkey);
+// EVP_set_buggy_rsa_parser does nothing.
+OPENSSL_EXPORT void EVP_set_buggy_rsa_parser(int buggy);
+
// Private structures.
diff --git a/include/openssl/rsa.h b/include/openssl/rsa.h
index 3f45318..74268cf 100644
--- a/include/openssl/rsa.h
+++ b/include/openssl/rsa.h
@@ -428,10 +428,6 @@
// 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);
