Libguestfs - Oct 2022 - [nbdkit PATCH] tls: Gracefully handle early NBD_OPT_ payload

When using --tls=require, we need to consume any payload sent by the
client on an option other than NBD_OPT_STARTTLS (for example,
NBD_OPT_INFO); otherwise we will be out of sync and unable to read the
next NBD_OPT_, most likely preventing us from being able to establish
TLS if the client next attempts NBD_OPT_STARTTLS.  Also, if the client
sends NBD_OPT_EXPORT_NAME, they are expecting us to disconnect on
failure, rather than trying to reply with an error (most clients use
NBD_OPT_GO these days because of that).

While it is unfortunate that a MitM proxy attacker can use this
weakness in nbdkit to prevent a TLS connection, it is not quite the
same as the prior fix for CVE-2021-3716 (commit 09a13daf) where a
plaintext NBD_OPT_STRUCTURED_REPLY could confuse a client; this is
because failure to establish TLS is easily detected at the client as a
proxy attack, and not a situation where the plaintext injection can
break client behavior without detection.

This bug has been latent for some time; that is because most people do
not try to connect a plaintext client to a server that requires TLS;
and even if it is done accidentally, clients like qemu or libnbd that
give a payload-free option request of NBD_OPT_STRUCTURED_REPLY first
don't tickle the problem, especially if they disconnect on getting the
NBD_REP_ERR_TLS_REQD rather than trying to proceed with another
NBD_OPT_*.  But with an upcoming libnbd release adding a new API
nbd_opt_starttls() to write a SELECTIVETLS client, it becomes easier
to tickle.  See also qemu commit d1129a8a.

Fixes: c5e76492 ("Add TLS support.", v1.1.15)
---
 server/protocol-handshake-newstyle.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 5ac45efa..4e16b72c 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -424,8 +424,16 @@ negotiate_handshake_newstyle_options (void)
      */
     if (tls == 2 && !conn->using_tls &&
         !(option == NBD_OPT_ABORT || option == NBD_OPT_STARTTLS)) {
+      if (option == NBD_OPT_EXPORT_NAME) {
+        debug ("newstyle negotiation: can't reply NBD_REP_ERR_TLS_REQD
to %s",
+               name_of_nbd_opt (option));
+        return -1;
+      }
       if (send_newstyle_option_reply (option, NBD_REP_ERR_TLS_REQD))
         return -1;
+      if (conn_recv_full (data, optlen,
+                          "waiting for starttls: %m") == -1)
+        return -1;
       continue;
     }

-- 
2.37.3

On Mon, Oct 03, 2022 at 03:13:33PM -0500, Eric Blake
wrote:
> When using --tls=require, we need to consume any payload sent by the
> client on an option other than NBD_OPT_STARTTLS (for example,
> NBD_OPT_INFO); otherwise we will be out of sync and unable to read the
> next NBD_OPT_, most likely preventing us from being able to establish
> TLS if the client next attempts NBD_OPT_STARTTLS.  Also, if the client
> sends NBD_OPT_EXPORT_NAME, they are expecting us to disconnect on
> failure, rather than trying to reply with an error (most clients use
> NBD_OPT_GO these days because of that).
> 
> While it is unfortunate that a MitM proxy attacker can use this
> weakness in nbdkit to prevent a TLS connection, it is not quite the
> same as the prior fix for CVE-2021-3716 (commit 09a13daf) where a
> plaintext NBD_OPT_STRUCTURED_REPLY could confuse a client; this is
> because failure to establish TLS is easily detected at the client as a
> proxy attack, and not a situation where the plaintext injection can
> break client behavior without detection.
> 
> This bug has been latent for some time; that is because most people do
> not try to connect a plaintext client to a server that requires TLS;
> and even if it is done accidentally, clients like qemu or libnbd that
> give a payload-free option request of NBD_OPT_STRUCTURED_REPLY first
> don't tickle the problem, especially if they disconnect on getting the
> NBD_REP_ERR_TLS_REQD rather than trying to proceed with another
> NBD_OPT_*.  But with an upcoming libnbd release adding a new API
> nbd_opt_starttls() to write a SELECTIVETLS client, it becomes easier
> to tickle.  See also qemu commit d1129a8a.
> 
> Fixes: c5e76492 ("Add TLS support.", v1.1.15)
> ---
>  server/protocol-handshake-newstyle.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
This one is now in as 282c3b03

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org