Skip to content

quiche: make cert verification check extended key usage of the cert #18309

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.

Sign up for GitHub

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

Merged
yanavlasov merged 11 commits into from
Oct 8, 2021
Merged

quiche: make cert verification check extended key usage of the cert #18309

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b3cace9
set default on X509_STORE_CTX
danzh1989 Sep 28, 2021
e634d47
added test
danzh1989 Sep 29, 2021
691701b
comment
danzh1989 Sep 30, 2021
50cda56
comments
danzh1989 Sep 30, 2021
6f575ea
comment
danzh1989 Oct 1, 2021
bf4fb3b
spelling
danzh1989 Oct 1, 2021
455c88d
add test for coverage
danzh1989 Oct 4, 2021
c296145
test coverage
danzh1989 Oct 5, 2021
e4b23d5
Merge branch 'master' into certverify
danzh1989 Oct 5, 2021
9e791bd
format
danzh1989 Oct 5, 2021
7073029
X509_VERIFY_PARAM_set1
danzh1989 Oct 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions source/extensions/transport_sockets/tls/context_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1171,6 +1171,12 @@ bool ContextImpl::verifyCertChain(X509& leaf_cert, STACK_OF(X509) & intermediate
error_details = "Failed to verify certificate chain: X509_STORE_CTX_init";
return false;
}
// Currently only EnvoyQuicProofVerifier, which is used by the client code, calls this method. So
// hard-code "ssl_server" for now.

@RyanTheOptimist RyanTheOptimist Sep 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that since this method is always called to verify a chain of certs presented by a server, it is fine to use ssl_server here. In other words I don't think we need to mention EnvoyQuicProofVerifier in the comment and instead can simply explain that this method is always call to verify server certs.

What do you think?

@danzh2010 danzh2010 Sep 30, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rephrased the comment.

if (!X509_STORE_CTX_set_default(ctx.get(), "ssl_server")) {
error_details = "Failed to verify certificate chain: X509_STORE_CTX_set_default";
return false;

@wbpcode wbpcode Oct 1, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a case to test this if branch?

@danzh2010 danzh2010 Oct 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, it's not clear to me how X509_STORE_CTX_set_default() would return 0 with "ssl_server". Probably this is impossible if the name is hard-coded to "ssl_server". But I'm not a boring SSL expert.

@danzh2010 danzh2010 Oct 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it is impossible with the hard-coded parameter, I would still prefer to handle the case where it returns 0 so that future BoringSSL change doesn't break Envoy in a unexpected way.

@wbpcode wbpcode Oct 2, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get it. But it seems that without this test, the test coverage is not up to the requirements. We can let the maintainer decide whether we can reduce the coverage requirement of this module.

@danzh2010 danzh2010 Oct 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a few irrelevant tls unit tests to raise coverage.

@sleevi sleevi Oct 7, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this is impossible with a hard-coded parameter. To synthetically generate code coverage, you'd have to allow the name ("ssl_server") to be injectable by the test, and then supply an invalid/unregistered name.

}

@sleevi sleevi Oct 7, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Should you also be inheriting the params from the SSL_CTX? I don't see this file manipulating them in any way, but it's an edge case I wanted to call out. Specifically, the following elements of the SSL_CTX are not carried over, so if they were ever used to configure the SSL_CTX, they wouldn't be propagated to the Quiche path:

  • SSL{_CTX}_set_verify_depth
  • SSL{_CTX}_set1_param
  • SSL{_CTX}_set_purpose
  • SSL{_CTX}_set_trust

Because this code already grabs the SSL_CTX_set_cert_store path (line 1169), perhaps it makes sense to do so here as well?

Suggested change
}
}
if (!X509_VERIFY_PARAM_set1(X509_STORE_CTX_get0_param(ctx.get()),
SSL_CTX_get0_param(ssl_ctx))) {
error_details = "Failed to verify certificate chain: X509_VERIFY_PARAM_set1";
return false;
}

This will copy the X509_VERIFY_PARAM config from the SSL_CTX into the X509_STORE_CTX's verify param as appropriate, and matches the SSL/TLS layer implementation.

@sleevi sleevi Oct 7, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To address the testing/coverage question: This is also an "impossible failure" scenario; the only way for this to fail is for SSL_CTX_get0_param to return an invalid parameter (since it will not return nullptr)

Because X509_VERIFY_PARAM is an opaque struct, whose setters validate the incoming parameters, to get an invalid parameter into the SSL_CTX_get0_param, you must have first used one of the X509_VERIFY_PARAM_set functions, which would have similarly validated the parameters then. So this leaves the only possible room for failure being a malloc failure, which presumably will blow things up spectacularly anyways out of necessity :)

@danzh2010 danzh2010 Oct 7, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Envoy doesn't config those aspects of SSL_CTX objects AFAIK. But sure that we can add X509_VERIFY_PARAM_set1() just in case.

Q please, with X509_VERIFY_PARAM_set1(), do I still need to call X509_STORE_CTX_set_default()?

