[libvirt] [PATCH] Don't chown qemu saved image back to root after save if dynamic_ownership=0
When dynamic_ownership=0, saved images must be owned by the same uid as is used to run the qemu process, otherwise restore won't work. To accomplish this, qemuSecurityDACRestoreSavedStateLabel() needs to simply return when it's called. This fix is in response to: https://bugzilla.redhat.com/show_bug.cgi?id=661720 --- Note that this still leaves open the issue discovered in this bug - if the saved image file already exists when it is "created" for the new save, whatever mode it has will be maintained, rather than forcing 0600. It would be simple to force the mode to 0600 (just add a flag to virFileOperation(), but I'm not sure if it would be safe to do so right now without a *lot* of testing (I'm concerned about possible scenarios where the chmod() that's done when the FORCE_PERMISSIONS flag is set might fail, making a previously working case fail). Any opinions on that? (At any rate, it should be done in a separate patch if we decide to do it). src/qemu/qemu_security_dac.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c index b5c52d1..6b6170a 100644 --- a/src/qemu/qemu_security_dac.c +++ b/src/qemu/qemu_security_dac.c @@ -533,7 +533,7 @@ qemuSecurityDACRestoreSavedStateLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, virDomainObjPtr vm ATTRIBUTE_UNUSED, const char *savefile) { - if (!driver->privileged) + if (!driver->privileged || !driver->dynamicOwnership) return 0; return qemuSecurityDACRestoreSecurityFileLabel(savefile); -- 1.7.3.4
On 01/05/2011 03:03 PM, Laine Stump wrote:
When dynamic_ownership=0, saved images must be owned by the same uid as is used to run the qemu process, otherwise restore won't work. To accomplish this, qemuSecurityDACRestoreSavedStateLabel() needs to simply return when it's called.
This fix is in response to:
ACK.
---
Note that this still leaves open the issue discovered in this bug - if the saved image file already exists when it is "created" for the new save, whatever mode it has will be maintained, rather than forcing 0600. It would be simple to force the mode to 0600 (just add a flag to virFileOperation(), but I'm not sure if it would be safe to do so right now without a *lot* of testing (I'm concerned about possible scenarios where the chmod() that's done when the FORCE_PERMISSIONS flag is set might fail, making a previously working case fail). Any opinions on that? (At any rate, it should be done in a separate patch if we decide to do it).
For that matter, we could argue that the bug is in whatever code created the file with the overly-permissive permissions in the first place, and that libvirt should not change (what gives libvirt the right to decide to lock down permissions on an already existing file?). So _if_ we decide that libvirt needs to do anything at all, then it's definitely material for a separate patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 01/05/2011 05:26 PM, Eric Blake wrote:
On 01/05/2011 03:03 PM, Laine Stump wrote:
When dynamic_ownership=0, saved images must be owned by the same uid as is used to run the qemu process, otherwise restore won't work. To accomplish this, qemuSecurityDACRestoreSavedStateLabel() needs to simply return when it's called.
This fix is in response to:
Thanks. I pushed this last night after I saw your ACK, but forgot to reply until now.
On Wed, Jan 05, 2011 at 05:03:48PM -0500, Laine Stump wrote:
When dynamic_ownership=0, saved images must be owned by the same uid as is used to run the qemu process, otherwise restore won't work. To accomplish this, qemuSecurityDACRestoreSavedStateLabel() needs to simply return when it's called.
This fix is in response to:
https://bugzilla.redhat.com/show_bug.cgi?id=661720 ---
Note that this still leaves open the issue discovered in this bug - if the saved image file already exists when it is "created" for the new save, whatever mode it has will be maintained, rather than forcing 0600. It would be simple to force the mode to 0600 (just add a flag to virFileOperation(), but I'm not sure if it would be safe to do so right now without a *lot* of testing (I'm concerned about possible scenarios where the chmod() that's done when the FORCE_PERMISSIONS flag is set might fail, making a previously working case fail). Any opinions on that? (At any rate, it should be done in a separate patch if we decide to do it).
We should unlink() the target path first, and then try to create it with O_CREAT|O_EXCL to ensure we are the ones who are creating it. Of course the unlink may well need to be re-tried as with user who owns the file, in case of root squash NFS Daniel
On 01/06/2011 05:50 AM, Daniel P. Berrange wrote:
On Wed, Jan 05, 2011 at 05:03:48PM -0500, Laine Stump wrote:
When dynamic_ownership=0, saved images must be owned by the same uid as is used to run the qemu process, otherwise restore won't work. To accomplish this, qemuSecurityDACRestoreSavedStateLabel() needs to simply return when it's called.
This fix is in response to:
https://bugzilla.redhat.com/show_bug.cgi?id=661720 ---
Note that this still leaves open the issue discovered in this bug - if the saved image file already exists when it is "created" for the new save, whatever mode it has will be maintained, rather than forcing 0600. It would be simple to force the mode to 0600 (just add a flag to virFileOperation(), but I'm not sure if it would be safe to do so right now without a *lot* of testing (I'm concerned about possible scenarios where the chmod() that's done when the FORCE_PERMISSIONS flag is set might fail, making a previously working case fail). Any opinions on that? (At any rate, it should be done in a separate patch if we decide to do it). We should unlink() the target path first, and then try to create it with O_CREAT|O_EXCL to ensure we are the ones who are creating it. Of course the unlink may well need to be re-tried as with user who owns the file, in case of root squash NFS
Do you think it's safe to put in such a change now? Or should we wait until after the upcoming release(s)?
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump