tls: reject non-P256 TLS certificates.#5224
Merged
htuch merged 5 commits intoenvoyproxy:masterfrom Dec 7, 2018
Merged
Conversation
In support of envoyproxy#5200. There are no performance or security advantages today of non-P256 ECDSA certs. Risk Level: Low (unlikely anyone is using these). Testing: Pending; will add when we have agreement this is where we want to go. Signed-off-by: Harvey Tuch <htuch@google.com>
PiotrSikora
suggested changes
Dec 5, 2018
Contributor
PiotrSikora
left a comment
PiotrSikora
left a comment
There was a problem hiding this comment.
Please add test for rejected message.
source/common/ssl/context_impl.cc
Outdated
| EC_KEY* ecdsa_public_key = EVP_PKEY_get0_EC_KEY(public_key.get()); | ||
| const int nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(ecdsa_public_key)); | ||
| if (nid != NID_X9_62_prime256v1) { | ||
| throw EnvoyException(fmt::format("ECDSA key is not P256 (NID = {})", nid)); |
Contributor
There was a problem hiding this comment.
Can you match the text with other errors here, e.g.
fmt::format("Failed to load certificate chain from {}, only P-256 ECDSA certificates are supported", ctx.cert_chain_file_path_)
Member
Author
There was a problem hiding this comment.
Sure. Is the PR good on other fronts? If so, I can update the error message and add tests.
Contributor
There was a problem hiding this comment.
Maybe check if EVP_PKEY_get0_EC_KEY() and EC_KEY_get0_group() return something, i.e.
EC_KEY* ecdsa_public_key = EVP_PKEY_get0_EC_KEY(public_key.get());
if (ecdsa_public_key == nullptr) {
throw EnvoyException(...);
}
const EC_GROUP *ecdsa_group = EC_KEY_get0_group(ecdsa_public_key);
if (group == nullptr || EC_GROUP_get_curve_name(group) != NID_X9_62_prime256v1) {
throw EnvoyException(...);
}
Throwing exceptions instead of RELEASE_ASSERT() to avoid malformed certificates sent via xDS from crashing Envoy at runtime.
Member
Author
There was a problem hiding this comment.
EVP_PKEY_get0_EC_KEY should always be non-null, since we checked the key type in the lines above. We probably should validate the group though, sure.
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
htuch
changed the title
PiotrSikora
previously approved these changes
Dec 6, 2018
Contributor
PiotrSikora
left a comment
PiotrSikora
left a comment
There was a problem hiding this comment.
LGTM, with one nit in the error message.
source/common/ssl/context_impl.cc
Outdated
| const EC_GROUP* ecdsa_group = EC_KEY_get0_group(ecdsa_public_key); | ||
| if (ecdsa_group == nullptr || EC_GROUP_get_curve_name(ecdsa_group) != NID_X9_62_prime256v1) { | ||
| throw EnvoyException(fmt::format("Failed to load certificate from chain {}, only P-256 " | ||
| "certificates are supported", |
Contributor
There was a problem hiding this comment.
Nit: it should be P-256 ECDSA certificates.
fredlas
pushed a commit
to fredlas/envoy
that referenced
this pull request
Mar 5, 2019
In support of envoyproxy#5200. There are no performance or security advantages today of non-P256 ECDSA certs. Risk Level: Low (unlikely anyone is using these). Testing: Unit tests added to ssl:context_impl_test for happy/sad paths. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
ggreenway
pushed a commit
that referenced
this pull request
Oct 15, 2024
Commercial National Security Algorithm Suite (CNSA) requires ECDSA keys be specified with P-384 curves. The assertion that there are [no security benefits to curves higher than P-256](#5224 (comment)) is no longer true. This change is intended to limit the adoptable curves to P-384 and P-521. Risk Level: Medium - removal of limitation of curves to be used for ECDSA certificates, with [potential misconfiguration and DoS risks](#10855 (comment)) mentioned in previous discourse on the issue. This risk is mitigated in this PR, however, by continuing to expressly limit the type of EC keys accepted to those associated with the P-256, P-384 or P-521 curves and no others. Fixes #10855 Signed-off-by: anitabyte <anita@anitabyte.xyz>
grnmeira
pushed a commit
to grnmeira/envoy
that referenced
this pull request
Oct 17, 2024
Commercial National Security Algorithm Suite (CNSA) requires ECDSA keys be specified with P-384 curves. The assertion that there are [no security benefits to curves higher than P-256](envoyproxy#5224 (comment)) is no longer true. This change is intended to limit the adoptable curves to P-384 and P-521. Risk Level: Medium - removal of limitation of curves to be used for ECDSA certificates, with [potential misconfiguration and DoS risks](envoyproxy#10855 (comment)) mentioned in previous discourse on the issue. This risk is mitigated in this PR, however, by continuing to expressly limit the type of EC keys accepted to those associated with the P-256, P-384 or P-521 curves and no others. Fixes envoyproxy#10855 Signed-off-by: anitabyte <anita@anitabyte.xyz> Signed-off-by: anitabyte <111812252+anitabyte@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In support of #5200. There are no performance or security advantages
today of non-P256 ECDSA certs.
Risk Level: Low (unlikely anyone is using these).
Testing: Unit tests added to ssl:context_impl_test for happy/sad paths.
Signed-off-by: Harvey Tuch htuch@google.com