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); } err: -- 2.45.2
