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