From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: "Frank Filz" To: "'Andy Lutomirski'" , "'Ben Hutchings'" Cc: "'Andy Lutomirski'" , , "'Konstantin Khlebnikov'" , "'Alexander Viro'" , "'Kees Cook'" , "'Willy Tarreau'" , , "'Andrew Morton'" , "'yalin wang'" , "'Linux Kernel Mailing List'" , "'Jan Kara'" , "'Linux FS Devel'" , "'stable'" References: <9318903980969a0e378dab2de4d803397adcd3cc.1485377903.git.luto@kernel.org> <1485380634.2998.161.camel@decadent.org.uk> In-Reply-To: Subject: RE: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() Date: Wed, 25 Jan 2017 15:15:16 -0800 Message-ID: <014301d27760$e4d8b9a0$ae8a2ce0$@mindspring.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Language: en-us Sender: owner-linux-mm@kvack.org List-ID: > On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings > 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 clie= nts. 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_capabl= e(), > 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 fo= r > this reason. Frank --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb0-f200.google.com (mail-yb0-f200.google.com [209.85.213.200]) by kanga.kvack.org (Postfix) with ESMTP id 5B26F6B0253 for ; Wed, 25 Jan 2017 18:16:16 -0500 (EST) Received: by mail-yb0-f200.google.com with SMTP id n21so270248752yba.7 for ; Wed, 25 Jan 2017 15:16:16 -0800 (PST) Received: from elasmtp-banded.atl.sa.earthlink.net (elasmtp-banded.atl.sa.earthlink.net. [209.86.89.70]) by mx.google.com with ESMTPS id n128si1134078ybn.105.2017.01.25.15.16.15 for (version=TLS1 cipher=AES128-SHA bits=128/128); Wed, 25 Jan 2017 15:16:15 -0800 (PST) From: "Frank Filz" References: <9318903980969a0e378dab2de4d803397adcd3cc.1485377903.git.luto@kernel.org> <1485380634.2998.161.camel@decadent.org.uk> In-Reply-To: Subject: RE: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() Date: Wed, 25 Jan 2017 15:15:16 -0800 Message-ID: <014301d27760$e4d8b9a0$ae8a2ce0$@mindspring.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Language: en-us Sender: owner-linux-mm@kvack.org List-ID: To: 'Andy Lutomirski' , 'Ben Hutchings' Cc: 'Andy Lutomirski' , security@kernel.org, 'Konstantin Khlebnikov' , 'Alexander Viro' , 'Kees Cook' , 'Willy Tarreau' , linux-mm@kvack.org, 'Andrew Morton' , 'yalin wang' , 'Linux Kernel Mailing List' , 'Jan Kara' , 'Linux FS Devel' , 'stable' > On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings > 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 clie= nts. 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_capabl= e(), > 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 fo= r > this reason. Frank --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751972AbdAYXcZ convert rfc822-to-8bit (ORCPT ); Wed, 25 Jan 2017 18:32:25 -0500 Received: from elasmtp-banded.atl.sa.earthlink.net ([209.86.89.70]:36676 "EHLO elasmtp-banded.atl.sa.earthlink.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750716AbdAYXcX (ORCPT ); Wed, 25 Jan 2017 18:32:23 -0500 X-Greylist: delayed 968 seconds by postgrey-1.27 at vger.kernel.org; Wed, 25 Jan 2017 18:32:23 EST DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=dk20050327; d=mindspring.com; b=lak1OtFT7zbIiK2Al3A1/UFJDZoZg32bkzo6hbkWd4t31OiRL9XosvdPGw1QO7nQ; h=Received:From:To:Cc:References:In-Reply-To:Subject:Date:Message-ID:MIME-Version:Content-Type:Content-Transfer-Encoding:X-Mailer:Thread-Index:Content-Language:X-Antivirus-Status:X-ELNK-Trace:X-Originating-IP; From: "Frank Filz" To: "'Andy Lutomirski'" , "'Ben Hutchings'" Cc: "'Andy Lutomirski'" , , "'Konstantin Khlebnikov'" , "'Alexander Viro'" , "'Kees Cook'" , "'Willy Tarreau'" , , "'Andrew Morton'" , "'yalin wang'" , "'Linux Kernel Mailing List'" , "'Jan Kara'" , "'Linux FS Devel'" , "'stable'" References: <9318903980969a0e378dab2de4d803397adcd3cc.1485377903.git.luto@kernel.org> <1485380634.2998.161.camel@decadent.org.uk> In-Reply-To: Subject: RE: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid() Date: Wed, 25 Jan 2017 15:15:16 -0800 Message-ID: <014301d27760$e4d8b9a0$ae8a2ce0$@mindspring.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8BIT X-Mailer: Microsoft Outlook 15.0 Thread-Index: AQGxRCguLcWFN7Z3NGnC4cPpvgpRZQIMn9+FAwAOPagBdfrw/aFYIptg Content-Language: en-us X-Antivirus: avast! (VPS 170125-1, 01/25/2017), Outbound message X-Antivirus-Status: Clean X-ELNK-Trace: 136157f01908a8929c7f779228e2f6aeda0071232e20db4dfcf68cf913c220157117d6cee969035e350badd9bab72f9c350badd9bab72f9c350badd9bab72f9c X-Originating-IP: 67.170.185.135 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings > 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