@sleevi sleevi Oct 7, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The combination - of setting purpose, and then inheriting from the SSL_CTX, more or less matches how BoringSSL/OpenSSL's verifier works (source (ignoring RFC 6125 name matching, which I believe you separately implement)

@danzh2010 danzh2010 Oct 7, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


int res = cert_validator_->doVerifyCertChain(ctx.get(), nullptr, leaf_cert, nullptr);
// If |SSL_VERIFY_NONE|, the error is non-fatal, but we keep the error details.
Expand Down
71 changes: 70 additions & 1 deletion test/common/quic/envoy_quic_proof_verifier_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class EnvoyQuicProofVerifierTest : public testing::Test {
const std::string empty_string_;
const std::vector<std::string> empty_string_list_;
const std::string cert_chain_{quic::test::kTestCertificateChainPem};
const std::string root_ca_cert_;
std::string root_ca_cert_;
const std::string leaf_cert_;
const absl::optional<envoy::config::core::v3::TypedExtensionConfig> custom_validator_config_{
absl::nullopt};
Expand Down Expand Up @@ -192,5 +192,74 @@ VdGXMAjeXhnOnPvmDi5hUz/uvI+Pg6cNmUoCRwSCnK/DazhA
EXPECT_EQ("Invalid leaf cert, only P-256 ECDSA certificates are supported", error_details);
}

TEST_F(EnvoyQuicProofVerifierTest, VerifyCertChainFailureNonServerAuthEKU) {
root_ca_cert_ = R"(-----BEGIN CERTIFICATE-----

@RyanTheOptimist RyanTheOptimist Sep 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have documentation somewhere that explains how these certs were generated?

@danzh2010 danzh2010 Sep 30, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under test/config/integration/certs/ there are bunch of cert configs and certs.sh to generate various kinda cert. I just modified servercert.cfg a bit and run the script.

Commented about where this cert comes from.

@RyanTheOptimist RyanTheOptimist Sep 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks!

MIID3TCCAsWgAwIBAgIUdCu/mLip3X/We37vh3BA9u/nxakwDQYJKoZIhvcNAQEL
BQAwdjELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExFjAUBgNVBAcM
DVNhbiBGcmFuY2lzY28xDTALBgNVBAoMBEx5ZnQxGTAXBgNVBAsMEEx5ZnQgRW5n
aW5lZXJpbmcxEDAOBgNVBAMMB1Rlc3QgQ0EwHhcNMjAwODA1MTkxNjAwWhcNMjIw
ODA1MTkxNjAwWjB2MQswCQYDVQQGEwJVUzETMBEGA1UECAwKQ2FsaWZvcm5pYTEW
MBQGA1UEBwwNU2FuIEZyYW5jaXNjbzENMAsGA1UECgwETHlmdDEZMBcGA1UECwwQ
THlmdCBFbmdpbmVlcmluZzEQMA4GA1UEAwwHVGVzdCBDQTCCASIwDQYJKoZIhvcN
AQEBBQADggEPADCCAQoCggEBALu2Ihi4DmaQG7zySZlWyM9SjxOXCI5840V7Hn0C
XoiI8sQQmKSC2YCzsaphQoJ0lXCi6Y47o5FkooYyLeNDQTGS0nh+IWm5RCyochtO
fnaKPv/hYxhpyFQEwkJkbF1Zt1s6j2rq5MzmbWZx090uXZEE82DNZ9QJaMPu6VWt
iwGoGoS5HF5HNlUVxLNUsklNH0ZfDafR7/LC2ty1vO1c6EJ6yCGiyJZZ7Ilbz27Q
HPAUd8CcDNKCHZDoMWkLSLN3Nj1MvPVZ5HDsHiNHXthP+zV8FQtloAuZ8Srsmlyg
rJREkc7gF3f6HrH5ShNhsRFFc53NUjDbYZuha1u4hiOE8lcCAwEAAaNjMGEwDwYD
VR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAQYwHQYDVR0OBBYEFJZL2ixTtL6V
xpNz4qekny4NchiHMB8GA1UdIwQYMBaAFJZL2ixTtL6VxpNz4qekny4NchiHMA0G
CSqGSIb3DQEBCwUAA4IBAQAcgG+AaCdrUFEVJDn9UsO7zqzQ3c1VOp+WAtAU8OQK
Oc4vJYVVKpDs8OZFxmukCeqm1gz2zDeH7TfgCs5UnLtkplx1YO1bd9qvserJVHiD
LAK+Yl24ZEbrHPaq0zI1RLchqYUOGWmi51pcXi1gsfc8DQ3GqIXoai6kYJeV3jFJ
jxpQSR32nx6oNN/6kVKlgmBjlWrOy7JyDXGim6Z97TzmS6Clctewmw/5gZ9g+M8e
g0ZdFbFkNUjzSNm44hiDX8nR6yJRn+gLaARaJvp1dnT+MlvofZuER17WYKH4OyMs
ie3qKR3an4KC20CtFbpZfv540BVuTTOCtQ5xqZ/LTE78
-----END CERTIFICATE-----)";
configCertVerificationDetails(true);
const std::string ocsp_response;
const std::string cert_sct;
std::string error_details;
// This is a cert same as test/config/integration/certs/servercert.pem but with extKeyUsage:
// clientAuth.

