[PATCH v3] libmount: accept X-mount.{owner,group,mode}=

[наб <nabijaczleweli@nabijaczleweli.xyz>]

[-- Attachment #1: Type: text/plain, Size: 18095 bytes --]

Parsing moved to utils.c,
optstr.c uses mnt_parse_uid(),
all declarations moved to top-of-block,
lchown() and chmod() have a debug message before them if they run.

Also, re-wrote the test (and actually added them this time :v).

On Tue, Apr 19, 2022 at 01:13:41PM +0200, Karel Zak wrote:
> In this case it would be nice to be strict and use uid_t and gid_t in
> the functions.
> 
> We already had CVE for this code, so I'd like to be paranoid here :-)

While in general I agree, and I wanted to do it Properly, util-linux is
already very cavalier about this and essentially asserts that [ug]id_t
is unsigned (or at least the same size as an unsigned; if that ever
changes you're gonna be in CVE city anyway), so I originally gave up
trying after not finding any library support for it.

Nevertheless, demonomorphised (but the bodies are essentially the same).

Updated scissor-patch below.

Best,
наб

-- >8 --
Which take a user, group, and mode, respectively, and set them on the
target after mounting

This is vaguely similar to tmpfs(5)'s [ug]id= and mode= options,
but we POSIX-parse the user- and group names

Oft requested in systemd/zram-generator, since a common use-case
is to use it to create /tmp or an equivalent directory that needs
to be a=rwx,o+t (or a user's private temp that needs to be owned
by them) ‒ this is impossible without terrible hacks, cf.
https://github.com/systemd/zram-generator/issues/150,
https://github.com/systemd/zram-generator/issues/146, &c.

This started off as a Set{User,Group,Mode}= systemd mount unit,
but was poetterung into libmount options:
https://github.com/systemd/systemd/pull/22889

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 libmount/src/context.c             |  12 ++-
 libmount/src/context_mount.c       |  87 +++++++++++++++++++-
 libmount/src/libmount.h.in         |  13 ++-
 libmount/src/mountP.h              |   7 ++
 libmount/src/optstr.c              |  19 +----
 libmount/src/utils.c               | 126 +++++++++++++++++++++++++++++
 sys-utils/mount.8.adoc             |   6 ++
 tests/expected/mount/set_ugid_mode |   1 +
 tests/ts/mount/set_ugid_mode       |  65 +++++++++++++++
 9 files changed, 315 insertions(+), 21 deletions(-)
 create mode 100644 tests/expected/mount/set_ugid_mode
 create mode 100755 tests/ts/mount/set_ugid_mode

diff --git a/libmount/src/context.c b/libmount/src/context.c
index 99ca58b9f..7927012d2 100644
--- a/libmount/src/context.c
+++ b/libmount/src/context.c
@@ -57,6 +57,10 @@ struct libmnt_context *mnt_new_context(void)
 	if (!cxt)
 		return NULL;
 
+	cxt->tgt_owner = (uid_t) -1;
+	cxt->tgt_group = (gid_t) -1;
+	cxt->tgt_mode = (mode_t) -1;
+
 	INIT_LIST_HEAD(&cxt->addmounts);
 
 	ruid = getuid();
@@ -76,7 +80,6 @@ struct libmnt_context *mnt_new_context(void)
 	DBG(CXT, ul_debugobj(cxt, "----> allocate %s",
 				cxt->restricted ? "[RESTRICTED]" : ""));
 
-
 	return cxt;
 }
 
@@ -130,7 +133,6 @@ void mnt_free_context(struct libmnt_context *cxt)
  *	mnt_context_set_options_pattern(cxt, NULL);
  *	mnt_context_set_target_ns(cxt, NULL);
  *
- *
  * to reset this stuff.
  *
  * Returns: 0 on success, negative number in case of error.
@@ -155,6 +157,10 @@ int mnt_reset_context(struct libmnt_context *cxt)
 	free(cxt->orig_user);
 	free(cxt->subdir);
 
+	cxt->tgt_owner = (uid_t) -1;
+	cxt->tgt_group = (gid_t) -1;
+	cxt->tgt_mode = (mode_t) -1;
+
 	cxt->fs = NULL;
 	cxt->mtab = NULL;
 	cxt->utab = NULL;
@@ -3108,7 +3114,7 @@ static void close_ns(struct libmnt_ns *ns)
  *
  * This function sets errno to ENOSYS and returns error if libmount is
  * compiled without namespaces support.
-*
+ *
  * Returns: 0 on success, negative number in case of error.
  *
  * Since: 2.33
diff --git a/libmount/src/context_mount.c b/libmount/src/context_mount.c
index f09e68860..6af83e041 100644
--- a/libmount/src/context_mount.c
+++ b/libmount/src/context_mount.c
@@ -837,7 +837,7 @@ static int do_mount(struct libmnt_context *cxt, const char *try_type)
 		cxt->syscall_status = 0;
 
 		DBG(CXT, ul_debugobj(cxt, "FAKE mount(2) "
-				"[source=%s, target=%s, type=%s, "
+				"[source=%s, target=%s, type=%s,"
 				" mountflags=0x%08lx, mountdata=%s]",
 				src, target, type,
 				flags, cxt->mountdata ? "yes" : "<none>"));
@@ -862,7 +862,7 @@ static int do_mount(struct libmnt_context *cxt, const char *try_type)
 		}
 
 		DBG(CXT, ul_debugobj(cxt, "mount(2) "
-			"[source=%s, target=%s, type=%s, "
+			"[source=%s, target=%s, type=%s,"
 			" mountflags=0x%08lx, mountdata=%s]",
 			src, target, type,
 			flags, cxt->mountdata ? "yes" : "<none>"));
