Skip to content
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

Save leaf certificate in SSL early to avoid losing external data #1074

Merged
merged 4 commits into from
Jul 6, 2023
Merged

Save leaf certificate in SSL early to avoid losing external data #1074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60ff3bc
do not cache leaf certificate in SSL
samuel40791765 Jun 26, 2023
8d416a9
PR comments; remove redundant null check
samuel40791765 Jun 28, 2023
19d006d
fix certificate parsing when set with ASN1
samuel40791765 Jun 30, 2023
6e78eaa
PR comments; bad spelling
samuel40791765 Jul 5, 2023
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
9 changes: 4 additions & 5 deletions ssl/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2605,14 +2605,13 @@ struct SSL_X509_METHOD {
void (*cert_clear)(CERT *cert);
// cert_free frees all X509-related state.
void (*cert_free)(CERT *cert);
// cert_flush_cached_chain drops any cached |X509|-based certificate chain
// from |cert|.
// cert_dup duplicates any needed fields from |cert| to |new_cert|.
void (*cert_dup)(CERT *new_cert, const CERT *cert);
void (*cert_flush_cached_chain)(CERT *cert);
// cert_flush_cached_chain drops any cached |X509|-based leaf certificate
// cert_flush_cached_chain drops any cached |X509|-based certificate chain
// from |cert|.
void (*cert_flush_cached_leaf)(CERT *cert);
void (*cert_flush_cached_chain)(CERT *cert);
// cert_flush_leaf drops the |X509|-based leaf certificate from |cert|.
void (*cert_flush_leaf)(CERT *cert);

// session_cache_objects fills out |sess->x509_peer| and |sess->x509_chain|
// from |sess->certs| and erases |sess->x509_chain_without_leaf|. It returns
Expand Down
7 changes: 5 additions & 2 deletions ssl/ssl_cert.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,11 @@ UniquePtr<CERT> ssl_cert_dup(CERT *cert) {
}
}

if (cert->x509_leaf != nullptr) {
X509_up_ref(cert->x509_leaf);
ret->x509_leaf = cert->x509_leaf;
}

ret->privatekey = UpRef(cert->privatekey);
ret->key_method = cert->key_method;

Expand Down Expand Up @@ -316,8 +321,6 @@ bool ssl_set_cert(CERT *cert, UniquePtr<CRYPTO_BUFFER> buffer) {
break;
}

cert->x509_method->cert_flush_cached_leaf(cert);

if (cert->chain != nullptr) {
CRYPTO_BUFFER_free(sk_CRYPTO_BUFFER_value(cert->chain.get(), 0));
sk_CRYPTO_BUFFER_set(cert->chain.get(), 0, buffer.release());
Expand Down
72 changes: 72 additions & 0 deletions ssl/ssl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4783,6 +4783,7 @@ TEST(SSLTest, GetCertificate) {

X509 *cert2 = SSL_CTX_get0_certificate(ctx.get());
ASSERT_TRUE(cert2);

X509 *cert3 = SSL_get_certificate(ssl.get());
ASSERT_TRUE(cert3);

Expand Down Expand Up @@ -4810,6 +4811,77 @@ TEST(SSLTest, GetCertificate) {
EXPECT_EQ(Bytes(der, der_len), Bytes(der3, der3_len));
}

TEST(SSLTest, GetCertificateExData) {
bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
ASSERT_TRUE(ctx);
bssl::UniquePtr<X509> cert = GetTestCertificate();
ASSERT_TRUE(cert);

int ex_data_index =
X509_get_ex_new_index(0, nullptr, nullptr, nullptr, nullptr);
const char ex_data[] = "AWS-LC external data";
ASSERT_TRUE(X509_set_ex_data(cert.get(), ex_data_index, (void *)ex_data));
ASSERT_TRUE(X509_get_ex_data(cert.get(), ex_data_index));

ASSERT_TRUE(SSL_CTX_use_certificate(ctx.get(), cert.get()));
bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get()));
ASSERT_TRUE(ssl);

X509 *cert2 = SSL_CTX_get0_certificate(ctx.get());
ASSERT_TRUE(cert2);
const char *ex_data2 = (const char *)X509_get_ex_data(cert2, ex_data_index);
EXPECT_TRUE(ex_data2);

X509 *cert3 = SSL_get_certificate(ssl.get());
ASSERT_TRUE(cert3);
const char *ex_data3 = (const char *)X509_get_ex_data(cert3, ex_data_index);
EXPECT_TRUE(ex_data3);

// The external data extracted must be identical.
EXPECT_EQ(ex_data2, ex_data);
EXPECT_EQ(ex_data3, ex_data);
}

