[Libguestfs] [PATCH] docs: Link to protocol security considerations in uri docs

[Wouter Verhelst]

Hi Eric,

On Wed, Aug 18, 2021 at 11:02:48AM -0500, Eric Blake wrote:
> Dan Berrangé and I thought about some more potential future problems:
> right now, even with FORCEDTLS mode (in both client and server), we
> have NO way to validate that the initial NBD_FLAG_[C_] bits advertised
> between client and server were not tampered with by a MitM during the
> plaintext portion of the exchange.  The flags field is 16 bits sent
> from the server, and 16 bits mirrored back by the client, to determine
> which protocol features will be in use the remainder of the
> connection.  Right now, exactly two of those bits are defined:
> 
> NBD_FLAG_FIXED_NEWSTYLE - the spec is clear that NBD_OPT_STARTTLS
> should not be sent unless this bit was negotiated.  Thus, both client
> and server will be sending the bit set, and absence of the bit will be
> evidence of a MitM attempting a downgrade attack, and there's nothing
> further to worry about in the protocol.  Once STARTTLS is executed, we
> already know this capability was available, so we don't need a way to
> re-verify it while encrypted.
> 
> NBD_FLAG_NO_ZEROES - this bit controls how the server will respond to
> NBD_OPT_EXPORT_NAME.  A mismatch between this bit (whether the MitM
> strips or adds the bit) will only be observable if the client uses
> NBD_OPT_EXPORT_NAME, but all clients that use STARTTLS are already
> encouraged to use NBD_OPT_GO.  We may want to tighten the security
> portion of the spec to make this recommendation even stronger (that
> is, any client that wants to ensure there is no MitM downgrade attack
> MUST use NBD_OPT_GO rather than NBD_OPT_EXPORT_NAME; and all servers
> that support TLS MUST support NBD_OPT_GO), but once a client uses the
> modern export selection code, we no longer care about mismatches in
> this bit, and therefore we don't need a way to re-verify it while
> encrypted.
> 
> However, I'm also worried about future extensions.  For example, we
> have been considering the addition of a way for clients to make 64-bit
> requests (basically, allowing NBD_OPT_WRITE_ZEROES to become a
> single-transaction bulk-zero for an export larger than 4G).  If the
> way this extension is haggled between client and server utilizes only
> a new NBD_FLAG_*, then we have introduced a potential security hole,
> because we are back in the situation of having a MitM flip bits prior
> to STARTTLS so that client and server do not agree on which protocol
> changes to use.  We can avoid this by adding a way to validate which
> capability bits are actually common between client and server via a
> new NBD_OPT_ sent after STARTTLS.  But rather than needing a way to
> re-verify which flags are common, it is just as easy to merely declare
> that ALL future protocol extensions will be haggled via NBD_OPT_ in
> the first place (and not by adding new NBD_FLAG_ bits).  That way,
> there will be no further places in the NBD protocol where a MitM
> plaintext injection can interfere with what the client and server use
> to talk to one another post-encryption.
> 
> Is it worth formalizing this decision into the NBD spec, so that we
> remember down the road that adding new NBD_FLAG_ bits is an inherent
> security risk thanks to the presence of STARTTLS?

I see the flags field as a way to negotiate incompatible changes *during
the negotiation phase*. This is true for both current flags:
FIXED_NEWSTYLE is supposed to fix the newstyle negotiation, and
NO_ZEROES changes the format of the reply to the EXPORT_NAME option,
which if used is the final message exchange during the negotiation
phase. Other protocol differences are negotiated with options or with
per-export flags (which are sent in the TLS part of the connection).

The only situation that I can think of in which we would need a new flag
is if for some reason we had to change the message format of the
NBD_OPT_* messages.  I don't see this happening. Even if we did have to
change that format, there are 2^32 option numbers available, which means
they are effectively unlimited; so if for whatever reason we had a
situation where the NBD_OPT_* message format is not flexible enough, we
could use a new option to signal "enable the new message format", which
would be written in the "old" format; if that new option number returns
NBD_REP_ERR_UNSUP, then we know the new message format is not supported
and we fall back to the old message format. That's slightly less
efficient than just setting a flag, but then who cares about a few bytes
in a protocol like NBD, which is expected to transfer large amounts of
data down the line.

I think we can indeed decide that we won't add any global flags anymore.

Thanks,

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