lkml.org 
[lkml]   [2017]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid()
Date
> On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@decadent.org.uk>
> wrote:
> > On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote:
> >> If an unprivileged program opens a setgid file for write and passes
> >> the fd to a privileged program and the privileged program writes to
> >> it, we currently fail to clear the setgid bit. Fix it by checking
> >> f_cred instead of current's creds whenever a struct file is involved.
> > [...]
> >
> > What if, instead, a privileged program passes the fd to an un
> > unprivileged program? It sounds like a bad idea to start with, but at
> > least currently the unprivileged program is going to clear the setgid
> > bit when it writes. This change would make that behaviour more
> > dangerous.
>
> Hmm. Although, if a privileged program does something like:
>
> (sudo -u nobody echo blah) >setuid_program
>
> presumably it wanted to make the change.

I'm not following all the intricacies here, though I need to...

What about a privileged program that drops privilege for certain operations?

Specifically the Ganesha user space NFS server runs as root, but sets fsuid/fsgid for specific threads performing I/O operations on behalf of NFS clients.

I want to make sure setgid bit handling is proper for these cases.

Ganesha does some permission checking, but this is one area I want to defer to the underlying filesystem because it's not easy for Ganesha to get it right.

> > Perhaps there should be a capability check on both the current
> > credentials and file credentials? (I realise that we've considered
> > file credential checks to be sufficient elsewhere, but those cases
> > involved virtual files with special semantics, where it's clearer that
> > a privileged process should not pass them to an unprivileged process.)
> >
>
> I could go either way.
>
> What I really want to do is to write a third patch that isn't for -stable that just
> removes the capable() check entirely. I'm reasonably confident it won't
> break things for a silly reason: because it's capable() and not ns_capable(),
> anything it would break would also be broken in an unprivileged container,
> and I haven't seen any reports of package managers or similar breaking for
> this reason.

Frank


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

\
 
 \ /
  Last update: 2017-01-26 00:32    [W:0.088 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site