TEST(SSLTest, GetCertificateASN1) {
bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
ASSERT_TRUE(ctx);
bssl::UniquePtr<X509> cert = GetTestCertificate();
ASSERT_TRUE(cert);

// Convert cert to ASN1 to pass in.
uint8_t *der = nullptr;
size_t der_len = i2d_X509(cert.get(), &der);
bssl::UniquePtr<uint8_t> free_der(der);

ASSERT_TRUE(SSL_CTX_use_certificate_ASN1(ctx.get(), der_len, der));
bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get()));
ASSERT_TRUE(ssl);

X509 *cert2 = SSL_CTX_get0_certificate(ctx.get());
ASSERT_TRUE(cert2);

X509 *cert3 = SSL_get_certificate(ssl.get());
ASSERT_TRUE(cert3);

// The old and new certificates must be identical.
EXPECT_EQ(0, X509_cmp(cert.get(), cert2));
EXPECT_EQ(0, X509_cmp(cert.get(), cert3));

uint8_t *der2 = nullptr;
long der2_len = i2d_X509(cert2, &der2);
ASSERT_LT(0, der2_len);
bssl::UniquePtr<uint8_t> free_der2(der2);

uint8_t *der3 = nullptr;
long der3_len = i2d_X509(cert3, &der3);
ASSERT_LT(0, der3_len);
bssl::UniquePtr<uint8_t> free_der3(der3);

// They must also encode identically.
EXPECT_EQ(Bytes(der, der_len), Bytes(der2, der2_len));
EXPECT_EQ(Bytes(der, der_len), Bytes(der3, der3_len));
}

TEST(SSLTest, SetChainAndKeyMismatch) {
bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_with_buffers_method()));
ASSERT_TRUE(ctx);
Expand Down
15 changes: 11 additions & 4 deletions ssl/ssl_x509.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ static bool ssl_cert_set_chain(CERT *cert, STACK_OF(X509) *chain) {
return true;
}

static void ssl_crypto_x509_cert_flush_cached_leaf(CERT *cert) {
static void ssl_crypto_x509_cert_flush_leaf(CERT *cert) {
X509_free(cert->x509_leaf);
cert->x509_leaf = nullptr;
}
Expand All @@ -260,7 +260,7 @@ static bool ssl_crypto_x509_check_client_CA_list(
}

static void ssl_crypto_x509_cert_clear(CERT *cert) {
ssl_crypto_x509_cert_flush_cached_leaf(cert);
ssl_crypto_x509_cert_flush_leaf(cert);
ssl_crypto_x509_cert_flush_cached_chain(cert);

X509_free(cert->x509_stash);
Expand Down Expand Up @@ -526,7 +526,7 @@ const SSL_X509_METHOD ssl_crypto_x509_method = {
ssl_crypto_x509_cert_free,
ssl_crypto_x509_cert_dup,
ssl_crypto_x509_cert_flush_cached_chain,
ssl_crypto_x509_cert_flush_cached_leaf,
ssl_crypto_x509_cert_flush_leaf,
ssl_crypto_x509_session_cache_objects,
ssl_crypto_x509_session_dup,
ssl_crypto_x509_session_clear,
Expand Down Expand Up @@ -758,6 +758,12 @@ static int ssl_use_certificate(CERT *cert, X509 *x) {
return 0;
}

// We set the |x509_leaf| here to prevent any external data set from being
// lost. The rest of the chain still uses |CRYPTO_BUFFER|s.
X509_free(cert->x509_leaf);
X509_up_ref(x);
cert->x509_leaf = x;

UniquePtr<CRYPTO_BUFFER> buffer = x509_to_buffer(x);
if (!buffer) {
return 0;
Expand All @@ -780,7 +786,8 @@ int SSL_CTX_use_certificate(SSL_CTX *ctx, X509 *x) {
}

// ssl_cert_cache_leaf_cert sets |cert->x509_leaf|, if currently NULL, from the
// first element of |cert->chain|.
// first element of |cert->chain|. This is the case when certs are set with
// |SSL_CTX_use_certificate_ASN1| or |SSL_use_certificate_ASN1| in AWS-LC.
static int ssl_cert_cache_leaf_cert(CERT *cert) {
assert(cert->x509_method);

Expand Down