On 03/09/2016 12:38 PM, Cole Robinson wrote:

file volume deletion, via virFileRemove, has some logic that depends on uid/gid owner of the path. This info is cached in the volume XML. We need to be sure we are using up to date uid/gid before attempting to delete the volume, otherwise we can hit a case like this:

- test.img created with uid=root - VM starts up using test.img, owner changed to uid=qemu - test.img pool is refreshed, uid=qemu is cached in the volume XML - VM shuts down, volume owner changed to gid=root - vol-delete test.img thinks uid=qemu, virFileRemove does setuid(qemu), fails to delete test.img since it is actually uid=root

https://bugzilla.redhat.com/show_bug.cgi?id=1289327 --- src/storage/storage_driver.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

Coincidentally I was thinking about this one today... Still not convinced this is the only "root" (ahem) cause... For the startup/stop path, I assume your reference is the path through virSecurityDACSetOwnershipInternal from virSecurityDACSetOwnership and virSecurityDACRestoreFileLabelInternal... But what if the mode, owner, group are provided in the XML when the volume is created: # cat vol.xml <volume type='file'> <name>test.img</name> <source> </source> <capacity unit='bytes'>10485760</capacity> <allocation unit='bytes'>10485760</allocation> <target> <path>/home/vm-images/test.img</path> <format type='raw'/> <permissions> <mode>0600</mode> <owner>107</owner> <group>107</group> <label>system_u:object_r:unlabeled_t:s0</label> </permissions> </target> </volume> # virsh vol-create default vol.xml Vol test.img created from vol.xml # virsh vol-delete test.img default error: Failed to delete vol test.img error: cannot unlink file '/home/vm-images/test.img': Permission denied # Yes, I know the following two patches resolve this case... What's interesting is the extents we've gone to on creation to set things up, but only a few bread crumbs are left for deletion. The assumption at deletion being that no one changed anything from creation, but we see that's not true. I've been under the assumption that the owner gets set/changed during virStorageBackendCreateRaw, virStorageFileBackendFileCreate, or virStorageBackendCreateExecCommand. The setting of the owner for the CreateRaw path would occur because the flags had FORCE_OWNER and FORCE_MODE set. The setting of the owner for the CreateExecCommand in the non POOL_NETFS path was because they were supplied in the XML and not taken as the default from the running of the command to create the file (eg qemu-img) and taking the default file/directory ownership. Since we run as root, this can all happen without issues. Then after any of the create processing happens, we can refresh the pool which calls virStorageBackendUpdateVolTargetInfoFD and changes the volume uid, gid, mode based on what the lstat(path, &sb) has found... This perhaps helps in the event But when we get to delete, we have no idea how things could have been originally created or perhaps (likely) updated, so we only check what we can... If we really wanted to be elaborate - save a provided uid, gid, mode at creation... That would help at deletion to determine if something changed. Doesn't work well for existing files (daemon start, restart, reload). We'd need to save volume xml's somewhere. Probably far more effort... OK - so a too long winded way to say - I don't think the following patch matters as anything (including libvirt) could have changed the file's ownership and/or protections, but not updated the volume XML. The refresh updates the volume XML to match the file's protection and furthermore only matters in the current virFileRemove if the change is back to root:root and only because we compare vs. gete{uid|gid}(). John

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 1d96618..ded54c9 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1801,6 +1801,16 @@ storageVolDelete(virStorageVolPtr obj, goto cleanup; }

+ /* Need to ensure we are using up to date uid/gid for deletion */ + if (backend->refreshVol && + backend->refreshVol(obj->conn, pool, vol) < 0) { + /* The file may have been removed behind libvirt's back. Don't + error here, let the deletion fail or succeed instead */ + VIR_INFO("Failed to refresh volume before deletion: %s", + virGetLastErrorMessage()); + virResetLastError(); + } + if (storageVolDeleteInternal(obj, backend, pool, vol, flags, true) < 0) goto cleanup;

[...]

...

OK - so a too long winded way to say - I don't think the following patch matters as anything (including libvirt) could have changed the file's ownership and/or protections, but not updated the volume XML. The refresh updates the volume XML to match the file's protection and furthermore only matters in the current virFileRemove if the change is back to root:root and only because we compare vs. gete{uid|gid}().

I don't follow this conclusion really... refreshVol syncs the XML/volume cache with the current uid/gid/mode on disk, which is then passed down to virFileRemove. If the cached uid is 'qemu' but the on disk uid is 'cole', the internal cache is synced, virFileRemove will setuid(cole) and successfully remove the disk image. Without this change, libvirt would setuid(qemu) and fail to remove the disk.

So I don't see how this patch is unneeded, or only works for root:root

I was considering the checks: + if (geteuid() != 0) + return false; We're running as root... + + /* uid/gid weren'd specified */ + if ((uid == (uid_t) -1) && (gid == (gid_t) -1)) + return false; We've provided qemu:qemu... + + /* already running as proper uid/gid */ + if (uid == geteuid() && gid == getegid()) + return false; + At this point geteuid() would return 0 (root) Maybe I'm wrong... I suppose it cannot hurt, but without patch 3 we'd go through the setuid path - I think. I could also be missing something really obvious. John

- Cole

...

John

...

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 1d96618..ded54c9 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1801,6 +1801,16 @@ storageVolDelete(virStorageVolPtr obj, goto cleanup; }

+ /* Need to ensure we are using up to date uid/gid for deletion */ + if (backend->refreshVol && + backend->refreshVol(obj->conn, pool, vol) < 0) { + /* The file may have been removed behind libvirt's back. Don't + error here, let the deletion fail or succeed instead */ + VIR_INFO("Failed to refresh volume before deletion: %s", + virGetLastErrorMessage()); + virResetLastError(); + } + if (storageVolDeleteInternal(obj, backend, pool, vol, flags, true) < 0) goto cleanup;

On 03/09/2016 12:38 PM, Cole Robinson wrote:

Break these checks out into their own function, and clearly document each one. This shouldn't change behavior --- src/util/virfile.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c index f45e18f..cea2674 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2314,6 +2314,32 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, }

+/* virFileRemoveNeedsSetuid: + * @uid: file uid to check + * @gid: file gid to check + * + * Return true if we should use setuid/setgid before deleting a file + * owned by the passed uid/gid pair. Needed for NFS with root-squash + */ +static bool +virFileRemoveNeedsSetuid(uid_t uid, gid_t gid) +{ + /* If running unprivileged, setuid isn't going to work */ + if (geteuid() != 0) + return false; + + /* uid/gid weren'd specified */

weren't ACK - with the nit fixed... The rest is me typing out my thoughts... John

+ if ((uid == (uid_t) -1) && (gid == (gid_t) -1)) + return false;

This "should" only happen as failure path of virStorageBackendCreateExecCommand when uid/gid not provided in the XML. The virStorageBackendCreateRaw failure path expects creation to have occurred where virFileOpenAs would have set the owner/mode due to the operation_flags setting.

+ + /* already running as proper uid/gid */ + if (uid == geteuid() && gid == getegid()) + return false; +

If the XML provided uid/gid and it's root - that's what we want

+ return true; +} + + /* virFileRemove: * @path: file to unlink or directory to remove * @uid: uid that was used to create the file (not required) @@ -2335,12 +2361,7 @@ virFileRemove(const char *path, gid_t *groups; int ngroups;

- /* If not running as root or if a non explicit uid/gid was being used for - * the file/volume or the explicit uid/gid matches, then use unlink directly - */ - if ((geteuid() != 0) || - ((uid == (uid_t) -1) && (gid == (gid_t) -1)) || - (uid == geteuid() && gid == getegid())) { + if (!virFileRemoveNeedsSetuid(uid, gid)) { if (virFileIsDir(path)) return rmdir(path); else

On 03/09/2016 12:39 PM, Cole Robinson wrote:

NFS with root-squash is the only reason we need to do setuid/setgid crazyness in virFileRemove, so limit that behavior to the NFS case. --- I'm not sure though if NFS is the only case we care about this here, or if we want to conditionalize this path on NFS since that makes it more of a pain to test... It's not required to fix the initial bug

src/util/virfile.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c index cea2674..3d1b118 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2322,7 +2322,7 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, * owned by the passed uid/gid pair. Needed for NFS with root-squash */ static bool -virFileRemoveNeedsSetuid(uid_t uid, gid_t gid) +virFileRemoveNeedsSetuid(const char *path, uid_t uid, gid_t gid)

You added a new parameter to document... ACK with that adjustment. John More outloud typing follows...

{ /* If running unprivileged, setuid isn't going to work */ if (geteuid() != 0) @@ -2336,6 +2336,12 @@ virFileRemoveNeedsSetuid(uid_t uid, gid_t gid) if (uid == geteuid() && gid == getegid()) return false;

+ /* Only perform the setuid stuff for NFS, which is the only case + that may actually need it. This can error, but just be safe and + only check for a clear negative result. */ + if (virFileIsSharedFSType(path, VIR_FILE_SHFS_NFS) == 0) + return false; +

So at this point we know we're root, the provided uid/gid isn't 0:0 and it's not -1:-1. So it's something else... Now if the target isn't a volume on an NFS server, then since we're root we can delete it regardless of what uid/gid is set on the file under the premise that we changed it to something, but the XML wasn't updated. At some future point in time perhaps we can track uid/gid changes such as this.

return true; }

@@ -2361,7 +2367,7 @@ virFileRemove(const char *path, gid_t *groups; int ngroups;

- if (!virFileRemoveNeedsSetuid(uid, gid)) { + if (!virFileRemoveNeedsSetuid(path, uid, gid)) { if (virFileIsDir(path)) return rmdir(path); else