[libvirt] [PATCH] safer default storage dir permissions
Hi, while cleaning out patchs that we held for a while on top of libvirt I found this patch of Serge (thanks!) which I think would make just as much sense in the upstream project itself. Or in case the discussion might unveil why it might not make sense, that would also be a win for us to adapt. I modified the older patch to match latest master (files owning the code changed) and added the matching doc update to be complete. Serge Hallyn (1): storage: use 0711 as the default perms for dirs docs/formatstorage.html.in | 2 +- src/storage/storage_util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.7.4
From: Serge Hallyn <serge.hallyn@ubuntu.com> There should be no need to make dir based pools world readable. So use 0711, not 0755, as the default perms for storage dirs. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- docs/formatstorage.html.in | 2 +- src/storage/storage_util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 225e190..4946ddf 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -444,7 +444,7 @@ namespace. It provides information about the permissions to use for the final directory when the pool is built. There are 4 child elements. The <code>mode</code> element contains the octal permission set. - The <code>mode</code> defaults to 0755 when not provided. + The <code>mode</code> defaults to 0711 when not provided. The <code>owner</code> element contains the numeric user ID. The <code>group</code> element contains the numeric group ID. If <code>owner</code> or <code>group</code> aren't specified when diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index a05c35d..6f2a1b1 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -138,7 +138,7 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755 +# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0711 # define VIR_STORAGE_DEFAULT_VOL_PERM_MODE 0600 int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, -- 2.7.4
On 05/11/2017 04:31 AM, Christian Ehrhardt wrote:
From: Serge Hallyn <serge.hallyn@ubuntu.com>
There should be no need to make dir based pools world readable. So use 0711, not 0755, as the default perms for storage dirs.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- docs/formatstorage.html.in | 2 +- src/storage/storage_util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Kinda surprised this didn't generate some immediate discussion... I would also think that if you had a desire to change defaults you'd also have a libvirt.spec.in adjustment... Still 0755 or umask(022) seem to be fairly prevalent setting and having the <mode> for the XML to be able to override a default certainly gives credence to arguments in either direction whether or not to change the defaults. It's been a long while since I considered system/directory/file security things, but I have this faint recollection of some strange issue when not having world or group "executable" as a default. Still for those that desire a higher level of protection and security there are ways to set more stringent values, but out of the box going with 755 still seems reasonable. Although I'm sure there's varying opinions on that depending upon your expectations of a secure system. Also your commit message notes "world readable", but by going from 755 to 711, you're also changing to "group readable" too ;-) John
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 225e190..4946ddf 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -444,7 +444,7 @@ namespace. It provides information about the permissions to use for the final directory when the pool is built. There are 4 child elements. The <code>mode</code> element contains the octal permission set. - The <code>mode</code> defaults to 0755 when not provided. + The <code>mode</code> defaults to 0711 when not provided. The <code>owner</code> element contains the numeric user ID. The <code>group</code> element contains the numeric group ID. If <code>owner</code> or <code>group</code> aren't specified when diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index a05c35d..6f2a1b1 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -138,7 +138,7 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755 +# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0711 # define VIR_STORAGE_DEFAULT_VOL_PERM_MODE 0600
int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
On Thu, May 11, 2017 at 06:36:22PM -0400, John Ferlan wrote:
On 05/11/2017 04:31 AM, Christian Ehrhardt wrote:
From: Serge Hallyn <serge.hallyn@ubuntu.com>
There should be no need to make dir based pools world readable. So use 0711, not 0755, as the default perms for storage dirs.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- docs/formatstorage.html.in | 2 +- src/storage/storage_util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Kinda surprised this didn't generate some immediate discussion... I would also think that if you had a desire to change defaults you'd also have a libvirt.spec.in adjustment...
Actually no it doesn't - the spec file is already marking /var/lib/libvirt/images as 0711.
Still 0755 or umask(022) seem to be fairly prevalent setting and having the <mode> for the XML to be able to override a default certainly gives credence to arguments in either direction whether or not to change the defaults.
It's been a long while since I considered system/directory/file security things, but I have this faint recollection of some strange issue when not having world or group "executable" as a default.
The fact that RPM spec ships with 0711 show that it works ok. So I think this change is reasonable. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, May 15, 2017 at 09:27:38AM +0100, Daniel P. Berrange wrote:
On Thu, May 11, 2017 at 06:36:22PM -0400, John Ferlan wrote:
On 05/11/2017 04:31 AM, Christian Ehrhardt wrote:
From: Serge Hallyn <serge.hallyn@ubuntu.com>
There should be no need to make dir based pools world readable. So use 0711, not 0755, as the default perms for storage dirs.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- docs/formatstorage.html.in | 2 +- src/storage/storage_util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Kinda surprised this didn't generate some immediate discussion... I would also think that if you had a desire to change defaults you'd also have a libvirt.spec.in adjustment...
Actually no it doesn't - the spec file is already marking /var/lib/libvirt/images as 0711.
Still 0755 or umask(022) seem to be fairly prevalent setting and having the <mode> for the XML to be able to override a default certainly gives credence to arguments in either direction whether or not to change the defaults.
It's been a long while since I considered system/directory/file security things, but I have this faint recollection of some strange issue when not having world or group "executable" as a default.
The fact that RPM spec ships with 0711 show that it works ok. So I think this change is reasonable.
Same here. I'm not sure, but I think even SELinux policy defaulted to that. Anyway, ACK to this one, I'll push this in a while. While we're on this, is there some global config for the group in all these permissions? I would love to add a user to one group and make all libvirt-related readable for that user with that one simple addition.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, May 15, 2017 at 10:27 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
Kinda surprised this didn't generate some immediate discussion... I would also think that if you had a desire to change defaults you'd also have a libvirt.spec.in adjustment...
Actually no it doesn't - the spec file is already marking /var/lib/libvirt/images as 0711.
As reference that is the current spec content: libvirt.spec.in:1745:%dir %attr(0711, root, root) %{_localstatedir}/lib/libvirt/images/
Still 0755 or umask(022) seem to be fairly prevalent setting and having the <mode> for the XML to be able to override a default certainly gives credence to arguments in either direction whether or not to change the defaults.
It's been a long while since I considered system/directory/file security things, but I have this faint recollection of some strange issue when not having world or group "executable" as a default.
The fact that RPM spec ships with 0711 show that it works ok. So I think this change is reasonable.
Interesting, I didn't check the RPM spec - thanks Daniel to point this out. It is 711 on Ubuntu as well for quite some time now. Both together make this even less likely to have hidden drawbacks. -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
On Fri, May 12, 2017 at 12:36 AM, John Ferlan <jferlan@redhat.com> wrote:
Also your commit message notes "world readable", but by going from 755 to 711, you're also changing to "group readable" too ;-)
Good catch John, the other feedback seems good, so for now I'm just rewording in regard to this and resubmit to the thread. -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
From: Serge Hallyn <serge.hallyn@ubuntu.com> There should be no need to make dir based pools world/group readable. So use 0711, not 0755, as the default perms for storage dirs. Updates in v2: - adapt commit wording to mention dropping group readable as well Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- docs/formatstorage.html.in | 2 +- src/storage/storage_util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 225e190..4946ddf 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -444,7 +444,7 @@ namespace. It provides information about the permissions to use for the final directory when the pool is built. There are 4 child elements. The <code>mode</code> element contains the octal permission set. - The <code>mode</code> defaults to 0755 when not provided. + The <code>mode</code> defaults to 0711 when not provided. The <code>owner</code> element contains the numeric user ID. The <code>group</code> element contains the numeric group ID. If <code>owner</code> or <code>group</code> aren't specified when diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index a05c35d..6f2a1b1 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -138,7 +138,7 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755 +# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0711 # define VIR_STORAGE_DEFAULT_VOL_PERM_MODE 0600 int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, -- 2.7.4
On Mon, May 15, 2017 at 01:05:31PM +0200, Christian Ehrhardt wrote:
From: Serge Hallyn <serge.hallyn@ubuntu.com>
There should be no need to make dir based pools world/group readable. So use 0711, not 0755, as the default perms for storage dirs.
Updates in v2: - adapt commit wording to mention dropping group readable as well
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- docs/formatstorage.html.in | 2 +- src/storage/storage_util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> Will push to git shortly. BTW, for libvir-list we recommend to send v2/v3/etc followup patches as top level threads, not in-reply-to the previous versions. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, May 15, 2017 at 1:10 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
BTW, for libvir-list we recommend to send v2/v3/etc followup patches as top level threads, not in-reply-to the previous versions.
I need a mapper which project prefers what :-), no really - thank you a lot! Since we are about to submit a bigger pile of apparmor changes that hint might certainly be handy the next days/weeks. -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
participants (4)
-
Christian Ehrhardt -
Daniel P. Berrange -
John Ferlan -
Martin Kletzander