[Qemu-devel] [PATCH] qemu-nbd: Ignore SIGPIPE

[Qemu-devel] [PATCH] qemu-nbd: Ignore SIGPIPE

[Max Reitz]

qemu proper has done so for 13 years
(8a7ddc38a60648257dc0645ab4a05b33d6040063), qemu-img and qemu-io have
done so for four years (526eda14a68d5b3596be715505289b541288ef2a).
Ignoring this signal is especially important in qemu-nbd because
otherwise a client can easily take down the qemu-nbd server by dropping
the connection when the server wants to send something, for example:

$ qemu-nbd -x foo -f raw -t null-co:// &
[1] 12726
$ qemu-io -c quit nbd://localhost/bar
can't open device nbd://localhost/bar: No export with name 'bar' available
[1]  + 12726 broken pipe  qemu-nbd -x foo -f raw -t null-co://

In this case, the client sends an NBD_OPT_ABORT and closes the
connection (because it is not required to wait for a reply), but the
server replies with an NBD_REP_ACK (because it is required to reply).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
I tried to find some other reproducer instead of using a qemu client
(e.g. nping -c 1 --tcp-connect localhost -p 10809, which gives the same
pattern of <FIN-ACK, >PSH-ACK, <RST as qemu-io) but to no avail. This
only results in the write being successful and the next read failing;
interestingly, sendmsg()'s man page states that EPIPE is only returned
when the local end is closed. However, I have not found the NBD server
to close that local end anywhere.

In any case, ignoring SIGPIPE is the right thing to do. We get an EPIPE
anyway, and this is fully sufficient to let us know that the connection
is dead.
---
 qemu-nbd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index b7ab86b..b2eeedb 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -580,6 +580,10 @@ int main(int argc, char **argv)
     sa_sigterm.sa_handler = termsig_handler;
     sigaction(SIGTERM, &sa_sigterm, NULL);
 
+#ifdef CONFIG_POSIX
+    signal(SIGPIPE, SIG_IGN);
+#endif
+
     module_call_init(MODULE_INIT_TRACE);
     qcrypto_init(&error_fatal);
 
-- 
2.9.4

Re: [Qemu-devel] [PATCH] qemu-nbd: Ignore SIGPIPE

[Paolo Bonzini]

On 11/06/2017 14:37, Max Reitz wrote:
> qemu proper has done so for 13 years
> (8a7ddc38a60648257dc0645ab4a05b33d6040063), qemu-img and qemu-io have
> done so for four years (526eda14a68d5b3596be715505289b541288ef2a).
> Ignoring this signal is especially important in qemu-nbd because
> otherwise a client can easily take down the qemu-nbd server by dropping
> the connection when the server wants to send something, for example:
> 
> $ qemu-nbd -x foo -f raw -t null-co:// &
> [1] 12726
> $ qemu-io -c quit nbd://localhost/bar
> can't open device nbd://localhost/bar: No export with name 'bar' available
> [1]  + 12726 broken pipe  qemu-nbd -x foo -f raw -t null-co://
> 
> In this case, the client sends an NBD_OPT_ABORT and closes the
> connection (because it is not required to wait for a reply), but the
> server replies with an NBD_REP_ACK (because it is required to reply).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> I tried to find some other reproducer instead of using a qemu client
> (e.g. nping -c 1 --tcp-connect localhost -p 10809, which gives the same
> pattern of <FIN-ACK, >PSH-ACK, <RST as qemu-io) but to no avail. This
> only results in the write being successful and the next read failing;
> interestingly, sendmsg()'s man page states that EPIPE is only returned
> when the local end is closed. However, I have not found the NBD server
> to close that local end anywhere.
> 
> In any case, ignoring SIGPIPE is the right thing to do. We get an EPIPE
> anyway, and this is fully sufficient to let us know that the connection
> is dead.
> ---
>  qemu-nbd.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index b7ab86b..b2eeedb 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -580,6 +580,10 @@ int main(int argc, char **argv)
>      sa_sigterm.sa_handler = termsig_handler;
>      sigaction(SIGTERM, &sa_sigterm, NULL);
>  
> +#ifdef CONFIG_POSIX
> +    signal(SIGPIPE, SIG_IGN);
> +#endif
> +
>      module_call_init(MODULE_INIT_TRACE);
>      qcrypto_init(&error_fatal);
>  
> 

