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

[Wouter Verhelst]

On Mon, Aug 16, 2021 at 01:02:55PM -0500, Eric Blake wrote:
> On Mon, Aug 16, 2021 at 05:31:10PM +0200, Wouter Verhelst wrote:
> 
> > > +++ 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.
> 
> Concur.
> 
> > 
> > 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.
> 
> Okay, how about this wording:
> 
> +When this command succeeds, the server MUST NOT preserve any
> +negotiation state (such as a request for `NBD_OPT_STRUCTURED_REPLY`,
> +or metadata contexts from `NBD_OPT_SET_META_CONTEXT`) issued before
> +this command.  A client SHOULD defer all stateful option requests
> +until after it determines whether encryption is available.

Yeah, that seems better.

> Unfortunately, nbdkit in opportunistic mode does not (yet) obey that:
> a request for structured replies prior to starttls is currently
> preserved across the jump into encryption - which may result in the
> server sending structured replies to a client not expecting them due
> to a MitM plaintext injection.  I'll be submitting a patch for that
> soon; sounds like we found a CVE in nbdkit.
> 
> On the other hand, qemu as NBD client always sends NBD_OPT_STARTTLS
> first (disconnecting rather than falling back to plaintext).  libnbd
> as NBD client does not (yet) expose any way to attempt starttls after
> other negotiation commands: in opportunistic mode, it currently probes
> for encryption before giving the user any control over other NBD_OPT_
> commands.  At one point, I had the idea of expanding the libnbd API to
> make it easier to try even NBD_OPT_STARTTLS under user control (which
> would make it easier to test issues like how servers behave with
> clients that don't lead off with STARTTLS), but that has not been
> implemented yet, so at the moment, we don't have clients in the wild
> that were relying on nbdkit's stateful behavior.

OK, that's great -- otherwise this would have been far more difficult to
fix.

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