From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DD433C2BC11 for ; Tue, 8 Sep 2020 19:43:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B593721919 for ; Tue, 8 Sep 2020 19:43:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732141AbgIHTmx (ORCPT ); Tue, 8 Sep 2020 15:42:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53014 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730895AbgIHPwr (ORCPT ); Tue, 8 Sep 2020 11:52:47 -0400 Received: from smtp-190f.mail.infomaniak.ch (smtp-190f.mail.infomaniak.ch [IPv6:2001:1600:3:17::190f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 21F52C0619C6 for ; Tue, 8 Sep 2020 08:44:30 -0700 (PDT) Received: from smtp-2-0001.mail.infomaniak.ch (unknown [10.5.36.108]) by smtp-2-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4Bm8bC0hq3zlhP6d; Tue, 8 Sep 2020 17:44:23 +0200 (CEST) Received: from ns3096276.ip-94-23-54.eu (unknown [94.23.54.103]) by smtp-2-0001.mail.infomaniak.ch (Postfix) with ESMTPA id 4Bm8b8365Jzlh8TD; Tue, 8 Sep 2020 17:44:20 +0200 (CEST) Subject: Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2) To: Mimi Zohar , linux-kernel@vger.kernel.org Cc: Aleksa Sarai , Alexei Starovoitov , Al Viro , Andrew Morton , Andy Lutomirski , Christian Brauner , Christian Heimes , Daniel Borkmann , Deven Bowers , Dmitry Vyukov , Eric Biggers , Eric Chiang , Florian Weimer , James Morris , Jan Kara , Jann Horn , Jonathan Corbet , Kees Cook , Lakshmi Ramasubramanian , Matthew Garrett , Matthew Wilcox , Michael Kerrisk , Miklos Szeredi , =?UTF-8?Q?Philippe_Tr=c3=a9buchet?= , Scott Shell , Sean Christopherson , Shuah Khan , Steve Dower , Steve Grubb , Tetsuo Handa , Thibaut Sautereau , Vincent Strubel , kernel-hardening@lists.openwall.com, linux-api@vger.kernel.org, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, linux-fsdevel@vger.kernel.org, Thibaut Sautereau , =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= , Stephen Smalley , John Johansen References: <20200908075956.1069018-1-mic@digikod.net> <20200908075956.1069018-2-mic@digikod.net> <75451684-58f3-b946-dca4-4760fa0d7440@digikod.net> <01c23b2607a7dbf734722399931473c053d9b362.camel@linux.ibm.com> From: =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= Message-ID: Date: Tue, 8 Sep 2020 17:44:19 +0200 User-Agent: MIME-Version: 1.0 In-Reply-To: <01c23b2607a7dbf734722399931473c053d9b362.camel@linux.ibm.com> Content-Type: text/plain; charset=iso-8859-15 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On 08/09/2020 17:24, Mimi Zohar wrote: > On Tue, 2020-09-08 at 14:43 +0200, Mickaël Salaün wrote: >> On 08/09/2020 14:28, Mimi Zohar wrote: >>> Hi Mickael, >>> >>> On Tue, 2020-09-08 at 09:59 +0200, Mickaël Salaün wrote: >>>> diff --git a/fs/open.c b/fs/open.c >>>> index 9af548fb841b..879bdfbdc6fa 100644 >>>> --- a/fs/open.c >>>> +++ b/fs/open.c >>>> @@ -405,9 +405,13 @@ static long do_faccessat(int dfd, const char __user *filename, int mode, int fla >>>> if (mode & ~S_IRWXO) /* where's F_OK, X_OK, W_OK, R_OK? */ >>>> return -EINVAL; >>>> >>>> - if (flags & ~(AT_EACCESS | AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) >>>> + if (flags & ~(AT_EACCESS | AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | >>>> + AT_INTERPRETED)) >>>> return -EINVAL; >>>> >>>> + /* Only allows X_OK with AT_INTERPRETED for now. */ >>>> + if ((flags & AT_INTERPRETED) && !(mode & S_IXOTH)) >>>> + return -EINVAL; >>>> if (flags & AT_SYMLINK_NOFOLLOW) >>>> lookup_flags &= ~LOOKUP_FOLLOW; >>>> if (flags & AT_EMPTY_PATH) >>>> @@ -426,7 +430,30 @@ static long do_faccessat(int dfd, const char __user *filename, int mode, int fla >>>> >>>> inode = d_backing_inode(path.dentry); >>>> >>>> - if ((mode & MAY_EXEC) && S_ISREG(inode->i_mode)) { >>>> + if ((flags & AT_INTERPRETED)) { >>>> + /* >>>> + * For compatibility reasons, without a defined security policy >>>> + * (via sysctl or LSM), using AT_INTERPRETED must map the >>>> + * execute permission to the read permission. Indeed, from >>>> + * user space point of view, being able to execute data (e.g. >>>> + * scripts) implies to be able to read this data. >>>> + * >>>> + * The MAY_INTERPRETED_EXEC bit is set to enable LSMs to add >>>> + * custom checks, while being compatible with current policies. >>>> + */ >>>> + if ((mode & MAY_EXEC)) { >>> >>> Why is the ISREG() test being dropped? Without dropping it, there >>> would be no reason for making the existing test an "else" clause. >> >> The ISREG() is not dropped, it is just moved below with the rest of the >> original code. The corresponding code (with the path_noexec call) for >> AT_INTERPRETED is added with the next commit, and it relies on the >> sysctl configuration for compatibility reasons. > > Dropping the S_ISREG() check here without an explanation is wrong and > probably unsafe, as it is only re-added in the subsequent patch and > only for the "sysctl_interpreted_access" case. Adding this new test > after the existing test is probably safer. If the original test fails, > it returns the same value as this test -EACCES. The original S_ISREG() is ANDed with a MAY_EXEC check and with path_noexec(). The goal of this patch is indeed to have a different behavior than the original faccessat2(2) thanks to the AT_INTERPRETED flag. This can't work if we add the sysctl check after the current path_noexec() check. Moreover, in this patch an exec check is translated to a read check. This new behavior is harmless because using AT_INTERPRETED with the current faccessat2(2) would return -EINVAL. The current vanilla behavior is then unchanged. The whole point of this patch series is to have a policy which do not break current systems and is easy to configure by the sysadmin through sysctl. This patch series also enable LSMs to take advantage of it without the current faccess* limitations. For instance, it is then possible for an LSM to implement more complex policies which may allow execution of data from pipes or sockets, while verifying the source of this data. Enforcing S_ISREG() in this patch would forbid such policies to be implemented. In the case of IMA, you may want to add the same S_ISREG() check. > > Mimi > >> >>> >>>> + mode |= MAY_INTERPRETED_EXEC; >>>> + /* >>>> + * For compatibility reasons, if the system-wide policy >>>> + * doesn't enforce file permission checks, then >>>> + * replaces the execute permission request with a read >>>> + * permission request. >>>> + */ >>>> + mode &= ~MAY_EXEC; >>>> + /* To be executed *by* user space, files must be readable. */ >>>> + mode |= MAY_READ; > >