Enforce strong hash (SHA256, SHA512) in subtle's digital signature.

PiperOrigin-RevId: 206035572 GitOrigin-RevId: daf347df71221d2033fac1a6d624f3a7a7106c41
sixh · Jul 27, 2018 · f1d5f84 · f1d5f84
1 parent 65c0767
commit f1d5f84
Show file tree

Hide file tree

Showing 8 changed files with 59 additions and 16 deletions.
diff --git a/cc/subtle/ecdsa_sign_boringssl.cc b/cc/subtle/ecdsa_sign_boringssl.cc
@@ -36,6 +36,10 @@ util::StatusOr<std::unique_ptr<EcdsaSignBoringSsl>>
 EcdsaSignBoringSsl::New(const SubtleUtilBoringSSL::EcKey& ec_key,
                         HashType hash_type) {
   // Check hash.
+  auto hash_status = SubtleUtilBoringSSL::ValidateSignatureHash(hash_type);
+  if (!hash_status.ok()) {
+    return hash_status;
+  }
   auto hash_result = SubtleUtilBoringSSL::EvpHash(hash_type);
   if (!hash_result.ok()) return hash_result.status();
   const EVP_MD* hash = hash_result.ValueOrDie();

diff --git a/cc/subtle/ecdsa_sign_boringssl_test.cc b/cc/subtle/ecdsa_sign_boringssl_test.cc
@@ -67,6 +67,13 @@ TEST_F(EcdsaSignBoringSslTest, testBasicSigning) {
   EXPECT_TRUE(status.ok()) << status;
 }
 
+TEST_F(EcdsaSignBoringSslTest, testNewErrors) {
+  auto ec_key = SubtleUtilBoringSSL::GetNewEcKey(EllipticCurveType::NIST_P256)
+                    .ValueOrDie();
+  auto signer_result = EcdsaSignBoringSsl::New(ec_key, HashType::SHA1);
+  EXPECT_FALSE(signer_result.ok()) << signer_result.status();
+}
+
 // TODO(bleichen): add Wycheproof tests.
 
 }  // namespace

