Very similar to the recent addition of nbd_opt_structured_reply, with the new nbd_opt_starttls() finally giving us fine-grained control over NBD_OPT_STARTTLS, and allowing productive handshaking with a server in SELECTIVETLS mode. With this patch, it is now easy to reproduce the scenario of nbdkit's CVE-2021-3716, where a plaintext meddler-in-the-middle attacker could cause client denial of service when a --tls=on server failed to reset state after NBD_OPT_STARTTLS. This also exposed the fact that nbdkit was not gracefully handling NBD_OPT_INFO from a plaintext client connecting to a --tls=require server; the testsuite is skipped unless using a fixed nbdkit (v1.33.2 or later). --- generator/API.ml | 98 +++++++++++-- generator/states-newstyle-opt-starttls.c | 40 ++++-- generator/states-newstyle.c | 3 + lib/opt.c | 50 +++++++ tests/Makefile.am | 5 + tests/opt-starttls.c | 166 +++++++++++++++++++++++ .gitignore | 1 + 7 files changed, 343 insertions(+), 20 deletions(-) create mode 100644 tests/opt-starttls.c diff --git a/generator/API.ml b/generator/API.ml index 69849102..8301ec9f 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -599,7 +599,11 @@ "set_tls", { =item C<LIBNBD_TLS_DISABLE> Disable TLS. (The default setting, unless using L<nbd_connect_uri(3)> with -a URI that requires TLS) +a URI that requires TLS). + +This setting is also useful during integration testing when using +L<nbd_set_opt_mode(3)> and L<nbd_opt_starttls(3)> for manual +control over when to request TLS negotiation. =item C<LIBNBD_TLS_ALLOW> @@ -632,7 +636,8 @@ "set_tls", { test whether this is the case with L<nbd_supports_tls(3)>."; example = Some "examples/encryption.c"; see_also = [SectionLink "ENCRYPTION AND AUTHENTICATION"; - Link "get_tls"; Link "get_tls_negotiated"]; + Link "get_tls"; Link "get_tls_negotiated"; + Link "opt_starttls"]; }; "get_tls", { @@ -657,7 +662,7 @@ "get_tls_negotiated", { After connecting you may call this to find out if the connection is using TLS. -This is only really useful if you set the TLS request mode +This is normally useful only if you set the TLS request mode to C<LIBNBD_TLS_ALLOW> (see L<nbd_set_tls(3)>), because in this mode we try to use TLS but fall back to unencrypted if it was not available. This function will tell you if TLS was @@ -665,8 +670,14 @@ "get_tls_negotiated", { In C<LIBNBD_TLS_REQUIRE> mode (the most secure) the connection would have failed if TLS could not be negotiated, and in -C<LIBNBD_TLS_DISABLE> mode TLS is not tried."; - see_also = [Link "set_tls"; Link "get_tls"]; +C<LIBNBD_TLS_DISABLE> mode TLS is not tried automatically. + +However, when doing manual integration testing of server +behavior, when you use L<nbd_set_opt_mode(3)> along with TLS +request mode C<LIBNBD_TLS_DISABLE> before triggering the TLS +handshake with L<nbd_opt_starttls(3)>, then this will report +the result of that manual effort."; + see_also = [Link "set_tls"; Link "get_tls"; Link "opt_starttls"]; }; "set_tls_certificates", { @@ -1092,11 +1103,12 @@ "set_opt_mode", { newstyle server. This setting has no effect when connecting to an oldstyle server. -Note that libnbd defaults to attempting +Note that libnbd defaults to attempting C<NBD_OPT_STARTTLS> and C<NBD_OPT_STRUCTURED_REPLY> before letting you control remaining -negotiation steps; if you need control over this step as well, -first set L<nbd_set_request_structured_replies(3)> to false before -starting the connection attempt. +negotiation steps; if you need control over these steps as well, +first set L<nbd_set_tls(3)> to C<LIBNBD_TLS_DISABLE> and +L<nbd_set_request_structured_replies(3)> to false before starting +the connection attempt. When option mode is enabled, you have fine-grained control over which options are negotiated, compared to the default of the server @@ -1110,9 +1122,9 @@ "set_opt_mode", { see_also = [Link "get_opt_mode"; Link "aio_is_negotiating"; Link "opt_abort"; Link "opt_go"; Link "opt_list"; Link "opt_info"; Link "opt_list_meta_context"; - Link "opt_set_meta_context"; + Link "opt_set_meta_context"; Link "opt_starttls"; Link "opt_structured_reply"; - Link "set_request_structured_replies"; + Link "set_tls"; Link "set_request_structured_replies"; Link "aio_connect"]; }; @@ -1166,6 +1178,44 @@ "opt_abort", { see_also = [Link "set_opt_mode"; Link "aio_opt_abort"; Link "opt_go"]; }; + "opt_starttls", { + default_call with + args = []; ret = RBool; + permitted_states = [ Negotiating ]; + shortdesc = "request the server to initiate TLS"; + longdesc = "\ +Request that the server initiate a secure TLS connection, by +sending C<NBD_OPT_STARTTLS>. This can only be used if +L<nbd_set_opt_mode(3)> enabled option mode; furthermore, if you +use L<nbd_set_tls(3)> to request anything other than the default +of C<LIBNBD_TLS_DISABLE>, then libnbd will have already attempted +a TLS connection prior to allowing you control over option +negotiation. This command is disabled if L<nbd_supports_tls(3)> +reports false. + +This function is mainly useful for integration testing of corner +cases in server handling; in particular, misuse of this function +when coupled with a server that is not careful about resetting +stateful commands such as L<nbd_opt_structured_reply(3)> could +result in a security hole (see CVE-2021-3716 against nbdkit, for +example). Thus, when security is a concern, you should instead +prefer to use L<nbd_set_tls(3)> with C<LIBNBD_TLS_REQUIRE> and +let libnbd negotiate TLS automatically. + +This function returns true if the server replies with success, +false if the server replies with an error, and fails only if +the server does not reply (such as for a loss of connection, +which can include when the server rejects credentials supplied +during the TLS handshake). Note that the NBD protocol documents +that requesting TLS after it is already enabled is a client +error; most servers will gracefully fail a second request, but +that does not downgrade a TLS session that has already been +established, as reported by L<nbd_get_tls_negotiated(3)>."; + see_also = [Link "set_opt_mode"; Link "aio_opt_starttls"; + Link "set_tls"; Link "get_tls_negotiated"; + Link "supports_tls"] + }; + "opt_structured_reply", { default_call with args = []; ret = RBool; @@ -2815,6 +2865,30 @@ "aio_opt_abort", { see_also = [Link "set_opt_mode"; Link "opt_abort"]; }; + "aio_opt_starttls", { + default_call with + args = []; + optargs = [ OClosure completion_closure ]; + ret = RErr; + permitted_states = [ Negotiating ]; + shortdesc = "request the server to initiate TLS"; + longdesc = "\ +Request that the server initiate a secure TLS connection, by +sending C<NBD_OPT_STARTTLS>. This behaves like the synchronous +counterpart L<nbd_opt_starttls(3)>, except that it does +not wait for the server's response. + +To determine when the request completes, wait for +L<nbd_aio_is_connecting(3)> to return false. Or supply the optional +C<completion_callback> which will be invoked as described in +L<libnbd(3)/Completion callbacks>, except that it is automatically +retired regardless of return value. Note that detecting whether the +server returns an error (as is done by the return value of the +synchronous counterpart) is only possible with a completion +callback."; + see_also = [Link "set_opt_mode"; Link "opt_starttls"]; + }; + "aio_opt_structured_reply", { default_call with args = []; @@ -3744,6 +3818,8 @@ let first_version = "aio_opt_set_meta_context_queries", (1, 16); "opt_structured_reply", (1, 16); "aio_opt_structured_reply", (1, 16); + "opt_starttls", (1, 16); + "aio_opt_starttls", (1, 16); (* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. diff --git a/generator/states-newstyle-opt-starttls.c b/generator/states-newstyle-opt-starttls.c index 1233263b..fc72c43c 100644 --- a/generator/states-newstyle-opt-starttls.c +++ b/generator/states-newstyle-opt-starttls.c @@ -21,10 +21,15 @@ STATE_MACHINE { NEWSTYLE.OPT_STARTTLS.START: assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE); - /* If TLS was not requested we skip this option and go to the next one. */ - if (h->tls == LIBNBD_TLS_DISABLE) { - SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); - return 0; + if (h->opt_current == NBD_OPT_STARTTLS) + assert (h->opt_mode); + else { + /* If TLS was not requested we skip this option and go to the next one. */ + if (h->tls == LIBNBD_TLS_DISABLE) { + SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + return 0; + } + assert (CALLBACK_IS_NULL (h->opt_cb.completion)); } h->sbuf.option.version = htobe64 (NBD_NEW_VERSION); @@ -68,12 +73,14 @@ NEWSTYLE.OPT_STARTTLS.RECV_REPLY_PAYLOAD: NEWSTYLE.OPT_STARTTLS.CHECK_REPLY: uint32_t reply; struct socket *new_sock; + int err = ENOTSUP; reply = be32toh (h->sbuf.or.option_reply.reply); switch (reply) { case NBD_REP_ACK: nbd_internal_reset_size_and_flags (h); h->structured_replies = false; + h->meta_valid = false; new_sock = nbd_internal_crypto_create_session (h, h->sock); if (new_sock == NULL) { SET_NEXT_STATE (%.DEAD); @@ -86,6 +93,9 @@ NEWSTYLE.OPT_STARTTLS.CHECK_REPLY: SET_NEXT_STATE (%TLS_HANDSHAKE_WRITE); return 0; + case NBD_REP_ERR_INVALID: + err = EINVAL; + /* fallthrough */ default: if (handle_reply_error (h) == -1) { SET_NEXT_STATE (%.DEAD); @@ -102,10 +112,16 @@ NEWSTYLE.OPT_STARTTLS.CHECK_REPLY: return 0; } - debug (h, - "server refused TLS (%s), continuing with unencrypted connection", - reply == NBD_REP_ERR_POLICY ? "policy" : "not supported"); - SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + debug (h, "server refused TLS (%s)", + reply == NBD_REP_ERR_POLICY ? "policy" : + reply == NBD_REP_ERR_INVALID ? "invalid request" : "not supported"); + CALL_CALLBACK (h->opt_cb.completion, &err); + if (h->opt_current == NBD_OPT_STARTTLS) + SET_NEXT_STATE (%.NEGOTIATING); + else { + debug (h, "continuing with unencrypted connection"); + SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + } return 0; } return 0; @@ -149,12 +165,18 @@ NEWSTYLE.OPT_STARTTLS.TLS_HANDSHAKE_WRITE: return 0; NEWSTYLE.OPT_STARTTLS.TLS_HANDSHAKE_DONE: + int err = 0; + /* Finished handshake. */ h->tls_negotiated = true; nbd_internal_crypto_debug_tls_enabled (h); + CALL_CALLBACK (h->opt_cb.completion, &err); /* Continue with option negotiation. */ - SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); + if (h->opt_current == NBD_OPT_STARTTLS) + SET_NEXT_STATE (%.NEGOTIATING); + else + SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START); return 0; } /* END STATE MACHINE */ diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c index 4652bc21..c88430e2 100644 --- a/generator/states-newstyle.c +++ b/generator/states-newstyle.c @@ -146,6 +146,9 @@ NEWSTYLE.START: case NBD_OPT_STRUCTURED_REPLY: SET_NEXT_STATE (%OPT_STRUCTURED_REPLY.START); return 0; + case NBD_OPT_STARTTLS: + SET_NEXT_STATE (%OPT_STARTTLS.START); + return 0; case 0: break; default: diff --git a/lib/opt.c b/lib/opt.c index 18bb4086..4ebb689e 100644 --- a/lib/opt.c +++ b/lib/opt.c @@ -139,6 +139,31 @@ nbd_unlocked_opt_abort (struct nbd_handle *h) return wait_for_option (h); } +/* Issue NBD_OPT_STARTTLS and wait for the reply. */ +int +nbd_unlocked_opt_starttls (struct nbd_handle *h) +{ + int err; + nbd_completion_callback c = { .callback = go_complete, .user_data = &err }; + int r = nbd_unlocked_aio_opt_starttls (h, &c); + + if (r == -1) + return r; + + r = wait_for_option (h); + if (r == 0) { + if (nbd_internal_is_state_negotiating (get_next_state (h))) + r = err == 0; + else { + assert (nbd_internal_is_state_dead (get_next_state (h))); + set_error (err, + "failed to get response to opt_starttls request"); + r = -1; + } + } + return r; +} + /* Issue NBD_OPT_STRUCTURED_REPLY and wait for the reply. */ int nbd_unlocked_opt_structured_reply (struct nbd_handle *h) @@ -336,6 +361,31 @@ nbd_unlocked_aio_opt_abort (struct nbd_handle *h) return 0; } +/* Issue NBD_OPT_STARTTLS without waiting. */ +int +nbd_unlocked_aio_opt_starttls (struct nbd_handle *h, + nbd_completion_callback *complete) +{ + if ((h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE) == 0) { + set_error (ENOTSUP, "server is not using fixed newstyle protocol"); + return -1; + } + +#ifndef HAVE_GNUTLS + set_error (ENOTSUP, "libnbd was compiled without TLS support"); + return -1; + +#else + h->opt_current = NBD_OPT_STARTTLS; + h->opt_cb.completion = *complete; + SET_CALLBACK_TO_NULL (*complete); + + if (nbd_internal_run (h, cmd_issue) == -1) + debug (h, "option queued, ignoring state machine failure"); + return 0; +#endif +} + /* Issue NBD_OPT_STRUCTURED_REPLY without waiting. */ int nbd_unlocked_aio_opt_structured_reply (struct nbd_handle *h, diff --git a/tests/Makefile.am b/tests/Makefile.am index 49ea6864..dd157518 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -658,12 +658,14 @@ if HAVE_PSKTOOL check_PROGRAMS += \ connect-tls-psk \ + opt-starttls \ aio-parallel-tls \ aio-parallel-load-tls \ synch-parallel-tls \ $(NULL) TESTS += \ connect-tls-psk \ + opt-starttls \ aio-parallel-tls.sh \ aio-parallel-load-tls.sh \ synch-parallel-tls.sh \ @@ -677,6 +679,9 @@ connect_tls_psk_CPPFLAGS = \ $(NULL) connect_tls_psk_LDADD = $(top_builddir)/lib/libnbd.la +opt_starttls_SOURCES = opt-starttls.c requires.c requires.h +opt_starttls_LDADD = $(top_builddir)/lib/libnbd.la + aio_parallel_tls_SOURCES = aio-parallel.c aio_parallel_tls_CPPFLAGS = \ $(AM_CPPFLAGS) \ diff --git a/tests/opt-starttls.c b/tests/opt-starttls.c new file mode 100644 index 00000000..b90a0dcd --- /dev/null +++ b/tests/opt-starttls.c @@ -0,0 +1,166 @@ +/* NBD client library in userspace + * Copyright (C) 2013-2022 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/* Test nbd_opt_starttls to nbdkit in permissive mode. */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <stdint.h> +#include <inttypes.h> +#include <fcntl.h> +#include <unistd.h> +#include <time.h> + +#include <libnbd.h> + +#include "requires.h" + +struct expected { + int first_opt_sr; + int64_t first_size; + int first_opt_tls; + int first_get_sr; + int second_opt_sr; + int second_opt_tls; + int get_tls; + int second_get_sr; + int64_t second_size; +}; + +#define check(got, exp) do_check (#got, got, exp) + +static void +do_check (const char *act, int64_t got, int64_t exp) +{ + fprintf (stderr, "trying %s\n", act); + if (got == -1) + fprintf (stderr, "%s\n", nbd_get_error ()); + else + fprintf (stderr, "succeeded, result %" PRId64 "\n", got); + if (got != exp) { + fprintf (stderr, "got %" PRId64 ", but expected %" PRId64 "\n", got, exp); + exit (EXIT_FAILURE); + } +} + +static void +do_test (const char *server_tls, struct expected exp) +{ + struct nbd_handle *nbd = nbd_create (); + + if (nbd == NULL) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + if (nbd_supports_tls (nbd) != 1) { + fprintf (stderr, "SKIP: missing TLS support in libnbd\n"); + exit (77); + } + + if (nbd_set_opt_mode (nbd, true) == -1 || + nbd_set_request_structured_replies (nbd, false) == -1 || + nbd_set_tls_username (nbd, "alice") == -1 || + nbd_set_tls_psk_file (nbd, "keys.psk") == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* Run nbdkit as a subprocess. */ + const char *args[] = { "nbdkit", "-sv", "--exit-with-parent", server_tls, + "--tls-verify-peer", "--tls-psk=keys.psk", + "--filter=tls-fallback", "pattern", + "size=1M", "tlsreadme=fallback", NULL }; + + if (nbd_connect_command (nbd, (char **) args) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + + check (nbd_opt_structured_reply (nbd), exp.first_opt_sr); + check (nbd_opt_info (nbd), exp.first_size > 0 ? 0 : -1); + check (nbd_get_size (nbd), exp.first_size); + check (nbd_opt_starttls (nbd), exp.first_opt_tls); + check (nbd_get_structured_replies_negotiated (nbd), exp.first_get_sr); + check (nbd_opt_structured_reply (nbd), exp.second_opt_sr); + check (nbd_opt_starttls (nbd), exp.second_opt_tls); + check (nbd_get_tls_negotiated (nbd), exp.get_tls); + check (nbd_get_structured_replies_negotiated (nbd), exp.second_get_sr); + check (nbd_opt_info (nbd), 0); + check (nbd_get_size (nbd), exp.second_size); + check (nbd_opt_abort (nbd), 0); + + nbd_close (nbd); +} + +int +main (int argc, char *argv[]) +{ + /* Check --tls-verify-peer option is supported. */ + requires ("nbdkit --tls-verify-peer -U - null --run 'exit 0'"); + /* Check for nbdkit tls-fallback filter. */ + requires ("nbdkit --filter=tls-fallback null --dump-plugin"); + + /* Reject older nbdkit that chokes on NBD_OPT_INFO to --tls=require */ + requires ("test 0 = \"$(nbdkit --tls=require --tls-psk=keys.psk -U - null " + "--run 'nbdinfo \"nbd+unix://?socket=$unixsocket\"' 2>&1 |" + "grep -c 'nbdkit.*unknown option version')\""); + + /* Behavior of a server with no TLS support */ + do_test ("--tls=no", (struct expected) { + .first_opt_sr = 1, /* Structured reply succeeds */ + .first_size = 512, /* Sees the tls-fallback safe size */ + .first_opt_tls = 0, /* Server lacks TLS, but connection stays up */ + .first_get_sr = 1, /* Structured reply still good */ + .second_opt_sr = 0, /* Server rejects second SR as redundant */ + .second_opt_tls = 0, /* Server still lacks TLS */ + .get_tls = 0, /* Final state of TLS - not secure */ + .second_get_sr = 1, /* Structured reply still good */ + .second_size = 512, /* Still the tls-fallback safe size */ + }); + + /* Behavior of a server with opportunistic TLS support */ + do_test ("--tls=on", (struct expected) { + .first_opt_sr = 1, /* Structured reply succeeds */ + .first_size = 512, /* Sees the tls-fallback safe size */ + .first_opt_tls = 1, /* Server takes TLS */ + .first_get_sr = 0, /* Structured reply wiped by TLS */ + .second_opt_sr = 1, /* Server accepts second SR */ + .second_opt_tls = 0, /* Server rejects second TLS as redundant */ + .get_tls = 1, /* Final state of TLS - secure */ + .second_get_sr = 1, /* Structured reply still good */ + .second_size = 1024*1024, /* Sees the actual size */ + }); + + /* Behavior of a server that requires TLS support */ + do_test ("--tls=require", (struct expected) { + .first_opt_sr = 0, /* Structured reply fails without TLS first */ + .first_size = -1, /* Cannot request info */ + .first_opt_tls = 1, /* Server takes TLS */ + .first_get_sr = 0, /* Structured reply hasn't been requested */ + .second_opt_sr = 1, /* Server accepts second SR */ + .second_opt_tls = 0, /* Server rejects second TLS as redundant */ + .get_tls = 1, /* Final state of TLS - secure */ + .second_get_sr = 1, /* Structured reply still good */ + .second_size = 1024*1024, /* Sees the actual size */ + }); + + exit (EXIT_SUCCESS); +} diff --git a/.gitignore b/.gitignore index 37166b73..fe929d6d 100644 --- a/.gitignore +++ b/.gitignore @@ -236,6 +236,7 @@ Makefile.in /tests/opt-list-meta-queries /tests/opt-set-meta /tests/opt-set-meta-queries +/tests/opt-starttls /tests/opt-structured-twice /tests/pki/ /tests/pread-initialize -- 2.37.3