From 6a263ce483b9ff03c7998bcae031ed42f092368e Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Fri, 11 Sep 2020 18:16:05 +0000 Subject: [PATCH] Revert "Check AlgorithmIdentifier parameters for RSA and ECDSA signatures." This reverts commit 9dd9d4fc242f31584f5a42e41e63d132c790421f. BUG=b/167375496,342 Original change's description: > Check AlgorithmIdentifier parameters for RSA and ECDSA signatures. > > This aligns with the Chromium certificate verifier, which allows NULL or > empty for RSA and requires empty for ECDSA. > > Bug: 342 > Change-Id: I34acf68f63b4d133dd47b73144b2f27224c499ee > Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/41804 > Reviewed-by: Adam Langley > Commit-Queue: David Benjamin TBR=agl@google.com,davidben@google.com # Not skipping CQ checks because original CL landed > 1 day ago. Change-Id: If8f136a09fea68e64c9f4f9ffae88b6209ede124 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/42804 Commit-Queue: Adam Langley Reviewed-by: Adam Langley --- crypto/x509/algorithm.c | 8 --- crypto/x509/x509_test.cc | 116 --------------------------------------- 2 files changed, 124 deletions(-) diff --git a/crypto/x509/algorithm.c b/crypto/x509/algorithm.c index b9f3314c9..8f53fff6d 100644 --- a/crypto/x509/algorithm.c +++ b/crypto/x509/algorithm.c @@ -142,14 +142,6 @@ int x509_digest_verify_init(EVP_MD_CTX *ctx, X509_ALGOR *sigalg, return 0; } - /* RSA signature algorithms include an explicit NULL parameter but we also - * accept omitted values for compatibility. Other algorithms must omit it. */ - if (sigalg->parameter != NULL && (pkey_nid != EVP_PKEY_RSA || - sigalg->parameter->type != V_ASN1_NULL)) { - OPENSSL_PUT_ERROR(X509, X509_R_INVALID_PARAMETER); - return 0; - } - /* Otherwise, initialize with the digest from the OID. */ const EVP_MD *digest = EVP_get_digestbynid(digest_nid); if (digest == NULL) { diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 38c9bc599..d71dee92f 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -233,13 +233,6 @@ static const char kRSAKey[] = "moZWgjHvB2W9Ckn7sDqsPB+U2tyX0joDdQEyuiMECDY8oQ==\n" "-----END RSA PRIVATE KEY-----\n"; -static const char kP256Key[] = - "-----BEGIN PRIVATE KEY-----\n" - "MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgBw8IcnrUoEqc3VnJ\n" - "TYlodwi1b8ldMHcO6NHJzgqLtGqhRANCAATmK2niv2Wfl74vHg2UikzVl2u3qR4N\n" - "Rvvdqakendy6WgHn1peoChj5w8SjHlbifINI2xYaHPUdfvGULUvPciLB\n" - "-----END PRIVATE KEY-----\n"; - // kCRLTestRoot is a test root certificate. It has private key: // // -----BEGIN RSA PRIVATE KEY----- @@ -2452,112 +2445,3 @@ TEST(X509Test, InvalidVersion) { EXPECT_FALSE(CertFromPEM(kV1WithIssuerUniqueIDPEM)); EXPECT_FALSE(CertFromPEM(kV1WithSubjectUniqueIDPEM)); } - -// The following strings are test certificates signed by kP256Key and kRSAKey, -// with missing, NULL, or invalid algorithm parameters. -static const char kP256NoParam[] = - "-----BEGIN CERTIFICATE-----\n" - "MIIBIDCBxqADAgECAgIE0jAKBggqhkjOPQQDAjAPMQ0wCwYDVQQDEwRUZXN0MCAX\n" - "DTAwMDEwMTAwMDAwMFoYDzIxMDAwMTAxMDAwMDAwWjAPMQ0wCwYDVQQDEwRUZXN0\n" - "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE5itp4r9ln5e+Lx4NlIpM1Zdrt6ke\n" - "DUb73ampHp3culoB59aXqAoY+cPEox5W4nyDSNsWGhz1HX7xlC1Lz3IiwaMQMA4w\n" - "DAYDVR0TBAUwAwEB/zAKBggqhkjOPQQDAgNJADBGAiEAqdIiF+bN9Cl44oUeICpy\n" - "aXd7HqhpVUaglYKw9ChmNUACIQCpMdL0fNkFNDbRww9dSl/y7kBdk/tp16HiqeSy\n" - "gGzFYg==\n" - "-----END CERTIFICATE-----\n"; -static const char kP256NullParam[] = - "-----BEGIN CERTIFICATE-----\n" - "MIIBJDCByKADAgECAgIE0jAMBggqhkjOPQQDAgUAMA8xDTALBgNVBAMTBFRlc3Qw\n" - "IBcNMDAwMTAxMDAwMDAwWhgPMjEwMDAxMDEwMDAwMDBaMA8xDTALBgNVBAMTBFRl\n" - "c3QwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATmK2niv2Wfl74vHg2UikzVl2u3\n" - "qR4NRvvdqakendy6WgHn1peoChj5w8SjHlbifINI2xYaHPUdfvGULUvPciLBoxAw\n" - "DjAMBgNVHRMEBTADAQH/MAwGCCqGSM49BAMCBQADSQAwRgIhAKILHmyo+F3Cn/VX\n" - "UUeSXOQQKX5aLzsQitwwmNF3ZgH3AiEAsYHcrVj/ftmoQIORARkQ/+PrqntXev8r\n" - "t6uPxHrmpUY=\n" - "-----END CERTIFICATE-----\n"; -static const char kP256InvalidParam[] = - "-----BEGIN CERTIFICATE-----\n" - "MIIBMTCBz6ADAgECAgIE0jATBggqhkjOPQQDAgQHZ2FyYmFnZTAPMQ0wCwYDVQQD\n" - "EwRUZXN0MCAXDTAwMDEwMTAwMDAwMFoYDzIxMDAwMTAxMDAwMDAwWjAPMQ0wCwYD\n" - "VQQDEwRUZXN0MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE5itp4r9ln5e+Lx4N\n" - "lIpM1Zdrt6keDUb73ampHp3culoB59aXqAoY+cPEox5W4nyDSNsWGhz1HX7xlC1L\n" - "z3IiwaMQMA4wDAYDVR0TBAUwAwEB/zATBggqhkjOPQQDAgQHZ2FyYmFnZQNIADBF\n" - "AiAglpDf/YhN89LeJ2WAs/F0SJIrsuhS4uoInIz6WXUiuQIhAIu5Pwhp5E3Pbo8y\n" - "fLULTZnynuQUULQkRcF7S7T2WpIL\n" - "-----END CERTIFICATE-----\n"; -static const char kRSANoParam[] = - "-----BEGIN CERTIFICATE-----\n" - "MIIBWzCBx6ADAgECAgIE0jALBgkqhkiG9w0BAQswDzENMAsGA1UEAxMEVGVzdDAg\n" - "Fw0wMDAxMDEwMDAwMDBaGA8yMTAwMDEwMTAwMDAwMFowDzENMAsGA1UEAxMEVGVz\n" - "dDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABOYraeK/ZZ+Xvi8eDZSKTNWXa7ep\n" - "Hg1G+92pqR6d3LpaAefWl6gKGPnDxKMeVuJ8g0jbFhoc9R1+8ZQtS89yIsGjEDAO\n" - "MAwGA1UdEwQFMAMBAf8wCwYJKoZIhvcNAQELA4GBAC1f8W3W0Ao7CPfIBQYDSbPh\n" - "brZpbxdBU5x27JOS7iSa+Lc9pEH5VCX9vIypHVHXLPEfZ38yIt11eiyrmZB6w62N\n" - "l9kIeZ6FVPmC30d3sXx70Jjs+ZX9yt7kD1gLyNAQQfeYfa4rORAZT1n2YitD74NY\n" - "TWUH2ieFP3l+ecj1SeQR\n" - "-----END CERTIFICATE-----\n"; -static const char kRSANullParam[] = - "-----BEGIN CERTIFICATE-----\n" - "MIIBXzCByaADAgECAgIE0jANBgkqhkiG9w0BAQsFADAPMQ0wCwYDVQQDEwRUZXN0\n" - "MCAXDTAwMDEwMTAwMDAwMFoYDzIxMDAwMTAxMDAwMDAwWjAPMQ0wCwYDVQQDEwRU\n" - "ZXN0MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE5itp4r9ln5e+Lx4NlIpM1Zdr\n" - "t6keDUb73ampHp3culoB59aXqAoY+cPEox5W4nyDSNsWGhz1HX7xlC1Lz3IiwaMQ\n" - "MA4wDAYDVR0TBAUwAwEB/zANBgkqhkiG9w0BAQsFAAOBgQAzVcfIv+Rq1KrMXqIL\n" - "fPq/cWZjgqFZA1RGaGElNaqp+rkJfamq5tDGzckWpebrK+jjRN7yIlcWDtPpy3Gy\n" - "seZfvtBDR0TwJm0S/pQl8prKB4wgALcwe3bmi56Rq85nzY5ZLNcP16LQxL+jAAua\n" - "SwmQUz4bRpckRBj+sIyp1We+pg==\n" - "-----END CERTIFICATE-----\n"; -static const char kRSAInvalidParam[] = - "-----BEGIN CERTIFICATE-----\n" - "MIIBbTCB0KADAgECAgIE0jAUBgkqhkiG9w0BAQsEB2dhcmJhZ2UwDzENMAsGA1UE\n" - "AxMEVGVzdDAgFw0wMDAxMDEwMDAwMDBaGA8yMTAwMDEwMTAwMDAwMFowDzENMAsG\n" - "A1UEAxMEVGVzdDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABOYraeK/ZZ+Xvi8e\n" - "DZSKTNWXa7epHg1G+92pqR6d3LpaAefWl6gKGPnDxKMeVuJ8g0jbFhoc9R1+8ZQt\n" - "S89yIsGjEDAOMAwGA1UdEwQFMAMBAf8wFAYJKoZIhvcNAQELBAdnYXJiYWdlA4GB\n" - "AHTJ6cWWjCNrZhqiWWVI3jdK+h5xpRG8jGMXxR4JnjtoYRRusJLOXhmapwCB6fA0\n" - "4vc+66O27v36yDmQX+tIc/hDrTpKNJptU8q3n2VagREvoHhkOTYkcCeS8vmnMtn8\n" - "5OMNZ/ajVwOssw61GcAlScRqEHkZFBoGp7e+QpgB2tf9\n" - "-----END CERTIFICATE-----\n"; - -TEST(X509Test, AlgorithmParameters) { - // P-256 requires the parameter be omitted. - bssl::UniquePtr key = PrivateKeyFromPEM(kP256Key); - ASSERT_TRUE(key); - - bssl::UniquePtr cert = CertFromPEM(kP256NoParam); - ASSERT_TRUE(cert); - EXPECT_TRUE(X509_verify(cert.get(), key.get())); - - cert = CertFromPEM(kP256NullParam); - ASSERT_TRUE(cert); - EXPECT_FALSE(X509_verify(cert.get(), key.get())); - uint32_t err = ERR_get_error(); - EXPECT_EQ(ERR_LIB_X509, ERR_GET_LIB(err)); - EXPECT_EQ(X509_R_INVALID_PARAMETER, ERR_GET_REASON(err)); - - cert = CertFromPEM(kP256InvalidParam); - ASSERT_TRUE(cert); - EXPECT_FALSE(X509_verify(cert.get(), key.get())); - err = ERR_get_error(); - EXPECT_EQ(ERR_LIB_X509, ERR_GET_LIB(err)); - EXPECT_EQ(X509_R_INVALID_PARAMETER, ERR_GET_REASON(err)); - - // RSA parameters should be NULL, but we accept omitted ones. - key = PrivateKeyFromPEM(kRSAKey); - ASSERT_TRUE(key); - - cert = CertFromPEM(kRSANoParam); - ASSERT_TRUE(cert); - EXPECT_TRUE(X509_verify(cert.get(), key.get())); - - cert = CertFromPEM(kRSANullParam); - ASSERT_TRUE(cert); - EXPECT_TRUE(X509_verify(cert.get(), key.get())); - - cert = CertFromPEM(kRSAInvalidParam); - ASSERT_TRUE(cert); - EXPECT_FALSE(X509_verify(cert.get(), key.get())); - err = ERR_get_error(); - EXPECT_EQ(ERR_LIB_X509, ERR_GET_LIB(err)); - EXPECT_EQ(X509_R_INVALID_PARAMETER, ERR_GET_REASON(err)); -}