Re: [PATCH 2/2] qemu-img: CVE-XXX Sanitize untrusted output from NBD server

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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>