--- tests/Makefile.am | 4 ++ tests/test-bad-filter-name.sh | 74 +++++++++++++++++++++++++++++++++++ tests/test-bad-plugin-name.sh | 68 ++++++++++++++++++++++++++++++++ 3 files changed, 146 insertions(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index 6bfa3b6fa5..1ba7991fe6 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -248,6 +248,8 @@ TESTS += \ test-probe-plugin.sh \ test-probe-filter-dump-plugin.sh \ test-no-parameters.sh \ + test-bad-plugin-name.sh \ + test-bad-filter-name.sh \ test-start.sh \ test-single.sh \ test-single-from-file.sh \ @@ -283,6 +285,8 @@ TESTS += \ $(NULL) endif EXTRA_DIST += \ + test-bad-filter-name.sh \ + test-bad-plugin-name.sh \ test-captive.sh \ test-captive-tls.sh \ test-debug-flags.sh \ diff --git a/tests/test-bad-filter-name.sh b/tests/test-bad-filter-name.sh new file mode 100755 index 0000000000..eb53410c07 --- /dev/null +++ b/tests/test-bad-filter-name.sh @@ -0,0 +1,74 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright Red Hat +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +# Test bad filter names and the resulting error messages. + +source ./functions.sh +set -e +set -x + +# Because below we are bypassing the wrapper, we also have to attempt +# to rewrite the null plugin name. This is difficult to predict, so +# skip the test if not found. +null_plugin=../plugins/null/.libs/nbdkit-null-plugin.$SOEXT +requires test -x $null_plugin + +out="test-bad-filter-name.out" +rm -f $out +cleanup_fn rm -f $out + +# A name that could exist but does not. +# +# Note here we have to run the server directly because the wrapper +# will rewrite the filter name. +../server/nbdkit $null_plugin --filter=nosuchname 2>$out ||: +cat $out +grep 'nosuchname' $out +# The error message should suggest a Fedora or Debian package name: +grep 'nbdkit-nosuchname-filter' $out +grep 'nbdkit-filter-nosuchname' $out +# The error should mention the manual: +grep -F 'nbdkit(1)' $out + +# Valid but unusual short name. +nbdkit null --filter="filter@name" 2>$out ||: +cat $out +grep 'filter@name' $out +# The error message should NOT suggest a package name: +! grep 'nbdkit-filter@name-filter' $out + +# Test quoting of output strings. +nbdkit null --filter="$(printf 'bad\r')" 2>$out ||: +cat $out +grep 'bad\\r' $out +# The error message should NOT suggest a package name: +! grep 'nbdkit-bad.?-filter' $out diff --git a/tests/test-bad-plugin-name.sh b/tests/test-bad-plugin-name.sh new file mode 100755 index 0000000000..7735adc4bd --- /dev/null +++ b/tests/test-bad-plugin-name.sh @@ -0,0 +1,68 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright Red Hat +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +# Test bad plugin names and the resulting error messages. + +source ./functions.sh +set -e +set -x + +out="test-bad-plugin-name.out" +rm -f $out +cleanup_fn rm -f $out + +# A name that could exist but does not. +# +# Note here we have to run the server directly because the wrapper +# will rewrite the plugin name. +../server/nbdkit nosuchname 2>$out ||: +cat $out +grep 'nosuchname' $out +# The error message should suggest a Fedora or Debian package name: +grep 'nbdkit-nosuchname-plugin' $out +grep 'nbdkit-plugin-nosuchname' $out +# The error should mention the manual: +grep -F 'nbdkit(1)' $out + +# Valid but unusual short name. +nbdkit "plugin@name" 2>$out ||: +cat $out +grep 'plugin@name' $out +# The error message should NOT suggest a package name: +! grep 'nbdkit-plugin@name-plugin' $out + +# Test quoting of output strings (also: test-shebang-crlf.sh) +nbdkit "$(printf 'bad\r')" 2>$out ||: +cat $out +grep 'bad\\r' $out +# The error message should NOT suggest a package name: +! grep 'nbdkit-bad.?-plugin' $out -- 2.44.0

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@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org