search

jimsalterjrs / sanoid

These are policy-driven snapshot management and replication tools which use OpenZFS for underlying next-gen storage. (Btrfs support plans are shelved unless and until btrfs becomes reliable.)
http://www.openoid.net/products/
GNU General Public License v3.0
3.14k stars 308 forks source link

syncoid doesn't properly escape zfs property values before passing them to the shell #852

Closed akorn closed 10 months ago

akorn commented 1 year ago

In https://github.com/jimsalterjrs/sanoid/blob/61000c9da2f8762424ba2de8a49abb1b7ec5154d/syncoid#L496, the value of a zfs property is placed in a string and then apparently passed to a shell without quoting.

I noticed this when a user property contained whitespace and this caused the resulting zfs command line to fail; but under unlucky circumstances, or when some zfs properties are controlled by an adversary (e.g. due to zfs allow userprops, or because you're working with a pool that was created by someone else), the consequences could be worse.

Instead of the quick-and-dirty escapeshellparam function I recommend you use the established https://metacpan.org/dist/String-ShellQuote/source/ShellQuote.pm for safe shell escaping.

In the longer term, perhaps it would be better to construct the entire command line in an array and pass that to system(), not involving the shell at all.

phreaker0 commented 10 months ago

fixed it the dirty way for now. Would be nice to use a proper command builder instead of string building but this would need a complete rework .