linux-kernel - RE: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 25 Jan 2017 15:15:16 -0800
From:   "Frank Filz" <ffilzlnx@...dspring.com>
To:     "'Andy Lutomirski'" <luto@...capital.net>,
        "'Ben Hutchings'" <ben@...adent.org.uk>
Cc:     "'Andy Lutomirski'" <luto@...nel.org>, <security@...nel.org>,
        "'Konstantin Khlebnikov'" <koct9i@...il.com>,
        "'Alexander Viro'" <viro@...iv.linux.org.uk>,
        "'Kees Cook'" <keescook@...omium.org>,
        "'Willy Tarreau'" <w@....eu>, <linux-mm@...ck.org>,
        "'Andrew Morton'" <akpm@...ux-foundation.org>,
        "'yalin wang'" <yalin.wang2010@...il.com>,
        "'Linux Kernel Mailing List'" <linux-kernel@...r.kernel.org>,
        "'Jan Kara'" <jack@...e.cz>,
        "'Linux FS Devel'" <linux-fsdevel@...r.kernel.org>,
        "'stable'" <stable@...r.kernel.org>
Subject: RE: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid()

> On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@...adent.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