[libvirt] [PATCH] storage: support all file permissions
sticky, setuid and setgid are no longer ignored. --- src/conf/storage_conf.c | 6 ++++-- src/storage/storage_backend.c | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 7944555..c492106 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1035,7 +1035,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) { virBufferAddLit(&buf," <permissions>\n"); virBufferAsprintf(&buf," <mode>0%o</mode>\n", - def->target.perms.mode); + def->target.perms.mode & + (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO)); virBufferAsprintf(&buf," <owner>%u</owner>\n", (unsigned int) def->target.perms.uid); virBufferAsprintf(&buf," <group>%u</group>\n", @@ -1275,7 +1276,8 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAddLit(buf," <permissions>\n"); virBufferAsprintf(buf," <mode>0%o</mode>\n", - def->perms.mode); + def->perms.mode & + (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO)); virBufferAsprintf(buf," <owner>%u</owner>\n", (unsigned int) def->perms.uid); virBufferAsprintf(buf," <group>%u</group>\n", diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e677cda..e9ec7a5 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1210,7 +1210,8 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, } } - target->perms.mode = sb.st_mode & S_IRWXUGO; + target->perms.mode = sb.st_mode & + (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO); target->perms.uid = sb.st_uid; target->perms.gid = sb.st_gid; -- 1.7.8.6
sticky, setuid and setgid are no longer ignored. --- v2: reading these permissions from XML works too --- src/conf/storage_conf.c | 9 ++++++--- src/storage/storage_backend.c | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 7944555..0335f99 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -665,7 +665,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, } else { int tmp; - if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { + if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || + (tmp & ~(S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO))) { VIR_FREE(mode); virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed octal mode")); @@ -1035,7 +1036,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) { virBufferAddLit(&buf," <permissions>\n"); virBufferAsprintf(&buf," <mode>0%o</mode>\n", - def->target.perms.mode); + def->target.perms.mode & + (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO)); virBufferAsprintf(&buf," <owner>%u</owner>\n", (unsigned int) def->target.perms.uid); virBufferAsprintf(&buf," <group>%u</group>\n", @@ -1275,7 +1277,8 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAddLit(buf," <permissions>\n"); virBufferAsprintf(buf," <mode>0%o</mode>\n", - def->perms.mode); + def->perms.mode & + (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO)); virBufferAsprintf(buf," <owner>%u</owner>\n", (unsigned int) def->perms.uid); virBufferAsprintf(buf," <group>%u</group>\n", diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e677cda..e9ec7a5 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1210,7 +1210,8 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, } } - target->perms.mode = sb.st_mode & S_IRWXUGO; + target->perms.mode = sb.st_mode & + (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO); target->perms.uid = sb.st_uid; target->perms.gid = sb.st_gid; -- 1.7.8.6
On 07/26/12 10:52, Ján Tomko wrote:
sticky, setuid and setgid are no longer ignored.
--- v2: reading these permissions from XML works too --- src/conf/storage_conf.c | 9 ++++++--- src/storage/storage_backend.c | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-)
ping
On 07/26/2012 04:52 AM, Ján Tomko wrote:
sticky, setuid and setgid are no longer ignored.
I'm always automatically wary of any code that allows setting the suid bit, in case it may allow some new security hole. I can't think of anything specific that could be allowed by setting the suid bit of a directory containing a disk image, but then again I haven't thought about it very hard :-) Can anyone convince me one way or the other? It might help to learn why you want to be able to set these bits. libvirt is generally very specific about explicitly setting the uid of disk images properly as required at runtime...
--- v2: reading these permissions from XML works too --- src/conf/storage_conf.c | 9 ++++++--- src/storage/storage_backend.c | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 7944555..0335f99 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -665,7 +665,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, } else { int tmp;
- if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { + if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || + (tmp & ~(S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO))) { VIR_FREE(mode); virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed octal mode")); @@ -1035,7 +1036,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) {
virBufferAddLit(&buf," <permissions>\n"); virBufferAsprintf(&buf," <mode>0%o</mode>\n", - def->target.perms.mode); + def->target.perms.mode & + (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO)); virBufferAsprintf(&buf," <owner>%u</owner>\n", (unsigned int) def->target.perms.uid); virBufferAsprintf(&buf," <group>%u</group>\n", @@ -1275,7 +1277,8 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
virBufferAddLit(buf," <permissions>\n"); virBufferAsprintf(buf," <mode>0%o</mode>\n", - def->perms.mode); + def->perms.mode & + (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO)); virBufferAsprintf(buf," <owner>%u</owner>\n", (unsigned int) def->perms.uid); virBufferAsprintf(buf," <group>%u</group>\n", diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e677cda..e9ec7a5 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1210,7 +1210,8 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, } }
- target->perms.mode = sb.st_mode & S_IRWXUGO; + target->perms.mode = sb.st_mode & + (S_ISUID | S_ISGID | S_ISVTX | S_IRWXUGO); target->perms.uid = sb.st_uid; target->perms.gid = sb.st_gid;
On 08/07/12 07:17, Laine Stump wrote:
On 07/26/2012 04:52 AM, Ján Tomko wrote:
sticky, setuid and setgid are no longer ignored.
I'm always automatically wary of any code that allows setting the suid bit, in case it may allow some new security hole. I can't think of anything specific that could be allowed by setting the suid bit of a directory containing a disk image, but then again I haven't thought about it very hard :-) Can anyone convince me one way or the other?
SUID on directories is ignored on most systems, but you could be able to create files belonging a group you're not a member of. But this patch enables it on files too, allowing everyone with access to privileged libvirtd to create SUID files. I don't know if it's possible to exploit this, but I don't like it, so NACK NACK NACK.
It might help to learn why you want to be able to set these bits. libvirt is generally very specific about explicitly setting the uid of disk images properly as required at runtime...
My issue was that vol-dumpxml reported wrong file permissions, as described in https://bugzilla.redhat.com/show_bug.cgi?id=839463 Writing the sticky bit seems harmless to me. Would it be safe to just read SGID and SUID without ever setting them? Or is it not worth the risk? Ján Tomko
On Tue, Aug 07, 2012 at 10:27:15AM +0200, Ján Tomko wrote:
On 08/07/12 07:17, Laine Stump wrote:
On 07/26/2012 04:52 AM, Ján Tomko wrote:
sticky, setuid and setgid are no longer ignored.
I'm always automatically wary of any code that allows setting the suid bit, in case it may allow some new security hole. I can't think of anything specific that could be allowed by setting the suid bit of a directory containing a disk image, but then again I haven't thought about it very hard :-) Can anyone convince me one way or the other?
SUID on directories is ignored on most systems, but you could be able to create files belonging a group you're not a member of.
But this patch enables it on files too, allowing everyone with access to privileged libvirtd to create SUID files. I don't know if it's possible to exploit this, but I don't like it, so NACK NACK NACK.
It might help to learn why you want to be able to set these bits. libvirt is generally very specific about explicitly setting the uid of disk images properly as required at runtime...
My issue was that vol-dumpxml reported wrong file permissions, as described in https://bugzilla.redhat.com/show_bug.cgi?id=839463
Writing the sticky bit seems harmless to me. Would it be safe to just read SGID and SUID without ever setting them? Or is it not worth the risk?
IMO we should preserve the existing behavior of not modifying the bits, but we should report them correctly, although I don't feel all that strongly about it. Dave
Ján Tomko
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 08/07/2012 10:40 AM, Dave Allan wrote:
On Tue, Aug 07, 2012 at 10:27:15AM +0200, Ján Tomko wrote:
On 08/07/12 07:17, Laine Stump wrote:
sticky, setuid and setgid are no longer ignored. I'm always automatically wary of any code that allows setting the suid bit, in case it may allow some new security hole. I can't think of anything specific that could be allowed by setting the suid bit of a
On 07/26/2012 04:52 AM, Ján Tomko wrote: directory containing a disk image, but then again I haven't thought about it very hard :-) Can anyone convince me one way or the other? SUID on directories is ignored on most systems, but you could be able to create files belonging a group you're not a member of.
But this patch enables it on files too, allowing everyone with access to privileged libvirtd to create SUID files. I don't know if it's possible to exploit this, but I don't like it, so NACK NACK NACK.
It might help to learn why you want to be able to set these bits. libvirt is generally very specific about explicitly setting the uid of disk images properly as required at runtime... My issue was that vol-dumpxml reported wrong file permissions, as described in https://bugzilla.redhat.com/show_bug.cgi?id=839463
Writing the sticky bit seems harmless to me.
Yeah, I think I agree with that. (Heh - I'd never read the history of the sticky bit before (i.e. where it got the name "sticky") - "retain the text segment <http://en.wikipedia.org/wiki/Text_segment> of the program in swap space <http://en.wikipedia.org/wiki/Virtual_memory> after the process <http://en.wikipedia.org/wiki/Process_%28computing%29> exited". Remember when loading the text segment of a program took long enough for that to matter?)
Would it be safe to just read SGID and SUID without ever setting them? Or is it not worth the risk? IMO we should preserve the existing behavior of not modifying the bits, but we should report them correctly, although I don't feel all that strongly about it.
That sounds reasonable to me, as long as the reported difference only shows up during a dumpxml of the "live" XML (and not during a pool-dumpxml --inactive). I'm wondering if we should generate an error when someone tries to specify those bits in a pool-define (and vol-define), or just ignore them as we currently do. (no opinion, just wondering :-)
On 08/07/2012 08:39 PM, Laine Stump wrote:
Would it be safe to just read SGID and SUID without ever setting them? Or is it not worth the risk? IMO we should preserve the existing behavior of not modifying the bits, but we should report them correctly, although I don't feel all that strongly about it.
Reporting the values is always safe (and might be useful for someone to realize that 'gasp, my disk.img is setuid!'), but I agree that we are setting ourselves up for problems if we allow setting the bits on all file types. Then again, we are not in the business of creating executable storage volumes, so setuid on a non-executable may be a non-issue, and for directories, the story is a bit different, as the extra bits become useful for controlling default permissions used when creating new contents. Maybe we need to compromise based on what type of file (directory vs. other) is involved?
That sounds reasonable to me, as long as the reported difference only shows up during a dumpxml of the "live" XML (and not during a pool-dumpxml --inactive).
That's a good point - config should only output the bits that we will allow to be changed, and live can show all existing bits.
I'm wondering if we should generate an error when someone tries to specify those bits in a pool-define (and vol-define), or just ignore them as we currently do. (no opinion, just wondering :-)
I'm 50-50 on whether to error out, but whatever policy we settle on, we should _definitely_ document it in the corresponding docs/*.html.in files where the fields are exposed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Dave Allan -
Eric Blake -
Ján Tomko -
Laine Stump