Re: [Qemu-devel] [PATCH] nbd: Fix regression on resiliency to port scan

[Eric Blake]

On 06/11/2017 06:50 AM, Max Reitz wrote:
> On 2017-06-09 00:26, Eric Blake wrote:
>> Back in qemu 2.5, qemu-nbd was immune to port probes (a transient
>> server would not quit, regardless of how many probe connections
>> came and went, until a connection actually negotiated).  But we
>> broke that in commit ee7d7aa

>> Simple test across two terminals:
>> $ qemu-nbd -f raw -p 30001 file
> 
> Maybe this command line should contain a -t, because otherwise I don't
> find it very CVE-worthy.

I was on vacation when this patch got merged, so it's too late to modify
the commit message now.  However, I'm following up now; first to point
out that we now have the actual CVE number (it was not yet assigned when
I originally posted - and if we hadn't merged yet, I would also have
tweaked the commit message to call the id out): CVE-2017-9524

> 
> First, sure, you can kill this NBD server with the below nmap
> invocation, or with an "ncat localhost 30001". But you can also do so
> with a plain "qemu-io -c quit nbd://localhost:30001".

But in that case, without -t, you specifically wanted the server to go
away after the first successful client, and that was indeed a successful
client.

> 
> With -t you actually cannot kill it using ncat or qemu-io, but you can
> with nmap. So this is what the actual issue is.

You do make a point, though.  The command line example I gave is the
old-style NBD, where there is no named export, and where any client is
first in line for the given export - which is rather weak, and why
upstream NBD discourages the old protocol in favor of the new where the
client has to specify WHICH named export it wants.  And in the new
protocol, if you are not using -t, then you want the server to ignore
clients that aren't requesting the export that the server is willing to
serve, rather than hanging up on the first random client that requests
the wrong name.  And you properly investigated that scenario below...

> 
> And secondly, note that even after this patch you can make the NBD
> server quit with "ncat localhost 30001" (just like with qemu-io).
> 
>> $ nmap 127.0.0.1 -p 30001 && \
>>   qemu-io -c 'r 0 512' -f raw nbd://localhost:30001
>>
>> Note that this patch does not change what constitutes successful
>> negotiation (thus, a client must enter transmission phase before
>> that client can be considered as a reason to terminate the server
>> when the connection ends).
> 
> Well, the thing is that transmission phase starts immediately for the
> old-style protocol:
> 
> $ ./qemu-nbd -f raw null-co:// &
> [1] 24581
> $ ncat --send-only localhost 10809 < /dev/null
> nbd/server.c:nbd_receive_request():L706: read failed
> [1]  + 24581 done       ./qemu-nbd -f raw null-co://
> 
> I'm not sure whether this is intended, but then again I'm not sure
> whether we can or have to do anything about this (as I said above, you
> can easily make the server quit with a qemu-io invocation if you're so
> inclined.

Old-style negotiation is not as important these days as new-style where
a handshake occurs (in fact, upstream NBD documents but no longer
implements old-style negotiation, so it has definitely moved into
deprecated usage).

> 
> But the thing is that this really shows there should be a -t in the
> qemu-nbd invocation in this commit message. The merit of this patch is
> that it fixes a *crash* in qemu-nbd if someone closes the connection
> immediately after opening it, not that it allows qemu-nbd to continue to
> run when someone connects who is not an NBD client -- because it really
> doesn't.

So yes, your arguments are good that the REAL denial-of-service is not
against a server that expects only one client (or, with new-style, one
client that knows about the specific export), but against a server
running with -t that is supposed to be persistent regardless of how many
clients consecutively connect to it.

> 
> (With the new style, option negotiation fails so we do not go to
> transmission phase and the server stays alive:
> 
> $ ./qemu-nbd -x foo -f raw null-co:// &
> [1] 24609
> $ ncat --send-only localhost 10809 < /dev/null
> nbd/server.c:nbd_negotiate_options():L442: read failed
> nbd/server.c:nbd_negotiate():L673: option negotiation failed
> $ kill %1
> [1]  + 24609 done       ./qemu-nbd -x foo -f raw null-co://
> 
> )
> 
>>                             Perhaps we may want to tweak things
>> in a later patch to also treat a client that uses NBD_OPT_ABORT
>> as being a 'successful' negotiation (the client correctly talked
>> the NBD protocol, and informed us it was not going to use our
>> export after all), but that's a discussion for another day.
> 
> Well, about that...
> 
> $ ./qemu-nbd -x foo -f raw null-co:// &
> [1] 24389
> $ ./qemu-io -c quit nbd://localhost/bar
> can't open device nbd://localhost/bar: No export with name 'bar' available
> [1]  + 24389 broken pipe  ./qemu-nbd -x foo -f raw null-co://
> 
> Even worse, the same happens with -t, both before and after this patch.

You later submitted the fix for this (ignoring SIGPIPE); what I'm now
trying to figure out is whether your later patch is part of the same
CVE-2017-9524 fix, or whether we need a second CVE assigned for this
second denial-of-service scenario.

> 
> Having spent probably too much time for investigating, this happens
> because the client just closes the pipe which results in a SIGPIPE sent
> to qemu-nbd when it tries to sendmsg() (in qio_channel_socket_writev())
> the ACK.
> 
> qemu-io and qemu-img use signal(SIGPIPE, SIG_IGN) (and os-posix.c has a
> sigaction() call for the same). qemu-nbd should probably, too.
> ...Considering you're on vacation next week, maybe I should just write
> that patch.

Thanks for your investigations and followup patch.  Now, the task at
hand is to figure out how important your followup patch is to the CVE
question.

> 
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1451614
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>  include/block/nbd.h |  2 +-
>>  blockdev-nbd.c      |  6 +++++-
>>  nbd/server.c        | 24 +++++++++++++++---------
>>  qemu-nbd.c          |  4 ++--
>>  4 files changed, 23 insertions(+), 13 deletions(-)
> 
> With a -t in the qemu-nbd command line:
> 
> Reviewed-by: Max Reitz <address@hidden>
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature