uvol: update to 1.1.1, harden backend invocation and volume-name handling

Volume names were interpolated into shell command lines at every
backend call site, so a name containing shell metacharacters ran
arbitrary commands as root. ucode's system() takes an array and execs
it directly, so every call site that interpolates a name, a device or
a path now passes an argument vector instead. Three sites genuinely
want a shell and keep it, with their arguments quoted: the lvm()
helper reads its JSON report via popen(), the content-addressed digest
pipeline quotes the volume path, and taking a volume down attempts a
umount whether or not anything mounted it, so that one keeps its
redirection and is now shared by both backends instead of spelt out
per call site. Suppressed errors that were only hiding a useless
diagnostic are dropped, and the filesystem grow tool is now located by
searching PATH instead of asking a shell.

lvm matches lvname with its own regular expression, and a volume name
was placed in that expression raw, so a dot in a name matched any
character instead of itself: a volume called a.b resolved to an
unrelated axb. Dotted names are the norm, since uxc composes a data
volume as <container>.<volname>; the name is now escaped where the
selection expression is built.

Every lvs, vgs and pvs caller dereferenced straight into the JSON
report, so an lvm invocation that failed to produce one, or failed to
run at all, crashed with a reference error instead of an error code.
Both now report through the .retval every caller already checks.

Volume names reaching the command line are now restricted to
^[A-Za-z0-9_][A-Za-z0-9._-]*$: a leading dot is reserved for uvol's
own volumes such as .meta, and a leading dash would be read as an
option by the backend tools. Only the read-only verbs may name an
internal volume, closing remove, down and resize being able to target
the metadata volume, which the backends never rejected the way mount
registration does; uvol's own boot path calls the backend directly
and is unaffected. The check runs before the per-volume lock, which is
keyed on the volume name too, which is why the -j flag is now consumed
before both. A rejected name exits 22 rather than falling off the end
of the script, which reports success.

Reachable only by root today, through the command line or a volume
name composed by uxc from an author-controlled registration; fixed as
defence in depth.

Fixes: 6350c7bc6 ("uvol: replace with re-write in ucode")
Signed-off-by: Daniel Golle <redacted>

diff --git a/utils/uvol/Makefile b/utils/uvol/Makefile
index 3d022031998600cf4a1eda0ce59b0a872afa847b..02bf997c0eda43c2cb6e6ed320920257b5953001 100644 (file)
--- a/utils/uvol/Makefile
+++ b/utils/uvol/Makefile
@@ -1,7 +1,7 @@
 include $(TOPDIR)/rules.mk
 
 PKG_NAME:=uvol
-PKG_VERSION:=1.1
+PKG_VERSION:=1.1.1
 PKG_RELEASE:=1
 
 PKG_MAINTAINER:=Daniel Golle <daniel@makrotopia.org>

