Re: [PATCH] storage: Add support to set{uid,gid} and sticky bit

On Wed, Feb 19, 2020 at 05:51:44PM -0300, Julio Faracco wrote: >This commit add more features to storages that supports setuid, setgid >and sticky bit. This extend some permission levels of volumes when you >run an hypervisor using a specific user that can run but cannot delete >volumes for instance. I'm confused about the use case here - volumes should not be executable and setuid/setgid only make sense on executable files. >Additionally, when you create a directory without >`pool-build` command, you cannot import those extra permissions. >Example: > > # mkdir /var/lib/libvirt/images/ > # chmod 0755 /var/lib/libvirt/images/ > # chmod u+s /var/lib/libvirt/images/ > # pool-start default > # pool-dumpxml default > >No setuid from `<mode>0755</mode>`. >Output should expect `<mode>4755</mode>`. > FYI I proposed a similar patch ~7.5 years ago (and still haven't bothered to resend it O:-)): https://www.redhat.com/archives/libvir-list/2012-August/msg00687.html https://www.redhat.com/archives/libvir-list/2012-August/msg01004.html The consensus seemed to be * not wanting to touch the SGID/SUID bits * reporting the perms should be OK

For regular files, these bits seem to be useless for volumes, I think we should reject them. For directories, SGID and sticky might make sense >Signed-off-by: Julio Faracco <jcfaracco(a)gmail.com&gt; >--- > src/conf/storage_conf.c | 11 ++++++++--- > src/storage/storage_util.c | 12 ++++++++---- > 2 files changed, 16 insertions(+), 7 deletions(-) > >diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c >index 252d28cbfb..54e4a60ded 100644 >--- a/src/conf/storage_conf.c >+++ b/src/conf/storage_conf.c >@@ -746,7 +746,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, > if ((mode = virXPathString("string(./mode)", ctxt))) { > int tmp; > >- if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { >+ if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~07777)) { > virReportError(VIR_ERR_XML_ERROR, "%s", > _("malformed octal mode")); > goto error; To loosen this check, I'd rather see a new check added in the callers of this function, to make sure we won't allow creating a suid volume. >@@ -1187,9 +1187,14 @@ virStoragePoolDefFormatBuf(virBufferPtr buf, > def->target.perms.label) { > virBufferAddLit(buf, "<permissions>\n"); > virBufferAdjustIndent(buf, 2); >- if (def->target.perms.mode != (mode_t) -1) >- virBufferAsprintf(buf, "<mode>0%o</mode>\n", >+ if (def->target.perms.mode != (mode_t) -1) { >+ if (def->target.perms.mode & (S_ISUID | S_ISGID | S_ISVTX)) >+ virBufferAsprintf(buf, "<mode>%4o</mode>\n", Wouldn't this print it without the leading zero? > def->target.perms.mode); >+ else >+ virBufferAsprintf(buf, "<mode>0%o</mode>\n", >+ def->target.perms.mode); >+ } > if (def->target.perms.uid != (uid_t) -1) > virBufferAsprintf(buf, "<owner>%d</owner>\n", > (int) def->target.perms.uid); >diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c >index c2754dbb93..5352ab9120 100644 >--- a/src/storage/storage_util.c >+++ b/src/storage/storage_util.c >@@ -82,6 +82,10 @@ VIR_LOG_INIT("storage.storage_util"); > # define S_IRWXUGO (S_IRWXU | S_IRWXG | S_IRWXO) > #endif > >+#ifndef S_IALLUGO >+# define S_IALLUGO (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO) >+#endif >+ > /* virStorageBackendNamespaceInit: > * @poolType: virStoragePoolType > * @xmlns: Storage Pool specific namespace callback methods >@@ -512,7 +516,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, > > virCommandSetUID(cmd, vol->target.perms->uid); > virCommandSetGID(cmd, vol->target.perms->gid); >- virCommandSetUmask(cmd, S_IRWXUGO ^ mode); >+ virCommandSetUmask(cmd, S_IALLUGO ^ mode); > > if (virCommandRun(cmd, NULL) == 0) { > /* command was successfully run, check if the file was created */ >@@ -523,7 +527,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, > * If that doesn't match what we expect, then let's try to > * re-open the file and attempt to force the mode change. > */ >- if (mode != (st.st_mode & S_IRWXUGO)) { >+ if (mode != (st.st_mode & S_IALLUGO)) { > VIR_AUTOCLOSE fd = -1; > int flags = VIR_FILE_OPEN_FORK | VIR_FILE_OPEN_FORCE_MODE; > >@@ -569,7 +573,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, > goto cleanup; > } > >- if (mode != (st.st_mode & S_IRWXUGO) && >+ if (mode != (st.st_mode & S_IALLUGO) && > chmod(vol->target.path, mode) < 0) { > virReportSystemError(errno, > _("cannot set mode of '%s' to %04o"), The checks above are for volume creation and IMO should stay. Jano >@@ -1825,7 +1829,7 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, > > if (!target->perms && VIR_ALLOC(target->perms) < 0) > return -1; >- target->perms->mode = sb->st_mode & S_IRWXUGO; >+ target->perms->mode = sb->st_mode & S_IALLUGO; > target->perms->uid = sb->st_uid; > target->perms->gid = sb->st_gid; > >-- >2.20.1 > >