Hi Jano! Em qui., 20 de fev. de 2020 às 12:57, Ján Tomko <jtomko@xxxxxxxxxx> escreveu: > > 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 Sorry, I usually search if someone proposed a similar patch but probably Google didn't find a 7 years patch hehe What is the problem that I'm trying to solve? Basically, I have some directories that already have pre-configured permissions with those extra bits. If I run `build`, libvirt will overwrite all of them. The same for reading and dumpxml. If someone has a better idea to solve this, I would appreciate a lot. :-) > > 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@xxxxxxxxx> > >--- > > 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 > > > >