@@ -1020,6 +1020,54 @@ static int do_mount_by_pattern(struct libmnt_context *cxt, const char *pattern)
 	return rc;
 }
 
+/*
+ * Process X-mount.owner=, X-mount.group=, X-mount.mode=.
+ */
+static int parse_ownership_mode(struct libmnt_context *cxt)
+{
+	int rc;
+	char *value;
+	size_t valsz;
+
+	const char *o = mnt_fs_get_user_options(cxt->fs);
+	if (!o)
+		return 0;
+
+	if ((rc = mnt_optstr_get_option(o, "X-mount.owner", &value, &valsz)) < 0)
+		return -MNT_ERR_MOUNTOPT;
+	if (!rc) {
+		if (!valsz)
+			return errno = EINVAL, -MNT_ERR_MOUNTOPT;
+
+		if (mnt_parse_uid(value, valsz, &cxt->tgt_owner))
+			return -MNT_ERR_MOUNTOPT;
+	}
+
+	if ((rc = mnt_optstr_get_option(o, "X-mount.group", &value, &valsz)) < 0)
+		return -MNT_ERR_MOUNTOPT;
+	if (!rc) {
+		if (!valsz)
+			return errno = EINVAL, -MNT_ERR_MOUNTOPT;
+
+		if (mnt_parse_gid(value, valsz, &cxt->tgt_group))
+			return -MNT_ERR_MOUNTOPT;
+	}
+
+	if ((rc = mnt_optstr_get_option(o, "X-mount.mode", &value, &valsz)) < 0)
+		return -MNT_ERR_MOUNTOPT;
+	if (!rc) {
+		if (!valsz)
+			return errno = EINVAL, -MNT_ERR_MOUNTOPT;
+
+		if ((rc = mnt_parse_mode(value, valsz, &cxt->tgt_mode)))
+			return -MNT_ERR_MOUNTOPT;
+	}
+
+	DBG(CXT, ul_debugobj(cxt, "ownership %d:%d, mode %04o", cxt->tgt_owner, cxt->tgt_group, cxt->tgt_mode));
+
+	return 0;
+}
+
 /**
  * mnt_context_prepare_mount:
  * @cxt: context
@@ -1064,6 +1112,8 @@ int mnt_context_prepare_mount(struct libmnt_context *cxt)
 		rc = mnt_context_guess_fstype(cxt);
 	if (!rc)
 		rc = mnt_context_prepare_target(cxt);
+	if (!rc)
+		rc = parse_ownership_mode(cxt);
 	if (!rc)
 		rc = mnt_context_prepare_helper(cxt, "mount", NULL);
 
@@ -1089,6 +1139,25 @@ end:
 	return rc;
 }
 
+static int set_ownership_mode(struct libmnt_context *cxt)
+{
+	const char *target = mnt_fs_get_target(cxt->fs);
+
+	if (cxt->tgt_owner != (uid_t) -1 || cxt->tgt_group != (uid_t) -1) {
+		DBG(CXT, ul_debugobj(cxt, "mount: lchown(%s, %u, %u)", target, cxt->tgt_owner, cxt->tgt_group));
+		if (lchown(target, cxt->tgt_owner, cxt->tgt_group) == -1)
+			return -MNT_ERR_CHOWN;
+	}
+
+	if (cxt->tgt_mode != (mode_t) -1) {
+		DBG(CXT, ul_debugobj(cxt, "mount: chmod(%s, %04o)", target, cxt->tgt_mode));
+		if (chmod(target, cxt->tgt_mode) == -1)
+			return -MNT_ERR_CHMOD;
+	}
+
+	return 0;
+}
+
 /**
  * mnt_context_do_mount
  * @cxt: context
@@ -1191,6 +1260,9 @@ int mnt_context_do_mount(struct libmnt_context *cxt)
 	if (mnt_context_is_veritydev(cxt))
 		mnt_context_deferred_delete_veritydev(cxt);
 
+	if (!res)
+		res = set_ownership_mode(cxt);
+
 	if (!mnt_context_switch_ns(cxt, ns_old))
 		return -MNT_ERR_NAMESPACE;
 
@@ -1841,7 +1913,18 @@ int mnt_context_get_mount_excode(
 			if (buf)
 				snprintf(buf, bufsz, _("filesystem was mounted, but failed to switch namespace back"));
 			return MNT_EX_SYSERR;
+		}
+
+		if (rc == -MNT_ERR_CHOWN) {
+			if (buf)
+				snprintf(buf, bufsz, _("filesystem was mounted, but failed to change ownership to %u:%u: %m"), cxt->tgt_owner, cxt->tgt_group);
+			return MNT_EX_SYSERR;
+		}
 
+		if (rc == -MNT_ERR_CHMOD) {
+			if (buf)
+				snprintf(buf, bufsz, _("filesystem was mounted, but failed to change mode to %04o: %m"), cxt->tgt_mode);
+			return MNT_EX_SYSERR;
 		}
 
 		if (rc < 0)
diff --git a/libmount/src/libmount.h.in b/libmount/src/libmount.h.in
index 43d8e44ce..0ab1d6ece 100644
--- a/libmount/src/libmount.h.in
+++ b/libmount/src/libmount.h.in
@@ -220,6 +220,18 @@ enum {
  * filesystem mounted, but --onlyonce specified
  */
 #define MNT_ERR_ONLYONCE    5010
