Skip to content

Commit

Permalink
Limit the scope of the CAPI and CNG SHA-1 workarounds
Browse files Browse the repository at this point in the history 
In https://crbug.com/278370, we had to add some workarounds for
legacy CAPI and CNG keys that only supported SHA-1. We've since carried
these workarounds to current Chromium.

Histograms now suggest there is negligible use of SHA-1 in TLS client
signatures, with the exception of Windows. This is likely caused by
those workarounds. Ideally we'd be able to query the key for supported
signature algorithms, but this doesn't seem to be feasible. See
https://crbug.com/924284#c14

So, instead, when we import CAPI keys and RSA-1024 CNG keys, do a trial
signature with SHA-256. If it succeeds, disable the workaround. This
should be safe; even if we cause a PIN prompt a smartcard, we do so
immediately after the user selects the key, so it won't be out of
place. If the system doesn't cache PIN prompts, it is possible we'll
double prompt, but if PIN prompts aren't cached, users are likely
already spammed with prompts due to quirks of connection management.
Still, to mitigate the risk, the behavior change is gated on a
base::Feature so we can easily disable it.

If this sticks, we should hopefully reduce our SHA-1 signing on Windows.
Should the numbers then align with other platforms, we can consider
removing support for signing SHA-1 altogether.

Bug: 1377705
Change-Id: Ic0f88625fe87756fadd8b9d6baa22f7fcb49a27c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3972200
Commit-Queue: David Benjamin <davidben@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1070686}
  • Loading branch information
@davidben
davidben authored and Chromium LUCI CQ committed Nov 12, 2022
1 parent 9802ce3 commit 8849320
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 35 deletions.
6 changes: 6 additions & 0 deletions net/base/features.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -399,4 +399,10 @@ BASE_FEATURE(kBlockNewForbiddenHeaders,
"BlockNewForbiddenHeaders",
base::FEATURE_ENABLED_BY_DEFAULT);

#if BUILDFLAG(IS_WIN)
BASE_FEATURE(kPlatformKeyProbeSHA256,
"PlatformKeyProbeSHA256",
base::FEATURE_ENABLED_BY_DEFAULT);
#endif

} // namespace net::features
6 changes: 6 additions & 0 deletions net/base/features.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,12 @@ NET_EXPORT BASE_DECLARE_FEATURE(kUseNAT64ForIPv4Literal);
// Whether to block newly added forbidden headers (https://crbug.com/1362331).
NET_EXPORT BASE_DECLARE_FEATURE(kBlockNewForbiddenHeaders);

#if BUILDFLAG(IS_WIN)
// Whether to probe for SHA-256 on some legacy platform keys, before assuming
// the key requires SHA-1. See SSLPlatformKeyWin for details.
NET_EXPORT BASE_DECLARE_FEATURE(kPlatformKeyProbeSHA256);
#endif

} // namespace net::features

#endif // NET_BASE_FEATURES_H_
68 changes: 50 additions & 18 deletions net/ssl/ssl_platform_key_win.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@
#include <utility>
#include <vector>

