| Message ID | 20240802194156.2131519-6-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | NBD: don't print raw server error text to terminal | expand |
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>
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 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.
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.
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 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
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:
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(-)