Re: [Nbd] NBD_CMD_DISC

["Daniel P. Berrange" <berrange@...696...>]

On Sun, Apr 10, 2016 at 10:49:00AM +0100, Alex Bligh wrote:
> Wouter,
> 
> (Daniel added to CC list)
> 
> Having done some research, I'm going to cut this argument short and go
> straight to the TLS standard:
> 
> RFC5246 s7.2.1:
> 
>    "The client and the server must share knowledge that the connection is
>    ending in order to avoid a truncation attack.
> 
>    ...
> 
>    Either party may initiate a close by sending a close_notify alert.
>    Any data received after a closure alert is ignored.
> 
>    Unless some other fatal alert has been transmitted, each party is
>    required to send a close_notify alert before closing the write side
>    of the connection.  The other party MUST respond with a close_notify
>    alert of its own and close down the connection immediately,
>    discarding any pending writes.  It is not required for the initiator
>    of the close to wait for the responding close_notify alert before
>    closing the read side of the connection."
> 
> The way OUR standard currently reads is incompatible with that.
> It requires on NBD_CMD_DISC that the server simply drop the connection.
> That requires it breach RFC5246, and in my view that isn't acceptable.
> We should require the end terminating the connection shuts the
> TLS session down properly as opposed to requiring it to disconnect.
> 
> Qemu server side is in breach of RFC5426 precisely because it follows
> our standard.

IIUC that RFC is saying you need the close notify alert so that you
can distinguish between two different error scenarios - a network
layer error causing the connection to be dropped, vs an active
attacker who forces the connection to be truncated. The RFC also
says that it is not required to wait for the response to the close
alert. It only need wait if it plans to take some differing action
on the two error scenarios, which QEMU doesn't really care about.

> Qemu client side, is *also* in breach of RFC5246 because it's not shutting
> down its side of the connection properly. This is caused by two problems.
> Firstly, it's not calling gnutls_bye(). I suspect that may be
> for fear it might block. Secondly, it's needs to wait until
> not only it's internal coroutine write buffer is drained, but also
> any internal-to-tls buffer is drained (e.g. containing the close
> message if gnutls_bye() does not block); I'm not quite sure how
> one does that, and fixing an identical issue with a proprietary
> openssl-based proxy took me a good while.

gnutls_bye should return GNUTLS_E_AGAIN on blocking and
require that you call it again.

QEMU should absolutely ensure all pending buffers are flushed
before it drops the connection, but I'm not sure it is needed
to actually call gnutls_bye in order to achieve this, because
I don't believe gnutls should be caching any outgoing buffers
we're sending, since we don't use gnutls_cork.

> Contrary to what you've said, this isn't 'basic programming'.
> It would be basic programming to ensure your output is flushed
> properly if you were in control of the buffering and magic
> bytes and retries didn't appear from elsewhere. It's hard
> to get right. Sufficiently hard that the GnuTLS maintainer
> said "A select loop will be complex and I don't know if one
> could have a reasonable example." (I've now written one for
> him). The choices we are making are making it even harder
> to get right. If you want it that way, then fine, I'll give
> up arguing. But we should not be making it impossible to get right
> by mandating we break a standard.
> 
> (Daniel: if you want to replicate the issue, just run qemu-img info
> against my gonbdserver with TLS. Every fifth NBD_CMD_DISC doesn't
> get through before the TCP session closes).

Hmm, I'll have a look at this - I'm not sure its caused by
the lack of gnutls_bye, as opposed to incorrect handling
of EAGAIN when the block layer sends CMD_DISC

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 :|