Message ID | 20200908075956.1069018-2-mic@digikod.net (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Add support for AT_INTERPRETED (was O_MAYEXEC) | expand |
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. > + 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. 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.
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. >
On Tue, Sep 8, 2020 at 8:43 AM Mickaël Salaün <mic@digikod.net> 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: > >> + 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. They are not equivalent at least as far as SELinux is concerned. security_file_permission() was only to be used to revalidate read/write permissions previously checked at file open to support policy changes and file or process label changes. We'd have to modify the SELinux hook if we wanted to have it check execute access even if nothing has changed since open time.
On Tue, Sep 8, 2020 at 8:50 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Tue, Sep 8, 2020 at 8:43 AM Mickaël Salaün <mic@digikod.net> 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: > > >> + 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. > > They are not equivalent at least as far as SELinux is concerned. > security_file_permission() was only to be used to revalidate > read/write permissions previously checked at file open to support > policy changes and file or process label changes. We'd have to modify > the SELinux hook if we wanted to have it check execute access even if > nothing has changed since open time. Also Smack doesn't appear to implement file_permission at all, so it would skip Smack checking.
On Tue, 2020-09-08 at 08:52 -0400, Stephen Smalley wrote: > On Tue, Sep 8, 2020 at 8:50 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > On Tue, Sep 8, 2020 at 8:43 AM Mickaël Salaün <mic@digikod.net> 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: > > > >> + 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. I wasn't suggesting replacing the existing security_inode_permission hook later, but adding a new security_file_permission hook here. > > > > They are not equivalent at least as far as SELinux is concerned. > > security_file_permission() was only to be used to revalidate > > read/write permissions previously checked at file open to support > > policy changes and file or process label changes. We'd have to modify > > the SELinux hook if we wanted to have it check execute access even if > > nothing has changed since open time. > > Also Smack doesn't appear to implement file_permission at all, so it > would skip Smack checking. My question is whether adding a new security_file_permission call here would break either SELinux or Apparmor? Mimi
On Tue, Sep 8, 2020 at 9:29 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Tue, 2020-09-08 at 08:52 -0400, Stephen Smalley wrote: > > On Tue, Sep 8, 2020 at 8:50 AM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > > > > On Tue, Sep 8, 2020 at 8:43 AM Mickaël Salaün <mic@digikod.net> 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: > > > > >> + 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. > > I wasn't suggesting replacing the existing security_inode_permission > hook later, but adding a new security_file_permission hook here. > > > > > > > They are not equivalent at least as far as SELinux is concerned. > > > security_file_permission() was only to be used to revalidate > > > read/write permissions previously checked at file open to support > > > policy changes and file or process label changes. We'd have to modify > > > the SELinux hook if we wanted to have it check execute access even if > > > nothing has changed since open time. > > > > Also Smack doesn't appear to implement file_permission at all, so it > > would skip Smack checking. > > My question is whether adding a new security_file_permission call here > would break either SELinux or Apparmor? selinux_inode_permission() has special handling for MAY_ACCESS so we'd need to duplicate that into selinux_file_permission() -> selinux_revalidate_file_permission(). Also likely need to adjust selinux_file_permission() to explicitly check whether the mask includes any permissions not checked at open time. So some changes would be needed here. By default, it would be a no-op unless there was a policy reload or the file was relabeled between the open(2) and the faccessat(2) call.
On 08/09/2020 15:42, Stephen Smalley wrote: > On Tue, Sep 8, 2020 at 9:29 AM Mimi Zohar <zohar@linux.ibm.com> wrote: >> >> On Tue, 2020-09-08 at 08:52 -0400, Stephen Smalley wrote: >>> On Tue, Sep 8, 2020 at 8:50 AM Stephen Smalley >>> <stephen.smalley.work@gmail.com> wrote: >>>> >>>> On Tue, Sep 8, 2020 at 8:43 AM Mickaël Salaün <mic@digikod.net> 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: >>>>>>> + 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. >> >> I wasn't suggesting replacing the existing security_inode_permission >> hook later, but adding a new security_file_permission hook here. >> >>>> >>>> They are not equivalent at least as far as SELinux is concerned. >>>> security_file_permission() was only to be used to revalidate >>>> read/write permissions previously checked at file open to support >>>> policy changes and file or process label changes. We'd have to modify >>>> the SELinux hook if we wanted to have it check execute access even if >>>> nothing has changed since open time. >>> >>> Also Smack doesn't appear to implement file_permission at all, so it >>> would skip Smack checking. >> >> My question is whether adding a new security_file_permission call here >> would break either SELinux or Apparmor? > > selinux_inode_permission() has special handling for MAY_ACCESS so we'd > need to duplicate that into selinux_file_permission() -> > selinux_revalidate_file_permission(). Also likely need to adjust > selinux_file_permission() to explicitly check whether the mask > includes any permissions not checked at open time. So some changes > would be needed here. By default, it would be a no-op unless there > was a policy reload or the file was relabeled between the open(2) and > the faccessat(2) call. > We could create a new hook path_permission(struct path *path, int mask) as a superset of inode_permission(). To be more convenient, his new hook could then just call inode_permission() for every LSMs not implementing path_permission().
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. 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;
[Cc'ing Casey] On Tue, 2020-09-08 at 16:14 +0200, Mickaël Salaün wrote: > On 08/09/2020 15:42, Stephen Smalley wrote: > > On Tue, Sep 8, 2020 at 9:29 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > >> > >> On Tue, 2020-09-08 at 08:52 -0400, Stephen Smalley wrote: > >>> On Tue, Sep 8, 2020 at 8:50 AM Stephen Smalley > >>> <stephen.smalley.work@gmail.com> wrote: > >>>> > >>>> On Tue, Sep 8, 2020 at 8:43 AM Mickaël Salaün <mic@digikod.net> 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: > >>>>>>> + 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. > >> > >> I wasn't suggesting replacing the existing security_inode_permission > >> hook later, but adding a new security_file_permission hook here. > >> > >>>> > >>>> They are not equivalent at least as far as SELinux is concerned. > >>>> security_file_permission() was only to be used to revalidate > >>>> read/write permissions previously checked at file open to support > >>>> policy changes and file or process label changes. We'd have to modify > >>>> the SELinux hook if we wanted to have it check execute access even if > >>>> nothing has changed since open time. > >>> > >>> Also Smack doesn't appear to implement file_permission at all, so it > >>> would skip Smack checking. > >> > >> My question is whether adding a new security_file_permission call here > >> would break either SELinux or Apparmor? > > > > selinux_inode_permission() has special handling for MAY_ACCESS so we'd > > need to duplicate that into selinux_file_permission() -> > > selinux_revalidate_file_permission(). Also likely need to adjust > > selinux_file_permission() to explicitly check whether the mask > > includes any permissions not checked at open time. So some changes > > would be needed here. By default, it would be a no-op unless there > > was a policy reload or the file was relabeled between the open(2) and > > the faccessat(2) call. > > > > We could create a new hook path_permission(struct path *path, int mask) > as a superset of inode_permission(). To be more convenient, his new hook > could then just call inode_permission() for every LSMs not implementing > path_permission(). The LSM maintainers need to chime in here on this suggestion. In terms of the name, except for one hook, all the security_path_XXXX() hooks are dependent on CONFIG_SECURITY_PATH being configured. Mimi
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; > >
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. > > 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; > > > >
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; >>> >>> > >
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)) { + 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; + } + } 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. diff --git a/include/linux/fs.h b/include/linux/fs.h index 7519ae003a08..03f1b2da6a87 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -101,6 +101,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, #define MAY_CHDIR 0x00000040 /* called from RCU mode, don't block */ #define MAY_NOT_BLOCK 0x00000080 +/* interpreted accesses checked with faccessat2 and AT_INTERPRETED */ +#define MAY_INTERPRETED_EXEC 0x00000100 /* * flags in file.f_mode. Note that FMODE_READ and FMODE_WRITE must correspond diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h index 2f86b2ad6d7e..dca082b02634 100644 --- a/include/uapi/linux/fcntl.h +++ b/include/uapi/linux/fcntl.h @@ -90,7 +90,8 @@ * unlinkat. The two functions do completely different things and therefore, * the flags can be allowed to overlap. For example, passing AT_REMOVEDIR to * faccessat would be undefined behavior and thus treating it equivalent to - * AT_EACCESS is valid undefined behavior. + * AT_EACCESS is valid undefined behavior. The same goes for AT_SYMLINK_FOLLOW + * and AT_INTERPRETED. */ #define AT_FDCWD -100 /* Special value used to indicate openat should use the current @@ -100,6 +101,15 @@ effective IDs, not real IDs. */ #define AT_REMOVEDIR 0x200 /* Remove directory instead of unlinking file. */ +#define AT_INTERPRETED 0x400 /* Check if the current process should + grant access (e.g. execution) for a + specific file, i.e. enables RWX to + be enforced *by* user space. The + main usage is for script + interpreters to enforce a policy + consistent with the kernel's one + (through sysctl configuration or LSM + policy). */ #define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */ #define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount traversal */ #define AT_EMPTY_PATH 0x1000 /* Allow empty relative pathname */