Fix PSS advertising during TLS handshake for TPM signing on Windows [chromium/src : main]
Callum May (Gerrit)
Attention is currently required from: David Benjamin.
To view, visit change 2984231. To unsubscribe, or for help writing mail filters, visit settings.
David Benjamin (Gerrit)
Attention is currently required from: Callum May.
3 comments:
File net/ssl/ssl_platform_key_win.cc:
Patch Set #2, Line 240:
// algorithm preference if the salt length will match the digest length.Ah, interesting. We set pss_padding_info.cbSalt = EVP_MD_size(md) down in line 322 here, so we're not just relying on the TPM to provide whatever value. There's a particular one that we're asking for.
Are you saying that the Microsoft TPM provider was buggy and just ignored it, or that it would reject cbSalt values when they don't match the maximum salt length? And then this property is how you detect keys from the Microsoft TPM provider that handle this better?
How would a provider that supports arbitrary PSS salt sizes react to the property query? Are there docs on the NCRYPT_PCP_PSS_SALT_SIZE_PROPERTY property? (I couldn't find them.)
Patch Set #2, Line 242:
base::WideToUTF8It doesn't really matter here, but base::WideToUTF8 is potentially lossy if the provider name is not valid UTF-16. Maybe we should make GetCNGProviderName() return a std::wstring, store a std::wstring in provider_name_ so this comparison can be direct, and then move the base::WideToUTF8 call to GetProviderName().
To avoid duplicating things, let's write:
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);
}
return ret;(I wonder if we still need this workaround...)
To view, visit change 2984231. To unsubscribe, or for help writing mail filters, visit settings.
Callum May (Gerrit)
Attention is currently required from: David Benjamin.
3 comments:
File net/ssl/ssl_platform_key_win.cc:
Patch Set #2, Line 240:
// algorithm preference if the salt length will match the digest length.
Ah, interesting. We set pss_padding_info. […]
Until TPM revision 1.16, the TPM was _required_ to use the maximum salt length allowed, so it would reject the cbSalt value.
Yes this is how we detect if a TPM is FIPS compliant, in that if it returns
NCRYPT_TPM_PSS_SALT_SIZE_HASHSIZE instead of NCRYPT_TPM_PSS_SALT_SIZE_MAXIMUM we know that it will be FIPS 1.16 compliant.The documentation here is sparse unfortunately, we've leveraged internal folks to make this fix. It depends on provider. If the provider is a FIPS certified TPM provider, a requirement for FIPS certification is that the salt length is always equal to the hash size. Many TPMs are not FIPS certified, and therefore use the TCG specification of the maximum amount of salt possible. If the provider is not a TPM provider I'm not sure what the behaviour for the property query would be although from the looks of it there are only three values and their specific to TPM based salts. I can consult internally for more information on this if we need
Patch Set #2, Line 242:
base::WideToUTF8
It doesn't really matter here, but base::WideToUTF8 is potentially lossy if the provider name is not […]
Done
To avoid duplicating things, let's write: […]
Done
To view, visit change 2984231. To unsubscribe, or for help writing mail filters, visit settings.
David Benjamin (Gerrit)
Attention is currently required from: Callum May.
3 comments:
File net/ssl/ssl_platform_key_win.cc:
Patch Set #2, Line 240:
// algorithm preference if the salt length will match the digest length.
Until TPM revision 1. […]
Ah, I see. I hadn't realized TPM 2.0 mandated a PSS length, but I've pulled up the spec there and yeah you're right. Yuck. PSS really is the poster-child for why over-parameterizing your algorithms is a terrible idea!
Let me try to restate what you're saying to make sure I understand correctly:
1. TPM_ALG_RSAPSS does not take a salt length parameter, just a hash. The TPM_ALG_RSAPSS(TPM_ALG_SHA256) simply means "RSA-PSS with SHA-256, MGF-1(SHA-256), max salt length".
2. FIPS and TLS 1.3 instead use a different set of PSS parameters, with salt length = digest length. This is incompatible with the definition of TPM_ALG_RSAPSS.
3. Rather than define a new algorithm, FIPS TPMs just silently change the definition of TPM_ALG_RSAPSS and use a different algorithm. There's a tiny print NOTE in TPM 2.0, Part 1, B.7 which allows this. That seems to be what's new in the 1.16 specification.
4. The MS_PLATFORM_KEY_STORAGE_PROVIDER is the TPM storage provider (is it always TPM or is it ever something else?). Being the TPM storage provider it, correctly, rejects all BCRYPT_PSS_PADDING_INFO requests that are incompatible with what the TPM wants.
5. Somehow, the Microsoft TPM provider is able to figure out whether this is a max-salt-length TPM or a digest-salt-length TPM and exposes this out of the undocumented NCRYPT_PCP_PSS_SALT_SIZE_PROPERTY API. (Can you please pass along feedback to the crypto team that sometime this really should be documented?)
Is that all correct? In that case, some more questions for you:
First: What exactly is the failure case you're seeing?
Most of our platforms have very sensible ways to query algorithm support, e.g. SecKeyIsAlgorithmSupported, PK11_DoesMechanism, and trying to instantiate a Signature object in Android. It's just Windows where we were unable to find a good way to do this.
We instead work around this platform issue, by putting PSS at the end of our preference list. So that means we only select PSS at all if the server or protocol did not allow any other option. So it's actually okay if we think we support PSS, but really do not. A better read would turn ERR_SSL_CLIENT_AUTH_SIGNATURE_FAILED into ERR_SSL_CLIENT_AUTH_NO_COMMON_ALGORITHMS, which is a better error. But, at least right now, the request would fail either way. We don't trigger a TLS 1.2 fallback or anything, though we have pondered whether we can do that securely. I also wrote draft-davidben-tls13-pkcs1-00, but the TLSWG did not like it.
Is reporting a more accurate error your goal, or are you hoping to make the connection work? Making the connection work will require a bit more work, either client and server implementing the PKCS#1 backport draft or us figuring out a TLS 1.2 fallback (which we would only accept if it didn't admit a downgrade attack).
That leads me to my second question: Is there a cross-provider way to query for algorithm support on Windows? Without that, we already have to rely on not getting an accurate supports_pss answer on Windows anyway. (Unlike all our other platforms, which support this just fine.)
File net/ssl/ssl_platform_key_win.cc:
Nit: Extra space on this line and the line below. Although see comment below.
Patch Set #3, Line 240:
// broken from a TLS protocol perspective.Assuming I'm understanding this correctly, this text isn't quiiite right. It's not that the TPM is broken or not compliant w.r.t. TLS. RSA-PSS was mis-standardized and was over-parametrized. PSS with one set of parameters and PSS with another set of parameters are different signature schemes.
How about:
"""
Per TLS 1.3 (RFC 8446), the RSA-PSS code points in TLS correspond to RSA-PSS with salt length equal to the digest length. TPM 2.0's TPM_ALG_RSAPSS algorithm, however, uses the maximum possible salt length. The TPM provider will fail signing requests for other salt lengths and thus cannot generate TLS-compatible PSS sigantures.However, as of TPM revision 1.16, TPMs which follow FIPS 186-4 will instead interpret TPM_ALG_RSAPSS using salt length equal to the digest length. Those TPMs can generate TLS-compatible PSS signatures. As a result, if this is a TPM-based key, we only report PSS as supported if the salt length will match the digest length.
"""(I replaced "advertise" with "report" just because "advertise" to me reads like sending it over the wire. But that's not how TLS works. The server will send us a list of algorithms, and we pick one based on our internal preferences. To that end, see the question below...)
To view, visit change 2984231. To unsubscribe, or for help writing mail filters, visit settings.
Callum May (Gerrit)
Attention is currently required from: David Benjamin.
3 comments:
File net/ssl/ssl_platform_key_win.cc:
Patch Set #2, Line 240:
// algorithm preference if the salt length will match the digest length.
Ah, I see. I hadn't realized TPM 2. […]
Your understanding is correct. This failure stemmed from a report of a client cert failing auth. This client cert was TPM-backed. We eventually root caused the failure to this. The TLS connection was failing because of the mismatch in expected salts.
Ideally we are hoping to make the connections work. In this instance, because of the client cert requirement we weren't able to make it work but we had a clear message why. Nevertheless this seems like a productive change to prevent PSS reporting when it isn't actually supported.
Yes you can enumerate the supported algorithms by a provider:
https://docs.microsoft.com/en-us/windows/win32/api/ncrypt/nf-ncrypt-ncryptenumalgorithms
File net/ssl/ssl_platform_key_win.cc:
Nit: Extra space on this line and the line below. Although see comment below.
Done
Patch Set #3, Line 240:
// broken from a TLS protocol perspective.
Assuming I'm understanding this correctly, this text isn't quiiite right. […]
Done
To view, visit change 2984231. To unsubscribe, or for help writing mail filters, visit settings.
David Benjamin (Gerrit)
Attention is currently required from: Callum May.
Patch set 4:Code-Review +1
2 comments:
File net/ssl/ssl_platform_key_win.cc:
Patch Set #2, Line 240:
// algorithm preference if the salt length will match the digest length.
Ideally we are hoping to make the connections work. In this instance, because of the client cert requirement we weren't able to make it work but we had a clear message why. Nevertheless this seems like a productive change to prevent PSS reporting when it isn't actually supported.
Alright. It definitely won't make the connection work, but it will be a slightly more accurate error.
Yes you can enumerate the supported algorithms by a provider:
https://docs.microsoft.com/en-us/windows/win32/api/ncrypt/nf-ncrypt-ncryptenumalgorithms
Ah hrm. I could have sworn I'd tried that previously and it didn't work for some reason. Or maybe it was NCryptIsAlgSupported(). Something to look at for later I guess.
File net/ssl/ssl_platform_key_win.cc:
Patch Set #4, Line 238:
// TLS 1.3 RFC).I think we can drop this middle paragraph. It's redundant with the one above and below.
To view, visit change 2984231. To unsubscribe, or for help writing mail filters, visit settings.
Callum May (Gerrit)
1 comment:
File net/ssl/ssl_platform_key_win.cc:
Patch Set #4, Line 238:
// TLS 1.3 RFC).I think we can drop this middle paragraph. It's redundant with the one above and below.
Done
To view, visit change 2984231. To unsubscribe, or for help writing mail filters, visit settings.
Callum May (Gerrit)
Patch set 5:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Fix PSS advertising during TLS handshake for TPM signing on Windows
Some older TPMs are not FIPS compliant, and sign with the maximum salt
size instead of matching the salt size to the size of the message
digest. TLS 1.3 expects these salt sizes to match the digest size and
thus the connection fails. Here we add a check for TPM based keys to
ensure they're using the right salt size, and if they aren't, remove
PSS as a signing option during the handshake.
Bug: 1223462
Change-Id: I118d8beb8bc7f807efd7dbdfef879abe2d504f15
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2984231
Commit-Queue: Callum May <ca...@microsoft.com>
Reviewed-by: David Benjamin <davi...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#897135}
---
M net/ssl/ssl_platform_key_win.cc
1 file changed, 50 insertions(+), 17 deletions(-)
To view, visit change 2984231. To unsubscribe, or for help writing mail filters, visit settings.
David Benjamin (Gerrit)
1 comment:
File net/ssl/ssl_platform_key_win.cc:
Patch Set #2, Line 240:
// algorithm preference if the salt length will match the digest length.Yes you can enumerate the supported algorithms by a provider:
To circle back here belatedly, that's not right. It's the wrong notion of "algorithm" because the Windows API sub-categorizes things in too many ways:
https://bugs.chromium.org/p/chromium/issues/detail?id=924284#c14
To view, visit change 2984231. To unsubscribe, or for help writing mail filters, visit settings.
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
name.reserve(name_len / sizeof(wchar_t));FYI, this broke the provider name lookup so actually the new code you all added doesn't do anything. I'll upload something to fix this.
Related details
- Callum May
Code-Review
Custom-Rule
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
name.reserve(name_len / sizeof(wchar_t));FYI, this broke the provider name lookup so actually the new code you all added doesn't do anything. I'll upload something to fix this.
https://chromium-review.googlesource.com/c/chromium/src/+/7528873 but this is actually uncovering that we will go from never running this clearly untested code to actually running it, so I may back this code out altogether and replace it with something flag-protected (and done at the right place).
Related details
- Callum May
Code-Review
Custom-Rule
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |