Libguestfs - Aug 2021 - [PATCH] spec: Clarify STARTTLS vs. SET_META_CONTEXT interaction

Consider a SELECTIVETLS server and a MitM attacker, under the
following NBD_OPT_ handshake scenario:

 client:                  mitm:                    server:
 > _STARTTLS
                          > _SET_META_CONTEXT("A")
                                                   < _REP_META_CONTEXT
                                                   < _REP_ACK
                          > _STARTTLS
                                                   < _REP_ACK
                          < _REP_ACK
 > _SET_META_CONTEXT("B")
                          < _REP_META_CONTEXT
                          < _REP_ACK
 > _GO
                          > _GO
                                                   < _REP_ACK
                          < _REP_ACK
 > NBD_CMD_BLOCK_STATUS

While this scenario requires the MitM to be able to use encryption to
speak to the client (and thus a less likely scenario than a true
protocol downgrade or plaintext buffering attack), it results in a
situation where the client is asking for information on context "B",
but where the server only saw a request for context "A", which may
result in the client interpreting the results of BLOCK_STATUS
incorrectly even though it is coming over an encrypted connection.

The safest fix to this is to require that a server cannot use any meta
context requests from prior to enabling encryption with any successful
NBD_OPT_GO after encryption.  At this point, the spec already states
that the server should then return an error (the client is asking for
block status without proper negotiation), which is better than letting
the client blindly misinterpret a response sent for a different meta
context.

To date, the only known server that has implemented TLS with
SELECTIVETLS mode as well as support for NBD_OPT_SET_META_CONTEXT is
nbdkit (qemu-nbd only has FORCEDTLS mode, and nbd-server lacks meta
context support); thankfully, that implementation is in already line
with this stricter requirement.
---
 doc/proto.md | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/doc/proto.md b/doc/proto.md
index 61d57b5..7155b42 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -1165,6 +1165,14 @@ of the newstyle negotiation.
     permitted by this document (for example, `NBD_REP_ERR_INVALID` if
     the client included data).

+    When this command succeeds, the server MUST NOT preserve any
+    per-export state (such as metadata contexts from
+    `NBD_OPT_SET_META_CONTEXT`) issued before this command.  The
+    server MAY preserve global state such as a client request for
+    `NBD_OPT_STRUCTURED_REPLY`; however, a client SHOULD defer other
+    stateful option requests until after it determines whether
+    encryption is available.
+
     See the section on TLS above for further details.

 * `NBD_OPT_INFO` (6) and `NBD_OPT_GO` (7)
@@ -1406,8 +1414,8 @@ of the newstyle negotiation.
     negotiation phase it sent `NBD_OPT_SET_META_CONTEXT` at least
     once, and where the final time it was sent, it referred to the
     same export name that was ultimately selected for transmission
-    phase, and where the server responded by returning least one
-    metadata context without error.
+    phase with no intervening `NBD_OPT_STARTTLS`, and where the server
+    responded by returning least one metadata context without error.

     Data:
     - 32 bits, length of export name.  
-- 
2.31.1

On Thu, Aug 12, 2021 at 10:20:40AM -0500, Eric Blake
wrote:
> Consider a SELECTIVETLS server and a MitM attacker, under the
> following NBD_OPT_ handshake scenario:
> 
>  client:                  mitm:                    server:
>  > _STARTTLS
>                           > _SET_META_CONTEXT("A")
>                                                    < _REP_META_CONTEXT
>                                                    < _REP_ACK
>                           > _STARTTLS
>                                                    < _REP_ACK
>                           < _REP_ACK
>  > _SET_META_CONTEXT("B")
>                           < _REP_META_CONTEXT
>                           < _REP_ACK
>  > _GO
>                           > _GO
>                                                    < _REP_ACK
>                           < _REP_ACK
>  > NBD_CMD_BLOCK_STATUS
> 
> While this scenario requires the MitM to be able to use encryption to
> speak to the client (and thus a less likely scenario than a true
> protocol downgrade or plaintext buffering attack), it results in a
> situation where the client is asking for information on context
"B",
> but where the server only saw a request for context "A", which
may
> result in the client interpreting the results of BLOCK_STATUS
> incorrectly even though it is coming over an encrypted connection.
> 
> The safest fix to this is to require that a server cannot use any meta
> context requests from prior to enabling encryption with any successful
> NBD_OPT_GO after encryption.  At this point, the spec already states
> that the server should then return an error (the client is asking for
> block status without proper negotiation), which is better than letting
> the client blindly misinterpret a response sent for a different meta
> context.
> 
> To date, the only known server that has implemented TLS with
> SELECTIVETLS mode as well as support for NBD_OPT_SET_META_CONTEXT is
> nbdkit (qemu-nbd only has FORCEDTLS mode, and nbd-server lacks meta
> context support); thankfully, that implementation is in already line
> with this stricter requirement.
> ---
>  doc/proto.md | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index 61d57b5..7155b42 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -1165,6 +1165,14 @@ of the newstyle negotiation.
>      permitted by this document (for example, `NBD_REP_ERR_INVALID` if
>      the client included data).
> 
> +    When this command succeeds, the server MUST NOT preserve any
> +    per-export state (such as metadata contexts from
> +    `NBD_OPT_SET_META_CONTEXT`) issued before this command.  The
> +    server MAY preserve global state such as a client request for
> +    `NBD_OPT_STRUCTURED_REPLY`; however, a client SHOULD defer other
> +    stateful option requests until after it determines whether
> +    encryption is available.
I'm not sure I think that makes sense. It's safer to require that
STARTTLS wipes out everything.

There are two reasonable ways of communicating with a server that
supports opportunistic TLS: either you enable TLS as soon as possible
after opening the connection (and then you should do any state
modification after the STARTTLS command), or you don't do any STARTTLS
at all, ever (and then all state settings are done in the unencrypted
connection). Anything else seems like a silly idea.

As such, I think trying to support ways in which you configure things
before STARTTLS, then do STARTTLS, and then expect things to retain
state, is bound to invite security issues, and we should not even try to
support it.

-- 
     w at uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}