From 5301c10c53403834570451463b630fdc29989917 Mon Sep 17 00:00:00 2001 From: Matthew Braithwaite Date: Tue, 23 Jan 2018 12:08:55 -0800 Subject: [PATCH] ssl_verify_peer_cert: implement |SSL_VERIFY_NONE| as advertised. Since SSL{,_CTX}_set_custom_verify take a |mode| parameter that may be |SSL_VERIFY_NONE|, it should do what it says on the tin, which is to perform verification and ignore the result. Change-Id: I0d8490111fb199c6b325cc167cf205316ecd4b49 Reviewed-on: https://boringssl-review.googlesource.com/25224 Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org Reviewed-by: David Benjamin --- include/openssl/ssl.h | 4 ++- ssl/handshake.cc | 5 +++ ssl/ssl_test.cc | 77 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 1 deletion(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 247a1457..a44241aa 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -192,7 +192,9 @@ OPENSSL_EXPORT const SSL_METHOD *TLS_method(void); OPENSSL_EXPORT const SSL_METHOD *DTLS_method(void); // TLS_with_buffers_method is like |TLS_method|, but avoids all use of -// crypto/x509. +// crypto/x509. All client connections created with |TLS_with_buffers_method| +// will fail unless a certificate verifier is installed with +// |SSL_set_custom_verify| or |SSL_CTX_set_custom_verify|. OPENSSL_EXPORT const SSL_METHOD *TLS_with_buffers_method(void); // DTLS_with_buffers_method is like |DTLS_method|, but avoids all use of diff --git a/ssl/handshake.cc b/ssl/handshake.cc index cdd1b327..43afc6c4 100644 --- a/ssl/handshake.cc +++ b/ssl/handshake.cc @@ -334,6 +334,11 @@ enum ssl_verify_result_t ssl_verify_peer_cert(SSL_HANDSHAKE *hs) { hs->new_session->verify_result = X509_V_OK; break; case ssl_verify_invalid: + // If |SSL_VERIFY_NONE|, the error is non-fatal, but we keep the result. + if (ssl->verify_mode == SSL_VERIFY_NONE) { + ERR_clear_error(); + ret = ssl_verify_ok; + } hs->new_session->verify_result = X509_V_ERR_APPLICATION_VERIFICATION; break; case ssl_verify_retry: diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index a2a53f63..6c4282e4 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -3141,6 +3141,83 @@ TEST(SSLTest, SetChainAndKey) { server_ctx.get())); } +TEST(SSLTest, BuffersFailWithoutCustomVerify) { + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_with_buffers_method())); + ASSERT_TRUE(client_ctx); + bssl::UniquePtr server_ctx(SSL_CTX_new(TLS_with_buffers_method())); + ASSERT_TRUE(server_ctx); + + bssl::UniquePtr key = GetChainTestKey(); + ASSERT_TRUE(key); + bssl::UniquePtr leaf = GetChainTestCertificateBuffer(); + ASSERT_TRUE(leaf); + std::vector chain = { leaf.get() }; + ASSERT_TRUE(SSL_CTX_set_chain_and_key(server_ctx.get(), &chain[0], + chain.size(), key.get(), nullptr)); + + // Without SSL_CTX_set_custom_verify(), i.e. with everything in the default + // configuration, certificate verification should fail. + bssl::UniquePtr client, server; + ASSERT_FALSE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); + + // Whereas with a verifier, the connection should succeed. + SSL_CTX_set_custom_verify( + client_ctx.get(), SSL_VERIFY_PEER, + [](SSL *ssl, uint8_t *out_alert) -> ssl_verify_result_t { + return ssl_verify_ok; + }); + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); +} + +TEST(SSLTest, CustomVerify) { + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_with_buffers_method())); + ASSERT_TRUE(client_ctx); + bssl::UniquePtr server_ctx(SSL_CTX_new(TLS_with_buffers_method())); + ASSERT_TRUE(server_ctx); + + bssl::UniquePtr key = GetChainTestKey(); + ASSERT_TRUE(key); + bssl::UniquePtr leaf = GetChainTestCertificateBuffer(); + ASSERT_TRUE(leaf); + std::vector chain = { leaf.get() }; + ASSERT_TRUE(SSL_CTX_set_chain_and_key(server_ctx.get(), &chain[0], + chain.size(), key.get(), nullptr)); + + SSL_CTX_set_custom_verify( + client_ctx.get(), SSL_VERIFY_PEER, + [](SSL *ssl, uint8_t *out_alert) -> ssl_verify_result_t { + return ssl_verify_ok; + }); + + bssl::UniquePtr client, server; + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); + + // With SSL_VERIFY_PEER, ssl_verify_invalid should result in a dropped + // connection. + SSL_CTX_set_custom_verify( + client_ctx.get(), SSL_VERIFY_PEER, + [](SSL *ssl, uint8_t *out_alert) -> ssl_verify_result_t { + return ssl_verify_invalid; + }); + + ASSERT_FALSE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); + + // But with SSL_VERIFY_NONE, ssl_verify_invalid should not cause a dropped + // connection. + SSL_CTX_set_custom_verify( + client_ctx.get(), SSL_VERIFY_NONE, + [](SSL *ssl, uint8_t *out_alert) -> ssl_verify_result_t { + return ssl_verify_invalid; + }); + + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); +} + TEST(SSLTest, ClientCABuffers) { bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_with_buffers_method())); ASSERT_TRUE(client_ctx);