2:15 p.m.
The plugin/filter short name detection is very liberal, reserving only
'.' and '/'. Thus, at least in theory, short plugin names containing
almost arbitrary symbols and characters are permitted.
Backslash ought to have been reserved when we added Windows support.
We should probably reserve more characters, but in this commit I only
reserve:
* backslash (ie. directory separator on Windows)
* ':' and ';' (common path separators)
* '=' (used in nbdkit parameters)
* space and comma (commonly used to separate lists)
* 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 | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/server/options.h b/server/options.h
index 7d0730bae7..8b1bd679e9 100644
--- a/server/options.h
+++ b/server/options.h
@@ -117,7 +117,23 @@ 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 '\\': /* directory separators */
+ case ':': case ';': /* path separators */
+ case ' ':
+ case '.':
+ case ',':
+ case '=':
+ return false;
+ }
+ }
+
+ return strstr (filename, DIR_SEPARATOR_STR) == NULL;
}
#endif /* NBDKIT_OPTIONS_H */
--
2.44.0
2:15 p.m.
New subject: [PATCH nbdkit v2 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:58 p.m.
New subject: [PATCH nbdkit v2 1/2] server: Reserve more characters from plugin/filter short names
On Sun, Apr 21, 2024 at 08:15:42PM +0100, Richard W.M. Jones wrote:
The plugin/filter short name detection is very liberal, reserving
only
'.' and '/'. Thus, at least in theory, short plugin names containing
almost arbitrary symbols and characters are permitted.
Backslash ought to have been reserved when we added Windows support.
We should probably reserve more characters, but in this commit I only
reserve:
* backslash (ie. directory separator on Windows)
* ':' and ';' (common path separators)
* '=' (used in nbdkit parameters)
Oh, I reviewed v1 before you added '=' to the reject list in v2.
* space and comma (commonly used to separate lists)
* 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 | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
+
+ for (i = 0; i < n; ++i) {
+ switch (filename[i]) {
+ case '\0'...31: case 127: /* non-printable ASCII */
The comment about ranged case label is still present (I'm okay whether
you keep it in or explode it to long-hand).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
12:59 p.m.
New subject: [PATCH nbdkit v2 1/2] server: Reserve more characters from plugin/filter short names
On Mon, Apr 22, 2024 at 12:58:10PM -0500, Eric Blake wrote:
On Sun, Apr 21, 2024 at 08:15:42PM +0100, Richard W.M. Jones wrote:
> The plugin/filter short name detection is very liberal, reserving only
> '.' and '/'. Thus, at least in theory, short plugin names
containing
> almost arbitrary symbols and characters are permitted.
>
> Backslash ought to have been reserved when we added Windows support.
>
> We should probably reserve more characters, but in this commit I only
> reserve:
>
> * backslash (ie. directory separator on Windows)
> * ':' and ';' (common path separators)
> * '=' (used in nbdkit parameters)
Oh, I reviewed v1 before you added '=' to the reject list in v2.
> * space and comma (commonly used to separate lists)
> * 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 | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> +
> + for (i = 0; i < n; ++i) {
> + switch (filename[i]) {
> + case '\0'...31: case 127: /* non-printable ASCII */
The comment about ranged case label is still present (I'm okay whether
you keep it in or explode it to long-hand).
I'll change it to:
+ /* non-printable ASCII */
+ case 127:
+ return false;
+ default:
+ if ((unsigned)filename[i] <= 31)
+ return false;
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
246
days inactive
247
days old
3 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones