* Re: [PATCH net-next] netlabel: Fix some kernel-doc warnings
From: Paul Moore @ 2020-09-08 21:23 UTC (permalink / raw)
To: Wang Hai; +Cc: davem, kuba, netdev, linux-kernel, linux-security-module
In-Reply-To: <20200908140543.25514-1-wanghai38@huawei.com>
On Tue, Sep 8, 2020 at 10:09 AM Wang Hai <wanghai38@huawei.com> wrote:
>
> Fixes the following W=1 kernel build warning(s):
>
> net/netlabel/netlabel_calipso.c:438: warning: Excess function parameter 'audit_secid' description in 'calipso_doi_remove'
> net/netlabel/netlabel_calipso.c:605: warning: Excess function parameter 'reg' description in 'calipso_req_delattr'
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Wang Hai <wanghai38@huawei.com>
> ---
> net/netlabel/netlabel_calipso.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Looks good to me, thanks.
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/net/netlabel/netlabel_calipso.c b/net/netlabel/netlabel_calipso.c
> index 249da67d50a2..1a98247ab148 100644
> --- a/net/netlabel/netlabel_calipso.c
> +++ b/net/netlabel/netlabel_calipso.c
> @@ -426,7 +426,7 @@ void calipso_doi_free(struct calipso_doi *doi_def)
> /**
> * calipso_doi_remove - Remove an existing DOI from the CALIPSO protocol engine
> * @doi: the DOI value
> - * @audit_secid: the LSM secid to use in the audit message
> + * @audit_info: NetLabel audit information
> *
> * Description:
> * Removes a DOI definition from the CALIPSO engine. The NetLabel routines will
> @@ -595,7 +595,7 @@ int calipso_req_setattr(struct request_sock *req,
>
> /**
> * calipso_req_delattr - Delete the CALIPSO option from a request socket
> - * @reg: the request socket
> + * @req: the request socket
> *
> * Description:
> * Removes the CALIPSO option from a request socket, if present.
> --
> 2.17.1
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH net-next] cipso: fix 'audit_secid' kernel-doc warning in cipso_ipv4.c
From: Paul Moore @ 2020-09-08 21:22 UTC (permalink / raw)
To: Wang Hai
Cc: davem, kuznet, yoshfuji, kuba, netdev, linux-kernel,
linux-security-module
In-Reply-To: <20200908135915.22039-1-wanghai38@huawei.com>
On Tue, Sep 8, 2020 at 10:02 AM Wang Hai <wanghai38@huawei.com> wrote:
>
> Fixes the following W=1 kernel build warning(s):
>
> net/ipv4/cipso_ipv4.c:510: warning: Excess function parameter 'audit_secid' description in 'cipso_v4_doi_remove'
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Wang Hai <wanghai38@huawei.com>
> ---
> net/ipv4/cipso_ipv4.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks for catching this and submitting the fix.
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 2eb71579f4d2..471d33a0d095 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -498,7 +498,7 @@ static void cipso_v4_doi_free_rcu(struct rcu_head *entry)
> /**
> * cipso_v4_doi_remove - Remove an existing DOI from the CIPSO protocol engine
> * @doi: the DOI value
> - * @audit_secid: the LSM secid to use in the audit message
> + * @audit_info: NetLabel audit information
> *
> * Description:
> * Removes a DOI definition from the CIPSO engine. The NetLabel routines will
> --
> 2.17.1
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2)
From: Mickaël Salaün @ 2020-09-08 14:14 UTC (permalink / raw)
To: Stephen Smalley, Mimi Zohar
Cc: linux-kernel, 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,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Steve Dower, Steve Grubb, Tetsuo Handa,
Thibaut Sautereau, Vincent Strubel, kernel-hardening, linux-api,
linux-integrity, LSM List, Linux FS Devel, Thibaut Sautereau,
Mickaël Salaün, John Johansen
In-Reply-To: <CAEjxPJ5evWDSv-T-p=4OX29Pr584ZRAsnYoxSRd4qFDoryB+fQ@mail.gmail.com>
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().
^ permalink raw reply
* Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2)
From: Stephen Smalley @ 2020-09-08 12:52 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Mimi Zohar, linux-kernel, 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,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Steve Dower, Steve Grubb, Tetsuo Handa,
Thibaut Sautereau, Vincent Strubel, kernel-hardening, linux-api,
linux-integrity, LSM List, Linux FS Devel, Thibaut Sautereau,
Mickaël Salaün, John Johansen
In-Reply-To: <CAEjxPJ49_BgGX50ZAhHh79Qy3OMN6sssnUHT_2yXqdmgyt==9w@mail.gmail.com>
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.
^ permalink raw reply
* Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2)
From: Mimi Zohar @ 2020-09-08 13:29 UTC (permalink / raw)
To: Stephen Smalley, Mickaël Salaün
Cc: linux-kernel, 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,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Steve Dower, Steve Grubb, Tetsuo Handa,
Thibaut Sautereau, Vincent Strubel, kernel-hardening, linux-api,
linux-integrity, LSM List, Linux FS Devel, Thibaut Sautereau,
Mickaël Salaün, John Johansen
In-Reply-To: <CAEjxPJ6ZTKeunzJvWf_kS3QYjca6v1yJq=ad-jCCuDSgG6n60g@mail.gmail.com>
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
^ permalink raw reply
* Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2)
From: Mimi Zohar @ 2020-09-08 12:28 UTC (permalink / raw)
To: Mickaël Salaün, linux-kernel
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,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Steve Dower, Steve Grubb, Tetsuo Handa,
Thibaut Sautereau, Vincent Strubel, kernel-hardening, linux-api,
linux-integrity, linux-security-module, linux-fsdevel,
Thibaut Sautereau, Mickaël Salaün, Stephen Smalley,
John Johansen
In-Reply-To: <20200908075956.1069018-2-mic@digikod.net>
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.
^ permalink raw reply
* Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2)
From: Mickaël Salaün @ 2020-09-08 15:44 UTC (permalink / raw)
To: Mimi Zohar, linux-kernel
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,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Steve Dower, Steve Grubb, Tetsuo Handa,
Thibaut Sautereau, Vincent Strubel, kernel-hardening, linux-api,
linux-integrity, linux-security-module, linux-fsdevel,
Thibaut Sautereau, Mickaël Salaün, Stephen Smalley,
John Johansen
In-Reply-To: <01c23b2607a7dbf734722399931473c053d9b362.camel@linux.ibm.com>
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;
>
>
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Stephen Smalley @ 2020-09-08 11:58 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Paul Moore, Ondrej Mosnacek, Casey Schaufler,
Tyler Hicks, tusharsu, Sasha Levin, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <7c4e2e9f-54e1-1dee-c33c-64dac0fe9678@linux.microsoft.com>
On Tue, Sep 8, 2020 at 12:44 AM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> On 9/7/20 3:32 PM, Stephen Smalley wrote:
>
> >> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> >> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> >> Reported-by: kernel test robot <lkp@intel.com> # error: implicit declaration of function 'vfree'
> >> Reported-by: kernel test robot <lkp@intel.com> # error: implicit declaration of function 'crypto_alloc_shash'
> >> Reported-by: kernel test robot <lkp@intel.com> # sparse: symbol 'security_read_selinux_policy' was not declared. Should it be static?
> >
> > Not sure these Reported-by lines are useful since they were just on
> > submitted versions of the patch not on an actual merged commit.
>
> I'll remove them when I update the patch.
>
> >
> >> diff --git a/security/selinux/measure.c b/security/selinux/measure.c
> >> new file mode 100644
> >> index 000000000000..caf9107937d9
> >> --- /dev/null
> >> +++ b/security/selinux/measure.c
> > <snip>
> >> +void selinux_measure_state(struct selinux_state *state, bool policy_mutex_held)
> >> +{
> > <snip>
> >> +
> >> + if (!policy_mutex_held)
> >> + mutex_lock(&state->policy_mutex);
> >> +
> >> + rc = security_read_policy_kernel(state, &policy, &policy_len);
> >> +
> >> + if (!policy_mutex_held)
> >> + mutex_unlock(&state->policy_mutex);
> >
> > This kind of conditional taking of a mutex is generally frowned upon
> > in my experience.
> > You should likely just always take the mutex in the callers of
> > selinux_measure_state() instead.
> > In some cases, it may be the caller of the caller. Arguably selinuxfs
> > could be taking it around all state modifying operations (e.g.
> > enforce, checkreqprot) not just policy modifying ones although it
> > isn't strictly for that purpose.
>
> Since currently policy_mutex is not used to synchronize access to state
> variables (enforce, checkreqprot, etc.) I am wondering if
> selinux_measure_state() should measure only state if policy_mutex is not
> held by the caller - similar to how we skip measuring policy if
> initialization is not yet completed.
No, we want to measure policy whenever there is a policy to measure.
Just move the taking of the mutex to the callers of
selinux_measure_state() so that it can be unconditional.
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Stephen Smalley @ 2020-09-08 12:35 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Paul Moore, Ondrej Mosnacek, Casey Schaufler,
Tyler Hicks, tusharsu, Sasha Levin, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ4Swgi2Jewzja8MRiVdYn8H1-OkDy5BR7Vv4A4LaLWZ+Q@mail.gmail.com>
On Tue, Sep 8, 2020 at 8:28 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, Sep 7, 2020 at 5:39 PM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
> >
> > Critical data structures of security modules are currently not measured.
> > Therefore an attestation service, for instance, would not be able to
> > attest whether the security modules are always operating with the policies
> > and configuration that the system administrator had setup. The policies
> > and configuration for the security modules could be tampered with by
> > rogue user mode agents or modified through some inadvertent actions on
> > the system. Measuring such critical data would enable an attestation
> > service to reliably assess the security configuration of the system.
> >
> > SELinux configuration and policy are some of the critical data for this
> > security module that needs to be measured. This measurement can be used
> > by an attestation service, for instance, to verify if the configuration
> > and policies have been setup correctly and that they haven't been tampered
> > with at runtime.
> >
> > Measure SELinux configuration, policy capabilities settings, and the hash
> > of the loaded policy by calling the IMA hook ima_measure_critical_data().
> > Since the size of the loaded policy can be quite large, hash of the policy
> > is measured instead of the entire policy to avoid bloating the IMA log.
> >
> > Enable early boot measurement for SELinux in IMA since SELinux
> > initializes its state and policy before custom IMA policy is loaded.
> >
> > Sample measurement of SELinux state and hash of the policy:
> >
> > 10 e32e...5ac3 ima-buf sha256:86e8...4594 selinux-state-1595389364:287899386 696e697469616c697a65643d313b656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574776f726b5f706565725f636f6e74726f6c733d313b6f70656e5f7065726d733d313b657874656e6465645f736f636b65745f636c6173733d313b616c776179735f636865636b5f6e6574776f726b3d303b6367726f75705f7365636c6162656c3d313b6e6e705f6e6f737569645f7472616e736974696f6e3d313b67656e66735f7365636c6162656c5f73796d6c696e6b733d303
> > 10 9e81...0857 ima-buf sha256:4941...68fc selinux-policy-hash-1597335667:462051628 8d1d...1834
> >
> > To verify the measurement check the following:
> >
> > Execute the following command to extract the measured data
> > from the IMA log for SELinux configuration (selinux-state).
> >
> > grep -m 1 "selinux-state" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | cut -d' ' -f 6 | xxd -r -p
> >
> > The output should be the list of key-value pairs. For example,
> > initialized=1;enabled=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0;
> >
> > To verify the measured data with the current SELinux state:
> >
> > => enabled should be set to 1 if /sys/fs/selinux folder exists,
> > 0 otherwise
> >
> > For other entries, compare the integer value in the files
> > => /sys/fs/selinux/enforce
> > => /sys/fs/selinux/checkreqprot
> > And, each of the policy capabilities files under
> > => /sys/fs/selinux/policy_capabilities
> >
> > For selinux-policy-hash, the hash of SELinux policy is included
> > in the IMA log entry.
> >
> > To verify the measured data with the current SELinux policy run
> > the following commands and verify the output hash values match.
> >
> > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
> >
> > grep -m 1 "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | cut -d' ' -f 6
> >
> > This patch is based on commit 66ccd2560aff ("selinux: simplify away security_policydb_len()")
> > in "next" branch in https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
> >
> > This patch is dependent on the following patch series and must be
> > applied in the given order:
> > https://patchwork.kernel.org/patch/11709527/
> > https://patchwork.kernel.org/patch/11730193/
> > https://patchwork.kernel.org/patch/11730757/
> >
> > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > ---
> >
> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index 953314d145bb..9bf0f65d720b 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -324,8 +324,7 @@ config IMA_MEASURE_ASYMMETRIC_KEYS
> >
> > config IMA_QUEUE_EARLY_BOOT_DATA
> > bool
> > - depends on IMA_MEASURE_ASYMMETRIC_KEYS
> > - depends on SYSTEM_TRUSTED_KEYRING
> > + depends on (IMA_MEASURE_ASYMMETRIC_KEYS && SYSTEM_TRUSTED_KEYRING) || SECURITY_SELINUX
> > default y
>
> I don't see why this is necessary or desirable. We should avoid
> leaking dependencies on a single security module into other
> subsystems.
> It might not yet fully support other security modules but we shouldn't
> preclude adding that in the future.
> Up to the IMA maintainer but I would recommend dropping this part.
Sorry, I misread this part; it doesn't make IMA depend on SELinux it
just allows enabling this early boot data feature if SELinux is
enabled since SELinux is a user of it. Still, it seems unfortunate to
have to explicitly enumerate each consumer of this facility here
whenever a new one is introduced. Is there a reason for doing so and
not just allowing it to be enabled as long as the things on which it
depends are enabled (i.e. not based on its consumers)?
^ permalink raw reply
* [PATCH net-next] cipso: fix 'audit_secid' kernel-doc warning in cipso_ipv4.c
From: Wang Hai @ 2020-09-08 13:59 UTC (permalink / raw)
To: paul, davem, kuznet, yoshfuji, kuba
Cc: netdev, linux-kernel, linux-security-module
Fixes the following W=1 kernel build warning(s):
net/ipv4/cipso_ipv4.c:510: warning: Excess function parameter 'audit_secid' description in 'cipso_v4_doi_remove'
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
net/ipv4/cipso_ipv4.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 2eb71579f4d2..471d33a0d095 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -498,7 +498,7 @@ static void cipso_v4_doi_free_rcu(struct rcu_head *entry)
/**
* cipso_v4_doi_remove - Remove an existing DOI from the CIPSO protocol engine
* @doi: the DOI value
- * @audit_secid: the LSM secid to use in the audit message
+ * @audit_info: NetLabel audit information
*
* Description:
* Removes a DOI definition from the CIPSO engine. The NetLabel routines will
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Lakshmi Ramasubramanian @ 2020-09-08 16:01 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Paul Moore, Ondrej Mosnacek, Casey Schaufler,
Tyler Hicks, tusharsu, Sasha Levin, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ6eGcmbtGX7Kvn8e=ZxBUQD5G=8D+o9-BsVXyDFcyPYMw@mail.gmail.com>
On 9/8/20 4:58 AM, Stephen Smalley wrote:
> On Tue, Sep 8, 2020 at 12:44 AM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
>>
>> On 9/7/20 3:32 PM, Stephen Smalley wrote:
>>
>>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>>>> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
>>>> Reported-by: kernel test robot <lkp@intel.com> # error: implicit declaration of function 'vfree'
>>>> Reported-by: kernel test robot <lkp@intel.com> # error: implicit declaration of function 'crypto_alloc_shash'
>>>> Reported-by: kernel test robot <lkp@intel.com> # sparse: symbol 'security_read_selinux_policy' was not declared. Should it be static?
>>>
>>> Not sure these Reported-by lines are useful since they were just on
>>> submitted versions of the patch not on an actual merged commit.
>>
>> I'll remove them when I update the patch.
>>
>>>
>>>> diff --git a/security/selinux/measure.c b/security/selinux/measure.c
>>>> new file mode 100644
>>>> index 000000000000..caf9107937d9
>>>> --- /dev/null
>>>> +++ b/security/selinux/measure.c
>>> <snip>
>>>> +void selinux_measure_state(struct selinux_state *state, bool policy_mutex_held)
>>>> +{
>>> <snip>
>>>> +
>>>> + if (!policy_mutex_held)
>>>> + mutex_lock(&state->policy_mutex);
>>>> +
>>>> + rc = security_read_policy_kernel(state, &policy, &policy_len);
>>>> +
>>>> + if (!policy_mutex_held)
>>>> + mutex_unlock(&state->policy_mutex);
>>>
>>> This kind of conditional taking of a mutex is generally frowned upon
>>> in my experience.
>>> You should likely just always take the mutex in the callers of
>>> selinux_measure_state() instead.
>>> In some cases, it may be the caller of the caller. Arguably selinuxfs
>>> could be taking it around all state modifying operations (e.g.
>>> enforce, checkreqprot) not just policy modifying ones although it
>>> isn't strictly for that purpose.
>>
>> Since currently policy_mutex is not used to synchronize access to state
>> variables (enforce, checkreqprot, etc.) I am wondering if
>> selinux_measure_state() should measure only state if policy_mutex is not
>> held by the caller - similar to how we skip measuring policy if
>> initialization is not yet completed.
>
> No, we want to measure policy whenever there is a policy to measure.
> Just move the taking of the mutex to the callers of
> selinux_measure_state() so that it can be unconditional.
>
Will do.
-lakshmi
^ permalink raw reply
* Re: [RFC PATCH v8 0/3] Add support for AT_INTERPRETED (was O_MAYEXEC)
From: Al Viro @ 2020-09-08 18:50 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-kernel, Aleksa Sarai, Alexei Starovoitov, 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,
Mimi Zohar, Philippe Trébuchet, Scott Shell,
Sean Christopherson, Shuah Khan, Steve Dower, Steve Grubb,
Tetsuo Handa, Thibaut Sautereau, Vincent Strubel,
kernel-hardening, linux-api, linux-integrity,
linux-security-module, linux-fsdevel
In-Reply-To: <20200908075956.1069018-1-mic@digikod.net>
On Tue, Sep 08, 2020 at 09:59:53AM +0200, Mickaël Salaün wrote:
> Hi,
>
> This height patch series rework the previous O_MAYEXEC series by not
> adding a new flag to openat2(2) but to faccessat2(2) instead. As
> suggested, this enables to perform the access check on a file descriptor
> instead of on a file path (while opening it). This may require two
> checks (one on open and then with faccessat2) but it is a more generic
> approach [8].
Again, why is that folded into lookup/open/whatnot, rather than being
an operation applied to a file (e.g. O_PATH one)?
^ permalink raw reply
* Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2)
From: Mimi Zohar @ 2020-09-08 15:38 UTC (permalink / raw)
To: Mickaël Salaün, Stephen Smalley, Casey Schaufler
Cc: linux-kernel, 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,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Steve Dower, Steve Grubb, Tetsuo Handa,
Thibaut Sautereau, Vincent Strubel, kernel-hardening, linux-api,
linux-integrity, LSM List, Linux FS Devel, Thibaut Sautereau,
Mickaël Salaün, John Johansen
In-Reply-To: <532eefa8-49ca-1c23-1228-d5a4e2d8af90@digikod.net>
[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
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Stephen Smalley @ 2020-09-08 12:28 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Paul Moore, Ondrej Mosnacek, Casey Schaufler,
Tyler Hicks, tusharsu, Sasha Levin, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <20200907213855.3572-1-nramas@linux.microsoft.com>
On Mon, Sep 7, 2020 at 5:39 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> Critical data structures of security modules are currently not measured.
> Therefore an attestation service, for instance, would not be able to
> attest whether the security modules are always operating with the policies
> and configuration that the system administrator had setup. The policies
> and configuration for the security modules could be tampered with by
> rogue user mode agents or modified through some inadvertent actions on
> the system. Measuring such critical data would enable an attestation
> service to reliably assess the security configuration of the system.
>
> SELinux configuration and policy are some of the critical data for this
> security module that needs to be measured. This measurement can be used
> by an attestation service, for instance, to verify if the configuration
> and policies have been setup correctly and that they haven't been tampered
> with at runtime.
>
> Measure SELinux configuration, policy capabilities settings, and the hash
> of the loaded policy by calling the IMA hook ima_measure_critical_data().
> Since the size of the loaded policy can be quite large, hash of the policy
> is measured instead of the entire policy to avoid bloating the IMA log.
>
> Enable early boot measurement for SELinux in IMA since SELinux
> initializes its state and policy before custom IMA policy is loaded.
>
> Sample measurement of SELinux state and hash of the policy:
>
> 10 e32e...5ac3 ima-buf sha256:86e8...4594 selinux-state-1595389364:287899386 696e697469616c697a65643d313b656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574776f726b5f706565725f636f6e74726f6c733d313b6f70656e5f7065726d733d313b657874656e6465645f736f636b65745f636c6173733d313b616c776179735f636865636b5f6e6574776f726b3d303b6367726f75705f7365636c6162656c3d313b6e6e705f6e6f737569645f7472616e736974696f6e3d313b67656e66735f7365636c6162656c5f73796d6c696e6b733d303
> 10 9e81...0857 ima-buf sha256:4941...68fc selinux-policy-hash-1597335667:462051628 8d1d...1834
>
> To verify the measurement check the following:
>
> Execute the following command to extract the measured data
> from the IMA log for SELinux configuration (selinux-state).
>
> grep -m 1 "selinux-state" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | cut -d' ' -f 6 | xxd -r -p
>
> The output should be the list of key-value pairs. For example,
> initialized=1;enabled=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0;
>
> To verify the measured data with the current SELinux state:
>
> => enabled should be set to 1 if /sys/fs/selinux folder exists,
> 0 otherwise
>
> For other entries, compare the integer value in the files
> => /sys/fs/selinux/enforce
> => /sys/fs/selinux/checkreqprot
> And, each of the policy capabilities files under
> => /sys/fs/selinux/policy_capabilities
>
> For selinux-policy-hash, the hash of SELinux policy is included
> in the IMA log entry.
>
> To verify the measured data with the current SELinux policy run
> the following commands and verify the output hash values match.
>
> sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
>
> grep -m 1 "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | cut -d' ' -f 6
>
> This patch is based on commit 66ccd2560aff ("selinux: simplify away security_policydb_len()")
> in "next" branch in https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
>
> This patch is dependent on the following patch series and must be
> applied in the given order:
> https://patchwork.kernel.org/patch/11709527/
> https://patchwork.kernel.org/patch/11730193/
> https://patchwork.kernel.org/patch/11730757/
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
>
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 953314d145bb..9bf0f65d720b 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -324,8 +324,7 @@ config IMA_MEASURE_ASYMMETRIC_KEYS
>
> config IMA_QUEUE_EARLY_BOOT_DATA
> bool
> - depends on IMA_MEASURE_ASYMMETRIC_KEYS
> - depends on SYSTEM_TRUSTED_KEYRING
> + depends on (IMA_MEASURE_ASYMMETRIC_KEYS && SYSTEM_TRUSTED_KEYRING) || SECURITY_SELINUX
> default y
I don't see why this is necessary or desirable. We should avoid
leaking dependencies on a single security module into other
subsystems.
It might not yet fully support other security modules but we shouldn't
preclude adding that in the future.
Up to the IMA maintainer but I would recommend dropping this part.
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index cbdd3c7aff8b..c971ec09d855 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -209,6 +209,11 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
> return state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS];
> }
>
> +static inline bool selinux_checkreqprot(const struct selinux_state *state)
> +{
> + return READ_ONCE(state->checkreqprot);
> +}
> +
Since you are introducing this helper, you should also convert
existing reads of selinux_state.checkreqprot and
fsi->state->checkreqprot to use it, and writes to use WRITE_ONCE()
just like for enforcing and disabled. The introduction of the helper
and conversion to use it could be done as a separate patch before this
one.
> diff --git a/security/selinux/measure.c b/security/selinux/measure.c
> new file mode 100644
> index 000000000000..caf9107937d9
> --- /dev/null
> +++ b/security/selinux/measure.c
<snip>
> +static int read_selinux_state(char **state_str, int *state_str_len,
> + struct selinux_state *state)
> +{
> + char *buf, *str_fmt = "%s=%d;";
> + int i, buf_len, curr;
<snip>
> + for (i = 0; i < __POLICYDB_CAPABILITY_MAX; i++) {
> + buf_len += snprintf(NULL, 0, str_fmt,
> + selinux_policycap_names[i],
> + state->policycap[i]);
> + }
This will need to be converted to use
security_policycap_supported(state, i) rather than state->policycap[i]
since the latter is going to be removed by Ondrej's patches I think.
> + for (i = 0; i < __POLICYDB_CAPABILITY_MAX; i++) {
> + curr += snprintf((buf + curr), (buf_len - curr), str_fmt,
> + selinux_policycap_names[i],
> + state->policycap[i]);
Ditto.
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 45e9efa9bf5b..bb460954de03 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -176,6 +176,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
> from_kuid(&init_user_ns, audit_get_loginuid(current)),
> audit_get_sessionid(current));
> enforcing_set(state, new_value);
> + selinux_measure_state(state, false);
I think we should move this to after the avc_ss_reset call so that we
don't introduce a potentially long delay between setting the enforcing
mode and flushing the AVC at least. Potentially it could be moved to
the very end after selnl_notify_setenforce() too so that it doesn't
delay notifying userspace, but that's less crucial.
> if (new_value)
> avc_ss_reset(state->avc, 0);
> selnl_notify_setenforce(new_value);
> @@ -761,6 +762,8 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
>
> fsi->state->checkreqprot = new_value ? 1 : 0;
This should switch to using WRITE_ONCE() or a helper that uses it.
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 8dc111fbe23a..04a9c3d8c19b 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3874,6 +3875,30 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
> }
> #endif /* CONFIG_NETLABEL */
>
> +/**
> + * security_read_selinux_policy - read the policy.
> + * @policy: SELinux policy
> + * @data: binary policy data
> + * @len: length of data in bytes
> + *
> + */
> +static int security_read_selinux_policy(struct selinux_policy *policy,
> + void **data, size_t *len)
> +{
Since this only uses *data, why not just pass that here?
> + int rc;
> + struct policy_file fp;
> +
> + fp.data = *data;
> + fp.len = *len;
> +
> + rc = policydb_write(&policy->policydb, &fp);
> + if (rc)
> + return rc;
> +
> + *len = (unsigned long)fp.data - (unsigned long)*data;
> + return 0;
> +}
> +
> /**
> * security_read_policy - read the policy.
> * @data: binary policy data
^ permalink raw reply
* Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2)
From: Stephen Smalley @ 2020-09-08 12:50 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Mimi Zohar, linux-kernel, 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,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Steve Dower, Steve Grubb, Tetsuo Handa,
Thibaut Sautereau, Vincent Strubel, kernel-hardening, linux-api,
linux-integrity, LSM List, Linux FS Devel, Thibaut Sautereau,
Mickaël Salaün, John Johansen
In-Reply-To: <75451684-58f3-b946-dca4-4760fa0d7440@digikod.net>
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.
^ permalink raw reply
* Re: [RFC PATCH 00/30] ima: Introduce IMA namespace
From: Mimi Zohar @ 2020-09-08 14:03 UTC (permalink / raw)
To: Luke Hinds, Dr. Greg
Cc: Christian Brauner, krzysztof.struczynski, linux-integrity,
Linux Kernel Mailing List, containers, linux-security-module,
stefanb, sunyuqiong1988, mkayaalp, dmitry.kasatkin,
Serge E. Hallyn, James Morris, christian, silviu.vlasceanu,
roberto.sassu, ebiederm, viro, torvalds, luto, jannh, nick.dusek
In-Reply-To: <CAKrSGQR3Pw=Rad2RgUuCHqr0r2Nc6x2nLoo2cVAkD+_8Vbmd7A@mail.gmail.com>
On Mon, 2020-09-07 at 12:50 +0100, Luke Hinds wrote:
> > Candidly, given the politics of security technology being viewed as
> > 'constraining' user rights, I think that a lot of forthcoming security
> > technology may end up being out of tree moving forward.
> >
>
> I think it's prudent to look forward and plan diligently, but I would
> not want perfect to be the enemy of good.
Agreed. This isn't an abstract problem, but one that has already come
up and, hopefully, has been addressed appropriately.
>
> I approach this more from a user's perspective. We are using IMA in
> https://keylime.dev to measure a host and would like to measure
> within a container too. It's the most common request we hear from our
> users.
>
> Perhaps we all collaborate on a proposal extending Stefans work here:
> https://kernsec.org/wiki/index.php/IMA_Namespacing_design_considerati
> ons
>
> I have seen around 3-4 patches now get submitted, so work has been
> done before, and as above, users are present too. We could then have
> some consensus on how this should look and later patches might have
> more success at landing.
>
> Would anyone be interested in this and have recommendations on how we
> could approach this?
When Roberto Sassu and Krzysztof Struczynski contacted me about the
status of Stefan Berger's patch set, based on Yuqiong Sun's work, I was
under the impression that they would be rebasing it on the latest
kernel and going forward from there. Obviously things changed. I
pointed out to them resolving the "IMA namespacing" issue would be the
first thing that needs to be addressed. So here we are.
Definitely, let's have this discussion.
Mimi
^ permalink raw reply
* Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2)
From: Mimi Zohar @ 2020-09-08 15:24 UTC (permalink / raw)
To: Mickaël Salaün, linux-kernel
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,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Steve Dower, Steve Grubb, Tetsuo Handa,
Thibaut Sautereau, Vincent Strubel, kernel-hardening, linux-api,
linux-integrity, linux-security-module, linux-fsdevel,
Thibaut Sautereau, Mickaël Salaün, Stephen Smalley,
John Johansen
In-Reply-To: <75451684-58f3-b946-dca4-4760fa0d7440@digikod.net>
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;
^ permalink raw reply
* Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2)
From: Mickaël Salaün @ 2020-09-08 17:21 UTC (permalink / raw)
To: Mimi Zohar, linux-kernel
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,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Steve Dower, Steve Grubb, Tetsuo Handa,
Thibaut Sautereau, Vincent Strubel, kernel-hardening, linux-api,
linux-integrity, linux-security-module, linux-fsdevel,
Thibaut Sautereau, Mickaël Salaün, Stephen Smalley,
John Johansen
In-Reply-To: <fd635b544ba4f409a76047a4620656ad67738db1.camel@linux.ibm.com>
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;
>>>
>>>
>
>
^ permalink raw reply
* Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2)
From: Mimi Zohar @ 2020-09-08 16:44 UTC (permalink / raw)
To: Mickaël Salaün, linux-kernel
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,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Steve Dower, Steve Grubb, Tetsuo Handa,
Thibaut Sautereau, Vincent Strubel, kernel-hardening, linux-api,
linux-integrity, linux-security-module, linux-fsdevel,
Thibaut Sautereau, Mickaël Salaün, Stephen Smalley,
John Johansen
In-Reply-To: <ed832b7f-dc47-fe54-468b-41de3b64fd83@digikod.net>
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;
> >
> >
^ permalink raw reply
* Re: [RFC PATCH v8 1/3] fs: Introduce AT_INTERPRETED flag for faccessat2(2)
From: Mickaël Salaün @ 2020-09-08 12:43 UTC (permalink / raw)
To: Mimi Zohar, linux-kernel
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,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Steve Dower, Steve Grubb, Tetsuo Handa,
Thibaut Sautereau, Vincent Strubel, kernel-hardening, linux-api,
linux-integrity, linux-security-module, linux-fsdevel,
Thibaut Sautereau, Mickaël Salaün, Stephen Smalley,
John Johansen
In-Reply-To: <d216615b48c093ebe9349a9dab3830b646575391.camel@linux.ibm.com>
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.
>
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Ondrej Mosnacek @ 2020-09-08 13:03 UTC (permalink / raw)
To: Stephen Smalley
Cc: Lakshmi Ramasubramanian, Mimi Zohar, Paul Moore, Casey Schaufler,
Tyler Hicks, tusharsu, Sasha Levin, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ4Swgi2Jewzja8MRiVdYn8H1-OkDy5BR7Vv4A4LaLWZ+Q@mail.gmail.com>
On Tue, Sep 8, 2020 at 2:37 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Mon, Sep 7, 2020 at 5:39 PM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
<snip>
> > diff --git a/security/selinux/measure.c b/security/selinux/measure.c
> > new file mode 100644
> > index 000000000000..caf9107937d9
> > --- /dev/null
> > +++ b/security/selinux/measure.c
> <snip>
> > +static int read_selinux_state(char **state_str, int *state_str_len,
> > + struct selinux_state *state)
> > +{
> > + char *buf, *str_fmt = "%s=%d;";
> > + int i, buf_len, curr;
> <snip>
> > + for (i = 0; i < __POLICYDB_CAPABILITY_MAX; i++) {
> > + buf_len += snprintf(NULL, 0, str_fmt,
> > + selinux_policycap_names[i],
> > + state->policycap[i]);
> > + }
>
> This will need to be converted to use
> security_policycap_supported(state, i) rather than state->policycap[i]
> since the latter is going to be removed by Ondrej's patches I think.
Based on my testing so far, even with just moving the array under
struct selinux_policy, the RCU accessing still brings a significant
overhead (relative to the whole syscalls it is probably negligible,
but relative to the rest of the simpler hooks it is about 30%), so I
don't think it is necessary to adapt other patches to it yet. It will
be my responsibility to adapt to the newly added code when/if I rebase
and respin my patch.
>
> > + for (i = 0; i < __POLICYDB_CAPABILITY_MAX; i++) {
> > + curr += snprintf((buf + curr), (buf_len - curr), str_fmt,
> > + selinux_policycap_names[i],
> > + state->policycap[i]);
>
> Ditto.
>
--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.
^ permalink raw reply
* [PATCH net-next] netlabel: Fix some kernel-doc warnings
From: Wang Hai @ 2020-09-08 14:05 UTC (permalink / raw)
To: paul, davem, kuba; +Cc: netdev, linux-kernel, linux-security-module
Fixes the following W=1 kernel build warning(s):
net/netlabel/netlabel_calipso.c:438: warning: Excess function parameter 'audit_secid' description in 'calipso_doi_remove'
net/netlabel/netlabel_calipso.c:605: warning: Excess function parameter 'reg' description in 'calipso_req_delattr'
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
net/netlabel/netlabel_calipso.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netlabel/netlabel_calipso.c b/net/netlabel/netlabel_calipso.c
index 249da67d50a2..1a98247ab148 100644
--- a/net/netlabel/netlabel_calipso.c
+++ b/net/netlabel/netlabel_calipso.c
@@ -426,7 +426,7 @@ void calipso_doi_free(struct calipso_doi *doi_def)
/**
* calipso_doi_remove - Remove an existing DOI from the CALIPSO protocol engine
* @doi: the DOI value
- * @audit_secid: the LSM secid to use in the audit message
+ * @audit_info: NetLabel audit information
*
* Description:
* Removes a DOI definition from the CALIPSO engine. The NetLabel routines will
@@ -595,7 +595,7 @@ int calipso_req_setattr(struct request_sock *req,
/**
* calipso_req_delattr - Delete the CALIPSO option from a request socket
- * @reg: the request socket
+ * @req: the request socket
*
* Description:
* Removes the CALIPSO option from a request socket, if present.
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
From: Stephen Smalley @ 2020-09-08 13:35 UTC (permalink / raw)
To: John Johansen
Cc: Casey Schaufler, Paul Moore, Casey Schaufler, James Morris,
LSM List, SElinux list, linux-audit, Kees Cook, Tetsuo Handa,
Stephen Smalley
In-Reply-To: <CAEjxPJ7i5Ruy=NZ+sq3qCm8ux+sZXY5+XX_zJu3+OqFq3d_SLQ@mail.gmail.com>
On Mon, Sep 7, 2020 at 9:28 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Sat, Sep 5, 2020 at 3:07 PM John Johansen
> <john.johansen@canonical.com> wrote:
> >
> > On 9/5/20 11:13 AM, Casey Schaufler wrote:
> > > On 9/5/2020 6:25 AM, Paul Moore wrote:
> > >> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >>> On 9/4/2020 2:53 PM, Paul Moore wrote:
> > >>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
> > >> ...
> > >>
> > >>>> I understand the concerns you mention, they are all valid as far as
> > >>>> I'm concerned, but I think we are going to get burned by this code as
> > >>>> it currently stands.
> > >>> Yes, I can see that. We're getting burned by the non-extensibility
> > >>> of secids. It will take someone smarter than me to figure out how to
> > >>> fit N secids into 32bits without danger of either failure or memory
> > >>> allocation.
> > >> Sooo what are the next steps here? It sounds like there is some
> > >> agreement that the currently proposed unix_skb_params approach is a
> > >> problem, but it also sounds like you just want to merge it anyway?
> > >
> > > There are real problems with all the approaches. This is by far the
> > > least invasive of the lot. If this is acceptable for now I will commit
> > > to including the dynamic allocation version in the full stacking
> > > (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
> > > to take even longer than it already has. Sigh.
> > >
> > >
> > >> I was sorta hoping for something a bit better.
> > >
> > > I will be looking at alternatives. I am very much open to suggestions.
> > > I'm not even 100% convinced that Stephen's objections to my separate
> > > allocation strategy outweigh its advantages. If you have an opinion on
> > > that, I'd love to hear it.
> > >
> >
> > fwiw I prefer the separate allocation strategy, but as you have already
> > said it trading off one set of problems for another. I would rather see
> > this move forward and one set of trade offs isn't significantly worse
> > than the other to me so, either wfm.
>
> I remain unclear that AppArmor needs this patch at all even when
> support for SO_PEERSEC lands.
> Contrary to the patch description, it is about supporting SCM_SECURITY
> for datagram not SO_PEERSEC. And I don't know of any actual users of
> SCM_SECURITY even for SELinux, just SO_PEERSEC.
I remembered that systemd once tried using SCM_SECURITY but that was a
bug since systemd was using it with stream sockets and that wasn't
supported by the kernel at the time,
https://bugzilla.redhat.com/show_bug.cgi?id=1224211, so systemd
switched over to using SO_PEERSEC. Subsequently I did fix
SCM_SECURITY to work with stream sockets via kernel commit
37a9a8df8ce9de6ea73349c9ac8bdf6ba4ec4f70 but SO_PEERSEC is still
preferred. Looking around, I see that there is still one usage of
SCM_SECURITY in systemd-journald but it doesn't seem to be required
(if provided, journald will pass the label along but nothing seems to
depend on it AFAICT). In any event, I don't believe this patch is
needed to support stacking AppArmor.
^ permalink raw reply
* Re: [RFC PATCH] sched: only issue an audit on privileged operation
From: Ondrej Mosnacek @ 2020-09-08 11:28 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Christian Göttsche, Linux kernel mailing list, SElinux list,
Linux Security Module list, Ingo Molnar, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman
In-Reply-To: <20200908102537.GU2674@hirez.programming.kicks-ass.net>
On Tue, Sep 8, 2020 at 12:26 PM <peterz@infradead.org> wrote:
> On Fri, Sep 04, 2020 at 06:00:31PM +0200, Christian Göttsche wrote:
> > sched_setattr(2) does via kernel/sched/core.c:__sched_setscheduler()
> > issue a CAP_SYS_NICE audit event unconditionally, even when the requested
> > operation does not require that capability / is un-privileged.
> >
> > Perform privilged/unprivileged catigorization first and perform a
> > capable test only if needed.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > ---
> > kernel/sched/core.c | 65 ++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 47 insertions(+), 18 deletions(-)
>
> So who sodding cares about audit, and why is that a reason to make a
> trainwreck of code?
The commit message should be more specific. I believe Christian is
talking about the case where SELinux or other LSM denies the
capability, in which case the denial is usually logged to the audit
log. Obviously, we don't want to get a denial logged when the
capability wasn't actually required for the operation to be allowed.
Unfortunately, the capability interface doesn't provide a way to first
get the decision value and only trigger the auditing when it was
actually used in the decision, so in complex scenarios like this the
caller needs to jump through some hoops to avoid such false-positive
denial records.
--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH v20 17/23] LSM: security_secid_to_secctx in netlink netfilter
From: Pablo Neira Ayuso @ 2020-09-08 10:46 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel, paul, sds,
netdev
In-Reply-To: <20200826145247.10029-18-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 07:52:41AM -0700, Casey Schaufler wrote:
> Change netlink netfilter interfaces to use lsmcontext
> pointers, and remove scaffolding.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org
> ---
> net/netfilter/nfnetlink_queue.c | 31 ++++++++++++-------------------
> 1 file changed, 12 insertions(+), 19 deletions(-)
>
[...]
> static u32 nfqnl_get_bridge_size(struct nf_queue_entry *entry)
> @@ -401,8 +399,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> enum ip_conntrack_info ctinfo;
> struct nfnl_ct_hook *nfnl_ct;
> bool csum_verify;
> - struct lsmcontext scaff; /* scaffolding */
> - char *secdata = NULL;
> + struct lsmcontext context = { };
> u32 seclen = 0;
While at it, please introduce reverse xmas tree in variable
definitions incrementally:
struct lsmcontext context = { };
enum ip_conntrack_info ctinfo;
struct nfnl_ct_hook *nfnl_ct;
bool csum_verify;
u32 seclen = 0;
And Cc: netfilter-devel@vger.kernel.org for patches that update the
Netfilter codebase.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox