This test succeeds, which is wrong: $ nbdsh -c 'h.set_tls(nbd.TLS_REQUIRE)' \ -c 'h.connect_command(["nbdkit", "-o", "-s", "null"])' \ -c 'print(h.get_size())' 0 Consider the case of a server that allows, but does not require, TLS encryption. A client that wants to only use the server if encrypted (as evidenced by the request for LIBNBD_TLS_REQUIRE) can be subjected to a protocol downgrade attack: a man-in-the-middle attacker can translate the original server's unencrypted newstyle offerings into an oldstyle server, such that the client is now unaware that it is sending plain-text rather than the desired encrypted session. Red Hat security will probably assign this a CVE, but we felt it reasonable to publish the fix now, in part due to the rarity of oldstyle servers these days. Workaround: if the server offers extensions that are only possible in newstyle connections, a client can check post-connection but before sending any read or write requests that any of those extensions are enabled, to ensure that a newstyle connection is in use (unfortunately, this doesn't help for all servers). Known witnesses: - if nbd_can_df(h) returns true - if the client requested nbd_add_meta_context(h, context) prior to connection, then after connection nbd_can_meta_context(h, context) returns true (the most common context is LIBNBD_CONTEXT_BASE_ALLOCATION) --- generator/states-oldstyle.c | 10 ++++++++++ tests/oldstyle.c | 17 +++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/generator/states-oldstyle.c b/generator/states-oldstyle.c index 668931b..1aff185 100644 --- a/generator/states-oldstyle.c +++ b/generator/states-oldstyle.c @@ -46,6 +46,16 @@ gflags = be16toh (h->sbuf.old_handshake.gflags); eflags = be16toh (h->sbuf.old_handshake.eflags); + /* Server is unable to upgrade to TLS. If h->tls is not require (2) + * then we can continue unencrypted. + */ + if (h->tls == 2) { + SET_NEXT_STATE (%.DEAD); + set_error (ENOTSUP, "handshake: server is oldstyle, " + "but handle TLS setting is require (2)"); + return 0; + } + h->gflags = gflags; debug (h, "gflags: 0x%" PRIx16, gflags); diff --git a/tests/oldstyle.c b/tests/oldstyle.c index 64862b7..c179c45 100644 --- a/tests/oldstyle.c +++ b/tests/oldstyle.c @@ -87,6 +87,23 @@ main (int argc, char *argv[]) progname = argv[0]; + /* Initial sanity check that we can't require TLS */ + nbd = nbd_create (); + if (nbd == NULL) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_set_tls (nbd, LIBNBD_TLS_REQUIRE) == -1) { + fprintf (stderr, "%s\n", nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_connect_command (nbd, args) != -1) { + fprintf (stderr, "%s\n", "expected failure"); + exit (EXIT_FAILURE); + } + nbd_close (nbd); + + /* Now for a working connection */ nbd = nbd_create (); if (nbd == NULL) { fprintf (stderr, "%s\n", nbd_get_error ()); -- 2.21.0