Richard W.M. Jones
2020-Apr-07 16:24 UTC
[Libguestfs] [PATCH nbdkit v2] tmpdisk: Generalize the tmpdisk plugin
An evolution of v1 here: https://www.redhat.com/archives/libguestfs/2020-April/msg00035.html I want to generalize the tmpdisk plugin, particularly so you can use commands like ‘qemu-img create’ or even ‘virt-builder’. (Actually virt-builder really works - I tested it - but of course it causes a 30+ second delay when connecting to the server.) You can now use commands such as: nbdkit tmpdisk 16G command=' truncate -s $size "$disk" ' The way it works is it creates a temporary directory under $TMPDIR, runs the external command with disk=$TMPDIR/<random>/disk, then when the external command finishes the plugin opens the disk, finds out the size and serves it. At the same time the plugin deletes the disk and temporary subdirectory so it is all automatically cleaned up even if nbdkit crashes. It seems this is secure because mkdtemp(3) creates the subdirectory with 0700 permissions, so no other user on the same machine should be able to monkey around with the disk. Hopefully. But I suggest Eric takes a closer look with his much wider experience :-) Rich.
Richard W.M. Jones
2020-Apr-07 16:24 UTC
[Libguestfs] [PATCH nbdkit v2] tmpdisk: Pass any parameters as shell variables to the command.
This allows us to be much more flexible about what commands can be
used. It also means we do not need to encode any special behaviour
for type or label parameters.
---
plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod | 91 +++++++++-----
plugins/tmpdisk/tmpdisk.c | 147 ++++++++++++++--------
plugins/tmpdisk/default-command.sh.in | 6 +
3 files changed, 164 insertions(+), 80 deletions(-)
diff --git a/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod
b/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod
index 490bcf6c..f9e3296a 100644
--- a/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod
+++ b/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod
@@ -4,9 +4,9 @@ nbdkit-tmpdisk-plugin - create a fresh temporary filesystem for
each client
=head1 SYNOPSIS
- nbdkit tmpdisk [size=]SIZE
- [type=ext4|xfs|vfat|...] [label=LABEL]
- [command=COMMAND]
+ nbdkit tmpdisk [size=]SIZE [type=ext4|xfs|vfat|...] [label=LABEL]
+
+ nbdkit tmpdisk [size=]SIZE command=COMMAND
=head1 DESCRIPTION
@@ -23,10 +23,38 @@ this plugin sees a different disk.
The size of the disk is chosen using the C<size> parameter. The
filesystem type is C<ext4> but this can be changed using the
C<type>
-parameter (controlling the I<-t> option of mkfs).
+parameter (controlling the I<-t> option of mkfs). The filesystem
+label may be set using C<label>.
-Instead of running mkfs you can run an arbitrary C<command> to create
-the disk.
+=head2 The command parameter
+
+Instead of running mkfs you can run an arbitrary command (a shell
+script fragment) to create the disk.
+
+The other parameters to the plugin are turned into shell variables
+passed to the command. For example C<type> becomes the shell variable
+C<$type>, etc. Any parameters you want can be passed to the plugin
+and will be turned into shell variables (not only C<type> and
+C<label>) making this a very flexible method to create temporary disks
+of all kinds.
+
+Two special variables are also passed to the shell script fragment:
+
+=over 4
+
+=item C<$disk>
+
+The absolute path of the disk file. Note that this is not
+pre-created, you must create it yourself, for example using:
+
+ truncate -s $size "$disk"
+
+=item C<$size>
+
+The virtual size in bytes. This is the C<size> parameter, converted
+to bytes.
+
+=back
=head2 Security considerations
@@ -37,7 +65,7 @@ best to limit the number of clients using
L<nbdkit-limit-filter(1)> or
take steps to limit where clients can connect from using
L<nbdkit-ip-filter(1)>, firewalls, or TLS client certificates.
-=head1 EXAMPLE
+=head1 EXAMPLES
=head2 Remote tmpfs
@@ -56,6 +84,23 @@ containing:
Clients would see a fresh, empty C</var/scratch> directory after boot.
+=head2 Overriding mkfs options
+
+Using C<command> allows you to easily override any mkfs option, for
+example:
+
+ nbdkit tmpdisk 16G command=' mke2fs -F -t ext4 "$disk" -N 10000
'
+
+=head2 Serve a fresh blank disk to each client
+
+ nbdkit tmpdisk 16G command=' truncate -s $size "$disk" '
+
+=head2 Serve a fresh operating system to each client
+
+ nbdkit tmpdisk 16G \
+ command=' virt-builder -o "$disk" --size ${size}b
"$os" ' \
+ os=fedora-31
+
=head1 PARAMETERS
=over 4
@@ -63,30 +108,8 @@ Clients would see a fresh, empty C</var/scratch>
directory after boot.
=item B<command='>COMMANDB<'>
Instead of running L<mkfs(8)> to create the initial filesystem, run
-C<COMMAND> (which usually must be quoted to protect it from the
-shell). The following shell variables may be used in C<COMMAND>:
-
-=over 4
-
-=item C<$disk>
-
-The absolute path of the file that the command must initialize. This
-file is created by nbdkit before the command runs.
-
-=item C<$label>
-
-The filesystem label (from the C<label> parameter).
-
-=item C<$size>
-
-The virtual size in bytes, which is the same as the file size.
-
-=item C<$type>
-
-The filesystem type (from the C<type> parameter), defaulting to
-C<ext4>. (Commands can ignore this if it is not relevant).
-
-=back
+C<COMMAND> (a shell script fragment which usually must be quoted to
+protect it from the shell). See L</The command parameter> above.
=item B<label=>LABEL
@@ -96,6 +119,10 @@ Select the filesystem label. The default is not set.
Specify the virtual size of the disk image.
+If using C<command>, this is only a suggested size. The actual size
+of the resulting disk will be the size of the disk created by
+C<command>.
+
This parameter is required.
C<size=> is a magic config key and may be omitted in most cases.
@@ -141,11 +168,13 @@ C<nbdkit-tmpdisk-plugin> first appeared in nbdkit
1.20.
L<nbdkit(1)>,
L<nbdkit-plugin(3)>,
L<nbdkit-data-plugin(1)>,
+L<nbdkit-eval-plugin(1)>,
L<nbdkit-file-plugin(1)>,
L<nbdkit-ip-filter(1)>,
L<nbdkit-limit-filter(1)>,
L<nbdkit-linuxdisk-plugin(1)>,
L<nbdkit-memory-plugin(1)>,
+L<nbdkit-sh-plugin(1)>,
L<nbdkit-loop(1)>,
L<nbdkit-tls(1)>,
L<mkfs(8)>.
diff --git a/plugins/tmpdisk/tmpdisk.c b/plugins/tmpdisk/tmpdisk.c
index 5e8df151..38f003b1 100644
--- a/plugins/tmpdisk/tmpdisk.c
+++ b/plugins/tmpdisk/tmpdisk.c
@@ -42,6 +42,7 @@
#include <fcntl.h>
#include <errno.h>
#include <sys/types.h>
+#include <sys/stat.h>
#include <sys/wait.h>
#define NBDKIT_API_VERSION 2
@@ -51,9 +52,13 @@
#include "utils.h"
static const char *tmpdir = "/var/tmp";
-static int64_t size = -1;
-static const char *label = NULL;
-static const char *type = "ext4";
+static int64_t requested_size = -1; /* size parameter on the command line */
+
+/* Shell variables. */
+static struct var {
+ struct var *next;
+ const char *key, *value;
+} *vars;
/* This comes from default-command.c which is generated from
* default-command.sh.in.
@@ -70,29 +75,49 @@ tmpdisk_load (void)
tmpdir = s;
}
+static void
+tmpdisk_unload (void)
+{
+ struct var *v, *v_next;
+
+ for (v = vars; v != NULL; v = v_next) {
+ v_next = v->next;
+ free (v);
+ }
+}
+
static int
tmpdisk_config (const char *key, const char *value)
{
if (strcmp (key, "command") == 0) {
command = value;
}
- else if (strcmp (key, "label") == 0) {
- if (strcmp (value, "") == 0)
- label = NULL;
- else
- label = value;
- }
else if (strcmp (key, "size") == 0) {
- size = nbdkit_parse_size (value);
- if (size == -1)
+ requested_size = nbdkit_parse_size (value);
+ if (requested_size == -1)
return -1;
}
- else if (strcmp (key, "type") == 0) {
- type = value;
+
+ /* This parameter cannot be set on the command line since it is used
+ * to pass the disk name to the command.
+ */
+ else if (strcmp (key, "disk") == 0) {
+ nbdkit_error ("'disk' parameter cannot be set on the command
line");
+ return -1;
}
+
+ /* Any other parameter will be forwarded to a shell variable. */
else {
- nbdkit_error ("unknown parameter '%s'", key);
- return -1;
+ struct var *v_next = vars;
+
+ vars = malloc (sizeof *vars);
+ if (vars == NULL) {
+ perror ("malloc");
+ exit (EXIT_FAILURE);
+ }
+ vars->next = v_next;
+ vars->key = key;
+ vars->value = value;
}
return 0;
@@ -101,7 +126,7 @@ tmpdisk_config (const char *key, const char *value)
static int
tmpdisk_config_complete (void)
{
- if (size == -1) {
+ if (requested_size == -1) {
nbdkit_error ("size parameter is required");
return -1;
}
@@ -117,6 +142,7 @@ tmpdisk_config_complete (void)
struct handle {
int fd;
+ int64_t size;
bool can_punch_hole;
};
@@ -152,7 +178,9 @@ tmpdisk_can_fua (void *handle)
static int64_t
tmpdisk_get_size (void *handle)
{
- return size;
+ struct handle *h = handle;
+
+ return h->size;
}
/* This creates and runs the full "mkfs" (or whatever) command. */
@@ -163,6 +191,7 @@ run_command (const char *disk)
CLEANUP_FREE char *cmd = NULL;
size_t len = 0;
int r;
+ struct var *v;
fp = open_memstream (&cmd, &len);
if (fp == NULL) {
@@ -173,21 +202,26 @@ run_command (const char *disk)
/* Avoid stdin/stdout leaking (because of nbdkit -s). */
fprintf (fp, "exec </dev/null >/dev/null\n");
- /* Set the shell variables. */
+ /* Set the standard shell variables. */
fprintf (fp, "disk=");
shell_quote (disk, fp);
putc ('\n', fp);
- if (label) {
- fprintf (fp, "label=");
- shell_quote (label, fp);
+ fprintf (fp, "size=%" PRIi64 "\n", requested_size);
+ putc ('\n', fp);
+
+ /* The other parameters/shell variables. */
+ for (v = vars; v != NULL; v = v->next) {
+ /* Keys probably can never contain shell-unsafe chars (because of
+ * nbdkit's own restrictions), but quoting it makes it safe.
+ */
+ shell_quote (v->key, fp);
+ putc ('=', fp);
+ shell_quote (v->value, fp);
putc ('\n', fp);
}
- fprintf (fp, "size=%" PRIi64 "\n", size);
- fprintf (fp, "type=");
- shell_quote (type, fp);
putc ('\n', fp);
- putc ('\n', fp);
+ /* The command. */
fprintf (fp, "%s", command);
if (fclose (fp) == EOF) {
@@ -220,7 +254,9 @@ static void *
tmpdisk_open (int readonly)
{
struct handle *h;
- CLEANUP_FREE char *disk = NULL;
+ CLEANUP_FREE char *dir = NULL, *disk = NULL;
+ int flags;
+ struct stat statbuf;
h = malloc (sizeof *h);
if (h == NULL) {
@@ -228,36 +264,25 @@ tmpdisk_open (int readonly)
goto error;
}
h->fd = -1;
+ h->size = -1;
h->can_punch_hole = true;
- /* Create the new disk image for this connection. */
- if (asprintf (&disk, "%s/tmpdiskXXXXXX", tmpdir) == -1) {
+ /* For security reasons we have to create a temporary directory
+ * under tmpdir that only the current user can access. If we
+ * created it in a shared directory then another user might be able
+ * to see the temporary file being created and interfere with it
+ * before we reopen it in the plugin below.
+ */
+ if (asprintf (&dir, "%s/tmpdiskXXXXXX", tmpdir) == -1) {
nbdkit_error ("asprintf: %m");
goto error;
}
-
-#ifdef HAVE_MKOSTEMP
- h->fd = mkostemp (disk, O_CLOEXEC);
-#else
- /* Racy, fix your libc. */
- h->fd = mkstemp (disk);
- if (h->fd >= 0) {
- h->fd = set_cloexec (h->fd);
- if (h->fd == -1) {
- int e = errno;
- unlink (disk);
- errno = e;
- }
- }
-#endif
- if (h->fd == -1) {
- nbdkit_error ("mkstemp: %m");
+ if (mkdtemp (dir) == NULL) {
+ nbdkit_error ("%s: %m", dir);
goto error;
}
-
- /* Truncate the disk to a sparse file of the right size. */
- if (ftruncate (h->fd, size) == -1) {
- nbdkit_error ("ftruncate: %s: %m", disk);
+ if (asprintf (&disk, "%s/disk", dir) == -1) {
+ nbdkit_error ("asprintf: %m");
goto error;
}
@@ -265,11 +290,34 @@ tmpdisk_open (int readonly)
if (run_command (disk) == -1)
goto error;
+ /* The external command must have created the disk, and then we must
+ * find the true size.
+ */
+ if (readonly)
+ flags = O_RDONLY | O_CLOEXEC;
+ else
+ flags = O_RDWR | O_CLOEXEC | O_NOCTTY;
+ h->fd = open (disk, flags);
+ if (h->fd == -1) {
+ nbdkit_error ("open: %s: %m", disk);
+ goto error;
+ }
+
+ if (fstat (h->fd, &statbuf) == -1) {
+ nbdkit_error ("fstat: %s: %m", disk);
+ goto error;
+ }
+
+ h->size = statbuf.st_size;
+ nbdkit_debug ("tmpdisk: requested_size = %" PRIi64 ", size =
%" PRIi64,
+ requested_size, h->size);
+
/* We don't need the disk to appear in the filesystem since we hold
* a file descriptor and access it through that, so unlink the disk.
* This also ensures it is always cleaned up.
*/
unlink (disk);
+ rmdir (dir);
/* Return the handle. */
return h;
@@ -414,6 +462,7 @@ static struct nbdkit_plugin plugin = {
.version = PACKAGE_VERSION,
.load = tmpdisk_load,
+ .unload = tmpdisk_unload,
.config = tmpdisk_config,
.config_complete = tmpdisk_config_complete,
.config_help = tmpdisk_config_help,
diff --git a/plugins/tmpdisk/default-command.sh.in
b/plugins/tmpdisk/default-command.sh.in
index 251e0b7b..d60402b5 100644
--- a/plugins/tmpdisk/default-command.sh.in
+++ b/plugins/tmpdisk/default-command.sh.in
@@ -30,6 +30,9 @@
# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
# SUCH DAMAGE.
+# If not set, default to ext4.
+type="${type:-ext4}"
+
labelopt='-L'
case "$type" in
@@ -44,6 +47,9 @@ case "$type" in
extra='-f' ;;
esac
+# Create the output disk.
+truncate -s $size "$disk"
+
if [ "x$label" = "x" ]; then
mkfs -t "$type" $extra "$disk"
else
--
2.25.0
Eric Blake
2020-Apr-07 18:59 UTC
Re: [Libguestfs] [PATCH nbdkit v2] tmpdisk: Pass any parameters as shell variables to the command.
On 4/7/20 11:24 AM, Richard W.M. Jones wrote:> This allows us to be much more flexible about what commands can be > used. It also means we do not need to encode any special behaviour > for type or label parameters. > --- > plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod | 91 +++++++++----- > plugins/tmpdisk/tmpdisk.c | 147 ++++++++++++++-------- > plugins/tmpdisk/default-command.sh.in | 6 + > 3 files changed, 164 insertions(+), 80 deletions(-) > > diff --git a/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod b/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod > index 490bcf6c..f9e3296a 100644 > --- a/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod > +++ b/plugins/tmpdisk/nbdkit-tmpdisk-plugin.pod > @@ -4,9 +4,9 @@ nbdkit-tmpdisk-plugin - create a fresh temporary filesystem for each client > > =head1 SYNOPSIS > > - nbdkit tmpdisk [size=]SIZE > - [type=ext4|xfs|vfat|...] [label=LABEL] > - [command=COMMAND] > + nbdkit tmpdisk [size=]SIZE [type=ext4|xfs|vfat|...] [label=LABEL] > + > + nbdkit tmpdisk [size=]SIZE command=COMMAND >Is it worth spelling this: nbdkit tmpdisk [size=]SIZE command=COMMAND [VAR=VALUE...]> +=item C<$size> > + > +The virtual size in bytes. This is the C<size> parameter, converted > +to bytes.Is it worth mentioning that if the command ends up rounding the passed in size, the size passed in to nbdkit may differ from the size advertised to the client?> +=head2 Serve a fresh operating system to each client > + > + nbdkit tmpdisk 16G \ > + command=' virt-builder -o "$disk" --size ${size}b "$os" ' \ > + os=fedora-31Cool, even if it is time-consuming setup :)> @@ -96,6 +119,10 @@ Select the filesystem label. The default is not set. > > Specify the virtual size of the disk image. > > +If using C<command>, this is only a suggested size. The actual size > +of the resulting disk will be the size of the disk created by > +C<command>.Ah, this addresses my comment above.> +++ b/plugins/tmpdisk/tmpdisk.c> +/* Shell variables. */ > +static struct var { > + struct var *next; > + const char *key, *value; > +} *vars;Linked-list, where...> static int > tmpdisk_config (const char *key, const char *value) > {> + /* Any other parameter will be forwarded to a shell variable. */ > else { > - nbdkit_error ("unknown parameter '%s'", key); > - return -1; > + struct var *v_next = vars; > + > + vars = malloc (sizeof *vars); > + if (vars == NULL) { > + perror ("malloc"); > + exit (EXIT_FAILURE); > + } > + vars->next = v_next; > + vars->key = key; > + vars->value = value;...the list is populated with later entries first, and...> @@ -163,6 +191,7 @@ run_command (const char *disk) > CLEANUP_FREE char *cmd = NULL; > size_t len = 0; > int r; > + struct var *v; > > fp = open_memstream (&cmd, &len); > if (fp == NULL) { > @@ -173,21 +202,26 @@ run_command (const char *disk) > /* Avoid stdin/stdout leaking (because of nbdkit -s). */ > fprintf (fp, "exec </dev/null >/dev/null\n"); > > - /* Set the shell variables. */ > + /* Set the standard shell variables. */ > fprintf (fp, "disk="); > shell_quote (disk, fp); > putc ('\n', fp); > - if (label) { > - fprintf (fp, "label="); > - shell_quote (label, fp); > + fprintf (fp, "size=%" PRIi64 "\n", requested_size); > + putc ('\n', fp); > + > + /* The other parameters/shell variables. */ > + for (v = vars; v != NULL; v = v->next) { > + /* Keys probably can never contain shell-unsafe chars (because of > + * nbdkit's own restrictions), but quoting it makes it safe. > + */ > + shell_quote (v->key, fp);You are correct that nbdkit itself prevents non-safe names from being passed as a key to config. But if it did not, (for example, if '1=a' were allowed through instead of an error), quoting the key would end up trying to invoke a command rather than setting a shell variable. If anything, I think it might be a bit cleaner to assert() that key is a valid shell name, instead of trying to shell_quote() it. On the other hand, I don't see it as a security bug: even if 'nbdkit tmpdisk size=... command=... 1=a' sneaks through and caused the shell to attempt execution of a command named '1=a', it's merely user error rather than a CVE because nbdkit isn't running with any additional privileges compared to the user just running '1=a' himself in the first place. But it's all theoretical (shell_quote() is a no-op on safe names, and as the comment says, we are currently guaranteed a safe name), so I can live with what you have.> + putc ('=', fp); > + shell_quote (v->value, fp); > putc ('\n', fp); > }...then processed front-to-back in passing to the shell. This means that: nbdkit tmpdisk command=... a=1 a=2 results in command seeing $a as 1, which is a bit confusing (most command lines allow later parameters to override earlier ones). Fixing it would mean maintaining two pointers: the head of the list (unchanging, used when reading the list), and the tail (updated with each call to .config when building the list).> @@ -228,36 +264,25 @@ tmpdisk_open (int readonly) > goto error; > } > h->fd = -1; > + h->size = -1; > h->can_punch_hole = true; > > - /* Create the new disk image for this connection. */ > - if (asprintf (&disk, "%s/tmpdiskXXXXXX", tmpdir) == -1) { > + /* For security reasons we have to create a temporary directory > + * under tmpdir that only the current user can access. If we > + * created it in a shared directory then another user might be able > + * to see the temporary file being created and interfere with it > + * before we reopen it in the plugin below. > + */ > + if (asprintf (&dir, "%s/tmpdiskXXXXXX", tmpdir) == -1) { > nbdkit_error ("asprintf: %m"); > goto error; > } > - > -#ifdef HAVE_MKOSTEMP > - h->fd = mkostemp (disk, O_CLOEXEC); > -#else > - /* Racy, fix your libc. */ > - h->fd = mkstemp (disk); > - if (h->fd >= 0) { > - h->fd = set_cloexec (h->fd); > - if (h->fd == -1) { > - int e = errno; > - unlink (disk); > - errno = e; > - } > - } > -#endif > - if (h->fd == -1) { > - nbdkit_error ("mkstemp: %m"); > + if (mkdtemp (dir) == NULL) {Hmm - even though we know Haiku still lacks mkostemp, https://github.com/haiku/haiku/blob/master/src/system/libroot/posix/stdlib/mktemp.c says that it at least has mkdtemp. Blindly using it for now is fine; if we later need to add a fallback for whatever platform lacks it, we can deal with it then. The use of mkdtemp is indeed safe.> + nbdkit_error ("%s: %m", dir); > goto error; > } > - > - /* Truncate the disk to a sparse file of the right size. */ > - if (ftruncate (h->fd, size) == -1) { > - nbdkit_error ("ftruncate: %s: %m", disk); > + if (asprintf (&disk, "%s/disk", dir) == -1) { > + nbdkit_error ("asprintf: %m"); > goto error; > } > > @@ -265,11 +290,34 @@ tmpdisk_open (int readonly) > if (run_command (disk) == -1) > goto error; > > + /* The external command must have created the disk, and then we must > + * find the true size. > + */ > + if (readonly) > + flags = O_RDONLY | O_CLOEXEC; > + else > + flags = O_RDWR | O_CLOEXEC | O_NOCTTY;It looks odd to use O_NOCTTY for write but not read. O_NOCTTY is important if we are worried that the user may have created a pty at $disk (in which case, we do not want that pty becoming the controlling terminal for nbdkit itself), but such a user is crazy ($command should normally create a regular file). And if we really are worried about O_NOCTTY, why are we not worried about O_NONBLOCK to avoid hanging if the user creates a FIFO? Are we also worried enough about the user trying to trick us into a symlink attack to use O_NOFOLLOW? I could live with just 'O_RDRW | O_CLOEXEC'. I suspect Haiku might need a followup for handling the fact that it lacks O_CLOEXEC, but saving it for a separate patch is fine.> + h->fd = open (disk, flags); > + if (h->fd == -1) { > + nbdkit_error ("open: %s: %m", disk); > + goto error; > + } > + > + if (fstat (h->fd, &statbuf) == -1) { > + nbdkit_error ("fstat: %s: %m", disk); > + goto error; > + } > + > + h->size = statbuf.st_size;Is this going to be the right size if command created a block device (either directly at $disk via mknod, or more likely elsewhere with $disk being a symlink to that alternate location)? Should we check fstat() results for S_ISREG/S_ISBLK to decide whether we need to lseek() to determine the actual size? But as to your question about whether the security looks reasonable, yes, I think your use of mkdtemp ensures that no one else is trampling on the image between $command creating it and when we finally open() our fd. Overall this looks like a sensible direction to head in. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org