diff --git a/cc/subtle/ecdsa_verify_boringssl.cc b/cc/subtle/ecdsa_verify_boringssl.cc
@@ -34,6 +34,10 @@ util::StatusOr<std::unique_ptr<EcdsaVerifyBoringSsl>>
 EcdsaVerifyBoringSsl::New(const SubtleUtilBoringSSL::EcKey& ec_key,
                         HashType hash_type) {
   // Check hash.
+  auto hash_status = SubtleUtilBoringSSL::ValidateSignatureHash(hash_type);
+  if (!hash_status.ok()) {
+    return hash_status;
+  }
   auto hash_result = SubtleUtilBoringSSL::EvpHash(hash_type);
   if (!hash_result.ok()) return hash_result.status();
   const EVP_MD* hash = hash_result.ValueOrDie();

diff --git a/cc/subtle/ecdsa_verify_boringssl_test.cc b/cc/subtle/ecdsa_verify_boringssl_test.cc
@@ -36,10 +36,9 @@ namespace tink {
 namespace subtle {
 namespace {
 
-class EcdsaSignBoringSslTest : public ::testing::Test {
-};
+class EcdsaVerifyBoringSslTest : public ::testing::Test {};
 
-TEST_F(EcdsaSignBoringSslTest, testBasicSigning) {
+TEST_F(EcdsaVerifyBoringSslTest, testBasicSigning) {
   auto ec_key_result = SubtleUtilBoringSSL::GetNewEcKey(
       EllipticCurveType::NIST_P256);
   ASSERT_TRUE(ec_key_result.ok()) << ec_key_result.status();
@@ -71,6 +70,13 @@ TEST_F(EcdsaSignBoringSslTest, testBasicSigning) {
   EXPECT_FALSE(status.ok());
 }
 
+TEST_F(EcdsaVerifyBoringSslTest, testNewErrors) {
+  auto ec_key = SubtleUtilBoringSSL::GetNewEcKey(EllipticCurveType::NIST_P256)
+                    .ValueOrDie();
+  auto verifier_result = EcdsaVerifyBoringSsl::New(ec_key, HashType::SHA1);
+  EXPECT_FALSE(verifier_result.ok()) << verifier_result.status();
+}
+
 // Integers in Wycheproof are represented as signed bigendian hexadecimal
 // strings in twos complement representation.
 // Integers in EcKey are unsigned and are represented as an array of bytes
@@ -179,15 +185,15 @@ bool TestSignatures(const std::string& filename, bool allow_skipping) {
   return failed_tests == 0;
 }
 
-TEST_F(EcdsaSignBoringSslTest, testVectorsNistP256) {
+TEST_F(EcdsaVerifyBoringSslTest, testVectorsNistP256) {
   ASSERT_TRUE(TestSignatures("ecdsa_secp256r1_sha256_test.json", false));
 }
 
-TEST_F(EcdsaSignBoringSslTest, testVectorsNistP384) {
+TEST_F(EcdsaVerifyBoringSslTest, testVectorsNistP384) {
   ASSERT_TRUE(TestSignatures("ecdsa_secp384r1_sha512_test.json", false));
 }
 
-TEST_F(EcdsaSignBoringSslTest, testVectorsNistP521) {
+TEST_F(EcdsaVerifyBoringSslTest, testVectorsNistP521) {
   ASSERT_TRUE(TestSignatures("ecdsa_secp521r1_sha512_test.json", false));
 }
 

diff --git a/cc/subtle/rsa_ssa_pss_verify_boringssl.cc b/cc/subtle/rsa_ssa_pss_verify_boringssl.cc
@@ -47,16 +47,10 @@ RsaSsaPssVerifyBoringSsl::New(
     const SubtleUtilBoringSSL::RsaPublicKey& pub_key,
     const SubtleUtilBoringSSL::RsaSsaPssParams& params) {
   // Check hash.
-  switch (params.sig_hash) {
-    case HashType::SHA256: /* fall through */
-    case HashType::SHA512:
-      break;
-    case HashType::SHA1:
-      return util::Status(util::error::INVALID_ARGUMENT,
-                          "SHA1 is not safe for digital signature");
-    default:
-      return util::Status(util::error::INVALID_ARGUMENT,
-                          "Unsupported hash function");
+  auto hash_status =
+      SubtleUtilBoringSSL::ValidateSignatureHash(params.sig_hash);
+  if (!hash_status.ok()) {
+    return hash_status;
   }
   auto sig_hash_result = SubtleUtilBoringSSL::EvpHash(params.sig_hash);
   if (!sig_hash_result.ok()) return sig_hash_result.status();

diff --git a/cc/subtle/subtle_util_boringssl.cc b/cc/subtle/subtle_util_boringssl.cc
@@ -347,6 +347,20 @@ util::StatusOr<std::string> SubtleUtilBoringSSL::EcPointEncode(
   }
 }
 
+// static
+util::Status SubtleUtilBoringSSL::ValidateSignatureHash(HashType sig_hash) {
+  switch (sig_hash) {
+    case HashType::SHA256: /* fall through */
+    case HashType::SHA512:
+      return util::Status::OK;
+    case HashType::SHA1:
+      return util::Status(util::error::INVALID_ARGUMENT,
+                          "SHA1 is not safe for digital signature");
+    default:
+      return util::Status(util::error::INVALID_ARGUMENT,
+                          "Unsupported hash function");
+  }
+}
 
 // static
 std::string SubtleUtilBoringSSL::GetErrors() {

diff --git a/cc/subtle/subtle_util_boringssl.h b/cc/subtle/subtle_util_boringssl.h
@@ -117,6 +117,10 @@ class SubtleUtilBoringSSL {
   // The EVP_MD instances are sigletons owned by BoringSSL.
   static crypto::tink::util::StatusOr<const EVP_MD *> EvpHash(
       HashType hash_type);
+
+  // Validates whether 'sig_hash' is safe to use for digital signature.
+  static crypto::tink::util::Status ValidateSignatureHash(
+      subtle::HashType sig_hash);
 };
 
 }  // namespace subtle

diff --git a/cc/subtle/subtle_util_boringssl_test.cc b/cc/subtle/subtle_util_boringssl_test.cc
@@ -128,6 +128,16 @@ TEST(SubtleUtilBoringSSLTest, testEcPointDecode) {
   }
 }
 
+TEST(SubtleUtilBoringSSLTest, testValidateSignatureHash) {
+  EXPECT_TRUE(
+      SubtleUtilBoringSSL::ValidateSignatureHash(HashType::SHA256).ok());
+  EXPECT_TRUE(
+      SubtleUtilBoringSSL::ValidateSignatureHash(HashType::SHA512).ok());
+  EXPECT_FALSE(SubtleUtilBoringSSL::ValidateSignatureHash(HashType::SHA1).ok());
+  EXPECT_FALSE(
+      SubtleUtilBoringSSL::ValidateSignatureHash(HashType::UNKNOWN_HASH).ok());
+}
+
 static std::string GetError() {
   auto err = ERR_peek_last_error();
   // Sometimes there is no error message on the stack.