diff --git a/src/tls/cert.h b/src/tls/cert.h index 306814f88af..7a811e2be3b 100644 --- a/src/tls/cert.h +++ b/src/tls/cert.h @@ -73,14 +73,8 @@ namespace tls ~Cert() = default; - void configure_ssl(SSL* ssl, SSL_CTX* ssl_ctx) const + void configure_context(SSL_CTX* ssl_ctx) { - if (peer_hostname.has_value()) - { - // Peer hostname for SNI - CHECK1(SSL_set_tlsext_host_name(ssl, peer_hostname->c_str())); - } - if (peer_ca) { peer_ca->configure_trusted_cert_store(ssl_ctx); @@ -94,7 +88,6 @@ namespace tls return ok; }; SSL_CTX_set_verify(ssl_ctx, opts, cb); - SSL_set_verify(ssl, opts, cb); } else { @@ -106,14 +99,21 @@ namespace tls // to verify it here, just request it. auto cb = [](int, x509_store_ctx_st*) { return 1; }; SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, cb); - SSL_set_verify(ssl, SSL_VERIFY_PEER, cb); } if (has_own_cert) { CHECK1( SSL_CTX_use_cert_and_key(ssl_ctx, own_cert, *own_pkey, chain, 1)); - CHECK1(SSL_use_cert_and_key(ssl, own_cert, *own_pkey, chain, 1)); + } + } + + void configure_connection(SSL* ssl) + { + if (peer_hostname.has_value()) + { + // Peer hostname for SNI + CHECK1(SSL_set_tlsext_host_name(ssl, peer_hostname->c_str())); } } }; diff --git a/src/tls/client.h b/src/tls/client.h index 4bbc5b65898..c3e6a2ac718 100644 --- a/src/tls/client.h +++ b/src/tls/client.h @@ -14,7 +14,9 @@ namespace tls public: Client(std::shared_ptr cert_) : Context(true), cert(std::move(cert_)) { - cert->configure_ssl(ssl, cfg); + cert->configure_context(cfg); + create_ssl(); + cert->configure_connection(get_ssl()); } }; } diff --git a/src/tls/context.h b/src/tls/context.h index 1c8a2a61440..a1cf9c9ddaf 100644 --- a/src/tls/context.h +++ b/src/tls/context.h @@ -17,16 +17,40 @@ namespace ccf::tls { protected: ccf::crypto::OpenSSL::Unique_SSL_CTX cfg; - ccf::crypto::OpenSSL::Unique_SSL ssl; + std::unique_ptr ssl; + bool client; + + void create_ssl() + { + ssl = std::make_unique(cfg); + + // Initialise connection + if (client) + { + SSL_set_connect_state(*ssl); + } + else + { + SSL_set_accept_state(*ssl); + } + } + + SSL* get_ssl() + { + // Context construction is split from SSL creation, so catch accidental + // use before create_ssl(). + CHECKNULL(ssl.get()); + CHECKNULL(*ssl); + return *ssl; + } public: - Context(bool client) : - cfg(client ? TLS_client_method() : TLS_server_method()), - ssl(cfg) + Context(bool client_) : + cfg(client_ ? TLS_client_method() : TLS_server_method()), + client(client_) { // Require at least TLS 1.2, support up to 1.3 - SSL_CTX_set_min_proto_version(cfg, TLS1_2_VERSION); - SSL_set_min_proto_version(ssl, TLS1_2_VERSION); + CHECK1(SSL_CTX_set_min_proto_version(cfg, TLS1_2_VERSION)); // Disable renegotiation to avoid DoS SSL_CTX_set_options( @@ -34,11 +58,6 @@ namespace ccf::tls SSL_OP_CIPHER_SERVER_PREFERENCE | SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION | SSL_OP_NO_RENEGOTIATION); - SSL_set_options( - ssl, - SSL_OP_CIPHER_SERVER_PREFERENCE | - SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION | - SSL_OP_NO_RENEGOTIATION); // Set cipher for TLS 1.2 const auto* const cipher_list = @@ -46,38 +65,22 @@ namespace ccf::tls "ECDHE-ECDSA-AES128-GCM-SHA256:" "ECDHE-RSA-AES256-GCM-SHA384:" "ECDHE-RSA-AES128-GCM-SHA256"; - SSL_CTX_set_cipher_list(cfg, cipher_list); - SSL_set_cipher_list(ssl, cipher_list); + CHECK1(SSL_CTX_set_cipher_list(cfg, cipher_list)); // Set cipher for TLS 1.3 const auto* const ciphersuites = "TLS_AES_256_GCM_SHA384:" "TLS_AES_128_GCM_SHA256"; - SSL_CTX_set_ciphersuites(cfg, ciphersuites); - SSL_set_ciphersuites(ssl, ciphersuites); + CHECK1(SSL_CTX_set_ciphersuites(cfg, ciphersuites)); // Restrict the curves to approved ones - SSL_CTX_set1_curves_list(cfg, "P-521:P-384:P-256"); - SSL_set1_curves_list(ssl, "P-521:P-384:P-256"); + CHECK1(SSL_CTX_set1_curves_list(cfg, "P-521:P-384:P-256")); // Allow buffer to be relocated between WANT_WRITE retries, and do partial // writes if possible SSL_CTX_set_mode( cfg, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER | SSL_MODE_ENABLE_PARTIAL_WRITE); - SSL_set_mode( - ssl, - SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER | SSL_MODE_ENABLE_PARTIAL_WRITE); - - // Initialise connection - if (client) - { - SSL_set_connect_state(ssl); - } - else - { - SSL_set_accept_state(ssl); - } } virtual ~Context() = default; @@ -86,27 +89,33 @@ namespace ccf::tls void* cb_obj, BIO_callback_fn_ex send, BIO_callback_fn_ex recv) { // Read/Write BIOs will be used by TLS - BIO* rbio = BIO_new(BIO_s_mem()); - BIO_set_mem_eof_return(rbio, -1); - BIO_set_callback_arg(rbio, static_cast(cb_obj)); - BIO_set_callback_ex(rbio, recv); - SSL_set0_rbio(ssl, rbio); - - BIO* wbio = BIO_new(BIO_s_mem()); - BIO_set_mem_eof_return(wbio, -1); - BIO_set_callback_arg(wbio, static_cast(cb_obj)); - BIO_set_callback_ex(wbio, send); - SSL_set0_wbio(ssl, wbio); + std::unique_ptr rbio( + BIO_new(BIO_s_mem()), BIO_free); + CHECKNULL(rbio.get()); + + std::unique_ptr wbio( + BIO_new(BIO_s_mem()), BIO_free); + CHECKNULL(wbio.get()); + + BIO_set_mem_eof_return(rbio.get(), -1); + BIO_set_callback_arg(rbio.get(), static_cast(cb_obj)); + BIO_set_callback_ex(rbio.get(), recv); + SSL_set0_rbio(get_ssl(), rbio.release()); + + BIO_set_mem_eof_return(wbio.get(), -1); + BIO_set_callback_arg(wbio.get(), static_cast(cb_obj)); + BIO_set_callback_ex(wbio.get(), send); + SSL_set0_wbio(get_ssl(), wbio.release()); } virtual int handshake() { - if (SSL_is_init_finished(ssl) != 0) + if (SSL_is_init_finished(get_ssl()) != 0) { return 0; } - int rc = SSL_do_handshake(ssl); + int rc = SSL_do_handshake(get_ssl()); // Success in OpenSSL is 1, MBed is 0 if (rc > 0) { @@ -115,12 +124,12 @@ namespace ccf::tls } // Want read/write needs special return - if (SSL_want_read(ssl)) + if (SSL_want_read(get_ssl())) { return TLS_ERR_WANT_READ; } - if (SSL_want_write(ssl)) + if (SSL_want_write(get_ssl())) { return TLS_ERR_WANT_WRITE; } @@ -135,7 +144,7 @@ namespace ccf::tls LOG_TRACE_FMT("Context::handshake() : Error code {}", rc); // As an MBedTLS emulation, we return negative for errors. - return -SSL_get_error(ssl, rc); + return -SSL_get_error(get_ssl(), rc); } virtual int read(uint8_t* buf, size_t len) @@ -145,12 +154,12 @@ namespace ccf::tls return 0; } size_t readbytes = 0; - int rc = SSL_read_ex(ssl, buf, len, &readbytes); + int rc = SSL_read_ex(get_ssl(), buf, len, &readbytes); if (rc > 0) { return readbytes; } - if (SSL_want_read(ssl)) + if (SSL_want_read(get_ssl())) { return TLS_ERR_WANT_READ; } @@ -159,7 +168,7 @@ namespace ccf::tls LOG_TRACE_FMT("Context::read() : Error code {}", rc); // As an MBedTLS emulation, we return negative for errors. - return -SSL_get_error(ssl, rc); + return -SSL_get_error(get_ssl(), rc); } virtual int write(const uint8_t* buf, size_t len) @@ -169,12 +178,12 @@ namespace ccf::tls return 0; } size_t written = 0; - int rc = SSL_write_ex(ssl, buf, len, &written); + int rc = SSL_write_ex(get_ssl(), buf, len, &written); if (rc > 0) { return written; } - if (SSL_want_write(ssl)) + if (SSL_want_write(get_ssl())) { return TLS_ERR_WANT_WRITE; } @@ -183,23 +192,23 @@ namespace ccf::tls LOG_TRACE_FMT("Context::write() : Error code {}", rc); // As an MBedTLS emulation, we return negative for errors. - return -SSL_get_error(ssl, rc); + return -SSL_get_error(get_ssl(), rc); } virtual int close() { LOG_TRACE_FMT("Context::close() : Shutdown"); - return SSL_shutdown(ssl); + return SSL_shutdown(get_ssl()); } virtual bool peer_cert_ok() { - return SSL_get_verify_result(ssl) == X509_V_OK; + return SSL_get_verify_result(get_ssl()) == X509_V_OK; } virtual std::string get_verify_error() { - return X509_verify_cert_error_string(SSL_get_verify_result(ssl)); + return X509_verify_cert_error_string(SSL_get_verify_result(get_ssl())); } virtual std::string host() @@ -214,7 +223,7 @@ namespace ccf::tls // SSL_get_peer_certificate just to extract it from the context. ccf::crypto::OpenSSL::Unique_X509 cert( - SSL_get_peer_certificate(ssl), /*check_null=*/false); + SSL_get_peer_certificate(get_ssl()), /*check_null=*/false); if (cert == nullptr) { LOG_TRACE_FMT("Empty peer cert"); diff --git a/src/tls/server.h b/src/tls/server.h index 3da1d877f2e..eb777603538 100644 --- a/src/tls/server.h +++ b/src/tls/server.h @@ -47,7 +47,7 @@ namespace tls Context(false), cert(cert_) { - cert->configure_ssl(ssl, cfg); + cert->configure_context(cfg); // Configure protocols negotiated by ALPN // See https://nghttp2.org/documentation/tutorial-server.html and use of @@ -67,6 +67,8 @@ namespace tls alpn_protos_data, sizeof(alpn_protos_data)}; SSL_CTX_set_alpn_select_cb(cfg, alpn_select_cb, &alpn_protos); } + + create_ssl(); } }; } diff --git a/src/tls/test/main.cpp b/src/tls/test/main.cpp index 2791ef16775..18a5bc5a89c 100644 --- a/src/tls/test/main.cpp +++ b/src/tls/test/main.cpp @@ -305,9 +305,10 @@ TEST_CASE("Cert configures TLS verification and own certificate") auto ca = get_ca(); auto cert = get_dummy_cert(ca, "server"); ccf::crypto::OpenSSL::Unique_SSL_CTX ctx(TLS_method()); - ccf::crypto::OpenSSL::Unique_SSL ssl(ctx); - cert->configure_ssl(ssl, ctx); + cert->configure_context(ctx); + ccf::crypto::OpenSSL::Unique_SSL ssl(ctx); + cert->configure_connection(ssl); constexpr auto expected_verify_mode = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT; @@ -466,6 +467,38 @@ void run_test_case( server.close(); } +class InspectableClient : public tls::Client +{ +public: + using tls::Client::Client; + + int verify_mode() + { + auto* ssl = get_ssl(); + REQUIRE(ssl != nullptr); + return SSL_get_verify_mode(ssl); + } +}; + +TEST_CASE("connection inherits verification mode from context") +{ + auto ca = get_ca(); + + InspectableClient verified_client(get_dummy_cert(ca, "verified")); + REQUIRE( + (verified_client.verify_mode() & + (SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) == + (SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT)); + + InspectableClient request_only_client( + get_dummy_cert(ca, "request_only", false)); + // auth_required=false still requests a peer certificate, but does not fail + // the handshake if the peer certificate is missing. + REQUIRE((request_only_client.verify_mode() & SSL_VERIFY_PEER) != 0); + REQUIRE( + (request_only_client.verify_mode() & SSL_VERIFY_FAIL_IF_NO_PEER_CERT) == 0); +} + TEST_CASE("unverified handshake") { // Create a CA