[PATCH] storage: Add support to set{uid,gid} and sticky bit
Julio Faracco
jcfaracco at gmail.com
Fri Feb 21 17:36:59 UTC 2020
Hi Jano!
Em qui., 20 de fev. de 2020 às 12:57, Ján Tomko <jtomko at redhat.com> 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 at gmail.com>
> >---
> > 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
> >
> >
More information about the libvir-list
mailing list