Reject DSA trailing garbage in EVP layer, add test cases (#2289)

# Description
The EVP API layer was not rejecting signatures that contained trailing
garbage after the `DSA_SIG` ASN.1 encoding (note it correctly reject
invalid signatures regardless). This did not match the behavior of the
`DSA_verify` and `DSA_check_signature` APIs which explicitly check and
reject trailing garbage. This aligns the EVP layer with this behavior.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

Author: skmcgrail

crypto/evp_extra/evp_extra_test.cc

@@ -3316,7 +3316,7 @@ TEST(EVPExtraTest, DSASignDigestVerify) {
     bssl::UniquePtr<EVP_MD_CTX> md_ctx(EVP_MD_CTX_new());
     ASSERT_EQ(1, EVP_DigestVerifyInit(md_ctx.get(), nullptr, EVP_sha1(), nullptr, public_key.get()));
     ASSERT_EQ(1, EVP_DigestVerifyUpdate(md_ctx.get(), data, data_len));
-    ASSERT_EQ(1, EVP_DigestVerifyFinal(md_ctx.get(), sig.data(), sig.size()));
+    ASSERT_EQ(1, EVP_DigestVerifyFinal(md_ctx.get(), sig.data(), siglen));
   }
 
 }
@@ -3390,7 +3390,7 @@ TEST(EVPExtraTest, DSADigestSignVerify) {
     bssl::UniquePtr<EVP_MD_CTX> md_ctx(EVP_MD_CTX_new());
     ASSERT_TRUE(md_ctx);
     ASSERT_TRUE(EVP_DigestVerifyInit(md_ctx.get(), nullptr, EVP_sha256(), nullptr, public_key.get()));
-    ASSERT_TRUE(EVP_DigestVerify(md_ctx.get(), sig.data(), sig.size(), (const uint8_t*)data, data_len));
+    ASSERT_TRUE(EVP_DigestVerify(md_ctx.get(), sig.data(), siglen, (const uint8_t*)data, data_len));
   }
 }
 
@@ -3402,3 +3402,82 @@ TEST(EVPExtraTest, RawKeyUnsupported) {
       EVP_PKEY_new_raw_private_key(EVP_PKEY_RSA, nullptr, kKey, sizeof(kKey)));
 }
 
