2:51 p.m.
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. 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>`.
Signed-off-by: Julio Faracco <jcfaracco(a)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;
@@ -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",
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"),
@@ -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
9:57 a.m.
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>
---
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
11:36 a.m.
Hi Jano!
Em qui., 20 de fev. de 2020 às 12:57, Ján Tomko <jtomko(a)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(a)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
>
>
6:17 p.m.
On Fri, Feb 21, 2020 at 02:36:59PM -0300, Julio Faracco wrote:
Hi Jano!
Em qui., 20 de fev. de 2020 às 12:57, Ján Tomko <jtomko(a)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.
Glad to hear this can actually be useful for someone :)
If I run `build`, libvirt will overwrite all of them.
The same for reading and dumpxml.
Building a directory is handled in virStorageBackendBuildLocal,
a different code path than you're touching in src/storage.
It seems loosening up the parser checks is all it takes to get libvirt
to create directories with those permissions, which is why I suggested
checks in the callers of virStorageDefParsePerms to make sure we do it
because we want to allow it, not because we forgot to check it.
Same goes for every user of target.perms.mode
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
>
Jano
1734
days inactive
1739
days old
3 comments
2 participants
participants (2)
-
Julio Faracco -
Ján Tomko