[Libguestfs] [libnbd PATCH] api: Add nbd_[aio_]opt_starttls
Eric Blake
eblake at redhat.com
Tue Oct 4 12:51:18 UTC 2022
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
More information about the Libguestfs
mailing list