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/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).
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é <phi...@linaro.org>
