Libguestfs - Aug 2021 - [nbdkit PATCH] server: CVE-2021-???? reset structured replies on starttls

https://nostarttls.secvuln.info/ pointed out a common implementation
flaw in various SMTP and IMAP servers with regards to improperly
caching plaintext state across the STARTTLS encryption boundary.  It
turns out that nbdkit has the same vulnerability in regards to the NBD
protocol: an attacker is able to inject a plaintext
NBD_OPT_STRUCTURED_REPLY before proxying everything else a client
sends to the server; if the server then acts on that plaintext request
(as nbdkit did before this patch), then the server ends up sending
structured replies to at least NBD_CMD_READ, even though the client
was not expecting them.  The NBD spec has been recently tightened to
declare the nbdkit behavior to be a security hole.

---

[NB: I'm still in the process of getting a CVE assigned; there is no
embargo since the issue is already public, but I may wait to apply
this patch until the commit message can be tweaked]
---
 server/protocol-handshake-newstyle.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index a2c89c9a..7e6b7b1b 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -495,7 +495,8 @@ negotiate_handshake_newstyle_options (void)
           return -1;
         conn->using_tls = true;
         debug ("using TLS on this connection");
-        /* Wipe out any cached default export name. */
+        /* Wipe out any cached state. */
+        conn->structured_replies = false;
         for_each_backend (b) {
           free (conn->default_exportname[b->i]);
           conn->default_exportname[b->i] = NULL;
-- 
2.31.1

On Mon, Aug 16, 2021 at 01:50:46PM -0500, Eric Blake
wrote:
> https://nostarttls.secvuln.info/ pointed out a common implementation
> flaw in various SMTP and IMAP servers with regards to improperly
> caching plaintext state across the STARTTLS encryption boundary.  It
> turns out that nbdkit has the same vulnerability in regards to the NBD
> protocol: an attacker is able to inject a plaintext
> NBD_OPT_STRUCTURED_REPLY before proxying everything else a client
> sends to the server; if the server then acts on that plaintext request
> (as nbdkit did before this patch), then the server ends up sending
> structured replies to at least NBD_CMD_READ, even though the client
> was not expecting them.  The NBD spec has been recently tightened to
> declare the nbdkit behavior to be a security hole.
> 
> ---
> 
> [NB: I'm still in the process of getting a CVE assigned; there is no
> embargo since the issue is already public, but I may wait to apply
> this patch until the commit message can be tweaked]
> ---
>  server/protocol-handshake-newstyle.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
> index a2c89c9a..7e6b7b1b 100644
> --- a/server/protocol-handshake-newstyle.c
> +++ b/server/protocol-handshake-newstyle.c
> @@ -495,7 +495,8 @@ negotiate_handshake_newstyle_options (void)
>            return -1;
>          conn->using_tls = true;
>          debug ("using TLS on this connection");
> -        /* Wipe out any cached default export name. */
> +        /* Wipe out any cached state. */
> +        conn->structured_replies = false;
>          for_each_backend (b) {
>            free (conn->default_exportname[b->i]);
>            conn->default_exportname[b->i] = NULL;
It's be good to either reference the nostarttls website, or
the relevant section in NBD proto.md (if it's upstream yet)
in the comment.

But yes - ACK.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Mon, Aug 16, 2021 at 01:50:46PM -0500, Eric Blake
wrote:
> https://nostarttls.secvuln.info/ pointed out a common implementation
> flaw in various SMTP and IMAP servers with regards to improperly
> caching plaintext state across the STARTTLS encryption boundary.  It
> turns out that nbdkit has the same vulnerability in regards to the NBD
> protocol: an attacker is able to inject a plaintext
> NBD_OPT_STRUCTURED_REPLY before proxying everything else a client
> sends to the server; if the server then acts on that plaintext request
> (as nbdkit did before this patch), then the server ends up sending
> structured replies to at least NBD_CMD_READ, even though the client
> was not expecting them.  The NBD spec has been recently tightened to
> declare the nbdkit behavior to be a security hole.
> 
> ---
> 
> [NB: I'm still in the process of getting a CVE assigned; there is no
> embargo since the issue is already public, but I may wait to apply
> this patch until the commit message can be tweaked]
> ---
>  server/protocol-handshake-newstyle.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
> index a2c89c9a..7e6b7b1b 100644
> --- a/server/protocol-handshake-newstyle.c
> +++ b/server/protocol-handshake-newstyle.c
> @@ -495,7 +495,8 @@ negotiate_handshake_newstyle_options (void)
>            return -1;
>          conn->using_tls = true;
>          debug ("using TLS on this connection");
> -        /* Wipe out any cached default export name. */
> +        /* Wipe out any cached state. */
> +        conn->structured_replies = false;
>          for_each_backend (b) {
>            free (conn->default_exportname[b->i]);
>            conn->default_exportname[b->i] = NULL;
While backporting this patch, I found one more piece of state that
nbdkit accidentally preserved across STARTTLS:
conn->meta_context_base_allocation.  Fortunately, this one does not
change the protocol the server speaks to a compliant client: since a
client should never use NBD_CMD_BLOCK_STATUS unless it first used
NBD_OPT_SET_META_CONTEXT beforehand, an injection of SET_META_CONTEXT
by MitM will either never be noticed (the client doesn't want to use
block status) or will be overwritten (when the client itself
negotiates meta context after the encryption is set up).  Thus, while
related to the CVE, it is not in the same category as the
STRUCTURED_REPLY injection (where a completely compliant client still
gets unexpected traffic from the server).  I'll go ahead and patch it
upstream, and backport it as well.  But it missed the 1.27.7, 1.26.5,
and 1.24.6 releases, yet I don't see it as a show-stopper worth
cutting another release.

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