linux-kernel - Re: [RFC PATCH v8 1/3] fs: Introduce AT

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:   Tue, 8 Sep 2020 14:43:32 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     Mimi Zohar <zohar@...ux.ibm.com>, linux-kernel@...r.kernel.org
Cc:     Aleksa Sarai <cyphar@...har.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Christian Heimes <christian@...hon.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Deven Bowers <deven.desai@...ux.microsoft.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Eric Biggers <ebiggers@...nel.org>,
        Eric Chiang <ericchiang@...gle.com>,
        Florian Weimer <fweimer@...hat.com>,
        James Morris <jmorris@...ei.org>, Jan Kara <jack@...e.cz>,
        Jann Horn <jannh@...gle.com>, Jonathan Corbet <corbet@....net>,
        Kees Cook <keescook@...omium.org>,
        Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>,
        Matthew Garrett <mjg59@...gle.com>,
        Matthew Wilcox <willy@...radead.org>,
        Michael Kerrisk <mtk.manpages@...il.com>,
        Miklos Szeredi <mszeredi@...hat.com>,
        Philippe Trébuchet 
        <philippe.trebuchet@....gouv.fr>,
        Scott Shell <scottsh@...rosoft.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Shuah Khan <shuah@...nel.org>,
        Steve Dower <steve.dower@...hon.org>,
        Steve Grubb <sgrubb@...hat.com>,
        Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        Thibaut Sautereau <thibaut.sautereau@...p-os.org>,
        Vincent Strubel <vincent.strubel@....gouv.fr>,
        kernel-hardening@...ts.openwall.com, linux-api@...r.kernel.org,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        Thibaut Sautereau <thibaut.sautereau@....gouv.fr>,
        Mickaël Salaün <mic@...ux.microsoft.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        John Johansen <john.johansen@...onical.com>
Subject: Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for
 faccessat2(2)


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.

> 
>> +			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;
> 
> After this change, I'm wondering if it makes sense to add a call to
> security_file_permission().  IMA doesn't currently define it, but
> could.

Yes, that's the idea. We could replace the following inode_permission()
with file_permission(). I'm not sure how this will impact other LSMs though.

> 
> Mimi
> 
>> +		}
>> +	} else if ((mode & MAY_EXEC) && S_ISREG(inode->i_mode)) {
>>  		/*
>>  		 * MAY_EXEC on regular files is denied if the fs is mounted
>>  		 * with the "noexec" flag.
>