[3/3] util: virfile: Only setuid for virFileRemove if on NFS

Message ID b44c4525461bc2a101edd447e7cd210c6493fb68.1457544659.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson March 9, 2016, 5:39 p.m.
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(-)

-- 
2.5.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Cole Robinson March 9, 2016, 9:10 p.m. | #1
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

Patch hide | download patch | download mbox

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