@wbpcode wbpcode Oct 1, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here should be a more accurate description of the process of generating this cert.

May be: This is a cert generated with the test/config/integration/certs/certs.sh. And the config that used to generate this cert is same as test/config/integration/certs/servercert.cfg but with 'extKeyUsage: clientAuth'.

@danzh2010 danzh2010 Oct 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

const std::string certs{R"(-----BEGIN CERTIFICATE-----
MIIEYjCCA0qgAwIBAgIUWzmfQSTX8xfzUzdByjCjCJN8E/wwDQYJKoZIhvcNAQEL
BQAwdjELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExFjAUBgNVBAcM
DVNhbiBGcmFuY2lzY28xDTALBgNVBAoMBEx5ZnQxGTAXBgNVBAsMEEx5ZnQgRW5n
aW5lZXJpbmcxEDAOBgNVBAMMB1Rlc3QgQ0EwHhcNMjEwOTI5MTY0NTM3WhcNMjMw
OTI5MTY0NTM3WjCBpjELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWEx
FjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xDTALBgNVBAoMBEx5ZnQxGTAXBgNVBAsM
EEx5ZnQgRW5naW5lZXJpbmcxGjAYBgNVBAMMEVRlc3QgQmFja2VuZCBUZWFtMSQw
IgYJKoZIhvcNAQkBFhViYWNrZW5kLXRlYW1AbHlmdC5jb20wggEiMA0GCSqGSIb3
DQEBAQUAA4IBDwAwggEKAoIBAQC9JgaI7hxjPM0tsUna/QmivBdKbCrLnLW9Teak
RH/Ebg68ovyvrRIlybDT6XhKi+iVpzVY9kqxhGHgrFDgGLBakVMiYJ5EjIgHfoo4
UUAHwIYbunJluYCgANzpprBsvTC/yFYDVMqUrjvwHsoYYVm36io994k9+t813b70
o0l7/PraBsKkz8NcY2V2mrd/yHn/0HAhv3hl6iiJme9yURuDYQrae2ACSrQtsbel
KwdZ/Re71Z1awz0OQmAjMa2HuCop+Q/1QLnqBekT5+DH1qKUzJ3Jkq6NRkERXOpi
87j04rtCBteCogrO67qnuBZ2lH3jYEMb+lQdLkyNMLltBSdLAgMBAAGjgbYwgbMw
DAYDVR0TAQH/BAIwADALBgNVHQ8EBAMCBeAwEwYDVR0lBAwwCgYIKwYBBQUHAwIw
QQYDVR0RBDowOIYec3BpZmZlOi8vbHlmdC5jb20vYmFja2VuZC10ZWFtgghseWZ0
LmNvbYIMd3d3Lmx5ZnQuY29tMB0GA1UdDgQWBBTZdxNltzTEpl+A1UpK8BsxkkIG
hjAfBgNVHSMEGDAWgBSWS9osU7S+lcaTc+KnpJ8uDXIYhzANBgkqhkiG9w0BAQsF
AAOCAQEAhiXkQJZ53L3uoQMX6xNhAFThomirnLm2RT10kPIbr5mmf3wcR8+EKrWX
dWCj56bk1tSDbQZqx33DSGbhvNaydggbo69Pkie5b7J9O7AWzT21NME6Jis9hHED
VUI63L+7SgJ2oZs0o8xccUaLFeknuNdQL4qUEwhMwCC8kYLz+c6g0qwDwZi1MtdL
YR4qm2S6KveVPGzBHpUjfWf/whSCM3JN5Fm8gWfC6d6XEYz6z1dZrj3lpwmhRgF6
Wb72f68jzCQ3BFqKRFsJI2xz3EP6PoQ+e6EQjMpjQLomxIhIN/aTsgrKwA5wf6vQ
ZCFbredVxDBZuoVsfrKPSQa407Jj1Q==
-----END CERTIFICATE-----)"};
std::stringstream pem_stream(certs);
std::vector<std::string> chain = quic::CertificateView::LoadPemFromStream(&pem_stream);
std::unique_ptr<quic::CertificateView> cert_view =
quic::CertificateView::ParseSingleCertificate(chain[0]);
ASSERT(cert_view);
EXPECT_EQ(quic::QUIC_FAILURE,
verifier_->VerifyCertChain("lyft.com", 54321, chain, ocsp_response, cert_sct, nullptr,
&error_details, nullptr, nullptr, nullptr));
EXPECT_EQ("X509_verify_cert: certificate verification error at depth 0: unsupported certificate "
"purpose",
error_details);
}

} // namespace Quic
} // namespace Envoy
1 change: 1 addition & 0 deletions tools/spelling/spelling_dictionary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1083,6 +1083,7 @@ sendto
serializable
serializer
serv
servercert
setenv
setsockopt
sig
Expand Down