diff --git a/utils/uvol/files/lvm.uc b/utils/uvol/files/lvm.uc
index 133daa2d17cf461e775a1f4dde73c02ee528879e..cc751397c6546f62305ca730ea1a3bbb89c8c08a 100644 (file)
--- a/utils/uvol/files/lvm.uc
+++ b/utils/uvol/files/lvm.uc
@@ -17,7 +17,7 @@ function lvm(cmd, ...args) {
                let json_param = "";
                if (cmd in lvm_json_cmds)
                        json_param = "--reportformat json --units b ";
-               let stdout = fs.popen(sprintf("LVM_SUPPRESS_FD_WARNINGS=1 %s %s %s%s", lvm_exec, cmd, json_param, join(" ", args)));
+               let stdout = fs.popen(sprintf("LVM_SUPPRESS_FD_WARNINGS=1 %s %s %s%s", lvm_exec, cmd, json_param, join(" ", map(args, shell_quote))));
                let tmp;
                if (stdout) {
                        tmp = stdout.read("all");
@@ -37,7 +37,9 @@ function lvm(cmd, ...args) {
        } catch(e) {
                printf("Failed to parse lvm cli output: %s\n%s\n", e, e.stacktrace[0].context);
        }
-       return null;
+       // callers read .retval without checking, so failure is reported as a
+       // return value rather than as an absent object
+       return { retval: -1 };
 }
 
 function pvs() {
@@ -53,17 +55,17 @@ function pvs() {
        if (!rootdev)
                return null;
 
-       let tmp = lvm("pvs", "-o", "vg_name", "-S", sprintf("\"pv_name=~^/dev/%s.*\$\"", rootdev));
-       if (tmp.report.pv[0])
+       let tmp = lvm("pvs", "-o", "vg_name", "-S", sprintf("pv_name=~^/dev/%s.*$", rootdev));
+       if (tmp?.report?.pv?.[0])
                return tmp.report.pv[0].vg_name;
        else
                return null;
 }
 
 function vgs(vg_name) {
-       let tmp = lvm("vgs", "-o", "vg_extent_size,vg_extent_count,vg_free_count", "-S", sprintf("\"vg_name=%s\"", vg_name));
+       let tmp = lvm("vgs", "-o", "vg_extent_size,vg_extent_count,vg_free_count", "-S", sprintf("vg_name=%s", vg_name));
        let ret = null;
-       if (tmp && tmp.report.vg) {
+       if (tmp?.report?.vg) {
                ret = tmp.report.vg;
                for (let r in ret) {
                        r.vg_extent_size = +(rtrim(r.vg_extent_size, "B"));
@@ -77,15 +79,21 @@ function vgs(vg_name) {
                return null;
 }
 
+// lvm matches lvname with its own regex, so a name is escaped before it goes
+// in. name_valid() limits names to [A-Za-z0-9._-], of which only '.' means
+// anything to that parser; widen this if the permitted set ever widens.
+function lvname_quote(vol_name) {
+       return replace(`${vol_name}`, /\./g, "\\.");
+}
+
 function lvs(vg_name, vol_name, extra_exp) {
        let ret = [];
-       if (!vol_name)
-               vol_name = ".*";
+       vol_name = vol_name ? lvname_quote(vol_name) : ".*";
 
-       let lvexpr = sprintf("\"lvname=~^[rw][owp]_%s\$ && vg_name=%s%s%s\"",
+       let lvexpr = sprintf("lvname=~^[rw][owp]_%s$ && vg_name=%s%s%s",
                             vol_name, vg_name, extra_exp?" && ":"", extra_exp?extra_exp:"");
        let tmp = lvm("lvs", "-o", "lv_active,lv_name,lv_full_name,lv_size,lv_path,lv_dm_path", "-S", lvexpr);
-       if (tmp && tmp.report.lv) {
+       if (tmp?.report?.lv) {
                ret = tmp.report.lv;
                for (let r in ret) {
                        r.lv_size = +(rtrim(r.lv_size, "B"));
@@ -98,8 +106,8 @@ function lvs(vg_name, vol_name, extra_exp) {
 function lvs_deleting(vg_name) {
        let ret = [];
        let tmp = lvm("lvs", "-o", "lv_active,lv_name,lv_full_name,lv_dm_path", "-S",
-                     sprintf("\"lvname=~^dd_.* && vg_name=%s\"", vg_name));
-       if (tmp && tmp.report.lv) {
+                     sprintf("lvname=~^dd_.* && vg_name=%s", vg_name));
+       if (tmp?.report?.lv) {
                ret = tmp.report.lv;
                for (let r in ret)
                        r.lv_active = (r.lv_active == "active");
@@ -110,8 +118,9 @@ function lvs_deleting(vg_name) {
 function lvs_incomplete(vg_name, vol_name) {
        let ret = [];
        let tmp = lvm("lvs", "-o", "lv_active,lv_name,lv_full_name,lv_size", "-S",
-                     sprintf("\"lvname=~^w[op]_%s\$ && vg_name=%s\"", vol_name ?? ".*", vg_name));
-       if (tmp && tmp.report.lv) {
+                     sprintf("lvname=~^w[op]_%s$ && vg_name=%s",
+                             vol_name ? lvname_quote(vol_name) : ".*", vg_name));
+       if (tmp?.report?.lv) {
                ret = tmp.report.lv;
                for (let r in ret) {
                        r.lv_active = (r.lv_active == "active");
@@ -154,6 +163,8 @@ function getdev(lv) {
 function lvm_init(ctx) {
        cursor = ctx.cursor;
        fs = ctx.fs;
+       shell_quote = ctx.shell_quote;
+       umount_dev = ctx.umount_dev;
        if (type(fs.access) == "function" && !fs.access(lvm_exec, "x"))
                return false;
 
@@ -280,7 +291,7 @@ function lvm_updown(vol_name, up) {
                let devname = getdev(lv);
                if (devname) {
                        unregister(devname);
-                       system(sprintf("umount /dev/%s 2>/dev/null", devname));
+                       umount_dev(devname);
                }
        }
 
@@ -353,7 +364,7 @@ function lvm_create(vol_name, vol_size, vol_mode) {
 
        let use_f2fs = (lv.lv_size > (100 * 1024 * 1024));
        if (use_f2fs) {
-               let mkfs_ret = system(sprintf("/usr/sbin/mkfs.f2fs -f -l \"%s\" \"%s\"", vol_name, lv.lv_path));
+               let mkfs_ret = system([ "/usr/sbin/mkfs.f2fs", "-f", "-l", vol_name, lv.lv_path ]);
                if (mkfs_ret != 0 && mkfs_ret != 134) {
                        lvchange_r = lvm("lvchange", "-a", "n", lv.lv_full_name);
                        if (lvchange_r.retval != 0)
@@ -361,7 +372,7 @@ function lvm_create(vol_name, vol_size, vol_mode) {
                        return mkfs_ret;
                }
        } else {
-               let mkfs_ret = system(sprintf("/usr/sbin/mke2fs -F -t ext4 -O has_journal -L \"%s\" \"%s\"", vol_name, lv.lv_path));
+               let mkfs_ret = system([ "/usr/sbin/mke2fs", "-F", "-t", "ext4", "-O", "has_journal", "-L", vol_name, lv.lv_path ]);
                if (mkfs_ret != 0) {
                        lvchange_r = lvm("lvchange", "-a", "n", lv.lv_full_name);
                        if (lvchange_r.retval != 0)
@@ -398,6 +409,19 @@ function fs_type(devpath) {
        return null;
 }
 
+function tool_path(name) {
+       let dirs = split(getenv("PATH") ?? "", ":");
+       push(dirs, "/usr/sbin", "/usr/bin", "/sbin", "/bin");
+       for (let dir in dirs) {
+               if (!dir)
+                       continue;
+               let path = sprintf("%s/%s", dir, name);
+               if (fs.access(path, "x"))
+                       return path;
+       }
+       return null;
+}
+
 // Grow a rw volume in place; the fs (ext4 or f2fs, chosen at create by size) is
 // grown with its own tool. Shrink is refused. The fs-grow tool is an optional
 // dependency: if absent, report and refuse rather than grow the LV past the fs.
@@ -438,7 +462,8 @@ function lvm_resize(vol_name, vol_size) {
        }
 
        // the fs-grow tool first, so the LV is never left larger than its filesystem
-       if (system(sprintf("command -v %s >/dev/null 2>&1", tool)) != 0) {
+       let tool_exec = tool_path(tool);
+       if (!tool_exec) {
                warn(sprintf("uvol: %s not found; install %s to grow this %s volume\n",
                             tool, (fstype == "f2fs") ? "f2fs-tools" : "e2fsprogs", fstype));
                return 95;
@@ -448,10 +473,7 @@ function lvm_resize(vol_name, vol_size) {
        if (ret.retval != 0)
                return ret.retval;
 
-       if (fstype == "f2fs")
-               return system(sprintf("resize.f2fs /dev/%s", dev));
-
-       return system(sprintf("resize2fs /dev/%s", dev));
+       return system([ tool_exec, sprintf("/dev/%s", dev) ]);
 }
 
 // Reap volumes marked for deferred deletion (dd_ prefix). A volume can only be

diff --git a/utils/uvol/files/ubi.uc b/utils/uvol/files/ubi.uc
index 69edd8c29b42a64b5f2d56e91514ab32f791ae8e..01f65a779b15472048c2b404eb1d7f9d4934f507 100644 (file)
--- a/utils/uvol/files/ubi.uc
+++ b/utils/uvol/files/ubi.uc
@@ -27,6 +27,18 @@ function mkdtemp() {
        return dirname;
 }
 
+function ubirename(from_name, to_name) {
+       return system([ "ubirename", sprintf("/dev/%s", ubidev), from_name, to_name ]);
+}
+
+function ubirmvol(volnum) {
+       return system([ "ubirmvol", sprintf("/dev/%s", ubidev), "-n", sprintf("%d", +volnum) ]);
+}
+
+function ubiblock(action, vol_dev) {
+       return system([ "ubiblock", action, sprintf("/dev/%s", vol_dev) ]);
+}
+
 function ubi_get_dev(vol_name) {
        let wcstring = sprintf("uvol-[rw][owpd]-%s", vol_name);
        for (vol_dir in fs.glob(sprintf("/sys/class/ubi/%s_*", ubidev))) {
@@ -44,8 +56,8 @@ function vol_get_mode(vol_dev, mode) {
 
 function mkubifs(vol_dev) {
        let temp_mp = mkdtemp();
-       system(sprintf("mount -t ubifs /dev/%s %s", vol_dev, temp_mp));
-       system(sprintf("umount %s", temp_mp));
+       system([ "mount", "-t", "ubifs", sprintf("/dev/%s", vol_dev), temp_mp ]);
+       system([ "umount", temp_mp ]);
        fs.rmdir(temp_mp);
        return 0;
 }
@@ -74,6 +86,7 @@ function ubi_init(ctx) {
 
        register = ctx.register;
        unregister = ctx.unregister;
+       umount_dev = ctx.umount_dev;
 
        return true;
 }
@@ -149,7 +162,8 @@ function ubi_create(vol_name, vol_size, vol_mode) {
        if (vol_dev)
                return 17;
 
-       let ret = system(sprintf("ubimkvol /dev/%s -N \"uvol-%s-%s\" -s %d", ubidev, mode, vol_name, vol_size));
+       let ret = system([ "ubimkvol", sprintf("/dev/%s", ubidev), "-N",
+                          sprintf("uvol-%s-%s", mode, vol_name), "-s", sprintf("%d", vol_size) ]);
        if (ret != 0)
                return ret;
 
@@ -157,7 +171,7 @@ function ubi_create(vol_name, vol_size, vol_mode) {
        if (!vol_dev)
                return 2;
 
-       let ret = system(sprintf("ubiupdatevol -t /dev/%s", vol_dev));
+       let ret = system([ "ubiupdatevol", "-t", sprintf("/dev/%s", vol_dev) ]);
        if (ret != 0)
                return ret;
 
@@ -168,7 +182,7 @@ function ubi_create(vol_name, vol_size, vol_mode) {
        if (ret != 0)
                return ret;
 
-       let ret = system(sprintf("ubirename /dev/%s \"uvol-wp-%s\" \"uvol-wd-%s\"", ubidev, vol_name, vol_name));
+       let ret = ubirename(sprintf("uvol-wp-%s", vol_name), sprintf("uvol-wd-%s", vol_name));
        if (ret != 0)
                return ret;
 
@@ -199,7 +213,8 @@ function ubi_resize(vol_name, vol_size) {
        if (req_lebs < cur_lebs)
                return 22;
 
-       return system(sprintf("ubirsvol /dev/%s -N \"uvol-rw-%s\" -s %d", ubidev, vol_name, vol_size));
+       return system([ "ubirsvol", sprintf("/dev/%s", ubidev), "-N",
+                       sprintf("uvol-rw-%s", vol_name), "-s", sprintf("%d", vol_size) ]);
 }
 
 function ubi_get_deleting() {
@@ -243,8 +258,7 @@ function ubi_purge_incomplete(vol_name, match_size) {
                        if (v.lebs != want)
                                continue;
                }
-               let volnum = split(v.dev, "_")[1];
-               system(sprintf("ubirmvol /dev/%s -n %d 2>/dev/null", ubidev, volnum));
+               ubirmvol(split(v.dev, "_")[1]);
        }
        return 0;
 }
@@ -255,13 +269,13 @@ function ubi_reap() {
                let blkdev = sprintf("ubiblock%s", substr(vol_dev, 3));
                let isblock = fs.access(sprintf("/dev/%s", blkdev), "r");
 
-               if (isblock && system(sprintf("ubiblock --remove /dev/%s 2>/dev/null", vol_dev)) != 0)
+               if (isblock && ubiblock("--remove", vol_dev) != 0)
                        continue;
 
                if (!isblock)
-                       system(sprintf("umount /dev/%s 2>/dev/null", vol_dev));
+                       umount_dev(vol_dev);
 
-               if (system(sprintf("ubirmvol /dev/%s -n %d 2>/dev/null", ubidev, volnum)) != 0)
+               if (ubirmvol(volnum) != 0)
                        continue;
 
                unregister(isblock ? blkdev : vol_dev);
@@ -275,7 +289,7 @@ function ubi_remove(vol_name) {
                return 2;
 
        let vol_ubiname = read_file(sprintf("/sys/class/ubi/%s/name", vol_dev));
-       let ret = system(sprintf("ubirename /dev/%s \"%s\" \"uvol-dd-%s\"", ubidev, vol_ubiname, vol_name));
+       let ret = ubirename(vol_ubiname, sprintf("uvol-dd-%s", vol_name));
        if (ret != 0)
                return ret;
 
@@ -296,17 +310,17 @@ function ubi_up(vol_name) {
                return 16;
 
        if (vol_mode == "rd") {
-               let ret = system(sprintf("ubirename /dev/%s \"uvol-rd-%s\" \"uvol-ro-%s\"", ubidev, vol_name, vol_name));
+               let ret = ubirename(sprintf("uvol-rd-%s", vol_name), sprintf("uvol-ro-%s", vol_name));
                if (ret != 0)
                        return ret;
 
-               ret = system(sprintf("ubiblock --create /dev/%s", vol_dev));
+               ret = ubiblock("--create", vol_dev);
                if (ret != 0)
                        return ret;
 
                return register(vol_name, sprintf("ubiblock%s", substr(vol_dev, 3)), true);
        } else if (vol_mode == "wd") {
-               let ret = system(sprintf("ubirename /dev/%s \"uvol-wd-%s\" \"uvol-rw-%s\"", ubidev, vol_name, vol_name));
+               let ret = ubirename(sprintf("uvol-wd-%s", vol_name), sprintf("uvol-rw-%s", vol_name));
                if (ret != 0)
                        return ret;
 
@@ -329,14 +343,14 @@ function ubi_down(vol_name) {
                return 16;
        else if (vol_mode == "ro") {
                unregister(sprintf("ubiblock%s", substr(vol_dev, 3)));
-               system(sprintf("umount /dev/ubiblock%s 2>&1 >/dev/null", substr(vol_dev, 3)));
-               system(sprintf("ubiblock --remove /dev/%s", vol_dev));
-               let ret = system(sprintf("ubirename /dev/%s \"uvol-ro-%s\" \"uvol-rd-%s\"", ubidev, vol_name, vol_name));
+               umount_dev(sprintf("ubiblock%s", substr(vol_dev, 3)));
+               ubiblock("--remove", vol_dev);
+               let ret = ubirename(sprintf("uvol-ro-%s", vol_name), sprintf("uvol-rd-%s", vol_name));
                return ret;
        } else if (vol_mode == "rw") {
                unregister(vol_dev);
-               system(sprintf("umount /dev/%s 2>&1 >/dev/null", vol_dev));
-               let ret = system(sprintf("ubirename /dev/%s \"uvol-rw-%s\" \"uvol-wd-%s\"", ubidev, vol_name, vol_name));
+               umount_dev(vol_dev);
+               let ret = ubirename(sprintf("uvol-rw-%s", vol_name), sprintf("uvol-wd-%s", vol_name));
                return ret;
        }
        return 0;
@@ -375,7 +389,7 @@ function ubi_register_active() {
                let vol_mode = substr(vol_ubiname, 5, 2);
 
                if (vol_mode == "ro") {
-                       system(sprintf("ubiblock --create /dev/%s", vol_dev));
+                       ubiblock("--create", vol_dev);
                        register(vol_name, sprintf("ubiblock%s", substr(vol_dev, 3)), true);
                } else {
                        register(vol_name, vol_dev, false);
@@ -408,14 +422,15 @@ function ubi_write(vol_name, write_size, verify) {
        if (vol_mode != "wo")
                return 22;
 
-       let ret = system(sprintf("ubiupdatevol -s %d /dev/%s -", write_size, vol_dev));
+       let ret = system([ "ubiupdatevol", "-s", sprintf("%d", write_size),
+                          sprintf("/dev/%s", vol_dev), "-" ]);
        if (ret)
                return ret;
 
        if (verify && !verify(sprintf("/dev/%s", vol_dev)))
                return 74;
 
-       system(sprintf("ubirename /dev/%s \"uvol-wo-%s\" \"uvol-rd-%s\"", ubidev, vol_name, vol_name));
+       ubirename(sprintf("uvol-wo-%s", vol_name), sprintf("uvol-rd-%s", vol_name));
 
        return 0;
 }

diff --git a/utils/uvol/files/uvol b/utils/uvol/files/uvol
index 1b06bcdf7ab4e933a34a6a3d994a218f83f0a992..44a7f141a404432dd76a4d0ad5cbab03b8608934 100644 (file)
--- a/utils/uvol/files/uvol
+++ b/utils/uvol/files/uvol
@@ -47,12 +47,36 @@ let fs = require("fs");
 let uci = require("uci");
 let cursor = uci ? uci.cursor() : null;
 
+let shell_quote = function(word) {
+       return "'" + replace(`${word}`, /'/g, "'\\''") + "'";
+};
+
+// volumes are taken down whether or not anything mounted them, so umount is
+// expected to fail and stays quiet. That needs a shell, hence the quoting.
+let umount_dev = function(dev) {
+       return system(sprintf("umount %s 2>/dev/null", shell_quote(sprintf("/dev/%s", dev))));
+};
+
+// volume names reach tool arguments and lock file paths; anything outside
+// this set is refused before it gets there. A leading '.' is reserved for
+// internal volumes such as .meta, and a leading '-' would be taken for an
+// option by the backend tools.
+let name_valid = function(name, allow_internal) {
+       if (type(name) != "string")
+               return false;
+       if (allow_internal && name == ".meta")
+               return true;
+       return !!match(name, /^[A-Za-z0-9_][A-Za-z0-9._-]*$/);
+};
+
 let ctx = {};
 ctx.cursor = cursor;
 ctx.fs = fs;
 include("/usr/lib/uvol/mount.uc");
 ctx.register = uvol_mount.register;
 ctx.unregister = uvol_mount.unregister;
+ctx.shell_quote = shell_quote;
+ctx.umount_dev = umount_dev;
 
 let backend = null;
 let tried_backends = [];
@@ -83,6 +107,24 @@ if (!(cmd in keys(backend))) {
        return 22;
 }
 
+let json_output = false;
+if (ARGV[0] == "-j") {
+       json_output = true;
+       shift(ARGV);
+}
+
+// only the read-only verbs may name an internal volume such as .meta; the
+// check runs before the lock, which is keyed on the volume name too
+let ro_name_cmds = [ "status", "device", "size", "list" ];
+let rw_name_cmds = [ "create", "remove", "up", "down", "resize", "write" ];
+let ro_name = index(ro_name_cmds, cmd) >= 0;
+if (index(rw_name_cmds, cmd) >= 0 || (ro_name && ARGV[0] != null)) {
+       if (!name_valid(ARGV[0], ro_name)) {
+               printf("invalid volume name\n");
+               exit(22);
+       }
+}
+
 // serialise state-mutating commands device-wide; released on process exit.
 // create and write additionally serialise per volume, as create may reclaim
 // an incomplete volume of the same name and size while a writer is still
@@ -127,12 +169,6 @@ if (cmd == "boot") {
        exit(0);
 }
 
-let json_output = false;
-if (ARGV[0] == "-j") {
-       json_output = true;
-       shift(ARGV);
-}
-
 let legacy_output = function(var) {
        let out = "";
        if (type(var) == "array") {
@@ -168,7 +204,7 @@ let ca_parse = function(name) {
 };
 
 let ca_digest = function(cmd, size, path) {
-       let src = path ? sprintf("head -c %d %s", size, path) : sprintf("head -c %d", size);
+       let src = path ? sprintf("head -c %d %s", size, shell_quote(path)) : sprintf("head -c %d", size);
        let f = fs.popen(sprintf("%s | %s", src, cmd), "r");
        if (!f)
                return null;