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

Ján Tomko jtomko at redhat.com
Thu Feb 20 15:57:33 UTC 2020


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 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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200220/b34036fc/attachment-0001.sig>


More information about the libvir-list mailing list