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@uter.{be,co.za} wouter@{grep.be,fosdem.org,debian.org} _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs