-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
tls: reject non-P256 TLS certificates. #5224
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>
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.
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)); |
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.
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_)
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.
Sure. Is the PR good on other fronts? If so, I can update the error message and add tests.
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.
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.
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.
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>
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.
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", |
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.
Nit: it should be P-256 ECDSA certificates.
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.
Thanks!
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>
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>
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>
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