Re: [Libguestfs] [nbdkit PATCH 2/2] ext2: Supply .list_exports and .default_export

When using ext2file=exportname to pick the file to server from the client's export name, we do not want to leak the underlying plugin's export list. While touching this, take advantage of string lifetimes via nbdkit_strdup_intern and similar for less cleanup bookkeeping. Note that we don't actually implement a full .list_exports; doing that with NBD_OPT_LIST is likely prohibitive (it's likely the disk contains LOTS of files), and would require that we can do next_ops->pread prior to the client reaching our .open. For the former issue, it would be better if the NBD protocol adds a new option NBD_OPT_LIST_HIER that has replies that differentiate between containers and leaves, and only lists elements within a container supplied as an argument. For the latter, we already know we want to tweak filters to be able to open a plugin independently of a client's connection. And for now we punt, and leave the advertised list empty. It's also high time we had some testsuite coverage of ext2file=exportname. Although ext2.img doesn't technically depend on the new test, including the dependency ensures the file gets rebuilt in an incremental build. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- filters/ext2/nbdkit-ext2-filter.pod | 3 +- tests/Makefile.am | 16 +++++-- filters/ext2/ext2.c | 73 ++++++++++++++++++++--------- tests/test-ext2-exportname.sh | 73 +++++++++++++++++++++++++++++ 4 files changed, 138 insertions(+), 27 deletions(-) create mode 100755 tests/test-ext2-exportname.sh diff --git a/filters/ext2/nbdkit-ext2-filter.pod b/filters/ext2/nbdkit-ext2-filter.pod index 1aef9c2e..c15a2fcb 100644 --- a/filters/ext2/nbdkit-ext2-filter.pod +++ b/filters/ext2/nbdkit-ext2-filter.pod @@ -67,7 +67,8 @@ a security risk in some situations. At present, when using this mode, the server does not advertise any particular exports; however, you may use -L<nbdkit-exportname-filter(1)> to perform that task. +L<nbdkit-exportname-filter(1)> to perform that task. Similarly, the +underlying plugin must support the default export name, C<"">. =back diff --git a/tests/Makefile.am b/tests/Makefile.am index 8a799ccf..911fa2b3 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1373,23 +1373,31 @@ EXTRA_DIST += test-exportname.sh # ext2 filter test. if HAVE_MKE2FS_WITH_D if HAVE_EXT2 -if HAVE_GUESTFISH -LIBGUESTFS_TESTS += test-ext2 +TESTS += test-ext2-exportname.sh +EXTRA_DIST += test-ext2-exportname.sh + check_DATA += ext2.img CLEANFILES += ext2.img -ext2.img: disk +ext2.img: disk test-ext2-exportname.sh rm -f $@ $@-t + echo /disks/disk.img > manifest guestfish \ sparse $@-t 2G : \ run : \ mkfs ext4 /dev/sda : \ mount /dev/sda / : \ mkdir /disks : \ - upload $< /disks/disk.img + upload $< /disks/disk.img : \ + upload manifest /manifest + rm manifest mv $@-t $@ +if HAVE_GUESTFISH + +LIBGUESTFS_TESTS += test-ext2 + test_ext2_SOURCES = test-ext2.c test.h test_ext2_CFLAGS = $(WARNINGS_CFLAGS) $(LIBGUESTFS_CFLAGS) test_ext2_LDADD = libtest.la $(LIBGUESTFS_LIBS) diff --git a/filters/ext2/ext2.c b/filters/ext2/ext2.c index 7ad4a005..cfc03a3c 100644 --- a/filters/ext2/ext2.c +++ b/filters/ext2/ext2.c @@ -50,8 +50,10 @@ #include "cleanup.h" #include "io.h" -/* Filename parameter, or NULL to honor export name. */ -static char *file; +/* Filename parameter, or NULL to honor export name. Using the export + * name is opt-in (see ext2_config_complete). + */ +static const char *file; static void ext2_load (void) @@ -59,12 +61,6 @@ ext2_load (void) initialize_ext2_error_table (); } -static void -ext2_unload (void) -{ - free (file); -} - static int ext2_config (nbdkit_next_config *next, void *nxdata, const char *key, const char *value) @@ -74,11 +70,7 @@ ext2_config (nbdkit_next_config *next, void *nxdata, nbdkit_error ("ext2file parameter specified more than once"); return -1; } - file = strdup (value); - if (file == NULL) { - nbdkit_error ("strdup: %m"); - return -1; - } + file = value; return 0; } else @@ -94,10 +86,8 @@ ext2_config_complete (nbdkit_next_config_complete *next, void *nxdata) return -1; } - if (strcmp (file, "exportname") == 0) { - free (file); + if (strcmp (file, "exportname") == 0) file = NULL; - } else if (file[0] != '/') { nbdkit_error ("the file parameter must refer to an absolute path"); return -1; @@ -112,13 +102,54 @@ ext2_config_complete (nbdkit_next_config_complete *next, void *nxdata) /* The per-connection handle. */ struct handle { - char *exportname; /* Client export name. */ + const char *exportname; /* Client export name. */ ext2_filsys fs; /* Filesystem handle. */ ext2_ino_t ino; /* Inode of open file. */ ext2_file_t file; /* File handle. */ struct nbdkit_next next; /* "name" parameter to ext2fs_open. */ }; +/* Export list. */ +static int +ext2_list_exports (nbdkit_next_list_exports *next, void *nxdata, + int readonly, int is_tls, struct nbdkit_exports *exports) +{ + /* If we are honoring export names, the default export "" won't + * work, and we must not leak export names from the underlying + * plugin. Advertising all filenames within the ext2 image could be + * huge, and even if we wanted to, it would require that we could + * open the plugin prior to the client reaching our .open. So leave + * the list empty instead. + */ + if (!file) + return 0; + + /* If we are serving a specific ext2file, we don't care what export + * name the user passes, but the underlying plugin might; there's no + * harm in advertising that list. + */ + return next (nxdata, readonly, exports); +} + +/* Default export. */ +static const char * +ext2_default_export (nbdkit_next_default_export *next, void *nxdata, + int readonly, int is_tls) +{ + /* If we are honoring exports, "" will fail (even if we resolve to + * the inode of embedded "/", we can't serve directories), and we + * don't really have a sane default. XXX picking the largest + * embedded file might be in interesting knob to add. + */ + if (file) + return NULL; + + /* Otherwise, we don't care about export name, so keeping things at + * "" is fine, regardless of the underlying plugin's default. + */ + return ""; +} + /* Create the per-connection handle. */ static void * ext2_open (nbdkit_next_open *next, void *nxdata, @@ -133,9 +164,8 @@ ext2_open (nbdkit_next_open *next, void *nxdata, } /* Save the client exportname in the handle. */ - h->exportname = strdup (exportname); + h->exportname = nbdkit_strdup_intern (exportname); if (h->exportname == NULL) { - nbdkit_error ("strdup: %m"); free (h); return NULL; } @@ -147,7 +177,6 @@ ext2_open (nbdkit_next_open *next, void *nxdata, /* Request write access to the underlying plugin, for journal replay. */ if (next (nxdata, 0, exportname) == -1) { - free (h->exportname); free (h); return NULL; } @@ -260,7 +289,6 @@ ext2_close (void *handle) ext2fs_file_close (h->file); ext2fs_close (h->fs); } - free (h->exportname); free (h); } @@ -434,11 +462,12 @@ static struct nbdkit_filter filter = { .name = "ext2", .longname = "nbdkit ext2 filter", .load = ext2_load, - .unload = ext2_unload, .config = ext2_config, .config_complete = ext2_config_complete, .config_help = ext2_config_help, .thread_model = ext2_thread_model, + .list_exports = ext2_list_exports, + .default_export = ext2_default_export, .open = ext2_open, .prepare = ext2_prepare, .close = ext2_close, diff --git a/tests/test-ext2-exportname.sh b/tests/test-ext2-exportname.sh new file mode 100755 index 00000000..b30aa870 --- /dev/null +++ b/tests/test-ext2-exportname.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2019-2020 Red Hat Inc. +# +# 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. + +source ./functions.sh +set -e +set -x + +requires_plugin file +requires nbdinfo --version +requires nbdsh -c 'print (h.set_full_info)' + +sock=`mktemp -u` +files="$sock ext2-exportname.pid ext2-exportname.out" +rm -f $files +cleanup_fn rm -f $files + +# Set up a long-running server responsive to the client's export name +start_nbdkit -P ext2-exportname.pid -U $sock --filter=ext2 \ + --filter=exportname file ext2.img ext2file=exportname \ + exportname-list=explicit exportname=hidden + +# Test that when serving by exportname, our description varies according +# to the client's request. +nbdinfo nbd+unix:///manifest\?socket=$sock > ext2-exportname.out +cat ext2-exportname.out +grep manifest ext2-exportname.out +grep 'content.*ASCII' ext2-exportname.out + +nbdinfo nbd+unix:///disks/disk.img\?socket=$sock > ext2-exportname.out +cat ext2-exportname.out +grep disk.img ext2-exportname.out +grep 'content.*MBR' ext2-exportname.out + +if nbdinfo nbd+unix://?socket=$sock > ext2-exportname.out; then + echo "unexpected success" + exit 1 +fi + +# Test that there is no export list advertised +nbdinfo --list --json nbd+unix://?socket=$sock > ext2-exportname.out +cat ext2-exportname.out +grep '"exports": \[]' ext2-exportname.out + +: --