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). > --- > 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 <rjo...@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
