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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Thu, Aug 12, 2021 at 03:43:56PM -0500, Eric Blake wrote:> > $ ./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.I'm wondering if we want to add int nbdkit_parse_int_suffix (const char *what, const char *str, char **suff, int *r); and friends, which sets *suff to the first unparsed byte in str on a successful integer prefix parse. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2021-Aug-12 21:20 UTC
[Libguestfs] [PATCH nbdkit 2/3] delay: Fix delay-close
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