+TEST(EVPExtraTest, DSATrailingSignatureGarbage) {
+  // Test case was sourced from https://github.com/C2SP/wycheproof
+  const uint8_t DSA_PUB_DER[] = {
+      0x30, 0x82, 0x01, 0xb6, 0x30, 0x82, 0x01, 0x2b, 0x06, 0x07, 0x2a, 0x86,
+      0x48, 0xce, 0x38, 0x04, 0x01, 0x30, 0x82, 0x01, 0x1e, 0x02, 0x81, 0x81,
+      0x00, 0xb3, 0x4c, 0xe9, 0xc1, 0xe7, 0x82, 0x94, 0xd3, 0x25, 0x84, 0x73,
+      0x84, 0x20, 0x05, 0xd2, 0xa4, 0x8c, 0x8c, 0x56, 0x6c, 0xfc, 0xa8, 0xf8,
+      0x4c, 0x06, 0x06, 0xf2, 0x52, 0x9b, 0x59, 0xa6, 0xd3, 0x8a, 0xae, 0x07,
+      0x1b, 0x53, 0xbb, 0x21, 0x67, 0xea, 0xa4, 0xfc, 0x3b, 0x01, 0xfe, 0x17,
+      0x6e, 0x78, 0x7e, 0x48, 0x1b, 0x60, 0x37, 0xaa, 0xc6, 0x2c, 0xbc, 0x3d,
+      0x08, 0x97, 0x99, 0x53, 0x6a, 0x86, 0x9f, 0xa8, 0xcd, 0xfe, 0xa1, 0xe8,
+      0xb1, 0xfd, 0x2d, 0x1c, 0xd3, 0xa3, 0x03, 0x50, 0x85, 0x9a, 0x2c, 0xd6,
+      0xb3, 0xec, 0x2f, 0x9b, 0xfb, 0xb6, 0x8b, 0xb1, 0x1b, 0x4b, 0xbe, 0x2a,
+      0xda, 0xa1, 0x8d, 0x64, 0xa9, 0x36, 0x39, 0x54, 0x3a, 0xe5, 0xe1, 0x62,
+      0x93, 0xe3, 0x11, 0xc0, 0xcf, 0x8c, 0x8d, 0x6e, 0x18, 0x0d, 0xf0, 0x5d,
+      0x08, 0xc2, 0xfd, 0x2d, 0x93, 0xd5, 0x70, 0x75, 0x1f, 0x02, 0x15, 0x00,
+      0xb9, 0x0b, 0x38, 0xba, 0x0a, 0x50, 0xa4, 0x3e, 0xc6, 0x89, 0x8d, 0x3f,
+      0x9b, 0x68, 0x04, 0x97, 0x77, 0xf4, 0x89, 0xb1, 0x02, 0x81, 0x80, 0x08,
+      0x35, 0xaa, 0x8c, 0x35, 0x8b, 0xbf, 0x01, 0xa1, 0x84, 0x6d, 0x12, 0x06,
+      0x32, 0x3f, 0xab, 0xe4, 0x08, 0xb0, 0xe9, 0x87, 0x89, 0xfc, 0xc6, 0x23,
+      0x9d, 0xa1, 0x4d, 0x4b, 0x3f, 0x86, 0xc2, 0x76, 0xa8, 0xf4, 0x8a, 0xa8,
+      0x5a, 0x59, 0x50, 0x7e, 0x62, 0x0a, 0xd1, 0xbc, 0x74, 0x5f, 0x0f, 0x1c,
+      0xbf, 0x63, 0xec, 0x98, 0xc2, 0x29, 0xc2, 0x61, 0x0d, 0x77, 0xc6, 0x34,
+      0xd1, 0x64, 0x2e, 0x40, 0x43, 0x54, 0x77, 0x16, 0x55, 0xb2, 0xd5, 0x66,
+      0x2f, 0x7a, 0x45, 0x22, 0x71, 0x78, 0xce, 0x34, 0x30, 0xaf, 0x0f, 0x6b,
+      0x3b, 0xb9, 0x4b, 0x52, 0xf7, 0xf5, 0x1e, 0x97, 0xba, 0xd6, 0x59, 0xb1,
+      0xba, 0x06, 0x84, 0xe2, 0x08, 0xbe, 0x62, 0x4c, 0x28, 0xd8, 0x2f, 0xb1,
+      0x16, 0x2f, 0x18, 0xdd, 0x9d, 0xce, 0x45, 0x21, 0x64, 0x61, 0x65, 0x4c,
+      0xf3, 0x37, 0x46, 0x24, 0xd1, 0x5a, 0x8d, 0x03, 0x81, 0x84, 0x00, 0x02,
+      0x81, 0x80, 0x17, 0x39, 0x31, 0xdd, 0xa3, 0x1e, 0xff, 0x32, 0xf2, 0x4b,
+      0x38, 0x30, 0x91, 0xbf, 0x77, 0xea, 0xcd, 0xc6, 0xef, 0xd5, 0x57, 0x62,
+      0x49, 0x11, 0xd8, 0xe9, 0xb9, 0xde, 0xbf, 0x0f, 0x25, 0x6d, 0x0c, 0xff,
+      0xac, 0x55, 0x67, 0xb3, 0x3f, 0x6e, 0xaa, 0xe9, 0xd3, 0x27, 0x5b, 0xbe,
+      0xd7, 0xef, 0x9f, 0x5f, 0x94, 0xc4, 0x00, 0x3c, 0x95, 0x9e, 0x49, 0xa1,
+      0xed, 0x3f, 0x58, 0xc3, 0x1b, 0x21, 0xba, 0xcc, 0xc0, 0xed, 0x88, 0x40,
+      0xb4, 0x61, 0x45, 0xf1, 0x21, 0xb8, 0x90, 0x6d, 0x07, 0x21, 0x29, 0xba,
+      0xe0, 0x1f, 0x07, 0x19, 0x47, 0x99, 0x7e, 0x8e, 0xf7, 0x60, 0xd2, 0xd9,
+      0xea, 0x21, 0xd0, 0x8a, 0x5e, 0xb7, 0xe8, 0x93, 0x90, 0xb2, 0x1a, 0x85,
+      0x66, 0x47, 0x13, 0xc5, 0x49, 0xe2, 0x5f, 0xed, 0xa6, 0xe9, 0xe6, 0xc3,
+      0x19, 0x70, 0x86, 0x6b, 0xdf, 0xbc, 0x8f, 0xa9, 0x81, 0xf6};
+  const uint8_t MESSAGE[] = {0x31, 0x32, 0x33, 0x34, 0x30, 0x30};
+  // SIGNATURE has extra bytes trailing after the ASN.1 DER encoding
+  const uint8_t SIGNATURE[] = {
+      0x30, 0x2d, 0x02, 0x15, 0x00, 0xaa, 0x6a, 0x25, 0x8f, 0xbf,
+      0x7d, 0x90, 0xe1, 0x56, 0x14, 0x67, 0x6d, 0x37, 0x7d, 0xf8,
+      0xb1, 0x0e, 0x38, 0xdb, 0x4a, 0x02, 0x14, 0x49, 0x6d, 0x52,
+      0x20, 0xb5, 0xf6, 0x7d, 0x35, 0x32, 0xd1, 0xf9, 0x91, 0x20,
+      0x3b, 0xc3, 0x52, 0x3b, 0x96, 0x4c, 0x3b, 0x00, 0x00};
+
+  CBS cbs;
+  CBS_init(&cbs, DSA_PUB_DER, sizeof(DSA_PUB_DER));
+
+  bssl::UniquePtr<EVP_PKEY> pkey(EVP_parse_public_key(&cbs));
+  ASSERT_EQ(EVP_PKEY_DSA, EVP_PKEY_id(pkey.get()));
+
+  bssl::UniquePtr<DSA> dsa(EVP_PKEY_get1_DSA(pkey.get()));
+  
+  uint8_t digest[sizeof(SIGNATURE)] = {0};
+  unsigned int digest_len = sizeof(digest);
+  ASSERT_TRUE(EVP_Digest(MESSAGE, sizeof(MESSAGE), digest, &digest_len, EVP_sha1(), nullptr));
+
+  EXPECT_EQ(-1, DSA_verify(0, digest, sizeof(digest), SIGNATURE, sizeof(SIGNATURE), dsa.get()));
+
+  int valid = 0;
+  EXPECT_EQ(0, DSA_check_signature(&valid, digest, sizeof(digest), SIGNATURE, sizeof(SIGNATURE), dsa.get()));
+  EXPECT_EQ(0, valid);
+
+  bssl::UniquePtr<EVP_PKEY_CTX> ctx(EVP_PKEY_CTX_new(pkey.get(), nullptr));
+  ASSERT_TRUE(ctx);
+  ASSERT_TRUE(EVP_PKEY_verify_init(ctx.get()));
+
+  EXPECT_FALSE(EVP_PKEY_verify(ctx.get(), SIGNATURE, sizeof(SIGNATURE), digest, sizeof(digest)));
+
+  bssl::UniquePtr<EVP_MD_CTX> md_ctx(EVP_MD_CTX_new());
+  ASSERT_TRUE(md_ctx);
+
+  ASSERT_TRUE(EVP_DigestVerifyInit(md_ctx.get(), nullptr, EVP_sha1(), nullptr, pkey.get()));
+  EXPECT_FALSE(EVP_DigestVerify(md_ctx.get(), SIGNATURE, sizeof(SIGNATURE), MESSAGE, sizeof(MESSAGE)));
+}

crypto/evp_extra/p_dsa.c

@@ -194,7 +194,8 @@ static int pkey_dsa_verify(EVP_PKEY_CTX *ctx, const unsigned char *sig,
   CBS sig_cbs;
   CBS_init(&sig_cbs, sig, siglen);
   dsa_sig = DSA_SIG_parse(&sig_cbs);
-  if (dsa_sig == NULL) {
+  // Allocation failure, invalid asn1 encoding, or trailing garbage
+  if (dsa_sig == NULL || CBS_len(&sig_cbs) != 0) {
     goto end;
   }
   if (1 != DSA_do_verify(tbs, tbslen, dsa_sig, dsa)) {