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: Is it worth spelling this: nbdkit tmpdisk [size=]SIZE command=COMMAND [VAR=VALUE...] 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? Cool, even if it is time-consuming setup :) Ah, this addresses my comment above. Linked-list, where... ...the list is populated with later entries first, and... 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. ...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). Hmm - even though we know Haiku still lacks mkostemp, https://github.com/haiku/haiku/blob/master/src/system/libroot/posix/stdli... 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. 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. 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