5:03 a.m.
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 */
+ case ' ':
+ case '.':
+ case '/': case '\\': /* directory separators */
+ case ':': case ';': /* path separators */
+ return false;
+ }
+ }
+
+ return strstr (filename, DIR_SEPARATOR_STR) == NULL;
}
#endif /* NBDKIT_OPTIONS_H */
--
2.44.0
5:03 a.m.
New subject: [PATCH nbdkit 2/2] tests: Add further tests of bad plugin and filter names
---
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
12:55 p.m.
New subject: [PATCH nbdkit 2/2] tests: Add further tests of bad plugin and filter names
On Sun, Apr 21, 2024 at 11:03:40AM +0100, Richard W.M. Jones wrote:
---
tests/Makefile.am | 4 ++
tests/test-bad-filter-name.sh | 74 +++++++++++++++++++++++++++++++++++
tests/test-bad-plugin-name.sh | 68 ++++++++++++++++++++++++++++++++
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
12:53 p.m.
New subject: [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>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
12:54 p.m.
New subject: [PATCH nbdkit 1/2] server: Reject more characters in plugin/filter short names
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 <eblake(a)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
247
days inactive
248
days old
4 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones