| Message ID | b44c4525461bc2a101edd447e7cd210c6493fb68.1457544659.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/09/2016 03:34 PM, John Ferlan wrote: > > > 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. > Thanks, I pushed #2 and #3 with your adjustments - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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) { /* 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; + 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