Re: [Qemu-devel] spec, RFC: TLS support for NBD

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Oct 21, 2014 at 12:10:39AM +0200, Wouter Verhelst wrote:
> On Mon, Oct 20, 2014 at 01:56:43PM +0200, Florian Weimer wrote:
> > I cannot comment on whether the proposed STARTTLS command is at the correct
> > stage of the NBD protocol.  If there is a protocol description for NBD, I
> > can have a look.
> 
> To make this discussion in that regard a bit easier, I've committed the
> proposed spec to a separate branch:
> 
> https://github.com/yoe/nbd/blob/tlsspec/doc/proto.txt

So, if I'm understanding correctly the new style "fixed" handshake
currently works like this:

Server starts transmission with version info

  - S: "NBDMAGIC"
  - S: 0x49484156454F5054
  - S: 0x1

And the client acknowledges with a 32 bits of and first bit set

  - C: 0x1


Now the client has to request one or more options, of which the
NBD_OPT_EXPORT_NAME (0x1) option is mandatory, so it will be the
first thing the client sends next . So it would send

  - C: 0x49484156454F5054
  - C: 0x1
  - C: 0xa  (length of name)
  - C: "volumename"



You're proposing to add a new option NBD_OPT_STARTTLS (0x5). For this to
work we would need to update the language to say that the NBD_OPT_STARTTLS
must be the first option that the client sends to the server, because we
want the option name to transmit in ciphertext.

So the new protocol startup would be

Server starts transmission with version info

  - S: "NBDMAGIC"
  - S: 0x49484156454F5054
  - S: 0x1

And the client acknowledges with a 32 bits of zero

  - C: 0x1

Client requests TLS

  - C: 0x49484156454F5054
  - C: 0x5

Server acknowledges TLS:

  - S: 0x3e889045565a9
  - S: 0x5
  - S: 0x1 (REP_ACK)
  - S: 0x0

Now the TLS handshake is performed by client + server



The client can now set other options using the secure
channel, of which the NBD_OPT_EXPORT_NAME (0x1) option
is mandatory, so it will be the first thing the client
sends next . So they would send

  - C: "0x49484156454F5054"
  - C: 0x1
  - C: 0xa  (length of name)
  - C: "volumename"

...etc...


This is secure when both client and server want to use TLS.

This is secure when the client wants TLS and the server does
not want TLS, because the server will reject TLS and the client
will then close the connection.

My concern is the case where the client does not want TLS but
the server does want TLS. In this case the client will immediately
send the NBD_OPT_EXPORT_NAME in clear text over the wire, not giving
the server any chance to reject the use of a clear text channel.

This problem is inherant to the approach of using the NBD protocol
options to negotiate TLS.



One way I see to solve that insecurity, would be for the server
to make use of one of the extra reserved bits in the very first
message it sends. ie we need to negotiate TLS immediately after the
version number / magic acknowledgment, before we actually start
the main body of the protocol

ie, with the new style fixed handshake the server should send

Server starts transmission with version info

  - S: "NBDMAGIC"
  - S: 0x49484156454F5054
  - S: 0x2 

And the client acknowledges with a 32bits and first two bits set

  - C: 0x2

The questionmark here is what happens when the client sees the
second bit of reserved field set ? 

The protocol docs merely say

  "S: 16 bits of zero (bits 1-15 reserved for future use; bit 0 in use to
   signal fixed newstyle (see below))"

But don't mention what clients should do upon seeing unknown bits
set in the server's message ?

If clients ignore unknown bits, then we have the same problem where a
client can ignore the TLS bit and start sending option name in the
clear before the server rejects the session.

If clients abort (close connection) on seeing unknown bits, then we
are good.



A 3rd alternative is to actually use a separate magic number, which
should guarantee the client would immediately close upon seeing a
magic number which indicated TLS is required.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|