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(a)uter.{be,co.za}
wouter(a){grep.be,fosdem.org,debian.org}