[for-9.1,0/2] NBD: don't print raw server error text to terminal

I've requested a CVE from Red Hat, and hope to have an assigned number
soon.  Meanwhile, we can get review started, to make sure this is
ready to include in 9.1.  'qemu-img info' should never print untrusted
data in a way that might take over a user's terminal.

There are probably other spots where qemu-img info is printing
untrusted data (such as filenames), where we probably ought to use the
same sanitization tactics as shown here.  Identifying those spots
would be a useful part of this review, and may mean a v2 that is even
more extensive in the number of patches.

In patch 1, I created mod_utf8_sanitize_len(), with the intent that I
could sanitize just a prefix of a string without having to copy it
into a NUL-terminated buffer.  I didn't end up needing it in my
current version of patch 2 (since the code was already copying to a
NUL-terminated buffer for trace purposes), but we may find uses for
it; in fact, it raises the question of whether any of our trace_ calls
need to sanitize untrusted data (or whether we can rely on ALL trace
engines to be doing that on our behalf, already).

Eric Blake (2):
  util: Refactor json-writer's string sanitizer to be public
  qemu-img: CVE-XXX Sanitize untrusted output from NBD server

 include/qemu/unicode.h |  3 ++
 nbd/client.c           |  5 ++-
 qobject/json-writer.c  | 47 +----------------------
 util/unicode.c         | 84 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 92 insertions(+), 47 deletions(-)

On Fri, Aug 02, 2024 at 02:26:04PM GMT, Eric Blake wrote:
> I've requested a CVE from Red Hat, and hope to have an assigned number
> soon.  Meanwhile, we can get review started, to make sure this is
> ready to include in 9.1.  'qemu-img info' should never print untrusted
> data in a way that might take over a user's terminal.
> 
> There are probably other spots where qemu-img info is printing
> untrusted data (such as filenames), where we probably ought to use the
> same sanitization tactics as shown here.  Identifying those spots
> would be a useful part of this review, and may mean a v2 that is even
> more extensive in the number of patches.

In fact, should we insist that 'qemu-img info XXX' refuse to accept
any control characters on any command-line filename, and that it
reject any backing file name with control characters as a malformed
qcow2 file?  For reference, we JUST fixed qemu-img info to change
qcow2 files with a claimed backing file of json:... to favor the local
file ./json:... over the potentially dangerous user-controlled
format/protocol description, so we are _already_ changing how strict
qemu-img is for 9.1, and adding one more restriction to avoid
escape-sequence madness makes sense.

Note that with:

touch $'\e[m' && qemu-img info --output=json $'\e[m'

we already escape our output, but without --output=json, we are
vulnerable to user-controlled ESC leaking through to stdout for more
than just the NBD server errors that I addressed in v1 of this patch
series.  Hence my question on whether v2 of the series should touch
more places in the code, and whether doing something like flat-out
refusing users stupid enough to embed control characters in their
filenames is a safe change this close to release.

On Mon, Aug 05, 2024 at 01:48:12PM -0500, Eric Blake wrote:
> On Fri, Aug 02, 2024 at 02:26:04PM GMT, Eric Blake wrote:
> > I've requested a CVE from Red Hat, and hope to have an assigned number
> > soon.  Meanwhile, we can get review started, to make sure this is
> > ready to include in 9.1.  'qemu-img info' should never print untrusted
> > data in a way that might take over a user's terminal.
> > 
> > There are probably other spots where qemu-img info is printing
> > untrusted data (such as filenames), where we probably ought to use the
> > same sanitization tactics as shown here.  Identifying those spots
> > would be a useful part of this review, and may mean a v2 that is even
> > more extensive in the number of patches.
> 
> In fact, should we insist that 'qemu-img info XXX' refuse to accept
> any control characters on any command-line filename, and that it
> reject any backing file name with control characters as a malformed
> qcow2 file?  For reference, we JUST fixed qemu-img info to change
> qcow2 files with a claimed backing file of json:... to favor the local
> file ./json:... over the potentially dangerous user-controlled
> format/protocol description, so we are _already_ changing how strict
> qemu-img is for 9.1, and adding one more restriction to avoid
> escape-sequence madness makes sense.
> 
> Note that with:
> 
> touch $'\e[m' && qemu-img info --output=json $'\e[m'
> 
> we already escape our output, but without --output=json, we are
> vulnerable to user-controlled ESC leaking through to stdout for more
> than just the NBD server errors that I addressed in v1 of this patch
> series.  Hence my question on whether v2 of the series should touch
> more places in the code, and whether doing something like flat-out
> refusing users stupid enough to embed control characters in their
> filenames is a safe change this close to release.

You could say if someone gives you a "malicious" text file which you
cat to stdout, it could change your terminal settings.  I don't think
therefore any of this is very serious.  We should probably fix any
obvious things, but it doesn't need to happen right before 9.1 is
released, we can take our time.

Rich.

On Mon, Aug 05, 2024 at 08:11:31PM GMT, Richard W.M. Jones wrote:
> On Mon, Aug 05, 2024 at 01:48:12PM -0500, Eric Blake wrote:
> > On Fri, Aug 02, 2024 at 02:26:04PM GMT, Eric Blake wrote:
> > > I've requested a CVE from Red Hat, and hope to have an assigned number
> > > soon.  Meanwhile, we can get review started, to make sure this is
> > > ready to include in 9.1.  'qemu-img info' should never print untrusted
> > > data in a way that might take over a user's terminal.
> > > 
> > > There are probably other spots where qemu-img info is printing
> > > untrusted data (such as filenames), where we probably ought to use the
> > > same sanitization tactics as shown here.  Identifying those spots
> > > would be a useful part of this review, and may mean a v2 that is even
> > > more extensive in the number of patches.
> > 
> > In fact, should we insist that 'qemu-img info XXX' refuse to accept
> > any control characters on any command-line filename, and that it
> > reject any backing file name with control characters as a malformed
> > qcow2 file?  For reference, we JUST fixed qemu-img info to change
> > qcow2 files with a claimed backing file of json:... to favor the local
> > file ./json:... over the potentially dangerous user-controlled
> > format/protocol description, so we are _already_ changing how strict
> > qemu-img is for 9.1, and adding one more restriction to avoid
> > escape-sequence madness makes sense.
> > 
> > Note that with:
> > 
> > touch $'\e[m' && qemu-img info --output=json $'\e[m'
> > 
> > we already escape our output, but without --output=json, we are
> > vulnerable to user-controlled ESC leaking through to stdout for more
> > than just the NBD server errors that I addressed in v1 of this patch
> > series.  Hence my question on whether v2 of the series should touch
> > more places in the code, and whether doing something like flat-out
> > refusing users stupid enough to embed control characters in their
> > filenames is a safe change this close to release.
> 
> You could say if someone gives you a "malicious" text file which you
> cat to stdout, it could change your terminal settings.  I don't think
> therefore any of this is very serious.  We should probably fix any
> obvious things, but it doesn't need to happen right before 9.1 is
> released, we can take our time.

After consulting with Red Hat security, they agree: their decision at
this time is that any CVE related to escape sequences taking over a
terminal would best be filed against the terminal and/or shell that
allowed the escape, rather than against every single app that can
produce such pass-through output.  So at this time they were unwilling
to issue a CVE against qemu for this particular issue, and I will
clean up the subject line for v2.

So I agree that cleaning up low-hanging fruit is still worth it, but
no longer a reason to rush this series into 9.1.  If it still gets a
timely positive review, I can include it alongside the other NBD
patches (we are fixing a CVE with qemu hitting SEGV on
nbd-server-stop).