+/**
+ * MNT_ERR_CHOWN:
+ *
+ * filesystem mounted, but subsequent X-mount.owner=/X-mount.group= lchown(2) failed
+ */
+#define MNT_ERR_CHOWN    5011
+/**
+ * MNT_ERR_CHMOD:
+ *
+ * filesystem mounted, but subsequent X-mount.mode= chmod(2) failed
+ */
+#define MNT_ERR_CHMOD    5012
 
 
 /*
@@ -246,7 +258,6 @@ enum {
  *
  * [u]mount(8) exit code: out of memory, cannot fork, ...
  */
-
 #define MNT_EX_SYSERR	2
 
 /**
diff --git a/libmount/src/mountP.h b/libmount/src/mountP.h
index 561ddcd8c..26158e8d9 100644
--- a/libmount/src/mountP.h
+++ b/libmount/src/mountP.h
@@ -109,6 +109,9 @@ extern int mnt_chdir_to_parent(const char *target, char **filename);
 extern char *mnt_get_username(const uid_t uid);
 extern int mnt_get_uid(const char *username, uid_t *uid);
 extern int mnt_get_gid(const char *groupname, gid_t *gid);
+extern int mnt_parse_uid(const char *user, size_t user_len, uid_t *gid);
+extern int mnt_parse_gid(const char *group, size_t group_len, gid_t *gid);
+extern int mnt_parse_mode(const char *mode, size_t mode_len, mode_t *gid);
 extern int mnt_in_group(gid_t gid);
 
 extern int mnt_open_uniq_filename(const char *filename, char **name);
@@ -294,6 +297,10 @@ struct libmnt_context
 
 	char	*subdir;		/* X-mount.subdir= */
 
