On Thu, Apr 07, 2022 at 08:39:13PM +0200, наб wrote:
> Thanks for the pointers!
Some other notes (sorry) ;)
> diff --git a/libmount/src/context_mount.c b/libmount/src/context_mount.c
> index f09e68860..13209d861 100644
> --- a/libmount/src/context_mount.c
> +++ b/libmount/src/context_mount.c
> @@ -1020,6 +1020,132 @@ static int do_mount_by_pattern(struct libmnt_context *cxt, const char *pattern)
> return rc;
> }
>
> +/* Extracted from mnt_optstr_get_uid() */
> +static int parse_ugid(const char *value, size_t valsz, unsigned *uid)
Can you move it to libmount/src/utils.c as mnt_parse_uid() and mnt_parse_gid() ?
And use mnt_parse_uid() in libmount/src/optstr.c:mnt_optstr_get_uid()?
It would be nice to have only one place (code) where we convert string
to uid (or gid).
Later we can re-use these functions for ID-mapping patches too.
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 :-)
> +{
> + char buf[sizeof(stringify_value(UINT64_MAX))];
> + int rc;
> + uint64_t num;
> +
> + assert(value);
> + assert(uid);
> +
> + if (valsz > sizeof(buf) - 1) {
> + rc = -ERANGE;
> + goto fail;
> + }
> + mem2strcpy(buf, value, valsz, sizeof(buf));
> +
> + rc = ul_strtou64(buf, &num, 10);
> + if (rc != 0) {
> + errno = ENOENT;
> + goto fail;
> + }
> + if (num > ULONG_MAX || (unsigned) num != num) {
|| (uid_t) num != num
for mnt_parse_uid() and
|| (gid_t) num != num
for mnt_parse_gid()
> + errno = ERANGE;
> + goto fail;
> + }
> + *uid = (unsigned) num;
> +
> + return 0;
> +fail:
> + DBG(UTILS, ul_debug("failed to convert '%.*s' to number [errno=%d]", (int) valsz, value, errno));
> + return -1;
> +}
> +
> +static int parse_mode(const char *value, size_t valsz, mode_t *uid)
Let's also introduce libmount/src/optstr.c:mnt_parse_mode() to be
consistent.
...
> +/*
> + * Process X-mount.owner=, X-mount.group=, X-mount.mode=.
> + */
> +static int parse_ownership_mode(struct libmnt_context *cxt)
> +{
> + int rc;
> +
> + const char *o = mnt_fs_get_user_options(cxt->fs);
> + if (!o)
> + return 0;
> +
> + char *owner;
> + size_t owner_len;
Please, please, define variables at the beginning of the code block.
> + if ((rc = mnt_optstr_get_option(o, "X-mount.owner", &owner, &owner_len)) < 0) {
> + return -MNT_ERR_MOUNTOPT;}
> + if (!rc) {
> + if (!owner_len)
> + return errno = EINVAL, -MNT_ERR_MOUNTOPT;
> +
> + char *owner_tofree = NULL;
here too
> + rc = mnt_get_uid(owner[owner_len] ? (owner_tofree = strndup(owner, owner_len)) : owner, &cxt->tgt_owner);
> + free(owner_tofree);
> + if (cxt->tgt_owner == (uid_t) -1 && isdigit(*owner))
> + rc = parse_ugid(owner, owner_len, &cxt->tgt_owner);
> + if (rc)
> + return -MNT_ERR_MOUNTOPT;
> + }
> +
> + char *group;
> + size_t group_len;
here too
> + if ((rc = mnt_optstr_get_option(o, "X-mount.group", &group, &group_len)) < 0)
> + return -MNT_ERR_MOUNTOPT;
> + if (!rc) {
> + if (!group_len)
> + return errno = EINVAL, -MNT_ERR_MOUNTOPT;
> +
> + char *group_tofree = NULL;
here too
> + rc = mnt_get_uid(group[group_len] ? (group_tofree = strndup(group, group_len)) : group, &cxt->tgt_group);
> + free(group_tofree);
> + if (cxt->tgt_group == (uid_t) -1 && isdigit(*group))
> + rc = parse_ugid(group, group_len, &cxt->tgt_group);
> + if (rc)
> + return errno = ENOENT, -MNT_ERR_MOUNTOPT;
> + }
> +
> + char *mode;
> + size_t mode_len;
here too
> + if ((rc = mnt_optstr_get_option(o, "X-mount.mode", &mode, &mode_len)) < 0)
> + return -MNT_ERR_MOUNTOPT;
> + if (!rc) {
> + if (!mode_len)
> + return errno = EINVAL, -MNT_ERR_MOUNTOPT;
> + if ((rc = parse_mode(mode, mode_len, &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 +1190,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 +1217,21 @@ 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)
Please, add here some debug message,
> + if (lchown(target, cxt->tgt_owner, cxt->tgt_group) == -1)
> + return -MNT_ERR_CHOWN;
> +
> + if (cxt->tgt_mode != (mode_t) -1)
and here too.
> + if (chmod(target, cxt->tgt_mode) == -1)
> + return -MNT_ERR_CHMOD;
> +
> + return 0;
> +}
> +
> /**
> * mnt_context_do_mount
> * @cxt: context
> @@ -1191,6 +1334,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 +1987,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;
> }
Yes, this is probably the best solution.
Thanks for your work. (Don't forget Signed-off-by:)
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