Queued, thanks.

Re: [Qemu-devel] [PATCH] qemu-nbd: Ignore SIGPIPE

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1743 bytes --]

On Sun, Jun 11, 2017 at 02:37:14PM +0200, Max Reitz wrote:
> qemu proper has done so for 13 years
> (8a7ddc38a60648257dc0645ab4a05b33d6040063), qemu-img and qemu-io have
> done so for four years (526eda14a68d5b3596be715505289b541288ef2a).
> Ignoring this signal is especially important in qemu-nbd because
> otherwise a client can easily take down the qemu-nbd server by dropping
> the connection when the server wants to send something, for example:
> 
> $ qemu-nbd -x foo -f raw -t null-co:// &
> [1] 12726
> $ qemu-io -c quit nbd://localhost/bar
> can't open device nbd://localhost/bar: No export with name 'bar' available
> [1]  + 12726 broken pipe  qemu-nbd -x foo -f raw -t null-co://
> 
> In this case, the client sends an NBD_OPT_ABORT and closes the
> connection (because it is not required to wait for a reply), but the
> server replies with an NBD_REP_ACK (because it is required to reply).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> I tried to find some other reproducer instead of using a qemu client
> (e.g. nping -c 1 --tcp-connect localhost -p 10809, which gives the same
> pattern of <FIN-ACK, >PSH-ACK, <RST as qemu-io) but to no avail. This
> only results in the write being successful and the next read failing;
> interestingly, sendmsg()'s man page states that EPIPE is only returned
> when the local end is closed. However, I have not found the NBD server
> to close that local end anywhere.
> 
> In any case, ignoring SIGPIPE is the right thing to do. We get an EPIPE
> anyway, and this is fully sufficient to let us know that the connection
> is dead.
> ---
>  qemu-nbd.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] qemu-nbd: Ignore SIGPIPE

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

On 06/11/2017 07:37 AM, Max Reitz wrote:
> qemu proper has done so for 13 years
> (8a7ddc38a60648257dc0645ab4a05b33d6040063), qemu-img and qemu-io have
> done so for four years (526eda14a68d5b3596be715505289b541288ef2a).
> Ignoring this signal is especially important in qemu-nbd because
> otherwise a client can easily take down the qemu-nbd server by dropping
> the connection when the server wants to send something, for example:
> 
> $ qemu-nbd -x foo -f raw -t null-co:// &
> [1] 12726
> $ qemu-io -c quit nbd://localhost/bar
> can't open device nbd://localhost/bar: No export with name 'bar' available
> [1]  + 12726 broken pipe  qemu-nbd -x foo -f raw -t null-co://
> 
> In this case, the client sends an NBD_OPT_ABORT and closes the
> connection (because it is not required to wait for a reply), but the
> server replies with an NBD_REP_ACK (because it is required to reply).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

As mentioned in another thread, I'm trying to figure out if this patch
belongs as a third patch to fix CVE-2017-9524, or whether we want to
open a second CVE by considering this a slightly different
denial-of-service attack than what my patches fixed.

At any rate, this is already merged, so it's too late for me to add my
R-b, although your analysis was spot-on correct ;)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] qemu-nbd: Ignore SIGPIPE

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1782 bytes --]

On 2017-06-27 19:09, Eric Blake wrote:
> On 06/11/2017 07:37 AM, Max Reitz wrote:
>> qemu proper has done so for 13 years
>> (8a7ddc38a60648257dc0645ab4a05b33d6040063), qemu-img and qemu-io have
>> done so for four years (526eda14a68d5b3596be715505289b541288ef2a).
>> Ignoring this signal is especially important in qemu-nbd because
>> otherwise a client can easily take down the qemu-nbd server by dropping
>> the connection when the server wants to send something, for example:
>>
>> $ qemu-nbd -x foo -f raw -t null-co:// &
>> [1] 12726
>> $ qemu-io -c quit nbd://localhost/bar
>> can't open device nbd://localhost/bar: No export with name 'bar' available
>> [1]  + 12726 broken pipe  qemu-nbd -x foo -f raw -t null-co://
>>
>> In this case, the client sends an NBD_OPT_ABORT and closes the
>> connection (because it is not required to wait for a reply), but the
>> server replies with an NBD_REP_ACK (because it is required to reply).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
> 
> As mentioned in another thread, I'm trying to figure out if this patch
> belongs as a third patch to fix CVE-2017-9524, or whether we want to
> open a second CVE by considering this a slightly different
> denial-of-service attack than what my patches fixed.

