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

[Daniel P . Berrangé]

On Wed, Aug 18, 2021 at 11:02:48AM -0500, Eric Blake wrote:
> On Mon, Aug 16, 2021 at 05:25:02PM +0200, Wouter Verhelst wrote:
> > > As a followup, I got this reply from Hanno Böck on oss-security:
> > > 
> > > https://www.openwall.com/lists/oss-security/2021/08/11/8
> > > | The buffering vulnerabilities we found are in STARTTLS implementations
> > > | that have the expectation to enforce a secure connection, but suffer
> > > | from various vulnerabilities in the implementation.
> > > 
> > > One of the reasons that SMTP and IMAP were particularly problematic in
> > > implementations is that they are line-based protocols, with
> > > arbitrary-sized packets where the length isn't known until the \n is
> > > reached.  Both clients and servers have to choose whether to read one
> > > character at a time (painfully slow) or read ahead into a buffer and
> > > then processing what is in the buffer.  Many of the CVEs raised were
> > > in regards to mishandling such buffers, such as acting on
> > > previously-buffered plaintext even after the switch to encryption.
> > > 
> > > Thankfully, the NBD protocol has a much more structured handshake
> > > (while different NBD_OPT commands can have different payload lenghts,
> > > at least the header of each packet designates the length in advance,
> > > reducing the need for read-ahead buffering), as well as documentation
> > > that the NBD_OPT_ phase is a lockstep algorithm (neither client nor
> > > server should be building up a buffer of more than one
> > > command/response).
> > > 
> > > Another aspect of the SMTP/IMAP security holes came from incorrectly
> > > carrying state across the transition to encryption (making a false
> > > assumption that the answer made in plaintext will be the same when
> > > encrypted).  Unfortunately, I have realized that the NBD spec has one
> > > bit of state (NBD_OPT_SET_META_CONTEXT) where it is currently silent
> > > on how to handle that state across a transition to encrypted.  So I
> > > plan on posting a followup patch that matches the actual
> > > implementation of nbdkit in opportunistic mode (qemu-nbd does not
> > > offer opportunistic mode, and nbd-server does not suport
> > > NBD_OPT_SET_META_CONTEXT): a server in opportunistic mode MUST treat
> > > the NBD_OPT_STARTTLS command as wiping out any earlier
> > > NBD_OPT_SET_META_CONTEXT.
> > 
> > This all makes sense, thanks.
> 
> 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?

Yes, I think we should.

The 'flag' bits are lightweight as they're simple bitmaskes, not adding
any roundtrips, as a NBD_OPT would likely require. In the unlikely event
that we came up with a use case where we can't accept the overhead of
many extra NBD_OPTs, we could add flags re-verification post-STARTTLS.
I don't tink it is worth doing that today though. The simple approach
is to just document our future intents in a simple way like

  "No additional capability flags will be defined in the NBD protocol
   since this phase is susceptible to MITM downgrade attacks when using
   TLS. Future features will be negotiated using protocol options."

and then document the impact of MITM on the current two pre-existing
flags.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|