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 AB47CC433E2 for ; Tue, 8 Sep 2020 17:22:17 +0000 (UTC) Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by mail.kernel.org (Postfix) with SMTP id 96318206B5 for ; Tue, 8 Sep 2020 17:22:16 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 96318206B5 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=digikod.net Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kernel-hardening-return-19821-kernel-hardening=archiver.kernel.org@lists.openwall.com Received: (qmail 19672 invoked by uid 550); 8 Sep 2020 17:22:10 -0000 Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Received: (qmail 19652 invoked from network); 8 Sep 2020 17:22:09 -0000 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 19:21:54 +0200 User-Agent: MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=iso-8859-15 Content-Language: en-US Content-Transfer-Encoding: 8bit On 08/09/2020 18:44, Mimi Zohar wrote: > On Tue, 2020-09-08 at 17:44 +0200, Mickaël Salaün wrote: >> 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. > > Don't get me wrong. I'm very interested in having this support and > appreciate all the work you're doing on getting it upstreamed. With > the change in this patch, I see the MAY_EXEC being changed to MAY_READ, > but I don't see -EINVAL being returned. It sounds like this change is > dependent on the faccessat2 version for -EINVAL to be returned. No worries, unfortunately the patch format doesn't ease this review. :) access(2) and faccessat(2) have a flag value of 0. Only faccessat2(2) takes a flag from userspace. The -EINVAL is currently returned (by faccessat2) if there is an unknown flag provided by userspace. With this patch, only a mode equal to X_OK is allowed for the AT_INTERPRETED flag (cf. second hunk in this patch). As described in the cover letter, we could handle the other modes in the future though. > >> >> 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. > >>> >>>> >>>>> >>>>>> + 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; >>> >>> > >