I think nobody would rip our heads off if we added it to it... I think
it's similar in the regard that the NBD server tries to send something
to a client that is no longer there, so it crashes (aborting in the
original case, due to SIGPIPE here).

But strictly speaking it's a different issue, even from the user's
perspective: In the original case you kill the server using nmap, here
you do so using a real NBD client. Hm, not sure, how hard is it to
assign a new CVE? O:-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH] qemu-nbd: Ignore SIGPIPE

[Daniel P. Berrange]

On Wed, Jun 28, 2017 at 04:27:00PM +0200, Max Reitz wrote:
> On 2017-06-27 19:09, Eric Blake wrote:
> > On 06/11/2017 07:37 AM, Max Reitz wrote:
> >> qemu proper has done so for 13 years
> >> (8a7ddc38a60648257dc0645ab4a05b33d6040063), qemu-img and qemu-io have
> >> done so for four years (526eda14a68d5b3596be715505289b541288ef2a).
> >> Ignoring this signal is especially important in qemu-nbd because
> >> otherwise a client can easily take down the qemu-nbd server by dropping
> >> the connection when the server wants to send something, for example:
> >>
> >> $ qemu-nbd -x foo -f raw -t null-co:// &
> >> [1] 12726
> >> $ qemu-io -c quit nbd://localhost/bar
> >> can't open device nbd://localhost/bar: No export with name 'bar' available
> >> [1]  + 12726 broken pipe  qemu-nbd -x foo -f raw -t null-co://
> >>
> >> In this case, the client sends an NBD_OPT_ABORT and closes the
> >> connection (because it is not required to wait for a reply), but the
> >> server replies with an NBD_REP_ACK (because it is required to reply).
> >>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >> ---
> > 
> > As mentioned in another thread, I'm trying to figure out if this patch
> > belongs as a third patch to fix CVE-2017-9524, or whether we want to
> > open a second CVE by considering this a slightly different
> > denial-of-service attack than what my patches fixed.
> 
> I think nobody would rip our heads off if we added it to it... I think
> it's similar in the regard that the NBD server tries to send something
> to a client that is no longer there, so it crashes (aborting in the
> original case, due to SIGPIPE here).
> 
> But strictly speaking it's a different issue, even from the user's
> perspective: In the original case you kill the server using nmap, here
> you do so using a real NBD client. Hm, not sure, how hard is it to
> assign a new CVE? O:-)

Have we issued a patch for CVE-2017-9524 yet ?  If so, then we *must*
request a new CVE, because vendors will need to track it as an
additional fix to backport & ship, if they've already shipped the
previous fix.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] qemu-nbd: Ignore SIGPIPE

[P J P]

+-- On Tue, 27 Jun 2017, Eric Blake wrote --+
| > $ qemu-nbd -x foo -f raw -t null-co:// &
| > [1] 12726
| > $ qemu-io -c quit nbd://localhost/bar
| > can't open device nbd://localhost/bar: No export with name 'bar' available
| > [1]  + 12726 broken pipe  qemu-nbd -x foo -f raw -t null-co://
| > 
| > In this case, the client sends an NBD_OPT_ABORT and closes the
| > connection (because it is not required to wait for a reply), but the
| > server replies with an NBD_REP_ACK (because it is required to reply).
| > 
| > Signed-off-by: Max Reitz <mreitz@redhat.com>
| > ---
| 
| As mentioned in another thread, I'm trying to figure out if this patch
| belongs as a third patch to fix CVE-2017-9524, or whether we want to
| open a second CVE by considering this a slightly different
| denial-of-service attack than what my patches fixed.

  Yes, this would be a separate issue, as it breaks 'qemu-nbd' server 
irrespective of whether 'CVE-2017-9524' fix is applied or not.

I'll get a CVE assigned and process it further.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F