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 <jcfara...@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
signature.asc
Description: PGP signature
