10:05 a.m.
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
3:52 a.m.
New subject: [libvirt] [PATCH v2] storage: support all file permissions
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
11:09 a.m.
New subject: [libvirt] [PATCH v2] storage: support all file permissions
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
12:17 a.m.
New subject: [libvirt] [PATCH v2] storage: support all file permissions
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;
3:27 a.m.
New subject: [libvirt] [PATCH v2] storage: support all file permissions
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
9:40 a.m.
New subject: [libvirt] [PATCH v2] storage: support all file permissions
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:39 p.m.
New subject: [libvirt] [PATCH v2] storage: support all file permissions
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:
>> 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.
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 :-)
6:25 p.m.
New subject: [libvirt] [PATCH v2] storage: support all file permissions
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4482
days inactive
4502
days old
7 comments
4 participants
participants (4)
-
Dave Allan -
Eric Blake -
Ján Tomko -
Laine Stump