From 0bdef0926371d95dff977fb9e0f846c4fe9dcfae Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Thu, 23 Feb 2017 15:02:58 -0800 Subject: [PATCH] Check CA names during the handshake. Rather than store CA names and only find out that they're unparsable when we're asked for a |STACK_OF(X509_NAME)|, check that we can parse them all during the handshake. This avoids changing the semantics with the previous change that kept CA names as |CRYPTO_BUFFER|s. Change-Id: I0fc7a4e6ab01685347e7a5be0d0579f45b8a4818 Reviewed-on: https://boringssl-review.googlesource.com/13969 Reviewed-by: Adam Langley Commit-Queue: Adam Langley CQ-Verified: CQ bot account: commit-bot@chromium.org --- ssl/internal.h | 5 +++++ ssl/ssl_cert.c | 6 ++++++ ssl/ssl_x509.c | 18 ++++++++++++++++++ ssl/tls_method.c | 6 ++++++ 4 files changed, 35 insertions(+) diff --git a/ssl/internal.h b/ssl/internal.h index db526a8e..519ed79f 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1446,6 +1446,11 @@ struct ssl_protocol_method_st { }; struct ssl_x509_method_st { + /* check_client_CA_list returns one if |names| is a good list of X.509 + * distinguished names and zero otherwise. This is used to ensure that we can + * reject unparsable values at handshake time when using crypto/x509. */ + int (*check_client_CA_list)(STACK_OF(CRYPTO_BUFFER) *names); + /* cert_clear frees and NULLs all X509-related state. */ void (*cert_clear)(CERT *cert); /* cert_flush_cached_chain drops any cached |X509|-based certificate chain diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c index c334ea65..0498cd26 100644 --- a/ssl/ssl_cert.c +++ b/ssl/ssl_cert.c @@ -726,6 +726,12 @@ STACK_OF(CRYPTO_BUFFER) * } } + if (!ssl->ctx->x509_method->check_client_CA_list(ret)) { + *out_alert = SSL_AD_INTERNAL_ERROR; + OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); + goto err; + } + return ret; err: diff --git a/ssl/ssl_x509.c b/ssl/ssl_x509.c index 8a132ffe..81d26428 100644 --- a/ssl/ssl_x509.c +++ b/ssl/ssl_x509.c @@ -331,6 +331,23 @@ static void ssl_crypto_x509_flush_cached_chain(CERT *cert) { cert->x509_chain = NULL; } +static int ssl_crypto_x509_check_client_CA_list( + STACK_OF(CRYPTO_BUFFER) *names) { + for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(names); i++) { + const CRYPTO_BUFFER *buffer = sk_CRYPTO_BUFFER_value(names, i); + const uint8_t *inp = CRYPTO_BUFFER_data(buffer); + X509_NAME *name = d2i_X509_NAME(NULL, &inp, CRYPTO_BUFFER_len(buffer)); + const int ok = name != NULL && inp == CRYPTO_BUFFER_data(buffer) + + CRYPTO_BUFFER_len(buffer); + X509_NAME_free(name); + if (!ok) { + return 0; + } + } + + return 1; +} + static void ssl_crypto_x509_clear(CERT *cert) { ssl_crypto_x509_flush_cached_leaf(cert); ssl_crypto_x509_flush_cached_chain(cert); @@ -427,6 +444,7 @@ static void ssl_crypto_x509_ssl_ctx_flush_cached_client_CA(SSL_CTX *ctx) { } const SSL_X509_METHOD ssl_crypto_x509_method = { + ssl_crypto_x509_check_client_CA_list, ssl_crypto_x509_clear, ssl_crypto_x509_flush_cached_chain, ssl_crypto_x509_flush_cached_leaf, diff --git a/ssl/tls_method.c b/ssl/tls_method.c index d7e4701c..dbb8183b 100644 --- a/ssl/tls_method.c +++ b/ssl/tls_method.c @@ -258,6 +258,11 @@ const SSL_METHOD *TLS_client_method(void) { return TLS_method(); } +static int ssl_noop_x509_check_client_CA_names( + STACK_OF(CRYPTO_BUFFER) *names) { + return 1; +} + static void ssl_noop_x509_clear(CERT *cert) {} static void ssl_noop_x509_flush_cached_leaf(CERT *cert) {} static void ssl_noop_x509_flush_cached_chain(CERT *cert) {} @@ -275,6 +280,7 @@ static void ssl_noop_x509_ssl_flush_cached_client_CA(SSL *ssl) {} static void ssl_noop_x509_ssl_ctx_flush_cached_client_CA(SSL_CTX *ctx) {} const SSL_X509_METHOD ssl_noop_x509_method = { + ssl_noop_x509_check_client_CA_names, ssl_noop_x509_clear, ssl_noop_x509_flush_cached_chain, ssl_noop_x509_flush_cached_leaf,