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

[Richard W.M. Jones]

On Thu, Aug 12, 2021 at 03:43:56PM -0500, Eric Blake wrote:
> 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
> now.
> 
> $ ./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.

Agreed .. (although I really wish POSIX would make sscanf more robust).

The reason for still using sscanf in the NNms case was only because to
use nbdkit_parse_* functions would either involve copying the string
(possible, but tedious), or we'd have to add alternate functions that
allow parsing string + length.

Also I hope we trust the command line.  We might make that clearer in
the documentation however.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top