+	uid_t	tgt_owner;		/* X-mount.owner= */
+	gid_t	tgt_group;		/* X-mount.group= */
+	mode_t	tgt_mode;		/* X-mount.mode= */
+
 	struct libmnt_fs *fs;		/* filesystem description (type, mountpoint, device, ...) */
 	struct libmnt_fs *fs_template;	/* used for @fs on mnt_reset_context() */
 
diff --git a/libmount/src/optstr.c b/libmount/src/optstr.c
index 43b983ebb..0ad5bfdc6 100644
--- a/libmount/src/optstr.c
+++ b/libmount/src/optstr.c
@@ -1019,15 +1019,13 @@ int mnt_optstr_fix_user(char **optstr)
 /*
  * Converts value from @optstr addressed by @name to uid.
  *
- * Returns: 0 on success, 1 if not found, <0 on error
+ * Returns: 0 on success, <0 on error
  */
 int mnt_optstr_get_uid(const char *optstr, const char *name, uid_t *uid)
 {
 	char *value = NULL;
 	size_t valsz = 0;
-	char buf[sizeof(stringify_value(UINT64_MAX))];
 	int rc;
-	uint64_t num;
 
 	assert(optstr);
 	assert(name);
@@ -1037,20 +1035,11 @@ int mnt_optstr_get_uid(const char *optstr, const char *name, uid_t *uid)
 	if (rc != 0)
 		goto fail;
 
-	if (valsz > sizeof(buf) - 1) {
-		rc = -ERANGE;
+	rc = mnt_parse_uid(value, valsz, uid);
+	if (rc != 0) {
+		rc = -errno;
 		goto fail;
 	}
-	mem2strcpy(buf, value, valsz, sizeof(buf));
-
-	rc = ul_strtou64(buf, &num, 10);
-	if (rc != 0)
-		goto fail;
-	if (num > ULONG_MAX || (uid_t) num != num) {
-		rc = -ERANGE;
-		goto fail;
-	}
-	*uid = (uid_t) num;
 
 	return 0;
 fail:
diff --git a/libmount/src/utils.c b/libmount/src/utils.c
index ff3acfb55..a138abe7e 100644
--- a/libmount/src/utils.c
+++ b/libmount/src/utils.c
@@ -635,6 +635,132 @@ int mnt_get_gid(const char *groupname, gid_t *gid)
 	return rc;
 }
 
+static int parse_uid_numeric(const char *value, size_t valsz, uid_t *uid)
+{
+	uint64_t num;
+
+	assert(value);
+	assert(uid);
+
+	if (valsz > sizeof(stringify_value(UINT64_MAX)) - 1) {
+		errno = ERANGE;
+		goto fail;
+	}
+
+	if (ul_strtou64(value, &num, 10) != 0) {
+		errno = ENOENT;
+		goto fail;
+	}
+	if (num > ULONG_MAX || (uid_t) num != num) {
+		errno = ERANGE;
+		goto fail;
+	}
+	*uid = (uid_t) num;
+
+	return 0;
+fail:
+	DBG(UTILS, ul_debug("failed to convert '%s' to number [errno=%d]", value, errno));
+	return -1;
+}
+
+/* POSIX-parse user_len-sized user; -1 and errno set, or 0 on success */
+int mnt_parse_uid(const char *user, size_t user_len, uid_t *uid)
+{
+	char *user_tofree = NULL;
+	int rc;
+
+	if (user[user_len]) {
+		user = user_tofree = strndup(user, user_len);
+		if (!user)
+			return -1;
+	}
+
+	rc = mnt_get_uid(user, uid);
+	if (rc != 0 && isdigit(*user))
+		rc = parse_uid_numeric(user, user_len, uid);
+
+	free(user_tofree);
+	return rc;
+}
+
+static int parse_gid_numeric(const char *value, size_t valsz, gid_t *gid)
+{
+	uint64_t num;
+
+	assert(value);
+	assert(gid);
+
+	if (valsz > sizeof(stringify_value(UINT64_MAX)) - 1) {
+		errno = ERANGE;
+		goto fail;
+	}
+
+	if (ul_strtou64(value, &num, 10) != 0) {
+		errno = ENOENT;
+		goto fail;
+	}
+	if (num > ULONG_MAX || (gid_t) num != num) {
+		errno = ERANGE;
+		goto fail;
+	}
+	*gid = (gid_t) num;
+
+	return 0;
+fail:
+	DBG(UTILS, ul_debug("failed to convert '%s' to number [errno=%d]", value, errno));
+	return -1;
+}
+
+/* POSIX-parse group_len-sized group; -1 and errno set, or 0 on success */
+int mnt_parse_gid(const char *group, size_t group_len, gid_t *gid)
+{
+	char *group_tofree = NULL;
+	int rc;
+
+	if (group[group_len]) {
+		group = group_tofree = strndup(group, group_len);
+		if (!group)
+			return -1;
+	}
+
+	rc = mnt_get_gid(group, gid);
+	if (rc != 0 && isdigit(*group))
+		rc = parse_gid_numeric(group, group_len, gid);
+
+	free(group_tofree);
+	return rc;
+}
+
+int mnt_parse_mode(const char *mode, size_t mode_len, mode_t *uid)
+{
+	char buf[sizeof(stringify_value(UINT32_MAX))];
+	uint32_t num;
+
+	assert(mode);
+	assert(uid);
+
+	if (mode_len > sizeof(buf) - 1) {
+		errno = ERANGE;
+		goto fail;
+	}
+	mem2strcpy(buf, mode, mode_len, sizeof(buf));
+
+	if (ul_strtou32(buf, &num, 8) != 0) {
+		errno = EINVAL;
+		goto fail;
+	}
+	if (num > 07777) {
+		errno = ERANGE;
+		goto fail;
+	}
+	*uid = (mode_t) num;
+
+	return 0;
+fail:
+	DBG(UTILS, ul_debug("failed to convert '%.*s' to mode [errno=%d]", (int) mode_len, mode, errno));
+	return -1;
+}
+
 int mnt_in_group(gid_t gid)
 {
 	int rc = 0, n, i;
diff --git a/sys-utils/mount.8.adoc b/sys-utils/mount.8.adoc
index 7465fcd0d..5f0ddd05b 100644
--- a/sys-utils/mount.8.adoc
+++ b/sys-utils/mount.8.adoc
@@ -636,6 +636,12 @@ Allow to make a target directory (mountpoint) if it does not exist yet. The opti
 **X-mount.subdir=**__directory__::
 Allow mounting sub-directory from a filesystem instead of the root directory. For now, this feature is implemented by temporary filesystem root directory mount in unshared namespace and then bind the sub-directory to the final mount point and umount the root of the filesystem. The sub-directory mount shows up atomically for the rest of the system although it is implemented by multiple *mount*(2) syscalls. This feature is EXPERIMENTAL.
 
+*X-mount.owner*=_username_|_UID_, *X-mount.group*=_group_|_GID_::
+Set _mountpoint_'s ownership after mounting. Names resolved in the target mount namespace, see *-N*.
+
+*X-mount.mode*=_mode_::
+Set _mountpoint_'s mode after mounting.
+
 *nosymfollow*::
 Do not follow symlinks when resolving paths. Symlinks can still be created, and *readlink*(1), *readlink*(2), *realpath*(1), and *realpath*(3) all still work properly.
 
diff --git a/tests/expected/mount/set_ugid_mode b/tests/expected/mount/set_ugid_mode
new file mode 100644
index 000000000..35821117c
--- /dev/null
+++ b/tests/expected/mount/set_ugid_mode
@@ -0,0 +1 @@
+Success
diff --git a/tests/ts/mount/set_ugid_mode b/tests/ts/mount/set_ugid_mode
new file mode 100755
index 000000000..810f81f38
--- /dev/null
+++ b/tests/ts/mount/set_ugid_mode
@@ -0,0 +1,65 @@
+#!/bin/bash
+# SPDX-License-Identifier: 0BSD
+
+
+TS_TOPDIR="${0%/*}/../.."
+TS_DESC="X-mount.{owner,group,mode}="
+
+. $TS_TOPDIR/functions.sh
+ts_init "$*"
+
+ts_check_test_command "$TS_CMD_MOUNT"
+ts_check_test_command "$TS_CMD_UMOUNT"
+
+ts_skip_nonroot
+ts_check_losetup
+ts_check_prog "mkfs.ext2"
+ts_check_prog "id"
+ts_check_prog "ls"
+
+
+do_one() {
+	expected="$1"; shift
+	what="$1"; shift
+	where="$1"; shift
+	$TS_CMD_MOUNT "$@" "$what" "$where" >> $TS_OUTPUT 2>> $TS_ERRLOG
+	read -r m _ o g _ < <(ls -nd "$where")
+	actual="$m $o $g"
+	[ "$actual" = "$expected" ] || echo "$*: $actual != $expected" >> $TS_ERRLOG
+	$TS_CMD_UMOUNT "$where" >> $TS_OUTPUT 2>> $TS_ERRLOG
+}
+
+ts_device_init
+
+mkfs.ext2 "$TS_LODEV" > /dev/null 2>&1  || ts_die "Cannot make ext2 on $TS_LODEV"
+ts_device_has "TYPE" "ext2" "$TS_LODEV" || ts_die "Cannot find ext2 on $TS_LODEV"
+
+user_1="$(id -un 1)"
+group_2="$(id -gn 2)"
+
+
+mkdir -p "$TS_MOUNTPOINT"
+
+do_one "drwxr-xr-x 0 0"     "$TS_LODEV" "$TS_MOUNTPOINT"
+do_one "drwxr-xr-x 1 0"     "$TS_LODEV" "$TS_MOUNTPOINT" -o "X-mount.owner=$user_1"
+do_one "drwxr-xr-x 1 2"     "$TS_LODEV" "$TS_MOUNTPOINT" -o "X-mount.group=$group_2"
+do_one "d-w--wxr-T 132 2"   "$TS_LODEV" "$TS_MOUNTPOINT" -o "X-mount.owner=132,X-mount.mode=1234"
+do_one "d-ws-w---x 132 123" "$TS_LODEV" "$TS_MOUNTPOINT" -o "X-mount.mode=4321,X-mount.group=123"
+do_one "d-ws-w---x 1 321"   "$TS_LODEV" "$TS_MOUNTPOINT" -o "X-mount.owner=$user_1,X-mount.group=321"
+
+
+> "$TS_MOUNTPOINT/bind"
+> "$TS_MOUNTPOINT/bindsrc"
+
+do_one "-rw-r--r-- 0 0"     "$TS_MOUNTPOINT/bindsrc" "$TS_MOUNTPOINT/bind" --bind
+do_one "-rw-r--r-- 1 0"     "$TS_MOUNTPOINT/bindsrc" "$TS_MOUNTPOINT/bind" --bind -o "X-mount.owner=$user_1"
+do_one "-rw-r--r-- 1 2"     "$TS_MOUNTPOINT/bindsrc" "$TS_MOUNTPOINT/bind" --bind -o "X-mount.group=$group_2"
+do_one "--w--wxr-T 132 2"   "$TS_MOUNTPOINT/bindsrc" "$TS_MOUNTPOINT/bind" --bind -o "X-mount.owner=132,X-mount.mode=1234"
+do_one "--ws-w---x 132 123" "$TS_MOUNTPOINT/bindsrc" "$TS_MOUNTPOINT/bind" --bind -o "X-mount.mode=4321,X-mount.group=123"
+do_one "--wx-w---x 1 321"   "$TS_MOUNTPOINT/bindsrc" "$TS_MOUNTPOINT/bind" --bind -o "X-mount.owner=$user_1,X-mount.group=321"
+
+
+rm -fd "$TS_MOUNTPOINT/bind"  "$TS_MOUNTPOINT/bindsrc" "$TS_MOUNTPOINT"
+
+ts_log "Success"
+ts_finalize
-- 
2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]