Error messages from an NBD server must be treated as untrusted; a
malicious server can inject escape sequences to try and trigger RCE
flaws via escape sequences to whatever terminal happens to be running
qemu-img. The easiest solution is to sanitize the output with the
same code we use to produce sanitized (pseudo-)JSON over QMP.
Rich Jones originally pointed this flaw out at:
https://lists.libguestfs.org/archives/list/guestfs@lists.libguestfs.org/thread/2NXA23G2V3HPWJYAO726PLNBEAAEUJAU/
With this patch, and a malicious server run with nbdkit 1.40 as:
$ nbdkit --log=null eval open=' printf \
"EPERM x\\r mess up the output \e[31mmess up the output\e[m mess up" >&2; \
exit 1 ' get_size=' echo 0 ' --run 'qemu-img info "$uri"'
we now get:
qemu-img: Could not open 'nbd://localhost': Requested export not available
server reported: /tmp/nbdkitOZHOKB/open: x\r mess up the output \u001B[31mmess up the output\u001B[m mess up
instead of an attempt to hide the name of the Unix socket and forcing
the terminal to render part of the text red.
Note that I did _not_ sanitize the string being sent through
trace-events in trace_nbd_server_error_msg; this is because I assume
that our trace engines already treat all string strings as untrusted
input and apply their own escaping as needed.
Reported-by: "Richard W.M. Jones" <rjones@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
If my assumption about allowing raw escape bytes through to trace_
calls is wrong (such as when tracing to stderr), let me know. That's
a much bigger audit to determine which trace points, if any, should
sanitize data before tracing, and/or change the trace engines to
sanitize all strings (with possible knock-on effects if trace output
changes unexpectedly for a tool expecting something unsanitized).
---
nbd/client.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/nbd/client.c b/nbd/client.c
index c89c7504673..baa20d10d69 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -23,6 +23,7 @@
#include "trace.h"
#include "nbd-internal.h"
#include "qemu/cutils.h"
+#include "qemu/unicode.h"
/* Definitions for opaque data types */
@@ -230,7 +231,9 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
}
if (msg) {
- error_append_hint(errp, "server reported: %s\n", msg);
+ g_autoptr(GString) buf = g_string_sized_new(reply->length);
+ mod_utf8_sanitize(buf, msg);
+ error_append_hint(errp, "server reported: %s\n", buf->str);
}
err:
--
2.45.2On Fri, Aug 02, 2024 at 02:26:06PM -0500, Eric Blake wrote: > Error messages from an NBD server must be treated as untrusted; a > malicious server can inject escape sequences to try and trigger RCE > flaws via escape sequences to whatever terminal happens to be running > qemu-img. This presentation is relevant: https://dgl.cx/2023/09/ansi-terminal-security Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On Fri, Aug 02, 2024 at 11:01:36PM +0100, Richard W.M. Jones wrote: > On Fri, Aug 02, 2024 at 02:26:06PM -0500, Eric Blake wrote: > > Error messages from an NBD server must be treated as untrusted; a > > malicious server can inject escape sequences to try and trigger RCE > > flaws via escape sequences to whatever terminal happens to be running > > qemu-img. > > This presentation is relevant: > > https://dgl.cx/2023/09/ansi-terminal-security This took way too long, but ... $ wget http://oirase.annexia.org/tmp/nyan.c $ nbdkit --log=null cc /tmp/nyan.c --run 'qemu-img info "$uri"' Needs nbdkit >= 1.40, and don't worry, it doesn't exploit the terminal except for silly internet memes. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 2/8/24 21:26, Eric Blake wrote: > Error messages from an NBD server must be treated as untrusted; a > malicious server can inject escape sequences to try and trigger RCE > flaws via escape sequences to whatever terminal happens to be running > qemu-img. The easiest solution is to sanitize the output with the > same code we use to produce sanitized (pseudo-)JSON over QMP. > > Rich Jones originally pointed this flaw out at: > https://lists.libguestfs.org/archives/list/guestfs@lists.libguestfs.org/thread/2NXA23G2V3HPWJYAO726PLNBEAAEUJAU/ > > With this patch, and a malicious server run with nbdkit 1.40 as: > > $ nbdkit --log=null eval open=' printf \ > "EPERM x\\r mess up the output \e[31mmess up the output\e[m mess up" >&2; \ > exit 1 ' get_size=' echo 0 ' --run 'qemu-img info "$uri"' > > we now get: > > qemu-img: Could not open 'nbd://localhost': Requested export not available > server reported: /tmp/nbdkitOZHOKB/open: x\r mess up the output \u001B[31mmess up the output\u001B[m mess up > > instead of an attempt to hide the name of the Unix socket and forcing > the terminal to render part of the text red. > > Note that I did _not_ sanitize the string being sent through > trace-events in trace_nbd_server_error_msg; this is because I assume > that our trace engines already treat all string strings as untrusted > input and apply their own escaping as needed. > > Reported-by: "Richard W.M. Jones" <rjones@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > > If my assumption about allowing raw escape bytes through to trace_ > calls is wrong (such as when tracing to stderr), let me know. That's > a much bigger audit to determine which trace points, if any, should > sanitize data before tracing, and/or change the trace engines to > sanitize all strings (with possible knock-on effects if trace output > changes unexpectedly for a tool expecting something unsanitized). I doubt the trace core layer sanitizes, but it feels it is the trace backend responsibility, since core layer might just pass pointer to the backends. > --- > nbd/client.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Fri, Aug 02, 2024 at 11:41:35PM +0200, Philippe Mathieu-Daudé wrote: > On 2/8/24 21:26, Eric Blake wrote: > > Error messages from an NBD server must be treated as untrusted; a > > malicious server can inject escape sequences to try and trigger RCE > > flaws via escape sequences to whatever terminal happens to be running > > qemu-img. The easiest solution is to sanitize the output with the > > same code we use to produce sanitized (pseudo-)JSON over QMP. > > > > Rich Jones originally pointed this flaw out at: > > https://lists.libguestfs.org/archives/list/guestfs@lists.libguestfs.org/thread/2NXA23G2V3HPWJYAO726PLNBEAAEUJAU/ > > > > With this patch, and a malicious server run with nbdkit 1.40 as: > > > > $ nbdkit --log=null eval open=' printf \ > > "EPERM x\\r mess up the output \e[31mmess up the output\e[m mess up" >&2; \ > > exit 1 ' get_size=' echo 0 ' --run 'qemu-img info "$uri"' > > > > we now get: > > > > qemu-img: Could not open 'nbd://localhost': Requested export not available > > server reported: /tmp/nbdkitOZHOKB/open: x\r mess up the output \u001B[31mmess up the output\u001B[m mess up > > > > instead of an attempt to hide the name of the Unix socket and forcing > > the terminal to render part of the text red. > > > > Note that I did _not_ sanitize the string being sent through > > trace-events in trace_nbd_server_error_msg; this is because I assume > > that our trace engines already treat all string strings as untrusted > > input and apply their own escaping as needed. > > > > Reported-by: "Richard W.M. Jones" <rjones@redhat.com> > > Signed-off-by: Eric Blake <eblake@redhat.com> > > > > --- > > > > If my assumption about allowing raw escape bytes through to trace_ > > calls is wrong (such as when tracing to stderr), let me know. That's > > a much bigger audit to determine which trace points, if any, should > > sanitize data before tracing, and/or change the trace engines to > > sanitize all strings (with possible knock-on effects if trace output > > changes unexpectedly for a tool expecting something unsanitized). > > I doubt the trace core layer sanitizes, but it feels it is the > trace backend responsibility, since core layer might just pass > pointer to the backends. Yes, strings are not escaped by the core tracing code. They probably should not be escaped by QEMU since the trace backend may wish to process the strings and it expects to operate on raw strings. > > > --- > > nbd/client.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >
On Fri, Aug 02, 2024 at 02:26:06PM -0500, Eric Blake wrote:
> Error messages from an NBD server must be treated as untrusted; a
> malicious server can inject escape sequences to try and trigger RCE
> flaws via escape sequences to whatever terminal happens to be running
> qemu-img. The easiest solution is to sanitize the output with the
> same code we use to produce sanitized (pseudo-)JSON over QMP.
>
> Rich Jones originally pointed this flaw out at:
> https://lists.libguestfs.org/archives/list/guestfs@lists.libguestfs.org/thread/2NXA23G2V3HPWJYAO726PLNBEAAEUJAU/
>
> With this patch, and a malicious server run with nbdkit 1.40 as:
>
> $ nbdkit --log=null eval open=' printf \
> "EPERM x\\r mess up the output \e[31mmess up the output\e[m mess up" >&2; \
> exit 1 ' get_size=' echo 0 ' --run 'qemu-img info "$uri"'
>
> we now get:
>
> qemu-img: Could not open 'nbd://localhost': Requested export not available
> server reported: /tmp/nbdkitOZHOKB/open: x\r mess up the output \u001B[31mmess up the output\u001B[m mess up
>
> instead of an attempt to hide the name of the Unix socket and forcing
> the terminal to render part of the text red.
>
> Note that I did _not_ sanitize the string being sent through
> trace-events in trace_nbd_server_error_msg; this is because I assume
> that our trace engines already treat all string strings as untrusted
> input and apply their own escaping as needed.
>
> Reported-by: "Richard W.M. Jones" <rjones@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
>
> If my assumption about allowing raw escape bytes through to trace_
> calls is wrong (such as when tracing to stderr), let me know. That's
> a much bigger audit to determine which trace points, if any, should
> sanitize data before tracing, and/or change the trace engines to
> sanitize all strings (with possible knock-on effects if trace output
> changes unexpectedly for a tool expecting something unsanitized).
> ---
> nbd/client.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index c89c7504673..baa20d10d69 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -23,6 +23,7 @@
> #include "trace.h"
> #include "nbd-internal.h"
> #include "qemu/cutils.h"
> +#include "qemu/unicode.h"
>
> /* Definitions for opaque data types */
>
> @@ -230,7 +231,9 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> }
>
> if (msg) {
> - error_append_hint(errp, "server reported: %s\n", msg);
> + g_autoptr(GString) buf = g_string_sized_new(reply->length);
> + mod_utf8_sanitize(buf, msg);
> + error_append_hint(errp, "server reported: %s\n", buf->str);
> }
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
On Fri, Aug 02, 2024 at 10:03:05PM +0100, Richard W.M. Jones wrote:
> On Fri, Aug 02, 2024 at 02:26:06PM -0500, Eric Blake wrote:
> > Error messages from an NBD server must be treated as untrusted; a
> > malicious server can inject escape sequences to try and trigger RCE
> > flaws via escape sequences to whatever terminal happens to be running
> > qemu-img. The easiest solution is to sanitize the output with the
> > same code we use to produce sanitized (pseudo-)JSON over QMP.
> >
> > Rich Jones originally pointed this flaw out at:
> > https://lists.libguestfs.org/archives/list/guestfs@lists.libguestfs.org/thread/2NXA23G2V3HPWJYAO726PLNBEAAEUJAU/
> >
> > With this patch, and a malicious server run with nbdkit 1.40 as:
> >
> > $ nbdkit --log=null eval open=' printf \
> > "EPERM x\\r mess up the output \e[31mmess up the output\e[m mess up" >&2; \
> > exit 1 ' get_size=' echo 0 ' --run 'qemu-img info "$uri"'
> >
> > we now get:
> >
> > qemu-img: Could not open 'nbd://localhost': Requested export not available
> > server reported: /tmp/nbdkitOZHOKB/open: x\r mess up the output \u001B[31mmess up the output\u001B[m mess up
> >
> > instead of an attempt to hide the name of the Unix socket and forcing
> > the terminal to render part of the text red.
> >
> > Note that I did _not_ sanitize the string being sent through
> > trace-events in trace_nbd_server_error_msg; this is because I assume
> > that our trace engines already treat all string strings as untrusted
> > input and apply their own escaping as needed.
> >
> > Reported-by: "Richard W.M. Jones" <rjones@redhat.com>
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> >
> > ---
> >
> > If my assumption about allowing raw escape bytes through to trace_
> > calls is wrong (such as when tracing to stderr), let me know. That's
> > a much bigger audit to determine which trace points, if any, should
> > sanitize data before tracing, and/or change the trace engines to
> > sanitize all strings (with possible knock-on effects if trace output
> > changes unexpectedly for a tool expecting something unsanitized).
For nearly all trace backends, QEMU is not emitting anything onthe
console, so there's no escaping QEMU needs to do.
The exception is the "log" backend which calls qemu_log(). IOW, if
that's a concern then the qemu_log() function needs to sanitize the
resulting buffer after printf, rather than the tracing code do anything.
The simpletrace.py script could probably need similar.
I lean towards "don't bother" though, as when tracing I think it is
important to see the raw un-modified output for the sake of accuracy.
> > nbd/client.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/nbd/client.c b/nbd/client.c
> > index c89c7504673..baa20d10d69 100644
> > --- a/nbd/client.c
> > +++ b/nbd/client.c
> > @@ -23,6 +23,7 @@
> > #include "trace.h"
> > #include "nbd-internal.h"
> > #include "qemu/cutils.h"
> > +#include "qemu/unicode.h"
> >
> > /* Definitions for opaque data types */
> >
> > @@ -230,7 +231,9 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> > }
> >
> > if (msg) {
> > - error_append_hint(errp, "server reported: %s\n", msg);
> > + g_autoptr(GString) buf = g_string_sized_new(reply->length);
> > + mod_utf8_sanitize(buf, msg);
> > + error_append_hint(errp, "server reported: %s\n", buf->str);
> > }
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
With 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 :|
© 2016 - 2024 Red Hat, Inc.