#include "base/feature_list.h"
#include "base/logging.h"
#include "base/ranges/algorithm.h"
#include "base/strings/utf_string_conversions.h"
#include "crypto/openssl_util.h"
#include "crypto/scoped_capi_types.h"
#include "crypto/scoped_cng_types.h"
#include "net/base/features.h"
#include "net/base/net_errors.h"
#include "net/cert/x509_certificate.h"
#include "net/ssl/ssl_platform_key_util.h"
Expand All @@ -29,6 +31,31 @@ namespace net {

namespace {

bool ProbeSHA256(ThreadedSSLPrivateKey::Delegate* delegate) {
if (!base::FeatureList::IsEnabled(features::kPlatformKeyProbeSHA256)) {
return false;
}

// This input is chosen to avoid colliding with other signing inputs used in
// TLS 1.2 or TLS 1.3. We use the construct in RFC 8446, section 4.4.3, but
// change the context string. The context string ensures we don't collide with
// TLS 1.3 and any future version. The 0x20 (space) prefix ensures we don't
// collide with TLS 1.2 ServerKeyExchange or CertificateVerify.
static const uint8_t kSHA256ProbeInput[] = {
0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 'C', 'h',
'r', 'o', 'm', 'i', 'u', 'm', ',', ' ', 'S', 'H', 'A',
'2', ' ', 'P', 'r', 'o', 'b', 'e', 0x00,
};
std::vector<uint8_t> signature;
return delegate->Sign(SSL_SIGN_RSA_PKCS1_SHA256, kSHA256ProbeInput,
&signature) == OK;
}

std::string GetCAPIProviderName(HCRYPTPROV provider) {
DWORD name_len;
if (!CryptGetProvParam(provider, PP_NAME, nullptr, &name_len, 0)) {
Expand All @@ -53,7 +80,12 @@ class SSLPlatformKeyCAPI : public ThreadedSSLPrivateKey::Delegate {
SSLPlatformKeyCAPI(crypto::ScopedHCRYPTPROV provider, DWORD key_spec)
: provider_name_(GetCAPIProviderName(provider.get())),
provider_(std::move(provider)),
key_spec_(key_spec) {}
key_spec_(key_spec) {
// Check for SHA-256 support. The CAPI service provider may only be able to
// sign pre-TLS-1.2 and SHA-1 hashes. If SHA-256 doesn't work, prioritize
// SHA-1 as a workaround. See https://crbug.com/278370.
prefer_sha1_ = !ProbeSHA256(this);
}

SSLPlatformKeyCAPI(const SSLPlatformKeyCAPI&) = delete;
SSLPlatformKeyCAPI& operator=(const SSLPlatformKeyCAPI&) = delete;
Expand All @@ -63,16 +95,12 @@ class SSLPlatformKeyCAPI : public ThreadedSSLPrivateKey::Delegate {
std::string GetProviderName() override { return "CAPI: " + provider_name_; }

std::vector<uint16_t> GetAlgorithmPreferences() override {
// If the key is in CAPI, assume conservatively that the CAPI service
// provider may only be able to sign pre-TLS-1.2 and SHA-1 hashes.
// Prioritize SHA-1, but if the server doesn't advertise it, leave the other
// algorithms enabled to try. See https://crbug.com/278370.
return {
SSL_SIGN_RSA_PKCS1_SHA1,
SSL_SIGN_RSA_PKCS1_SHA256,
SSL_SIGN_RSA_PKCS1_SHA384,
SSL_SIGN_RSA_PKCS1_SHA512,
};
if (prefer_sha1_) {
return {SSL_SIGN_RSA_PKCS1_SHA1, SSL_SIGN_RSA_PKCS1_SHA256,
SSL_SIGN_RSA_PKCS1_SHA384, SSL_SIGN_RSA_PKCS1_SHA512};
}
return {SSL_SIGN_RSA_PKCS1_SHA256, SSL_SIGN_RSA_PKCS1_SHA384,
SSL_SIGN_RSA_PKCS1_SHA512, SSL_SIGN_RSA_PKCS1_SHA1};
}

Error Sign(uint16_t algorithm,
Expand Down Expand Up @@ -152,6 +180,7 @@ class SSLPlatformKeyCAPI : public ThreadedSSLPrivateKey::Delegate {
std::string provider_name_;
crypto::ScopedHCRYPTPROV provider_;
DWORD key_spec_;
bool prefer_sha1_ = false;
};

std::wstring GetCNGProviderName(NCRYPT_KEY_HANDLE key) {
Expand Down Expand Up @@ -202,7 +231,13 @@ class SSLPlatformKeyCNG : public ThreadedSSLPrivateKey::Delegate {
: provider_name_(GetCNGProviderName(key.get())),
key_(std::move(key)),
type_(type),
max_length_(max_length) {}
max_length_(max_length) {
// If this is a 1024-bit RSA key or below, check for SHA-256 support. Older
// Estonian ID cards can only sign SHA-1 hashes. If SHA-256 does not work,
// prioritize SHA-1 as a workaround. See https://crbug.com/278370.
prefer_sha1_ =
type_ == EVP_PKEY_RSA && max_length_ <= 1024 / 8 && !ProbeSHA256(this);
}

SSLPlatformKeyCNG(const SSLPlatformKeyCNG&) = delete;
SSLPlatformKeyCNG& operator=(const SSLPlatformKeyCNG&) = delete;
Expand Down Expand Up @@ -235,21 +270,17 @@ class SSLPlatformKeyCNG : public ThreadedSSLPrivateKey::Delegate {
supports_pss = false;
}
}
// If this is a 1024-bit RSA key or below, conservatively prefer to sign
// SHA-1 hashes. Older Estonian ID cards can only sign SHA-1 hashes.
// Prioritize SHA-1, but if the server doesn't advertise it, leave the other
// algorithms enabled to try. See https://crbug.com/278370.
if (type_ == EVP_PKEY_RSA && max_length_ <= 1024 / 8) {
if (prefer_sha1_) {
std::vector<uint16_t> ret = {
SSL_SIGN_RSA_PKCS1_SHA1,
SSL_SIGN_RSA_PKCS1_SHA256,
SSL_SIGN_RSA_PKCS1_SHA384,
SSL_SIGN_RSA_PKCS1_SHA512,
};
if (supports_pss) {
// 1024-bit keys are too small for SSL_SIGN_RSA_PSS_SHA512.
ret.push_back(SSL_SIGN_RSA_PSS_SHA256);
ret.push_back(SSL_SIGN_RSA_PSS_SHA384);
ret.push_back(SSL_SIGN_RSA_PSS_SHA512);
}
return ret;
}
Expand Down Expand Up @@ -360,6 +391,7 @@ class SSLPlatformKeyCNG : public ThreadedSSLPrivateKey::Delegate {
crypto::ScopedNCryptKey key_;
int type_;
size_t max_length_;
bool prefer_sha1_ = false;
};

} // namespace
Expand Down
72 changes: 55 additions & 17 deletions net/ssl/ssl_platform_key_win_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@

#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/test/scoped_feature_list.h"
#include "base/test/task_environment.h"
#include "crypto/scoped_capi_types.h"
#include "crypto/scoped_cng_types.h"
#include "net/base/features.h"
#include "net/cert/x509_certificate.h"
#include "net/ssl/ssl_private_key.h"
#include "net/ssl/ssl_private_key_test_util.h"
Expand Down Expand Up @@ -53,8 +55,10 @@ const TestKey kTestKeys[] = {
/*is_rsa_1024=*/true},
};

std::string TestKeyToString(const testing::TestParamInfo<TestKey>& params) {
return params.param.name;
std::string TestParamsToString(
const testing::TestParamInfo<std::tuple<TestKey, bool>>& params) {
return std::string(std::get<0>(params.param).name) +
(std::get<1>(params.param) ? "" : "NoSHA1Probe");
}

// Appends |bn| to |cbb|, represented as |len| bytes in little-endian order,
Expand Down Expand Up @@ -228,11 +232,29 @@ bool PKCS8ToBLOBForCNG(const std::string& pkcs8,

} // namespace

class SSLPlatformKeyWinTest : public testing::TestWithParam<TestKey>,
public WithTaskEnvironment {};
class SSLPlatformKeyWinTest
: public testing::TestWithParam<std::tuple<TestKey, bool>>,
public WithTaskEnvironment {
public:
SSLPlatformKeyWinTest() {
if (SHA256ProbeEnabled()) {
scoped_feature_list_.InitAndEnableFeature(
features::kPlatformKeyProbeSHA256);
} else {
scoped_feature_list_.InitAndDisableFeature(
features::kPlatformKeyProbeSHA256);
}
}

const TestKey& GetTestKey() const { return std::get<0>(GetParam()); }
bool SHA256ProbeEnabled() const { return std::get<1>(GetParam()); }

private:
base::test::ScopedFeatureList scoped_feature_list_;
};

TEST_P(SSLPlatformKeyWinTest, KeyMatchesCNG) {
const TestKey& test_key = GetParam();
const TestKey& test_key = GetTestKey();

// Load test data.
scoped_refptr<X509Certificate> cert =
Expand Down Expand Up @@ -269,13 +291,14 @@ TEST_P(SSLPlatformKeyWinTest, KeyMatchesCNG) {
WrapCNGPrivateKey(cert.get(), std::move(ncrypt_key));
ASSERT_TRUE(key);

if (test_key.is_rsa_1024) {
// For RSA-1024 and below, we conservatively prefer to sign SHA-1 hashes.
// See https://crbug.com/278370.
if (test_key.is_rsa_1024 && !SHA256ProbeEnabled()) {
// For RSA-1024 and below, if SHA-256 probing is disabled, we conservatively
// prefer to sign SHA-1 hashes. See https://crbug.com/278370.
std::vector<uint16_t> expected = {
SSL_SIGN_RSA_PKCS1_SHA1, SSL_SIGN_RSA_PKCS1_SHA256,
SSL_SIGN_RSA_PKCS1_SHA384, SSL_SIGN_RSA_PKCS1_SHA512,
SSL_SIGN_RSA_PSS_SHA256, SSL_SIGN_RSA_PSS_SHA384};
SSL_SIGN_RSA_PSS_SHA256, SSL_SIGN_RSA_PSS_SHA384,
SSL_SIGN_RSA_PSS_SHA512};
EXPECT_EQ(expected, key->GetAlgorithmPreferences());
} else {
EXPECT_EQ(SSLPrivateKey::DefaultAlgorithmPreferences(test_key.type,
Expand All @@ -287,7 +310,7 @@ TEST_P(SSLPlatformKeyWinTest, KeyMatchesCNG) {
}

TEST_P(SSLPlatformKeyWinTest, KeyMatchesCAPI) {
const TestKey& test_key = GetParam();
const TestKey& test_key = GetTestKey();
if (test_key.type != EVP_PKEY_RSA) {
GTEST_SKIP() << "CAPI only supports RSA keys";
}
Expand Down Expand Up @@ -326,18 +349,33 @@ TEST_P(SSLPlatformKeyWinTest, KeyMatchesCAPI) {
WrapCAPIPrivateKey(cert.get(), std::move(prov), AT_SIGNATURE);
ASSERT_TRUE(key);

std::vector<uint16_t> expected = {
SSL_SIGN_RSA_PKCS1_SHA1, SSL_SIGN_RSA_PKCS1_SHA256,
SSL_SIGN_RSA_PKCS1_SHA384, SSL_SIGN_RSA_PKCS1_SHA512,
};
EXPECT_EQ(expected, key->GetAlgorithmPreferences());
if (SHA256ProbeEnabled()) {
std::vector<uint16_t> expected = {
SSL_SIGN_RSA_PKCS1_SHA256,
SSL_SIGN_RSA_PKCS1_SHA384,
SSL_SIGN_RSA_PKCS1_SHA512,
SSL_SIGN_RSA_PKCS1_SHA1,
};
EXPECT_EQ(expected, key->GetAlgorithmPreferences());
} else {
// When the SHA-256 probe is disabled, we conservatively assume every CAPI
// key may be SHA-1-only.
std::vector<uint16_t> expected = {
SSL_SIGN_RSA_PKCS1_SHA1,
SSL_SIGN_RSA_PKCS1_SHA256,
SSL_SIGN_RSA_PKCS1_SHA384,
SSL_SIGN_RSA_PKCS1_SHA512,
};
EXPECT_EQ(expected, key->GetAlgorithmPreferences());
}

TestSSLPrivateKeyMatches(key.get(), pkcs8);
}

INSTANTIATE_TEST_SUITE_P(All,
SSLPlatformKeyWinTest,
testing::ValuesIn(kTestKeys),
TestKeyToString);
testing::Combine(testing::ValuesIn(kTestKeys),
testing::Bool()),
TestParamsToString);

} // namespace net

0 comments on commit 8849320

Please sign in to comment.