From cca3b10fea96a349a9d718cc92f4216e44944216 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Sat, 14 Sep 2019 22:12:49 -0500
Subject: [PATCH] security: states: Fail oldstyle servers when tls==2

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 668931b1..1aff1852 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 64862b7e..c179c454 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 ());