On Mon, Apr 22, 2024 at 12:53:03PM -0500, Eric Blake wrote:
> 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;'.
I'll change it as suggested, thanks.
Rich.
> > + }
> > + }
> > +
> > + return strstr (filename, DIR_SEPARATOR_STR) == NULL;
> > }
> >
> > #endif /* NBDKIT_OPTIONS_H */
> > --
> > 2.44.0
> >
>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization: qemu.org | libguestfs.org
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
_______________________________________________
Libguestfs mailing list -- guestfs@lists.libguestfs.org
To unsubscribe send an email to guestfs-le...@lists.libguestfs.org
