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/gues...@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" <rjo...@redhat.com>
> > Signed-off-by: Eric Blake <ebl...@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é <berra...@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 :|
