-
Notifications
You must be signed in to change notification settings - Fork 5.3k
tls: client ECDSA detection during ClientHello. #5200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9217916
3a7c08a
cd2dfff
b480487
86ed9fb
36c0664
5e991d1
c1ef2a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,8 +24,27 @@ | |
| namespace Envoy { | ||
| namespace Ssl { | ||
|
|
||
| namespace { | ||
|
|
||
| bool cbsContainsU16(CBS& cbs, uint16_t n) { | ||
| while (CBS_len(&cbs) > 0) { | ||
| uint16_t v; | ||
| if (!CBS_get_u16(&cbs, &v)) { | ||
| return false; | ||
| } | ||
| if (v == n) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config, TimeSource& time_source) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would make sense to verify that you're adding at most one RSA and one ECDSA certificate, since
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't have particular plans to reject it though, at least with the PR as-is, I don't think anything but the first ECDSA certificate and the first RSA certificate will ever be used.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on what @davidben writes, I think we should be more permissive today. In general, considering that this code is throw away when SSL_IDENTITY arrives, we should aim to minimize the number of additional branches and testing complexity.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @htuch I think you are confusing two related decisions. If you only look at one curve, then obviously only one ECDSA certificate will be used and multiple are pointless. If you look at multiple curves, having both a P-256 and a P-384 certificate is potentially useful. However, P-384 and P-521 are themselves pointless, so that informs the first decision.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm going to stick to P-256 only for simplicity and validate the certificate at config ingestion time to ensure it meets this requirement.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #5224 |
||
| : scope_(scope), stats_(generateStats(scope)), time_source_(time_source) { | ||
| : scope_(scope), stats_(generateStats(scope)), time_source_(time_source), | ||
| tls_max_version_(config.maxProtocolVersion()) { | ||
| const auto tls_certificates = config.tlsCertificates(); | ||
| tls_contexts_.resize(std::max(1UL, tls_certificates.size())); | ||
|
|
||
|
|
@@ -539,6 +558,14 @@ ClientContextImpl::ClientContextImpl(Stats::Scope& scope, const ClientContextCon | |
| } | ||
| } | ||
|
|
||
| if (!config.signingAlgorithmsForTest().empty()) { | ||
| for (auto& ctx : tls_contexts_) { | ||
| int rc = | ||
| SSL_CTX_set1_sigalgs_list(ctx.ssl_ctx_.get(), config.signingAlgorithmsForTest().c_str()); | ||
| RELEASE_ASSERT(rc == 1, ""); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Note this will fail on syntax error. Is that okay for your purposes here?)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is just for tests; I've done the rename suggested by @PiotrSikora, so we are good here. |
||
| } | ||
| } | ||
|
|
||
| if (max_session_keys_ > 0) { | ||
| SSL_CTX_set_session_cache_mode(tls_contexts_[0].ssl_ctx_.get(), SSL_SESS_CACHE_CLIENT); | ||
| SSL_CTX_sess_set_new_cb( | ||
|
|
@@ -824,11 +851,88 @@ int ServerContextImpl::sessionTicketProcess(SSL*, uint8_t* key_name, uint8_t* iv | |
| } | ||
| } | ||
|
|
||
| bool ServerContextImpl::isClientEcdsaCapable(const SSL_CLIENT_HELLO* ssl_client_hello) { | ||
| CBS client_hello; | ||
| CBS_init(&client_hello, ssl_client_hello->client_hello, ssl_client_hello->client_hello_len); | ||
|
|
||
| // This is the TLSv1.3 case (TLSv1.2 on the wire and the supported_versions extensions present). | ||
| // We just need to loook at signature algorithms. | ||
| const uint16_t client_version = ssl_client_hello->version; | ||
| if (client_version == TLS1_2_VERSION && tls_max_version_ == TLS1_3_VERSION) { | ||
| // If the supported_versions extension is found then we assume that the client is competent | ||
| // enough that just checking the signature_algorithms is sufficient. | ||
| const uint8_t* supported_versions_data; | ||
| size_t supported_versions_len; | ||
| if (SSL_early_callback_ctx_extension_get(ssl_client_hello, TLSEXT_TYPE_supported_versions, | ||
| &supported_versions_data, &supported_versions_len)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory, we should verify that client announced TLS 1.3 in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to follow the reference implementation here and leave as is, we can discuss offline. |
||
| const uint8_t* signature_algorithms_data; | ||
| size_t signature_algorithms_len; | ||
| if (SSL_early_callback_ctx_extension_get(ssl_client_hello, TLSEXT_TYPE_signature_algorithms, | ||
| &signature_algorithms_data, | ||
| &signature_algorithms_len)) { | ||
| CBS signature_algorithms_ext, signature_algorithms; | ||
| CBS_init(&signature_algorithms_ext, signature_algorithms_data, signature_algorithms_len); | ||
| if (!CBS_get_u16_length_prefixed(&signature_algorithms_ext, &signature_algorithms) || | ||
| CBS_len(&signature_algorithms_ext) != 0) { | ||
| return false; | ||
| } | ||
| if (cbsContainsU16(signature_algorithms, SSL_SIGN_ECDSA_SECP256R1_SHA256)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // Otherwise we are < TLSv1.3 and need to look at both the curves in the supported_groups for | ||
| // ECDSA and also for a compatible cipher suite. https://tools.ietf.org/html/rfc4492#section-5.1.1 | ||
| const uint8_t* curvelist_data; | ||
| size_t curvelist_len; | ||
| if (!SSL_early_callback_ctx_extension_get(ssl_client_hello, TLSEXT_TYPE_supported_groups, | ||
| &curvelist_data, &curvelist_len)) { | ||
| return false; | ||
| } | ||
|
|
||
| CBS curvelist; | ||
| CBS_init(&curvelist, curvelist_data, curvelist_len); | ||
|
|
||
| // We only support P256 ECDSA curves today. | ||
| if (!cbsContainsU16(curvelist, SSL_CURVE_SECP256R1)) { | ||
| return false; | ||
| } | ||
|
|
||
| // The client must have offered an ECDSA ciphersuite that we like. | ||
| CBS cipher_suites; | ||
| CBS_init(&cipher_suites, ssl_client_hello->cipher_suites, ssl_client_hello->cipher_suites_len); | ||
|
|
||
| while (CBS_len(&cipher_suites) > 0) { | ||
| uint16_t cipher_id; | ||
| if (!CBS_get_u16(&cipher_suites, &cipher_id)) { | ||
| return false; | ||
| } | ||
| // All tls_context_ share the same set of enabled ciphers, so we can just look at the base | ||
| // context. | ||
| if (tls_contexts_[0].isCipherEnabled(cipher_id, client_version)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| enum ssl_select_cert_result_t | ||
| ServerContextImpl::selectTlsContext(const SSL_CLIENT_HELLO* ssl_client_hello) { | ||
| // This is currently a nop, since we only have a single cert, but this is where we will implement | ||
| // the certificate selection logic in #1319. | ||
| RELEASE_ASSERT(SSL_set_SSL_CTX(ssl_client_hello->ssl, tls_contexts_[0].ssl_ctx_.get()) != nullptr, | ||
| const bool client_ecdsa_capable = isClientEcdsaCapable(ssl_client_hello); | ||
| // Fallback on first certificate. | ||
| const TlsContext* selected_ctx = &tls_contexts_[0]; | ||
| for (const auto& ctx : tls_contexts_) { | ||
| if (client_ecdsa_capable == ctx.is_ecdsa_) { | ||
| selected_ctx = &ctx; | ||
| break; | ||
| } | ||
| } | ||
| RELEASE_ASSERT(SSL_set_SSL_CTX(ssl_client_hello->ssl, selected_ctx->ssl_ctx_.get()) != nullptr, | ||
| ""); | ||
| return ssl_select_cert_success; | ||
| } | ||
|
|
@@ -878,5 +982,25 @@ void ServerContextImpl::TlsContext::addClientValidationContext( | |
| } | ||
| } | ||
|
|
||
| bool ServerContextImpl::TlsContext::isCipherEnabled(uint16_t cipher_id, uint16_t client_version) { | ||
| const SSL_CIPHER* c = SSL_get_cipher_by_value(cipher_id); | ||
| if (c == nullptr) { | ||
| return false; | ||
| } | ||
| // Skip TLS 1.2 only ciphersuites unless the client supports it. | ||
| if (SSL_CIPHER_get_min_version(c) > client_version) { | ||
| return false; | ||
| } | ||
| if (SSL_CIPHER_get_auth_nid(c) != NID_auth_ecdsa) { | ||
| return false; | ||
| } | ||
| for (const SSL_CIPHER* our_c : SSL_CTX_get_ciphers(ssl_ctx_.get())) { | ||
| if (SSL_CIPHER_get_id(our_c) == SSL_CIPHER_get_id(c)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| } // namespace Ssl | ||
| } // namespace Envoy | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no standard string representation of a signature algorithm list. This should document what format is being used.