diff --git a/doc/architecture/tls_internals.rst b/doc/architecture/tls_internals.rst index 25dd6bd0685..114308c00fa 100644 --- a/doc/architecture/tls_internals.rst +++ b/doc/architecture/tls_internals.rst @@ -4,18 +4,16 @@ TLS Internals Overview ~~~~~~~~ -In CCF, the :term:`TLS` layer is implemented using OpenSSL 3.3. However, the original implementation was using OE's MbedTLS library, which the current implementation replaced. During the transition period, the OpenSSL implementation had to emulate the previous MbedTLS one and the remaining code isn't particularly suited to OpenSSL. - -This document is an attempt to describe how that works, to faciliate further changes. +In CCF, the :term:`TLS` layer is implemented using OpenSSL 3.3. Enclave Connections ~~~~~~~~~~~~~~~~~~~ CCF handles RPC requests by managing a collection of HTTPS sessions on each node. The HTTP session receives raw TCP bytes from the :term:`ring buffer` and passes it to the TLS implementation that tries to decrypt on read (after a successful handshake). -If there isn't enough information to decrypt on read, the TLS layer responds with a 'WANTS_READ' message, meaning more packets are needed to complete the message, so the endpoint tried to get more data from the ring buffer. +If there isn't enough information to decrypt on read, the TLS layer responds with a 'WANT_READ' status, meaning more packets are needed to complete the message, so the endpoint tries to get more data from the ring buffer. -Once all incoming data is decrypted, the HTTP session parses the plain text data and passes the parsed request to the application, which process it and produces a plain text response. The session then serialises this response, encrypts it through the TLS layer, and writes the encrypted data to the :term:`ring buffer` to be sent to the original caller. +Once all incoming data is decrypted, the HTTP session parses the plain text data and passes the parsed request to the application, which processes it and produces a plain text response. The session then serialises this response, encrypts it through the TLS layer, and writes the encrypted data to the :term:`ring buffer` to be sent to the original caller. TLS Implementation ~~~~~~~~~~~~~~~~~~ @@ -28,31 +26,42 @@ The TLS implementation in 'src/tls' has three main components: Both CA and Cert have internal logic to validate their certificates and private keys, and the Context has logic to complete a TLS handshake, read and write using encryption and to query the peer's certificate. -The next layer up is the 'TLSSession' (of which 'HTTPSession' owns an instance) that holds the ring buffer, the pending read and write buffers, and the call backs for sending and receiving data. +The next layer up is the 'TLSSession' (of which 'HTTPSession' owns an instance) that holds the ring buffer, the pending read and write buffers, and drives the I/O between the ring buffer and the Context. -Receiving Messages -~~~~~~~~~~~~~~~~~~ +BIOs +~~~~ + +The 'Context' uses a pair of in-memory ``BIO`` objects to exchange encrypted bytes with the peer. The TLS layer reads ciphertext from the 'read' BIO and writes ciphertext to the 'write' BIO. -When a REST node message is received through the ring buffer, the 'HTTPSession' ``recv_()`` method is called (as it was registered as a callback). That calls ``TLSSession::read()`` which in turn calls ``tls::Context::read()``. +The 'TLSSession' drives this I/O directly, without callbacks: -However, when creating the TLS 'Context', the 'TLSSession' had to register some callbacks of its own, too. This is one of the parts that was specific to MbedTLS and that was (unnaturally) replicated to OpenSSL. +- On receive, it writes the bytes that arrived from the ring buffer into the read BIO (``Context::recv``) before asking the TLS layer to decrypt them. +- After every TLS operation, it drains any ciphertext the TLS layer produced from the write BIO (``Context::send``) and forwards it to the ring buffer (via ``RINGBUFFER_TRY_WRITE_MESSAGE(tcp_outbound, ...)``). If the ring buffer is full, the bytes are retained and retried on the next operation. -Those callbacks are implemented in 'TLSSession' because they need access to the ``pending_read`` buffers to read from and the 'ring buffer' to write to. But the TLS context ('SSL' and 'SSL_CTX' objects) need to encrypt/decrypt data, and they can only do that from 'BIO's in OpenSSL, so the callback has a mix of ring buffer and BIO handling that tricks both sides to take the appropriate steps at the right time in this complex dance. +Receiving Messages +~~~~~~~~~~~~~~~~~~ -In the 'read' case, the message has arrived into the ``pending_read`` buffer via a (previously triggered) ring buffer callback, and that data is written to TLS's 'read' BIO (via ``BIO_write_ex``) `before` TLS itself reads it. That data is still encrypted, so when the TLS reads it with ``SSL_read_ex``, it tries to decrypt and on success, returns a plain text buffer. On error, it emits the error or a 'WANTS_READ' status, so the endpoint can try to extract more information from the ``pending_read`` buffer again. +When a node message is received through the ring buffer, the 'HTTPSession' ``recv_()`` method is called (as it was registered as a callback). That hands the bytes to ``TLSSession::recv_buffered()``, which writes them into the Context's read BIO, and then to ``TLSSession::read()`` which calls ``ccf::tls::Context::read()``. -Upon success, the read BIO is flushed and new data can be read again. The plain text result is returned all the way back to ``HTTPSession::recv_()`` that then sends it to the application to process. +``Context::read()`` calls ``SSL_read_ex``, which reads the (still encrypted) bytes from the read BIO and tries to decrypt them. On success it returns the plain text and a status of ``0``. If there isn't enough data, it returns ``SSL_ERROR_WANT_READ`` so the endpoint can try to extract more information from the ring buffer. + +The plain text result is returned all the way back to ``HTTPSession::recv_()`` that then sends it to the application to process. Sending Messages ~~~~~~~~~~~~~~~~ -When the application finishes, it returns a plain text response. That message is sent back to the client via the ``send()`` call. This in turn calls ``TLSSession::send_raw()`` that via a series of asynchronous dispatches ends up filling the ``pending_write`` buffer and calling ``write_some()`` which itself calls ``tls::Context::write()``. +When the application finishes, it returns a plain text response. That message is sent back to the client via the ``send()`` call, which ends up filling the ``pending_write`` buffer and calling ``flush()`` which itself calls ``ccf::tls::Context::write()``. + +``Context::write()`` calls ``SSL_write_ex`` which encrypts the data into the write BIO. The 'TLSSession' then drains the write BIO and sends the encrypted bytes through the ring buffer. -This is the same process as for reads: a callback in 'TLSSession' was registered for the 'write' BIO, with access to the ``pending_write`` and the ring buffer. But in this case, the callback is only executed `after` the TLS layer has encrypted the data and written to the 'write' BIO. +Different errors are treated at different levels (TLS status codes in Context, ring buffer errors in TLSSession and HTTP errors in HTTPSession). -Now, we take that (encrypted) data, flush the BIO ourselves (to avoid clogging the pipes with the next message) and send it through the ring buffer with the ``RINGBUFFER_TRY_WRITE_MESSAGE(tls_outbound,...)`` macro. +Reads and Writes +~~~~~~~~~~~~~~~~ + +``Context::handshake``, ``Context::read`` and ``Context::write`` return ``0`` on success and an OpenSSL ``SSL_ERROR_*`` status code otherwise (obtained from ``SSL_get_error``). The number of bytes read or written is returned separately through an output parameter, so the return value is never overloaded to mean both a byte count and an error. -This is what actually sends the message back to the client, so when this callback returns, the remaining stack just returns the status of that write. Different errors are treated at different levels (TLS errors in Context, ring buffer errors in TLSSession and HTTP errors in HTTPSession). +The caller (``TLSSession``) inspects the status code to decide whether to wait for more data (``SSL_ERROR_WANT_READ`` / ``SSL_ERROR_WANT_WRITE``), close the connection (``SSL_ERROR_ZERO_RETURN``), or treat it as an error. A certificate verification failure during the handshake is surfaced as a distinct status so it can be treated as an authentication failure. Why OpenSSL? ~~~~~~~~~~~~ @@ -62,48 +71,3 @@ The main reasons why we moved to OpenSSL are: - We already use OpenSSL for our crypto library ('src/crypto'). - We wanted TLS 1.3 support and MbedTLS doesn't have it. - We wanted to support QUIC, which doesn't work with MbedTLS. - -By now we have removed any traces of MbedTLS, but we are still using a version of OpenSSL which doesn't have QUIC support. - -MbedTLS vs OpenSSL -~~~~~~~~~~~~~~~~~~ - -As stated above, the current OpenSSL implementation is `emulating` the previous MbedTLS one, so some oddities are observed. - -First, MbedTLS returns errors as negative values and amount of data handled as positive values. OpenSSL concurs on positive values but returns 0 (or -1 in previous versions) for all errors, using ``SSL_get_error`` to then classify which error and what to do. The error values are also positive. - -To simulate this, we implement the error handling at each invocation and, on error, we negate the value of the error so that we can retain the old behavior of checking for negative values. - -Second, MbedTLS keeps all its context (configuration, connection info, read and write buffers) in a single large structure, while OpenSSL has separate structures for each and uses 'BIO' objects for buffers. Reads and writes in MbedTLS is done exclusively via callbacks. - -OpenSSL callbacks, however, are very different from MbedTLS ones. They are called twice for each action, one before the actual action and another after. - -To simulate this we had to implement a read callback `before` the BIO read (so we could fill it up with the contents of the ring buffer) and the write callback `after` the BIO write (so we could pick up its contents and send it into the ring buffer). - -There is a complex dance of return values in OpenSSL's callbacks. If any returns errors the action is canceled immediately. On reads, because the BIO was empty, the initial return value is an error, so we must make sure that, if there is anything in the ``pending_read`` buffer, we have to change the status to the amount of bytes read, so it can continue. - -Third, the handshake in MbedTLS had various types of errors, which we had to emulate by making the appropriate ``SSL_*`` calls, check the peer certificate, etc. to get the same types of responses for the same situations. - -Finally, in MbedTLS, the configuration and session objects were setup at the same time, while in OpenSSL they're separate. We ended up duplicating every single configuration, but this is unnecessary, because once the config object is correct, any session object created from it has the same properties. - -But the TLS Context doesn't handle more than one session per configuration, so we could set either of them once and ignore the other. The simplest thing would be to setup just the session, but if we end up having more than one session later, we'd have to refactor that. - -Pure OpenSSL Implementation -~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -With MbedTLS gone from the code base, we can now think of a pure OpenSSL implementation. - -The considerations are: - -- We don't need to handle errors inside the calls to read/write, but can leave for each caller to handle IFF there is an error by calling ``SSL_get_error``. This also means we don't need to negate error values, as they're in different domains. -- We can simplify the SSL configuration on startup, handshake handling and peer certificate handling. - -However, getting rid of the callbacks and using BIOs directly is going to be hard. - -First, the current callback is in 'TLSSession' because it has access to both pending buffers and the ring buffer. The TLS Context does not have access to it nor it would be wise to pass references to it, as that'd make the Context exclusive to the TLSSession. - -Second, both endpoint and TLS have a need to read and write asynchronously. Data arrives from the ring buffer at any time and the TLS implementation can request reads and writes (for example, during handshake) that the endpoint didn't request itself. - -So if ``SSL_handshake``, ``SSL_read_ex`` and ``SSL_write_ex`` don't have direct access to read and write from the ring buffers without direct requests from the endpoints, it won't be able to conclude the asynchronous handshake and start the connection. - -One possible way out of it is to create a `BIO pair `_ for each read/write action between the 'TLSSession' and the TLS 'Context', driven by two asynchronous tasks in 'TLSSession' that just poll the BIOs and buffers and pass data across. This removes a callback, but introduces polling, which is not an actual improvement. diff --git a/src/enclave/tls_session.h b/src/enclave/tls_session.h index ee5ca7121c6..82409662857 100644 --- a/src/enclave/tls_session.h +++ b/src/enclave/tls_session.h @@ -10,6 +10,8 @@ #include "tls/tls.h" #include +#include +#include namespace ccf { @@ -34,7 +36,9 @@ namespace ccf private: std::vector pending_write; - std::vector pending_read; + // Encrypted bytes produced by the TLS layer that are waiting to be written + // to the ring buffer (e.g. when the ring buffer was previously full). + std::vector pending_out; // Decrypted data std::vector read_buffer; @@ -64,7 +68,7 @@ namespace ccf session_id(session_id_), ctx(std::move(ctx_)) { - ctx->set_bio(this, send_callback_openssl, recv_callback_openssl); + ctx->set_bio(); } virtual ~TLSSession() @@ -157,16 +161,28 @@ namespace ccf // NB: If we continue past here, read_buffer is empty } - auto r = ctx->read(data + offset, size - offset); - LOG_TRACE_FMT("ctx->read returned: {}", r); + size_t readbytes = 0; + auto rc = ctx->read(data + offset, size - offset, readbytes); + LOG_TRACE_FMT("ctx->read returned: {} ({} bytes)", rc, readbytes); - switch (r) + // A read can cause the TLS layer to produce outbound bytes which must be + // sent to the peer for it to make progress (e.g. TLS 1.3 session tickets + // or key updates emitted after the handshake, or a SSL_ERROR_WANT_WRITE + // where the peer is waiting on data we have buffered). Drain them now, + // otherwise they are stranded in the write BIO and the connection stalls. + flush_outbound(); + + switch (rc) { case 0: - case TLS_ERR_CONN_CLOSE_NOTIFY: { - LOG_TRACE_FMT( - "TLS {} close on read: {}", session_id, ::tls::error_string(r)); + // Success, fall through to handle the bytes read below. + break; + } + + case SSL_ERROR_ZERO_RETURN: + { + LOG_TRACE_FMT("TLS {} close on read", session_id); stop(closed); @@ -180,8 +196,8 @@ namespace ccf return 0; } - case TLS_ERR_WANT_READ: - case TLS_ERR_WANT_WRITE: + case SSL_ERROR_WANT_READ: + case SSL_ERROR_WANT_WRITE: { if (!exact) { @@ -196,21 +212,19 @@ namespace ccf default: { + LOG_TRACE_FMT( + "TLS {} error on read: {}", + session_id, + ::tls::error_string(ERR_get_error())); + stop(error); + return 0; } } - if (r < 0) - { - LOG_TRACE_FMT( - "TLS {} error on read: {}", session_id, ::tls::error_string(r)); - stop(error); - return 0; - } - - auto total = r + offset; + auto total = readbytes + offset; // We read _some_ data but not enough, and didn't get - // TLS_ERR_WANT_READ. Probably hit an internal size limit - try + // SSL_ERROR_WANT_READ. Probably hit an internal size limit - try // again if (exact && (total < size)) { @@ -227,7 +241,7 @@ namespace ccf { if (can_recv()) { - pending_read.insert(pending_read.end(), data, data + size); + ctx->recv(data, size); } do_handshake(); @@ -249,19 +263,20 @@ namespace ccf case ready: case closing: { - int r = ctx->close(); + int rc = ctx->close(); + flush_outbound(); - switch (r) + switch (rc) { - case TLS_ERR_WANT_READ: - case TLS_ERR_WANT_WRITE: + case SSL_ERROR_WANT_READ: + case SSL_ERROR_WANT_WRITE: { - LOG_TRACE_FMT("TLS {} has pending data ({})", session_id, r); + LOG_TRACE_FMT("TLS {} has pending data ({})", session_id, rc); // FALLTHROUGH } case 0: { - LOG_TRACE_FMT("TLS {} closed ({})", session_id, r); + LOG_TRACE_FMT("TLS {} closed ({})", session_id, rc); stop(closed); break; } @@ -271,7 +286,7 @@ namespace ccf LOG_TRACE_FMT( "TLS {} error on_close: {}", session_id, - ::tls::error_string(r)); + ::tls::error_string(ERR_get_error())); stop(error); break; } @@ -317,6 +332,43 @@ namespace ccf pending_write.insert(pending_write.end(), data.begin(), data.end()); } + // Drains the encrypted bytes produced by the TLS layer and writes them to + // the ring buffer. Returns false if there are bytes left to send because + // the ring buffer was full (in which case they are retained and retried on + // the next call). + bool flush_outbound() + { + size_t pending = ctx->pending_write(); + if (pending > 0) + { + size_t cur = pending_out.size(); + pending_out.resize(cur + pending); + // If fewer than pending bytes are drained, the remainder stays in the + // BIO and is picked up by the next pending_write()/send() below. + size_t got = ctx->send(pending_out.data() + cur, pending); + pending_out.resize(cur + got); + } + + if (pending_out.empty()) + { + return true; + } + + auto wrote = RINGBUFFER_TRY_WRITE_MESSAGE( + ::tcp::tcp_outbound, + to_host, + session_id, + serializer::ByteRange{pending_out.data(), pending_out.size()}); + + if (wrote) + { + pending_out.clear(); + return true; + } + + return false; + } + void flush() { do_handshake(); @@ -326,23 +378,48 @@ namespace ccf return; } + // Retry any encrypted bytes that couldn't be sent previously. + if (!flush_outbound()) + { + return; + } + while (!pending_write.empty()) { - auto r = write_some(pending_write); + size_t written = 0; + auto rc = + ctx->write(pending_write.data(), pending_write.size(), written); - if (r > 0) + if (written > 0) { - pending_write.erase(pending_write.begin(), pending_write.begin() + r); + pending_write.erase( + pending_write.begin(), pending_write.begin() + written); } - else if (r == 0) + + bool flushed = flush_outbound(); + + switch (rc) { - break; + case 0: + break; + + case SSL_ERROR_WANT_READ: + case SSL_ERROR_WANT_WRITE: + return; + + default: + LOG_TRACE_FMT( + "TLS session {} error on flush: {}", + session_id, + ::tls::error_string(ERR_get_error())); + stop(error); + return; } - else + + if (!flushed) { - LOG_TRACE_FMT("TLS session {} error on flush: {}", session_id, -r); - stop(error); - break; + // Ring buffer is full - retry later. + return; } } } @@ -358,6 +435,11 @@ namespace ccf auto rc = ctx->handshake(); + // Push out any handshake records the TLS layer produced, regardless of + // the result (e.g. a ClientHello while waiting for more data, or an alert + // on failure). + flush_outbound(); + switch (rc) { case 0: @@ -366,26 +448,21 @@ namespace ccf break; } - case TLS_ERR_WANT_READ: - case TLS_ERR_WANT_WRITE: + case SSL_ERROR_WANT_READ: + case SSL_ERROR_WANT_WRITE: break; - case TLS_ERR_NEED_CERT: + case SSL_ERROR_WANT_X509_LOOKUP: { - on_handshake_error(fmt::format( - "TLS {} verify error on handshake: {}", - session_id, - ::tls::error_string(rc))); + on_handshake_error( + fmt::format("TLS {} verify error on handshake", session_id)); stop(authfail); break; } - case TLS_ERR_CONN_CLOSE_NOTIFY: + case SSL_ERROR_ZERO_RETURN: { - LOG_TRACE_FMT( - "TLS {} closed on handshake: {}", - session_id, - ::tls::error_string(rc)); + LOG_TRACE_FMT("TLS {} closed on handshake", session_id); stop(closed); break; } @@ -394,10 +471,7 @@ namespace ccf { auto err = ctx->get_verify_error(); on_handshake_error(fmt::format( - "TLS {} invalid cert on handshake: {} [{}]", - session_id, - err, - ::tls::error_string(rc))); + "TLS {} invalid cert on handshake: {}", session_id, err)); stop(authfail); return; } @@ -407,28 +481,13 @@ namespace ccf on_handshake_error(fmt::format( "TLS {} error on handshake: {}", session_id, - ::tls::error_string(rc))); + ::tls::error_string(ERR_get_error()))); stop(error); break; } } } - int write_some(const std::vector& data) - { - auto r = ctx->write(data.data(), data.size()); - - switch (r) - { - case TLS_ERR_WANT_READ: - case TLS_ERR_WANT_WRITE: - return 0; - - default: - return r; - } - } - void stop(SessionStatus status_) { switch (status) @@ -486,186 +545,5 @@ namespace ccf fmt::format("TLS {} unknown status: {}", session_id, status)); } } - - int handle_send(const uint8_t* buf, size_t len) - { - // Either write all of the data or none of it. - auto wrote = RINGBUFFER_TRY_WRITE_MESSAGE( - ::tcp::tcp_outbound, - to_host, - session_id, - serializer::ByteRange{buf, len}); - - if (!wrote) - { - return TLS_WRITING; - } - - return static_cast(len); - } - - int handle_recv(uint8_t* buf, size_t len) - { - if (!pending_read.empty()) - { - // Use the pending data vector. This is populated when the host - // writes a chunk larger than the size requested by the enclave. - size_t rd = std::min(len, pending_read.size()); - ::memcpy(buf, pending_read.data(), rd); - - if (rd >= pending_read.size()) - { - pending_read.clear(); - } - else - { - pending_read.erase(pending_read.begin(), pending_read.begin() + rd); - } - - return (int)rd; - } - - return TLS_READING; - } - - static int send_callback(void* ctx, const unsigned char* buf, size_t len) - { - return reinterpret_cast(ctx)->handle_send(buf, len); - } - - static int recv_callback(void* ctx, unsigned char* buf, size_t len) - { - return reinterpret_cast(ctx)->handle_recv(buf, len); - } - - // These callbacks below are complex, using the callbacks above and - // manipulating OpenSSL's BIO objects accordingly. This is just so we can - // emulate what MbedTLS used to do. - // Now that we have removed it from the code, we can move the callbacks - // above to handle BIOs directly and hopefully remove the complexity below. - // This work will be carried out in #3429. - static long send_callback_openssl( - BIO* b, - int oper, - const char* argp, - size_t len, - int argi, - long argl, - int ret, - size_t* processed) - { - // Unused arguments - (void)argi; - (void)argl; - (void)argp; - - if (ret != 0 && len > 0 && oper == (BIO_CB_WRITE | BIO_CB_RETURN)) - { - // Flush BIO so the "pipe doesn't clog", but we don't use the - // data here, because 'argp' already has it. - BIO_flush(b); - size_t pending = BIO_pending(b); - if (pending != 0) - { - BIO_reset(b); - } - - // Pipe object - void* ctx = BIO_get_callback_arg(b); - int put = - send_callback(ctx, reinterpret_cast(argp), len); - - // WANTS_WRITE - if (put == TLS_WRITING) - { - BIO_set_retry_write(b); - LOG_TRACE_FMT("TLS Session::send_cb() : WANTS_WRITE"); - *processed = 0; - return -1; - } - - LOG_TRACE_FMT("TLS Session::send_cb() : Put {} bytes", put); - - // Update the number of bytes to external users - *processed = put; - } - - // Unless we detected an error, the return value is always the same as the - // original operation. - return ret; - } - - static long recv_callback_openssl( - BIO* b, - int oper, - const char* argp, - size_t len, - int argi, - long argl, - int ret, - size_t* processed) - { - // Unused arguments - (void)argi; - (void)argl; - - if (ret == 1 && oper == (BIO_CB_CTRL | BIO_CB_RETURN)) - { - // This callback may be fired at the end of large batches of TLS frames - // on OpenSSL 3.x. Note that processed == nullptr in this case, hence - // the early exit. - return 0; - } - - if (ret != 0 && (oper == (BIO_CB_READ | BIO_CB_RETURN))) - { - // Pipe object - void* ctx = BIO_get_callback_arg(b); - // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) - int got = recv_callback( - ctx, reinterpret_cast(const_cast(argp)), len); - - // WANTS_READ - if (got == TLS_READING) - { - BIO_set_retry_read(b); - LOG_TRACE_FMT("TLS Session::recv_cb() : WANTS_READ"); - *processed = 0; - return -1; - } - - LOG_TRACE_FMT("TLS Session::recv_cb() : Got {} bytes of {}", got, len); - - // If got less than requested, return WANT_READ - if ((size_t)got < len) - { - *processed = got; - return 1; - } - - // Write to the actual BIO so SSL can use it - BIO_write_ex(b, argp, got, processed); - - // The buffer should be enough, we can't return WANT_WRITE here - if ((size_t)got != *processed) - { - LOG_TRACE_FMT("TLS Session::recv_cb() : BIO error"); - *processed = got; - return -1; - } - - // If original return was -1 because it didn't find anything to read, - // return 1 to say we actually read something. This is common when the - // buffer is empty and needs an external read, so let's not log this. - if (got > 0 && ret < 0) - { - return 1; - } - } - - // Unless we detected an error, the return value is always the same as the - // original operation. - return ret; - } }; } diff --git a/src/tls/README.md b/src/tls/README.md index 0585b053508..fddd2ebc185 100644 --- a/src/tls/README.md +++ b/src/tls/README.md @@ -1,67 +1,45 @@ # OpenSSL TLS Implementation -This is a TLS implementation using OpenSSL that mimics the existing MbedTLS -one in a similar fashion. Because of that, some structures and call backs -look odd and have some work-arounds to make it fit the current workflow. +This is a TLS implementation using OpenSSL. -Once we completely deprecate the MbedTLS implementation from CCF, we should -re-write the TLS implementation to fit the OpenSSL coding flow, which would -make it much simpler and easier to use. +It used to emulate the previous MbedTLS implementation, but now that MbedTLS has +been removed it follows OpenSSL's native style. ## CAs and Certificates -In the MbedTLS world, certificates can be null and have methods to change -some configurations in the TLS config/session objects. There isn't a lot of -cross-over, so updating the config does the trick. - -However, in OpenSSL, session objects (ssl) are created from config objects -(cfg) and inherit all its properties. Therefore, to emulate MbedTLS, we need -to do to the session object every action we do to the config object, which is -not only redundant, but could be unsafe, if the calls are slightly different. +In OpenSSL, session objects (`ssl`) are created from config objects (`cfg`) and +inherit all their properties. Because `Context` only handles a single session +per configuration, configuration is applied to both objects so that either can +be used safely. ### Validation -Certificate validation can be complex to handle if you can accept connections -with certificates or not, and if they come, when and how to validate. - -MbedTLS is a lot more lenient on checks. For example, CAs are not tested for -validity of actually signing other certificates, while OpenSSL has extensive -checks, which can fail functionality that was previously passing. - -For this reason, a number of extra checks in the OpenSSL side were disabled. -Once we get rid of MbedTLS we should revisit those checks again and improve -CCF's usage of TLS, and perhaps also creating weaker checks for non-CA -certificates, etc. +OpenSSL performs extensive certificate validation. The verification result is +queried via `SSL_get_verify_result`, and a verification failure during the +handshake is surfaced to the caller as a distinct status so it can be treated as +an authentication failure rather than a generic error. ## Context ### BIOs -MbedTLS operates reads and writes solely via callbacks, with a buffer in the -session object acting as async I/O. This is in stark contrast with OpenSSL -which uses BIO objects to pass information back and forth, and only have -callbacks for debug or very specialized cases. +The `Context` uses a pair of in-memory BIOs to exchange encrypted bytes with the +peer: the TLS layer reads ciphertext from the read BIO and writes ciphertext to +the write BIO. -We had to implement callbacks and specialize our case, but it could really be -just done with BIOs between the ring buffer and the context, but we'd have to -change a lot of code outside of the TLS implementation to add that. +`TLSSession` drives the I/O directly: it feeds bytes received from the ring +buffer into the read BIO (`Context::recv`) and drains the bytes the TLS layer +wants to send out of the write BIO (`Context::send`), forwarding them to the +ring buffer. There are no BIO callbacks. ### Reads and Writes -Reading and writing in MbedTLS returns a positive value for success (number of -bytes written) or a negative value for error (pre-defined error codes) including -WANTS_READ and WANTS_WRITE. - -In OpenSSL, those methods return 1 for success and 0 or -1 for errors (depending -on the version), with all errors, including WANTS_READ and WANTS_WRITE -accessible through `SSL_get_error`. This imposes a number of hacks needed to -mimic the MbedTLS implementation, including: - -- Multiple `#define`s with common error messages in `tls.h` -- Having to negate the error code to match -- Multiple checks to `SSL_want_read` and `SSL_want_write` - -### Error Handling +`Context::handshake`, `Context::read` and `Context::write` return `0` on success +and an OpenSSL `SSL_ERROR_*` status code otherwise (obtained from +`SSL_get_error`). The number of bytes read or written is returned separately +through an output parameter, so the return value is never overloaded to mean +both a byte count and an error. -As discussed above, the error handling is slightly different and promotes -verbose code in OpenSSL's side. +The caller (`TLSSession`) inspects the status code to decide whether to wait for +more data (`SSL_ERROR_WANT_READ` / `SSL_ERROR_WANT_WRITE`), close the connection +(`SSL_ERROR_ZERO_RETURN`), or treat it as an error. diff --git a/src/tls/context.h b/src/tls/context.h index 1c8a2a61440..3dc7548769d 100644 --- a/src/tls/context.h +++ b/src/tls/context.h @@ -82,23 +82,54 @@ namespace ccf::tls virtual ~Context() = default; - virtual void set_bio( - void* cb_obj, BIO_callback_fn_ex send, BIO_callback_fn_ex recv) + virtual void set_bio() { - // Read/Write BIOs will be used by TLS + // In-memory read/write BIOs hold the encrypted bytes exchanged with the + // peer. TLSSession feeds received bytes into the read BIO (recv) and + // drains bytes to be sent out of the write BIO (send). 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); + ccf::crypto::OpenSSL::CHECKNULL(rbio); + ccf::crypto::OpenSSL::CHECK1(BIO_set_mem_eof_return(rbio, -1)); 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); + ccf::crypto::OpenSSL::CHECKNULL(wbio); + ccf::crypto::OpenSSL::CHECK1(BIO_set_mem_eof_return(wbio, -1)); SSL_set0_wbio(ssl, wbio); } + // Feed encrypted bytes received from the peer into the read BIO. + virtual void recv(const uint8_t* buf, size_t len) + { + if (len == 0) + { + return; + } + // Writing to an in-memory BIO only fails on allocation failure, and is + // otherwise all-or-nothing. + int rc = BIO_write(SSL_get_rbio(ssl), buf, len); + if (rc < 0 || static_cast(rc) != len) + { + LOG_FAIL_FMT("Failed to buffer {} received bytes (rc={})", len, rc); + } + } + + // Number of encrypted bytes waiting in the write BIO to be sent to the + // peer. + virtual size_t pending_write() + { + return BIO_pending(SSL_get_wbio(ssl)); + } + + // Drain encrypted bytes to be sent to the peer out of the write BIO. + virtual size_t send(uint8_t* buf, size_t len) + { + // A negative return means no bytes were available to drain (the BIO is + // configured to retry rather than signal EOF), which we report as 0. + int rc = BIO_read(SSL_get_wbio(ssl), buf, len); + return rc < 0 ? 0 : static_cast(rc); + } + virtual int handshake() { if (SSL_is_init_finished(ssl) != 0) @@ -107,89 +138,69 @@ namespace ccf::tls } int rc = SSL_do_handshake(ssl); - // Success in OpenSSL is 1, MBed is 0 if (rc > 0) { LOG_TRACE_FMT("Context::handshake() : Success"); return 0; } - // Want read/write needs special return - if (SSL_want_read(ssl)) - { - return TLS_ERR_WANT_READ; - } + int err = SSL_get_error(ssl, rc); - if (SSL_want_write(ssl)) - { - return TLS_ERR_WANT_WRITE; - } - - // So does x509 validation - if (!peer_cert_ok()) + // A failed handshake with a bad peer certificate is reported as a generic + // SSL error, so we check the verification result explicitly to let the + // caller treat it as an authentication failure. + if (err == SSL_ERROR_SSL && !peer_cert_ok()) { return TLS_ERR_X509_VERIFY; } - // Everything else falls here. - LOG_TRACE_FMT("Context::handshake() : Error code {}", rc); - - // As an MBedTLS emulation, we return negative for errors. - return -SSL_get_error(ssl, rc); + LOG_TRACE_FMT("Context::handshake() : SSL error {}", err); + return err; } - virtual int read(uint8_t* buf, size_t len) + virtual int read(uint8_t* buf, size_t len, size_t& readbytes) { + readbytes = 0; if (len == 0) { return 0; } - size_t readbytes = 0; int rc = SSL_read_ex(ssl, buf, len, &readbytes); if (rc > 0) { - return readbytes; - } - if (SSL_want_read(ssl)) - { - return TLS_ERR_WANT_READ; + return 0; } - - // Everything else falls here. - LOG_TRACE_FMT("Context::read() : Error code {}", rc); - - // As an MBedTLS emulation, we return negative for errors. - return -SSL_get_error(ssl, rc); + int err = SSL_get_error(ssl, rc); + LOG_TRACE_FMT("Context::read() : SSL error {}", err); + return err; } - virtual int write(const uint8_t* buf, size_t len) + virtual int write(const uint8_t* buf, size_t len, size_t& written) { + written = 0; if (len == 0) { return 0; } - size_t written = 0; int rc = SSL_write_ex(ssl, buf, len, &written); if (rc > 0) { - return written; - } - if (SSL_want_write(ssl)) - { - return TLS_ERR_WANT_WRITE; + return 0; } - - // Everything else falls here. - LOG_TRACE_FMT("Context::write() : Error code {}", rc); - - // As an MBedTLS emulation, we return negative for errors. - return -SSL_get_error(ssl, rc); + int err = SSL_get_error(ssl, rc); + LOG_TRACE_FMT("Context::write() : SSL error {}", err); + return err; } virtual int close() { LOG_TRACE_FMT("Context::close() : Shutdown"); - return SSL_shutdown(ssl); + int rc = SSL_shutdown(ssl); + if (rc >= 0) + { + return 0; + } + return SSL_get_error(ssl, rc); } virtual bool peer_cert_ok() diff --git a/src/tls/plaintext_server.h b/src/tls/plaintext_server.h index 60648e09198..a7eeedee6bf 100644 --- a/src/tls/plaintext_server.h +++ b/src/tls/plaintext_server.h @@ -16,17 +16,38 @@ namespace nontls Unique_BIO write_bio; public: - void set_bio( - void* cb_obj, BIO_callback_fn_ex send, BIO_callback_fn_ex recv) override + void set_bio() override { - // Read/Write BIOs will be used by TLS - BIO_set_mem_eof_return(read_bio, -1); - BIO_set_callback_arg(read_bio, static_cast(cb_obj)); - BIO_set_callback_ex(read_bio, recv); + // Plaintext passthrough: the read/write BIOs hold the unencrypted bytes + // exchanged with the peer directly. + CHECK1(BIO_set_mem_eof_return(read_bio, -1)); + CHECK1(BIO_set_mem_eof_return(write_bio, -1)); + } - BIO_set_mem_eof_return(write_bio, -1); - BIO_set_callback_arg(write_bio, static_cast(cb_obj)); - BIO_set_callback_ex(write_bio, send); + void recv(const uint8_t* buf, size_t len) override + { + if (len == 0) + { + return; + } + int rc = BIO_write(read_bio, buf, len); + if (rc < 0 || static_cast(rc) != len) + { + LOG_FAIL_FMT("Failed to buffer {} received bytes (rc={})", len, rc); + } + } + + size_t pending_write() override + { + return BIO_pending(write_bio); + } + + size_t send(uint8_t* buf, size_t len) override + { + // A negative return means no bytes were available to drain, reported as + // 0. + int rc = BIO_read(write_bio, buf, len); + return rc < 0 ? 0 : static_cast(rc); } int handshake() override @@ -34,34 +55,34 @@ namespace nontls return 0; } - int read(uint8_t* buf, size_t len) override + int read(uint8_t* buf, size_t len, size_t& readbytes) override { + readbytes = 0; if (len == 0) { return 0; } - size_t readbytes = 0; - int rc = BIO_read_ex(read_bio, buf, len, &readbytes); - if (rc > 0) + int success = BIO_read_ex(read_bio, buf, len, &readbytes); + if (success > 0) { - return readbytes; + return 0; } - return -rc; + return SSL_ERROR_WANT_READ; } - int write(const uint8_t* buf, size_t len) override + int write(const uint8_t* buf, size_t len, size_t& written) override { + written = 0; if (len == 0) { return 0; } - size_t written = 0; - int rc = BIO_write_ex(write_bio, buf, len, &written); - if (rc > 0) + int success = BIO_write_ex(write_bio, buf, len, &written); + if (success > 0) { - return written; + return 0; } - return -rc; + return SSL_ERROR_WANT_WRITE; } int close() override diff --git a/src/tls/test/main.cpp b/src/tls/test/main.cpp index 2791ef16775..07e5711f9b8 100644 --- a/src/tls/test/main.cpp +++ b/src/tls/test/main.cpp @@ -13,203 +13,73 @@ #include #include #include +#include #define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN #include #include #include #include -#include -#include using namespace std; using namespace ccf::crypto; using namespace tls; -/// Server uses one pipe while client uses the other. -/// Writes always to one side, reads always from the other. -/// Use the send/recv template wrappers below as callbacks. -class TestPipe +/// Moves all the encrypted bytes that 'from' wants to send into 'to', as if +/// they had traversed the network. Returns the number of bytes transferred. +size_t transfer(ccf::tls::Context& from, ccf::tls::Context& to) { - int pfd[2]; - -public: - static const int SERVER = 0; - static const int CLIENT = 1; - - TestPipe() - { - if (socketpair(PF_LOCAL, SOCK_STREAM, 0, pfd) == -1) - { - throw runtime_error( - "Failed to create socketpair: " + string(ccf::nonstd::strerror(errno))); - } - } - ~TestPipe() + size_t total = 0; + std::vector buf(16384); + size_t got = 0; + while ((got = from.send(buf.data(), buf.size())) > 0) { - close(pfd[0]); - close(pfd[1]); + to.recv(buf.data(), got); + total += got; } - - size_t send(int id, const uint8_t* buf, size_t len) - { - int rc = write(pfd[id], buf, len); - if (rc == -1) - LOG_FAIL_FMT("Error while reading: {}", ccf::nonstd::strerror(errno)); - return rc; - } - - size_t recv(int id, uint8_t* buf, size_t len) - { - int rc = read(pfd[id], buf, len); - if (rc == -1) - LOG_FAIL_FMT("Error while reading: {}", ccf::nonstd::strerror(errno)); - return rc; - } -}; - -/// Callback wrapper around TestPipe->send(). -template -int send(void* ctx, const uint8_t* buf, size_t len) -{ - auto pipe = reinterpret_cast(ctx); - int rc = pipe->send(end, buf, len); - REQUIRE(rc == len); - return rc; + return total; } -/// Callback wrapper around TestPipe->recv(). -template -int recv(void* ctx, uint8_t* buf, size_t len) +/// Returns true if the handshake status code indicates a genuine error, as +/// opposed to success or a request for more data. +bool is_handshake_error(int rc) { - auto pipe = reinterpret_cast(ctx); - int rc = pipe->recv(end, buf, len); - REQUIRE(rc == len); - return rc; + return rc != 0 && rc != SSL_ERROR_WANT_READ && rc != SSL_ERROR_WANT_WRITE; } -// OpenSSL callbacks that call onto the pipe's ones -template -long send( - BIO* b, - int oper, - const char* argp, - size_t len, - int argi, - long argl, - int ret, - size_t* processed) +/// Drives the handshake between a client and a server connected via in-memory +/// BIOs, pumping bytes across after each step. +/// Returns 0 on success, 1 on client error, 2 on server error. +int handshake(ccf::tls::Context& client, ccf::tls::Context& server) { - // Unused arguments - (void)argi; - (void)argl; - (void)processed; - - if (ret && oper == (BIO_CB_WRITE | BIO_CB_RETURN)) - { - // Flush the BIO so the "pipe doesn't clog", but we don't use the - // data here, because 'argp' already has it. - BIO_flush(b); - size_t pending = BIO_pending(b); - if (pending) - BIO_reset(b); - - // Pipe object - auto pipe = reinterpret_cast(BIO_get_callback_arg(b)); - size_t put = send(pipe, (const uint8_t*)argp, len); - REQUIRE(put == len); - } - - // Unless we detected an error, the return value is always the same as the - // original operation. - return ret; -} - -template -long recv( - BIO* b, - int oper, - const char* argp, - size_t len, - int argi, - long argl, - int ret, - size_t* processed) -{ - // Unused arguments - (void)argi; - (void)argl; - - if (ret && oper == (BIO_CB_READ | BIO_CB_RETURN)) + constexpr int max_iterations = 50; + for (int i = 0; i < max_iterations; ++i) { - // Pipe object - auto pipe = reinterpret_cast(BIO_get_callback_arg(b)); - size_t got = recv(pipe, (uint8_t*)argp, len); + int cs = client.handshake(); + transfer(client, server); + int ss = server.handshake(); + transfer(server, client); - // Got nothing, return "WANTS READ" - if (got <= 0) - return ret; - - // Write to the actual BIO so SSL can use it - BIO_write_ex(b, argp, got, processed); - - // If original return was -1 because it didn't find anything to read, return - // 1 to say we actually read something - if (got > 0 && ret < 0) + if (is_handshake_error(cs)) + { + LOG_FAIL_FMT( + "Client handshake error: {}", ::tls::error_string(ERR_get_error())); return 1; - } - - // Unless we detected an error, the return value is always the same as the - // original operation. - return ret; -} - -/// Performs a TLS handshake, looping until there's nothing more to read/write. -/// Returns 0 on success, throws a runtime error with SSL error str on failure. -int handshake(ccf::tls::Context* ctx, std::atomic& keep_going) -{ - while (keep_going) - { - int rc = ctx->handshake(); + } + if (is_handshake_error(ss)) + { + LOG_FAIL_FMT( + "Server handshake error: {}", ::tls::error_string(ERR_get_error())); + return 2; + } - switch (rc) + if (cs == 0 && ss == 0) { - case 0: - return 0; - - case TLS_ERR_WANT_READ: - case TLS_ERR_WANT_WRITE: - // Continue calling handshake until finished - LOG_DEBUG_FMT("Handshake wants data"); - break; - - case TLS_ERR_NEED_CERT: - { - LOG_FAIL_FMT("Handshake error: {}", ::tls::error_string(rc)); - return 1; - } - - case TLS_ERR_CONN_CLOSE_NOTIFY: - { - LOG_FAIL_FMT("Handshake error: {}", ::tls::error_string(rc)); - return 1; - } - - case TLS_ERR_X509_VERIFY: - { - auto err = ctx->get_verify_error(); - LOG_FAIL_FMT("Handshake error: {} [{}]", err, ::tls::error_string(rc)); - return 1; - } - - default: - { - LOG_FAIL_FMT("Handshake error: {}", ::tls::error_string(rc)); - return 1; - } + return 0; } } - return 0; + LOG_FAIL_FMT("Handshake did not complete in {} iterations", max_iterations); + return 1; } struct NetworkCA @@ -317,24 +187,47 @@ TEST_CASE("Cert configures TLS verification and own certificate") REQUIRE(SSL_get_certificate(ssl) != nullptr); } -/// Helper to write past the maximum buffer (16k) -int write_helper(ccf::tls::Context& handler, const uint8_t* buf, size_t len) +/// Helper to write a full message, transferring the encrypted bytes to the +/// peer as they are produced. Returns the number of plaintext bytes written. +size_t write_helper( + ccf::tls::Context& handler, + ccf::tls::Context& peer, + const uint8_t* buf, + size_t len) { LOG_DEBUG_FMT("WRITE {} bytes", len); - int rc = handler.write(buf, len); - if (rc <= 0 || (size_t)rc == len) - return rc; - return rc + write_helper(handler, buf + rc, len - rc); + size_t total = 0; + while (total < len) + { + size_t written = 0; + int rc = handler.write(buf + total, len - total, written); + total += written; + transfer(handler, peer); + if (rc != 0) + { + break; + } + } + return total; } -/// Helper to read past the maximum buffer (16k) -int read_helper(ccf::tls::Context& handler, uint8_t* buf, size_t len) +/// Helper to read a full message from already-received encrypted bytes. +/// Returns the number of plaintext bytes read. +size_t read_helper(ccf::tls::Context& handler, uint8_t* buf, size_t len) { LOG_DEBUG_FMT("READ {} bytes", len); - int rc = handler.read(buf, len); - if (rc <= 0 || (size_t)rc == len) - return rc; - return rc + read_helper(handler, buf + rc, len - rc); + size_t total = 0; + while (total < len) + { + size_t readbytes = 0; + int rc = handler.read(buf + total, len - total, readbytes); + total += readbytes; + if (rc != 0) + { + break; + } + } + return total; } /// Helper to truncate long messages to make logs more readable @@ -363,61 +256,24 @@ void run_test_case( tls::Server server(std::move(server_cert)); tls::Client client(std::move(client_cert)); - // Connect BIOs together - TestPipe pipe; - server.set_bio(&pipe, send, recv); - client.set_bio(&pipe, send, recv); - - std::atomic keep_going = true; - std::optional client_exception, server_exception; + // Set up the in-memory BIOs used to exchange encrypted bytes + server.set_bio(); + client.set_bio(); - // Create a thread for the client handshake - thread client_thread([&client, &keep_going, &client_exception]() { - LOG_INFO_FMT("Client handshake"); - try - { - if (handshake(&client, keep_going)) - throw runtime_error("Client handshake error"); - } - catch (std::runtime_error& ex) - { - keep_going = false; - client_exception = ex; - } - }); - - // Create a thread for the server handshake - thread server_thread([&server, &keep_going, &server_exception]() { - LOG_INFO_FMT("Server handshake"); - try - { - if (handshake(&server, keep_going)) - throw runtime_error("Server handshake error"); - } - catch (std::runtime_error& ex) - { - keep_going = false; - server_exception = ex; - } - }); - - // Join threads - client_thread.join(); - server_thread.join(); - LOG_INFO_FMT("Handshake completed"); - - if (client_exception) + LOG_INFO_FMT("Handshake"); + switch (handshake(client, server)) { - throw *client_exception; - } - if (server_exception) - { - throw *server_exception; + case 0: + break; + case 1: + throw runtime_error("Client handshake error"); + default: + throw runtime_error("Server handshake error"); } + LOG_INFO_FMT("Handshake completed"); // The rest of the communication is deterministic and easy to simulate - // so we take them out of the thread, to guarantee there will be bytes - // to read at the right time. + // so we drive it directly, transferring bytes between the peers as needed. if (message_length == 0) { LOG_INFO_FMT("Empty message. Ignoring communication test"); @@ -430,11 +286,11 @@ void run_test_case( // Send the first message LOG_INFO_FMT( "Client sending message [{}]", truncate_message(message, message_length)); - int written = write_helper(client, message, message_length); + size_t written = write_helper(client, server, message, message_length); REQUIRE(written == message_length); // Receive the first message - int read = read_helper(server, buf.data(), message_length); + size_t read = read_helper(server, buf.data(), message_length); REQUIRE(read == message_length); buf[message_length] = '\0'; LOG_INFO_FMT( @@ -447,7 +303,7 @@ void run_test_case( // Send the response LOG_INFO_FMT( "Server sending message [{}]", truncate_message(response, message_length)); - written = write_helper(server, response, response_length); + written = write_helper(server, client, response, response_length); REQUIRE(written == response_length); // Receive the response diff --git a/src/tls/tls.h b/src/tls/tls.h index e293f793c84..adcf846fa36 100644 --- a/src/tls/tls.h +++ b/src/tls/tls.h @@ -2,25 +2,11 @@ // Licensed under the Apache 2.0 License. #pragma once -// These macros setup return values for when the connection is reading/writing -// and needs more data or has an error. -// -// In OpenSSL, the return is -1/0/1 and the error code depends on what -// SSL_want() returns. So we need to return some distinct negative number -// and then handle WANT_READ/WANT_WRITE and errors. -// -// Depending on the error, the connection needs to close with success, failure -// or auth-failure. -#define TLS_READING -SSL_READING -#define TLS_WRITING -SSL_WRITING -#define TLS_ERR_WANT_READ -SSL_ERROR_WANT_READ -#define TLS_ERR_WANT_WRITE -SSL_ERROR_WANT_WRITE -#define TLS_ERR_CONN_CLOSE_NOTIFY -SSL_ERROR_ZERO_RETURN -#define TLS_ERR_NEED_CERT -SSL_ERROR_WANT_X509_LOOKUP -// Specific error to check validity of certificate, not emitted by OpenSSL, but -// by Context. We set to a bogus negative value that won't match any OpenSSL -// error code. -// Once we refactor the code to match the OpenSSL style we may not need this. +// Specific error to flag a certificate validation failure during the handshake. +// This is not emitted by OpenSSL itself (its handshake failures surface as +// SSL_ERROR_SSL), but is set by Context so the caller can distinguish an +// authentication failure from a generic error. We use a bogus negative value +// that won't match any OpenSSL SSL_ERROR_* code. #define TLS_ERR_X509_VERIFY INT_MIN #include "ccf/crypto/openssl/openssl_wrappers.h"