From: Daniel Golle Date: Mon, 24 Aug 2026 02:06:10 +0000 (+0100) Subject: uvol: update to 1.1.1, harden backend invocation and volume-name handling X-Git-Url: http://git.99rst.org/?a=commitdiff_plain;h=f427fac2c4f4310d065445468e5d35da3a2ebc07;p=openwrt-packages.git 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 .; 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 --- diff --git a/utils/uvol/Makefile b/utils/uvol/Makefile index 3d0220319..02bf997c0 100644 --- 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 diff --git a/utils/uvol/files/lvm.uc b/utils/uvol/files/lvm.uc index 133daa2d1..cc751397c 100644 --- 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 69edd8c29..01f65a779 100644 --- 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 1b06bcdf7..44a7f141a 100644 --- 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;