[Libguestfs] Re: [PATCH nbdkit 1/2] server: Reject more characters in plugin/filter short names

On Sun, Apr 21, 2024 at 11:03:39AM +0100, Richard W.M. Jones wrote: > The plugin/filter short name detection is very liberal, disallowing > only '.' and '/'. Thus, at least in theory, short plugin names > containing almost arbitrary symbols and characters are permitted. > > We should probably reserve more characters, but in this commit I only > reserve: > > * backslash (ie. dir separator on Windows) > * ':' and ';' (common path separators) > * space > * non-printable ASCII characters > > Also DIR_SEPARATOR_STR, but that's likely to be already covered by the > other tests so probably does nothing here. > > This commit is mainly about tightening up corner cases with possible > security implications, for example if you managed to trick a program > to invoke 'nbdkit "plugin param"' that might have an ambiguous parsing > that you could use to your advantage. It should have no effect on > normal, non-adversarial usage. > --- > server/options.h | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/server/options.h b/server/options.h > index 7d0730bae7..55e6abc28c 100644 > --- a/server/options.h > +++ b/server/options.h > @@ -117,7 +117,21 @@ static const struct option long_options[] = { > static inline bool > is_short_name (const char *filename) > { > - return strchr (filename, '.') == NULL && strchr (filename, '/') == NULL; > + const size_t n = strlen (filename); > + size_t i; > + > + for (i = 0; i < n; ++i) { > + switch (filename[i]) { > + case '\0'...31: case 127: /* non-printable ASCII */ Compressed case label ranges are a gcc extension. I guess clang has it too, and we already require one of those two compilers for __attribute__((cleanup)), so it's not necessarily fatal. But as it appears to be our first use of that extension, it may be more conservative to instead > + case ' ': > + case '.': > + case '/': case '\\': /* directory separators */ > + case ':': case ';': /* path separators */ > + return false; add 'default: if ((unsigned)filename[i] < 31) return false;'.

> + } > + } > + > + return strstr (filename, DIR_SEPARATOR_STR) == NULL; > } > > #endif /* NBDKIT_OPTIONS_H */ > -- > 2.44.0 > Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org