[Libguestfs] [PATCH nbdkit 2/3] delay: Fix delay-close

Eric Blake eblake at redhat.com
Thu Aug 12 20:43:56 UTC 2021

On Tue, Aug 10, 2021 at 09:25:28AM -0500, Eric Blake wrote:
> On Tue, Aug 10, 2021 at 09:19:14AM +0100, Richard W.M. Jones wrote:
> > See comments in the code for how this has been fixed.
> > 
> > This only delays clients which use NBD_CMD_DISC (libnbd
> > nbd_shutdown(3)).  Clients which drop the connection obviously cannot
> > be delayed.  For example:

I also noticed that you added another commit 58187831a not posted on
list at the time to make delay parsing more robust.  However, it uses
sscanf() to parse an integer, which is risky on untrusted data (such
as the command-line input from the user).

First, the code paths that don't use sscanf:

$ ./nbdkit -f memory 1 --filter=delay delay-read=1oops
nbdkit: error: delay-read: could not parse number: "1oops": trailing garbage

Good - this is what we want to tell the user.

$ ./nbdkit -f memory 1 --filter=delay delay-read=oops
nbdkit: error: delay-read: empty string where we expected a number

So-so - we correctly give the user an error, but the error message is
poor quality: "oops" is not an empty string (this code path did not
use sscanf)

Now for the use of sscanf:

$ ./nbdkit -f memory 1 --filter=delay delay-read=oopsms
nbdkit: error: cannot parse delay-read in milliseconds parameter: oopsms

Good - we detected a bug, although the message is worded differently

$ ./nbdkit -f memory 1 --filter=delay delay-read=1oopsms

Oops - our use of sscanf didn't check for trailing garbage, and this
is behaving as delay-read=1ms.

$ ./nbdkit -fv memory 1 --filter=delay delay-read=999999999999999999999ms

Using gdb, I see that in glibc this results in the same as
delay-read=4294967295, but that behavior is unspecified by POSIX and
may result in other values on other platforms.  Better would be
detecting overflow, but sscanf() cannot detect numeric overflow.

Detecting trailing garbage could be done with sscanf(value, "%ums%n",
r, &n) == 1 followed by checking that n consumed strlen(value) bytes,
but detecting overflow really needs strtol() rather than sscanf.

We have other filters and plugins that use sscanf.  As long as their
inputs come from stable sources (such as scanning kernel /proc files)
or don't parse numbers, that is safe; but in general, use of sscanf to
parse user-provided data is risky.

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

More information about the Libguestfs mailing list