* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Casey Schaufler @ 2019-07-10 21:14 UTC (permalink / raw)
To: Xing, Cedric, linux-sgx, linux-security-module, selinux, casey
In-Reply-To: <e2a0d952-b4d4-f8f3-ee58-eba63f30dc66@intel.com>
On 7/9/2019 5:55 PM, Xing, Cedric wrote:
> On 7/9/2019 5:10 PM, Casey Schaufler wrote:
>> On 7/9/2019 3:13 PM, Xing, Cedric wrote:
>>> On 7/8/2019 4:53 PM, Casey Schaufler wrote:
>>>> On 7/8/2019 10:16 AM, Xing, Cedric wrote:
>>>>> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>>>>>> In this scheme you use an ema LSM to manage your ema data.
>>>>>> A quick sketch looks like:
>>>>>>
>>>>>> sgx_something_in() calls
>>>>>> security_enclave_load() calls
>>>>>> ema_enclave_load()
>>>>>> selinux_enclave_load()
>>>>>> otherlsm_enclave_load()
>>>>>>
>>>>>> Why is this better than:
>>>>>>
>>>>>> sgx_something_in() calls
>>>>>> ema_enclave_load()
>>>>>> security_enclave_load() calls
>>>>>> selinux_enclave_load()
>>>>>> otherlsm_enclave_load()
>>>>>
>>>>> Are you talking about moving EMA somewhere outside LSM?
>>>>
>>>> Yes. That's what I've been saying all along.
>>>>
>>>>> If so, where?
>>>>
>>>> I tried to make it obvious. Put the call to your EMA code
>>>> on the line before you call security_enclave_load().
>>>
>>> Sorry but I'm still confused.
>>>
>>> EMA code is used by LSMs only. Making it callable from other parts of the kernel IMHO is probably not a good idea. And more importantly I don't understand the motivation behind it. Would you please elaborate?
>>
>> LSM modules implement additional access control restrictions.
>> The EMA code does not do that, it provides management of data
>> that is used by security modules. It is not one itself. VFS
>> also performs this role, but no one would consider making VFS
>> a security module.
>
> You are right.
So far, so good ...
> EMA is more like a helper library than a real LSM.
Then you should use it as such.
> But the practical problem is, it has a piece of initialization code, to basically request some space in the file blob from the LSM infrastructure.
The security modules that want to use EMA should
request that space.
> That cannot be done by any LSMs at runtime.
Sure it can. And it has to. What if you don't
have any security modules that use the EMA data?
Surely you don't want to be allocating blob space
for EMA data if no one is going to use it.
> So it has to either be done in LSM infrastructure directly, or make itself an LSM to make its initialization function invoked by LSM infrastructure automatically.
That is not true. The security module that wants to use
the EMA data can call whatever allocation function you use.
Or, the call can be made from the code just before you call
the security hook, which would be identical to calling it
as a "first" hook.
> You have objected to the former, so I switched to the latter. Are you now objecting to the latter as well? Then what are you suggesting, really?
Call your allocation function just before you call
security_enclave_load(). There is no way that selinux_enclave_load()
could tell the difference.
> VFS is a completely different story. It's the file system abstraction so it has a natural place to live in the kernel, and its initialization doesn't depend on the LSM infrastructure. EMA on the other hand, shall belong to LSM because it is both produced and consumed within LSM.
And this is the enclave abstraction, or rather, should be
according to at least half the people joining in on the
thread. It does not belong in the LSM infrastructure because
it is it's own thing, with its own state and data, which it
needs to maintain in its own way and place. It needs interfaces
so that security modules can use that information appropriately.
It needs a hook or two so that the enclave abstraction can ask
the security modules to make decisions.
>
> And, Stephen, do you have an opinion on this?
>
>>>>>>> +/**
>>>>>>> + * ema - Enclave Memory Area structure for LSM modules
>>>>>>
>>>>>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>>>>>
>>>>> Noted
>>>>>
>>>>>>> diff --git a/security/Makefile b/security/Makefile
>>>>>>> index c598b904938f..b66d03a94853 100644
>>>>>>> --- a/security/Makefile
>>>>>>> +++ b/security/Makefile
>>>>>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>>>>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>>>>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>>>>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>>>>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>>>>>
>>>>>> The config option and the file name ought to match,
>>>>>> or at least be closer.
>>>>>
>>>>> Just trying to match file names as "capability" uses commoncap.c.
>>>>
>>>> Fine, then you should be using CONFIG_SECURITY_EMA.
>>>>
>>>>>
>>>>> Like I said, this feature could potentially be used by TEEs other than SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you have a preference?
>>>>
>>>> Make
>>>> CONFIG_SECURITY_EMA
>>>> depends on CONFIG_INTEL_SGX
>>>>
>>>> When another TEE (maybe MIPS_SSRPQ) comes along you can have
>>>>
>>>> CONFIG_SECURITY_EMA
>>>> depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
>>>
>>> Your suggestions are reasonable. Given such config change wouldn't affect any code, can we do it later,
>>
>> That doesn't make the current options any less confusing,
>> and it will be easier to make the change now than at some
>> point in the future.
>>
>>> e.g., when additional TEEs come online and make use of these new hooks? After all, security_enclave_init() will need amendment anyway as one of its current parameters is of type 'struct sgx_sigstruct', which will need to be replaced with something more generic. At the time being, I'd like to keep things intuitive so as not to confuse reviewers.
>>
>> Reviewers (including me) are already confused by the inconsistency.
>
> OK. Let me make this change.
Thank you.
>>>>>
>>>>>>> diff --git a/security/commonema.c b/security/commonema.c
>>>>>>
>>>>>> Put this in a subdirectory. Please.
>>>>>
>>>>> Then why is commoncap.c located in this directory? I'm just trying to match the existing convention.
>>>>
>>>> commoncap is not optional. It is a base part of the
>>>> security subsystem. ema is optional.
>>>
>>> Alright. I'd move it into a sub-folder and rename it to ema.c. Would you be ok with that?
>>
>> Sounds fine.
>
> This is another part that confuses me. Per you comment here, I think you are OK with EMA being part of LSM
Ah. Being in the security directory does not mean it's
a part of the LSM system. Keys and integrity are security
subsystems that are related to, but not part of, the LSM
sub-system.
> (I mean, living somewhere under security/). But your other comment of calling ema_enclave_load() alongside security_enclave_load() made me think EMA and LSM were separate. What do you want really?
Please stop asking the same question over and over.
You're not going to get a different answer from what
you've gotten already. Look back at it what's already
been said.
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Sean Christopherson @ 2019-07-10 20:31 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190710201930.ly7ykediuy35cjze@linux.intel.com>
On Wed, Jul 10, 2019 at 11:19:30PM +0300, Jarkko Sakkinen wrote:
> On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
> > On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
> > > > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
> > > > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> > > > >
> > > > > I still don't get why we need this whole mess and do not simply admit
> > > > > that there are two distinct roles:
> > > > >
> > > > > 1. Creator
> > > > > 2. User
> > > >
> > > > Because SELinux has existing concepts of EXECMEM and EXECMOD.
> > >
> > > What is the official documentation for those? I've only found some
> > > explanations from discussions and some RHEL sysadmin guides.
> >
> > No clue. My knowledge was gleaned from the code and from Stephen's
> > feedback.
>
> OK, thanks for elaboration. Got nailed some details I was missing :-)
>
> Anyway, to accompany your code changes I'm eager to document this not
> least because it is a good peer test that this all make sense (you
> cannot "unit test" a security model so that is the next best thing).
>
> Still, we need a documentation reference to reflect the narrative
> for these changes, seriously. It cannot be that SELinux is widely
> deployed and it completely lacks documentation for its basic
> objects, can it?
The vast majority of documentation I've found is on *using* SELinux,
e.g. writing policies and whatnot. I haven't found anything on its
internal details, although I admitedly haven't looked all that hard.
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Jarkko Sakkinen @ 2019-07-10 20:19 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190709170917.GD25369@linux.intel.com>
On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
> On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
> > > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
> > > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> > > >
> > > > I still don't get why we need this whole mess and do not simply admit
> > > > that there are two distinct roles:
> > > >
> > > > 1. Creator
> > > > 2. User
> > >
> > > Because SELinux has existing concepts of EXECMEM and EXECMOD.
> >
> > What is the official documentation for those? I've only found some
> > explanations from discussions and some RHEL sysadmin guides.
>
> No clue. My knowledge was gleaned from the code and from Stephen's
> feedback.
OK, thanks for elaboration. Got nailed some details I was missing :-)
Anyway, to accompany your code changes I'm eager to document this not
least because it is a good peer test that this all make sense (you
cannot "unit test" a security model so that is the next best thing).
Still, we need a documentation reference to reflect the narrative
for these changes, seriously. It cannot be that SELinux is widely
deployed and it completely lacks documentation for its basic
objects, can it?
/Jarkko
^ permalink raw reply
* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
From: Eric Biggers @ 2019-07-10 20:15 UTC (permalink / raw)
To: Linus Torvalds, David Howells, James Morris, keyrings, Netdev,
linux-nfs, CIFS, linux-afs, linux-fsdevel, linux-integrity,
LSM List, Linux List Kernel Mailing
In-Reply-To: <20190710194620.GA83443@gmail.com>
On Wed, Jul 10, 2019 at 12:46:22PM -0700, Eric Biggers wrote:
> On Wed, Jul 10, 2019 at 11:35:07AM -0700, Linus Torvalds wrote:
> > On Fri, Jul 5, 2019 at 2:30 PM David Howells <dhowells@redhat.com> wrote:
> > >
> > > Here's my fourth block of keyrings changes for the next merge window. They
> > > change the permissions model used by keys and keyrings to be based on an
> > > internal ACL by the following means:
> >
> > It turns out that this is broken, and I'll probably have to revert the
> > merge entirely.
> >
> > With this merge in place, I can't boot any of the machines that have
> > an encrypted disk setup. The boot just stops at
> >
> > systemd[1]: Started Forward Password Requests to Plymouth Directory Watch.
> > systemd[1]: Reached target Paths.
> >
> > and never gets any further. I never get the prompt for a passphrase
> > for the disk encryption.
> >
> > Apparently not a lot of developers are using encrypted volumes for
> > their development machines.
> >
> > I'm not sure if the only requirement is an encrypted volume, or if
> > this is also particular to a F30 install in case you need to be able
> > to reproduce. But considering that you have a redhat email address,
> > I'm sure you can find a F30 install somewhere with an encrypted disk.
> >
> > David, if you can fix this quickly, I'll hold off on the revert of it
> > all, but I can wait only so long. I've stopped merging stuff since I
> > noticed my machines don't work (this merge window has not been
> > pleasant so far - in addition to this issue I had another entirely
> > unrelated boot failure which made bisecting this one even more fun).
> >
> > So if I don't see a quick fix, I'll just revert in order to then
> > continue to do pull requests later today. Because I do not want to do
> > further pulls with something that I can't boot as a base.
> >
> > Linus
>
> This also broke 'keyctl new_session' and hence all the fscrypt tests
> (https://lkml.kernel.org/lkml/20190710011559.GA7973@sol.localdomain/), and it
> also broke loading in-kernel X.509 certificates
> (https://lore.kernel.org/lkml/27671.1562384658@turing-police/T/#u).
>
> I'm *guessing* these are all some underlying issue where keyrings aren't being
> given all the needed permissions anymore.
>
> But just FYI, David had said he's on vacation with no laptop or email access for
> 2 weeks starting from Sunday (3 days ago). So I don't think you can expect a
> quick fix from him.
>
> I was planning to look into this to fix the fscrypt tests, but it might be a few
> days before I get to it. And while I'm *guessing* it will be a simple fix, it
> might not be. So I can't speak for David, but personally I'm fine with the
> commits being reverted for now.
>
> I'm also unhappy that the new keyctl KEYCTL_GRANT_PERMISSION doesn't have any
> documentation or tests. (Which seems to be a common problem with David's
> work... None of the new mount syscalls in v5.2 have any tests, for example, and
> the man pages are still work-in-progress and last sent out for review a year
> ago, despite API changes that occurred before the syscalls were merged.)
>
Also worth noting that the key ACL patches were only in linux-next for 9 days
before the pull request was sent. The X.509 certificate loading bug (which
might be the same underlying bug) was reported on July 6 by someone testing
linux-next, but the pull request had already been sent on July 5. I suspect
these bug(s) would have been fixed if they had been in linux-next for longer.
- Eric
^ permalink raw reply
* Re: [RFC PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Casey Schaufler @ 2019-07-10 20:09 UTC (permalink / raw)
To: Stephen Smalley, Aaron Goidel, paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, linux-kernel, casey
In-Reply-To: <8edfc3d7-9944-9aed-061b-b81f54ebddc3@tycho.nsa.gov>
On 7/10/2019 11:39 AM, Stephen Smalley wrote:
> On 7/10/19 12:38 PM, Casey Schaufler wrote:
>> On 7/10/2019 6:34 AM, Aaron Goidel wrote:
>>> As of now, setting watches on filesystem objects has, at most, applied a
>>> check for read access to the inode, and in the case of fanotify, requires
>>> CAP_SYS_ADMIN. No specific security hook or permission check has been
>>> provided to control the setting of watches. Using any of inotify, dnotify,
>>> or fanotify, it is possible to observe, not only write-like operations, but
>>> even read access to a file. Modeling the watch as being merely a read from
>>> the file is insufficient.
>>
>> That's a very model-specific viewpoint. It is true for
>> a fine-grained model such as SELinux, but not necessarily
>> for a model with more traditional object definitions.
>> I'm not saying you're wrong, I'm saying that stating it
>> as a given assumes your model. You can do that all you want
>> within SELinux, but it doesn't hold when you're talking
>> about the LSM infrastructure.
>
> I think you'll find that even for Smack, merely checking read access to the watched inode is insufficient for your purposes, because the watch permits more than just observing changes to the state of the inode. The absence of a hook is a gap in LSM coverage, regardless of security model. If you are just objecting to the wording choice, then I suppose that can be amended to "is insufficient for SELinux" or "is insufficient for some needs" or something.
More an objection to the assumption of model than anything else.
There are enough differing viewpoints on what is necessary and/or
sufficient that I wouldn't want the assumption to be a bone of
contention later on.
>
>> Have you coordinated this with the work that David Howells
>> is doing on generic notifications?
>
> We're following that work but to date it hasn't appeared to address dnotify/inotify/fanotify IIUC. I think it is complementary; we are adding LSM control over an existing kernel notification mechanism while he is adding a new notification facility for other kinds of events along with corresponding LSM hooks. It is consistent in that it provides a way to control setting of watches based on the watched object.
All true. My hope is that LSM controls on notification mechanisms
have some sort of coordination. I'd rather have one hook that's used
in multiple places than yet another set of disparate hooks that do
mostly the same thing.
>
>>> Furthermore, fanotify watches grant more power to
>>> an application in the form of permission events. While notification events
>>> are solely, unidirectional (i.e. they only pass information to the
>>> receiving application), permission events are blocking. Permission events
>>> make a request to the receiving application which will then reply with a
>>> decision as to whether or not that action may be completed.
>>
>> You're not saying why this is an issue.
>
> It allows the watching application control over the process that is attempting the access. Are you just asking for that to be stated more explicitly?
Yes, that would be good.
>
>>> In order to solve these issues, a new LSM hook is implemented and has been
>>> placed within the system calls for marking filesystem objects with inotify,
>>> fanotify, and dnotify watches. These calls to the hook are placed at the
>>> point at which the target inode has been resolved and are provided with
>>> both the inode and the mask of requested notification events. The mask has
>>> already been translated into common FS_* values shared by the entirety of
>>> the fs notification infrastructure.
>>>
>>> This only provides a hook at the point of setting a watch, and presumes
>>> that permission to set a particular watch implies the ability to receive
>>> all notification about that object which match the mask. This is all that
>>> is required for SELinux. If other security modules require additional hooks
>>> or infrastructure to control delivery of notification, these can be added
>>> by them. It does not make sense for us to propose hooks for which we have
>>> no implementation. The understanding that all notifications received by the
>>> requesting application are all strictly of a type for which the application
>>> has been granted permission shows that this implementation is sufficient in
>>> its coverage.
>>
>> A reasonable approach. It would be *nice* if you had
>> a look at the other security modules to see what they
>> might need from such a hook or hook set.
>>
>>> Fanotify further has the issue that it returns a file descriptor with the
>>> file mode specified during fanotify_init() to the watching process on
>>> event. This is already covered by the LSM security_file_open hook if the
>>> security module implements checking of the requested file mode there.
>>
>> How is this relevant?
>
> It is part of ensuring complete control over fanotify. Some existing security modules (like Smack, for example) currently do not perform this checking of the requested file mode and therefore are subject to this privilege escalation scenario through fanotify. A watcher that only has read access to the file can get a read-write descriptor to it in this manner. You may argue that this doesn't matter because fanotify requires CAP_SYS_ADMIN but even for Smack that isn't the same as CAP_MAC_OVERRIDE.
Yes, there's a difference in the assumptions security modules
make about the privilege escalation. Again the point is that
it isn't a good idea to include a single module's policy regarding
that in the argument for the generic hook. It's enough to explain
why SELinux needs it.
>
>>
>>> The selinux_inode_notify hook implementation works by adding three new
>>> file permissions: watch, watch_reads, and watch_with_perm (descriptions
>>> about which will follow). The hook then decides which subset of these
>>> permissions must be held by the requesting application based on the
>>> contents of the provided mask. The selinux_file_open hook already checks
>>> the requested file mode and therefore ensures that a watching process
>>> cannot escalate its access through fanotify.
>>
>> Thereby increasing the granularity of control available.
>
> It isn't merely a question of granularity but also completeness and preventing privilege escalation.
I was simply making an observation.
>
>>> The watch permission is the baseline permission for setting a watch on an
>>> object and is a requirement for any watch to be set whatsoever. It should
>>> be noted that having either of the other two permissions (watch_reads and
>>> watch_with_perm) does not imply the watch permission, though this could be
>>> changed if need be.
>>>
>>> The watch_reads permission is required to receive notifications from
>>> read-exclusive events on filesystem objects. These events include accessing
>>> a file for the purpose of reading and closing a file which has been opened
>>> read-only. This distinction has been drawn in order to provide a direct
>>> indication in the policy for this otherwise not obvious capability. Read
>>> access to a file should not necessarily imply the ability to observe read
>>> events on a file.
>>>
>>> Finally, watch_with_perm only applies to fanotify masks since it is the
>>> only way to set a mask which allows for the blocking, permission event.
>>> This permission is needed for any watch which is of this type. Though
>>> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
>>> trust to root, which we do not do, and does not support least privilege.
>>>
>>> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
>>> ---
>>> fs/notify/dnotify/dnotify.c | 14 +++++++++++---
>>> fs/notify/fanotify/fanotify_user.c | 11 +++++++++--
>>> fs/notify/inotify/inotify_user.c | 12 ++++++++++--
>>> include/linux/lsm_hooks.h | 2 ++
>>> include/linux/security.h | 7 +++++++
>>> security/security.c | 5 +++++
>>> security/selinux/hooks.c | 22 ++++++++++++++++++++++
>>> security/selinux/include/classmap.h | 2 +-
>>> 8 files changed, 67 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
>>> index 250369d6901d..e91ce092efb1 100644
>>> --- a/fs/notify/dnotify/dnotify.c
>>> +++ b/fs/notify/dnotify/dnotify.c
>>> @@ -22,6 +22,7 @@
>>> #include <linux/sched/signal.h>
>>> #include <linux/dnotify.h>
>>> #include <linux/init.h>
>>> +#include <linux/security.h>
>>> #include <linux/spinlock.h>
>>> #include <linux/slab.h>
>>> #include <linux/fdtable.h>
>>> @@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>>> goto out_err;
>>> }
>>> + /*
>>> + * convert the userspace DN_* "arg" to the internal FS_*
>>> + * defined in fsnotify
>>> + */
>>> + mask = convert_arg(arg);
>>> +
>>> + error = security_inode_notify(inode, mask);
>>> + if (error)
>>> + goto out_err;
>>> +
>>> /* expect most fcntl to add new rather than augment old */
>>> dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
>>> if (!dn) {
>>> @@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>>> goto out_err;
>>> }
>>> - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
>>> - mask = convert_arg(arg);
>>> -
>>> /* set up the new_fsn_mark and new_dn_mark */
>>> new_fsn_mark = &new_dn_mark->fsn_mark;
>>> fsnotify_init_mark(new_fsn_mark, dnotify_group);
>>> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
>>> index a90bb19dcfa2..c0d9fa998377 100644
>>> --- a/fs/notify/fanotify/fanotify_user.c
>>> +++ b/fs/notify/fanotify/fanotify_user.c
>>> @@ -528,7 +528,7 @@ static const struct file_operations fanotify_fops = {
>>> };
>>> static int fanotify_find_path(int dfd, const char __user *filename,
>>> - struct path *path, unsigned int flags)
>>> + struct path *path, unsigned int flags, __u64 mask)
>>> {
>>> int ret;
>>> @@ -567,8 +567,15 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>>> /* you can only watch an inode if you have read permissions on it */
>>> ret = inode_permission(path->dentry->d_inode, MAY_READ);
>>> + if (ret) {
>>> + path_put(path);
>>> + goto out;
>>> + }
>>> +
>>> + ret = security_inode_notify(path->dentry->d_inode, mask);
>>> if (ret)
>>> path_put(path);
>>> +
>>> out:
>>> return ret;
>>> }
>>> @@ -1014,7 +1021,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>>> goto fput_and_out;
>>> }
>>> - ret = fanotify_find_path(dfd, pathname, &path, flags);
>>> + ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
>>> if (ret)
>>> goto fput_and_out;
>>> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
>>> index 7b53598c8804..47b079f20aad 100644
>>> --- a/fs/notify/inotify/inotify_user.c
>>> +++ b/fs/notify/inotify/inotify_user.c
>>> @@ -39,6 +39,7 @@
>>> #include <linux/poll.h>
>>> #include <linux/wait.h>
>>> #include <linux/memcontrol.h>
>>> +#include <linux/security.h>
>>> #include "inotify.h"
>>> #include "../fdinfo.h"
>>> @@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
>>> /*
>>> * find_inode - resolve a user-given path to a specific inode
>>> */
>>> -static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
>>> +static int inotify_find_inode(const char __user *dirname, struct path *path,
>>> + unsigned int flags, __u64 mask)
>>> {
>>> int error;
>>> @@ -351,8 +353,14 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
>>> return error;
>>> /* you can only watch an inode if you have read permissions on it */
>>> error = inode_permission(path->dentry->d_inode, MAY_READ);
>>> + if (error) {
>>> + path_put(path);
>>> + return error;
>>> + }
>>> + error = security_inode_notify(path->dentry->d_inode, mask);
>>> if (error)
>>> path_put(path);
>>> +
>>> return error;
>>> }
>>> @@ -744,7 +752,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
>>> if (mask & IN_ONLYDIR)
>>> flags |= LOOKUP_DIRECTORY;
>>> - ret = inotify_find_inode(pathname, &path, flags);
>>> + ret = inotify_find_inode(pathname, &path, flags, mask);
>>> if (ret)
>>> goto fput_and_out;
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index 47f58cfb6a19..ef6b74938dd8 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>
>> Hook description comment is missing.
>>
>>> @@ -1571,6 +1571,7 @@ union security_list_options {
>>> int (*inode_getxattr)(struct dentry *dentry, const char *name);
>>> int (*inode_listxattr)(struct dentry *dentry);
>>> int (*inode_removexattr)(struct dentry *dentry, const char *name);
>>> + int (*inode_notify)(struct inode *inode, u64 mask);
>>> int (*inode_need_killpriv)(struct dentry *dentry);
>>> int (*inode_killpriv)(struct dentry *dentry);
>>> int (*inode_getsecurity)(struct inode *inode, const char *name,
>>> @@ -1881,6 +1882,7 @@ struct security_hook_heads {
>>> struct hlist_head inode_getxattr;
>>> struct hlist_head inode_listxattr;
>>> struct hlist_head inode_removexattr;
>>> + struct hlist_head inode_notify;
>>> struct hlist_head inode_need_killpriv;
>>> struct hlist_head inode_killpriv;
>>> struct hlist_head inode_getsecurity;
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index 659071c2e57c..50106fb9eef9 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -301,6 +301,7 @@ int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer
>>> void security_inode_getsecid(struct inode *inode, u32 *secid);
>>> int security_inode_copy_up(struct dentry *src, struct cred **new);
>>> int security_inode_copy_up_xattr(const char *name);
>>> +int security_inode_notify(struct inode *inode, u64 mask);
>>> int security_kernfs_init_security(struct kernfs_node *kn_dir,
>>> struct kernfs_node *kn);
>>> int security_file_permission(struct file *file, int mask);
>>> @@ -392,6 +393,7 @@ void security_inode_invalidate_secctx(struct inode *inode);
>>> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
>>> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
>>> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
>>> +
>>
>> Please don't change whitespace unless it's directly adjacent to your code.
>>
>>> #else /* CONFIG_SECURITY */
>>> static inline int call_lsm_notifier(enum lsm_event event, void *data)
>>> @@ -776,6 +778,11 @@ static inline int security_inode_removexattr(struct dentry *dentry,
>>> return cap_inode_removexattr(dentry, name);
>>> }
>>> +static inline int security_inode_notify(struct inode *inode, u64 mask)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> static inline int security_inode_need_killpriv(struct dentry *dentry)
>>> {
>>> return cap_inode_need_killpriv(dentry);
>>> diff --git a/security/security.c b/security/security.c
>>> index 613a5c00e602..57b2a96c1991 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -1251,6 +1251,11 @@ int security_inode_removexattr(struct dentry *dentry, const char *name)
>>> return evm_inode_removexattr(dentry, name);
>>> }
>>> +int security_inode_notify(struct inode *inode, u64 mask)
>>> +{
>>> + return call_int_hook(inode_notify, 0, inode, mask);
>>> +}
>>> +
>>> int security_inode_need_killpriv(struct dentry *dentry)
>>> {
>>> return call_int_hook(inode_need_killpriv, 0, dentry);
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index c61787b15f27..1a37966c2978 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -92,6 +92,7 @@
>>> #include <linux/kernfs.h>
>>> #include <linux/stringhash.h> /* for hashlen_string() */
>>> #include <uapi/linux/mount.h>
>>> +#include <linux/fsnotify.h>
>>> #include "avc.h"
>>> #include "objsec.h"
>>> @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
>>> return -EACCES;
>>> }
>>> +static int selinux_inode_notify(struct inode *inode, u64 mask)
>>> +{
>>> + u32 perm = FILE__WATCH; // basic permission, can a watch be set?
>>
>> We don't use // comments in the Linux kernel.
>>
>>> +
>>> + struct common_audit_data ad;
>>> +
>>> + ad.type = LSM_AUDIT_DATA_INODE;
>>> + ad.u.inode = inode;
>>> +
>>> + // check if the mask is requesting ability to set a blocking watch
>>> + if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM))
>>> + perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
>>> +
>>> + // is the mask asking to watch file reads?
>>> + if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
>>> + perm |= FILE__WATCH_READS; // check that permission as well
>>> +
>>> + return inode_has_perm(current_cred(), inode, perm, &ad);
>>> +}
>>> +
>>> /*
>>> * Copy the inode security context value to the user.
>>> *
>>> @@ -6797,6 +6818,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>>> LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
>>> LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
>>> LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
>>> + LSM_HOOK_INIT(inode_notify, selinux_inode_notify),
>>> LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
>>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
>>> index 201f7e588a29..0654dd2fbebf 100644
>>> --- a/security/selinux/include/classmap.h
>>> +++ b/security/selinux/include/classmap.h
>>> @@ -7,7 +7,7 @@
>>> #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
>>> "rename", "execute", "quotaon", "mounton", "audit_access", \
>>> - "open", "execmod"
>>> + "open", "execmod", "watch", "watch_with_perm", "watch_reads"
>>> #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
>>> "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
>>
>
^ permalink raw reply
* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
From: Eric Biggers @ 2019-07-10 19:46 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Howells, James Morris James Morris, keyrings, Netdev,
linux-nfs, CIFS, linux-afs, linux-fsdevel, linux-integrity,
LSM List, Linux List Kernel Mailing
In-Reply-To: <CAHk-=wjxoeMJfeBahnWH=9zShKp2bsVy527vo3_y8HfOdhwAAw@mail.gmail.com>
On Wed, Jul 10, 2019 at 11:35:07AM -0700, Linus Torvalds wrote:
> On Fri, Jul 5, 2019 at 2:30 PM David Howells <dhowells@redhat.com> wrote:
> >
> > Here's my fourth block of keyrings changes for the next merge window. They
> > change the permissions model used by keys and keyrings to be based on an
> > internal ACL by the following means:
>
> It turns out that this is broken, and I'll probably have to revert the
> merge entirely.
>
> With this merge in place, I can't boot any of the machines that have
> an encrypted disk setup. The boot just stops at
>
> systemd[1]: Started Forward Password Requests to Plymouth Directory Watch.
> systemd[1]: Reached target Paths.
>
> and never gets any further. I never get the prompt for a passphrase
> for the disk encryption.
>
> Apparently not a lot of developers are using encrypted volumes for
> their development machines.
>
> I'm not sure if the only requirement is an encrypted volume, or if
> this is also particular to a F30 install in case you need to be able
> to reproduce. But considering that you have a redhat email address,
> I'm sure you can find a F30 install somewhere with an encrypted disk.
>
> David, if you can fix this quickly, I'll hold off on the revert of it
> all, but I can wait only so long. I've stopped merging stuff since I
> noticed my machines don't work (this merge window has not been
> pleasant so far - in addition to this issue I had another entirely
> unrelated boot failure which made bisecting this one even more fun).
>
> So if I don't see a quick fix, I'll just revert in order to then
> continue to do pull requests later today. Because I do not want to do
> further pulls with something that I can't boot as a base.
>
> Linus
This also broke 'keyctl new_session' and hence all the fscrypt tests
(https://lkml.kernel.org/lkml/20190710011559.GA7973@sol.localdomain/), and it
also broke loading in-kernel X.509 certificates
(https://lore.kernel.org/lkml/27671.1562384658@turing-police/T/#u).
I'm *guessing* these are all some underlying issue where keyrings aren't being
given all the needed permissions anymore.
But just FYI, David had said he's on vacation with no laptop or email access for
2 weeks starting from Sunday (3 days ago). So I don't think you can expect a
quick fix from him.
I was planning to look into this to fix the fscrypt tests, but it might be a few
days before I get to it. And while I'm *guessing* it will be a simple fix, it
might not be. So I can't speak for David, but personally I'm fine with the
commits being reverted for now.
I'm also unhappy that the new keyctl KEYCTL_GRANT_PERMISSION doesn't have any
documentation or tests. (Which seems to be a common problem with David's
work... None of the new mount syscalls in v5.2 have any tests, for example, and
the man pages are still work-in-progress and last sent out for review a year
ago, despite API changes that occurred before the syscalls were merged.)
- Eric
^ permalink raw reply
* Re: [RFC PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Stephen Smalley @ 2019-07-10 18:39 UTC (permalink / raw)
To: Casey Schaufler, Aaron Goidel, paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, linux-kernel
In-Reply-To: <4fd98c88-61a6-a155-5028-db22a778d3c1@schaufler-ca.com>
On 7/10/19 12:38 PM, Casey Schaufler wrote:
> On 7/10/2019 6:34 AM, Aaron Goidel wrote:
>> As of now, setting watches on filesystem objects has, at most, applied a
>> check for read access to the inode, and in the case of fanotify, requires
>> CAP_SYS_ADMIN. No specific security hook or permission check has been
>> provided to control the setting of watches. Using any of inotify, dnotify,
>> or fanotify, it is possible to observe, not only write-like operations, but
>> even read access to a file. Modeling the watch as being merely a read from
>> the file is insufficient.
>
> That's a very model-specific viewpoint. It is true for
> a fine-grained model such as SELinux, but not necessarily
> for a model with more traditional object definitions.
> I'm not saying you're wrong, I'm saying that stating it
> as a given assumes your model. You can do that all you want
> within SELinux, but it doesn't hold when you're talking
> about the LSM infrastructure.
I think you'll find that even for Smack, merely checking read access to
the watched inode is insufficient for your purposes, because the watch
permits more than just observing changes to the state of the inode. The
absence of a hook is a gap in LSM coverage, regardless of security
model. If you are just objecting to the wording choice, then I suppose
that can be amended to "is insufficient for SELinux" or "is insufficient
for some needs" or something.
> Have you coordinated this with the work that David Howells
> is doing on generic notifications?
We're following that work but to date it hasn't appeared to address
dnotify/inotify/fanotify IIUC. I think it is complementary; we are
adding LSM control over an existing kernel notification mechanism while
he is adding a new notification facility for other kinds of events along
with corresponding LSM hooks. It is consistent in that it provides a
way to control setting of watches based on the watched object.
>> Furthermore, fanotify watches grant more power to
>> an application in the form of permission events. While notification events
>> are solely, unidirectional (i.e. they only pass information to the
>> receiving application), permission events are blocking. Permission events
>> make a request to the receiving application which will then reply with a
>> decision as to whether or not that action may be completed.
>
> You're not saying why this is an issue.
It allows the watching application control over the process that is
attempting the access. Are you just asking for that to be stated more
explicitly?
>> In order to solve these issues, a new LSM hook is implemented and has been
>> placed within the system calls for marking filesystem objects with inotify,
>> fanotify, and dnotify watches. These calls to the hook are placed at the
>> point at which the target inode has been resolved and are provided with
>> both the inode and the mask of requested notification events. The mask has
>> already been translated into common FS_* values shared by the entirety of
>> the fs notification infrastructure.
>>
>> This only provides a hook at the point of setting a watch, and presumes
>> that permission to set a particular watch implies the ability to receive
>> all notification about that object which match the mask. This is all that
>> is required for SELinux. If other security modules require additional hooks
>> or infrastructure to control delivery of notification, these can be added
>> by them. It does not make sense for us to propose hooks for which we have
>> no implementation. The understanding that all notifications received by the
>> requesting application are all strictly of a type for which the application
>> has been granted permission shows that this implementation is sufficient in
>> its coverage.
>
> A reasonable approach. It would be *nice* if you had
> a look at the other security modules to see what they
> might need from such a hook or hook set.
>
>> Fanotify further has the issue that it returns a file descriptor with the
>> file mode specified during fanotify_init() to the watching process on
>> event. This is already covered by the LSM security_file_open hook if the
>> security module implements checking of the requested file mode there.
>
> How is this relevant?
It is part of ensuring complete control over fanotify. Some existing
security modules (like Smack, for example) currently do not perform this
checking of the requested file mode and therefore are subject to this
privilege escalation scenario through fanotify. A watcher that only has
read access to the file can get a read-write descriptor to it in this
manner. You may argue that this doesn't matter because fanotify
requires CAP_SYS_ADMIN but even for Smack that isn't the same as
CAP_MAC_OVERRIDE.
>
>> The selinux_inode_notify hook implementation works by adding three new
>> file permissions: watch, watch_reads, and watch_with_perm (descriptions
>> about which will follow). The hook then decides which subset of these
>> permissions must be held by the requesting application based on the
>> contents of the provided mask. The selinux_file_open hook already checks
>> the requested file mode and therefore ensures that a watching process
>> cannot escalate its access through fanotify.
>
> Thereby increasing the granularity of control available.
It isn't merely a question of granularity but also completeness and
preventing privilege escalation.
>> The watch permission is the baseline permission for setting a watch on an
>> object and is a requirement for any watch to be set whatsoever. It should
>> be noted that having either of the other two permissions (watch_reads and
>> watch_with_perm) does not imply the watch permission, though this could be
>> changed if need be.
>>
>> The watch_reads permission is required to receive notifications from
>> read-exclusive events on filesystem objects. These events include accessing
>> a file for the purpose of reading and closing a file which has been opened
>> read-only. This distinction has been drawn in order to provide a direct
>> indication in the policy for this otherwise not obvious capability. Read
>> access to a file should not necessarily imply the ability to observe read
>> events on a file.
>>
>> Finally, watch_with_perm only applies to fanotify masks since it is the
>> only way to set a mask which allows for the blocking, permission event.
>> This permission is needed for any watch which is of this type. Though
>> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
>> trust to root, which we do not do, and does not support least privilege.
>>
>> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
>> ---
>> fs/notify/dnotify/dnotify.c | 14 +++++++++++---
>> fs/notify/fanotify/fanotify_user.c | 11 +++++++++--
>> fs/notify/inotify/inotify_user.c | 12 ++++++++++--
>> include/linux/lsm_hooks.h | 2 ++
>> include/linux/security.h | 7 +++++++
>> security/security.c | 5 +++++
>> security/selinux/hooks.c | 22 ++++++++++++++++++++++
>> security/selinux/include/classmap.h | 2 +-
>> 8 files changed, 67 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
>> index 250369d6901d..e91ce092efb1 100644
>> --- a/fs/notify/dnotify/dnotify.c
>> +++ b/fs/notify/dnotify/dnotify.c
>> @@ -22,6 +22,7 @@
>> #include <linux/sched/signal.h>
>> #include <linux/dnotify.h>
>> #include <linux/init.h>
>> +#include <linux/security.h>
>> #include <linux/spinlock.h>
>> #include <linux/slab.h>
>> #include <linux/fdtable.h>
>> @@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>> goto out_err;
>> }
>>
>> + /*
>> + * convert the userspace DN_* "arg" to the internal FS_*
>> + * defined in fsnotify
>> + */
>> + mask = convert_arg(arg);
>> +
>> + error = security_inode_notify(inode, mask);
>> + if (error)
>> + goto out_err;
>> +
>> /* expect most fcntl to add new rather than augment old */
>> dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
>> if (!dn) {
>> @@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>> goto out_err;
>> }
>>
>> - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
>> - mask = convert_arg(arg);
>> -
>> /* set up the new_fsn_mark and new_dn_mark */
>> new_fsn_mark = &new_dn_mark->fsn_mark;
>> fsnotify_init_mark(new_fsn_mark, dnotify_group);
>> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
>> index a90bb19dcfa2..c0d9fa998377 100644
>> --- a/fs/notify/fanotify/fanotify_user.c
>> +++ b/fs/notify/fanotify/fanotify_user.c
>> @@ -528,7 +528,7 @@ static const struct file_operations fanotify_fops = {
>> };
>>
>> static int fanotify_find_path(int dfd, const char __user *filename,
>> - struct path *path, unsigned int flags)
>> + struct path *path, unsigned int flags, __u64 mask)
>> {
>> int ret;
>>
>> @@ -567,8 +567,15 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>>
>> /* you can only watch an inode if you have read permissions on it */
>> ret = inode_permission(path->dentry->d_inode, MAY_READ);
>> + if (ret) {
>> + path_put(path);
>> + goto out;
>> + }
>> +
>> + ret = security_inode_notify(path->dentry->d_inode, mask);
>> if (ret)
>> path_put(path);
>> +
>> out:
>> return ret;
>> }
>> @@ -1014,7 +1021,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>> goto fput_and_out;
>> }
>>
>> - ret = fanotify_find_path(dfd, pathname, &path, flags);
>> + ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
>> if (ret)
>> goto fput_and_out;
>>
>> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
>> index 7b53598c8804..47b079f20aad 100644
>> --- a/fs/notify/inotify/inotify_user.c
>> +++ b/fs/notify/inotify/inotify_user.c
>> @@ -39,6 +39,7 @@
>> #include <linux/poll.h>
>> #include <linux/wait.h>
>> #include <linux/memcontrol.h>
>> +#include <linux/security.h>
>>
>> #include "inotify.h"
>> #include "../fdinfo.h"
>> @@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
>> /*
>> * find_inode - resolve a user-given path to a specific inode
>> */
>> -static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
>> +static int inotify_find_inode(const char __user *dirname, struct path *path,
>> + unsigned int flags, __u64 mask)
>> {
>> int error;
>>
>> @@ -351,8 +353,14 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
>> return error;
>> /* you can only watch an inode if you have read permissions on it */
>> error = inode_permission(path->dentry->d_inode, MAY_READ);
>> + if (error) {
>> + path_put(path);
>> + return error;
>> + }
>> + error = security_inode_notify(path->dentry->d_inode, mask);
>> if (error)
>> path_put(path);
>> +
>> return error;
>> }
>>
>> @@ -744,7 +752,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
>> if (mask & IN_ONLYDIR)
>> flags |= LOOKUP_DIRECTORY;
>>
>> - ret = inotify_find_inode(pathname, &path, flags);
>> + ret = inotify_find_inode(pathname, &path, flags, mask);
>> if (ret)
>> goto fput_and_out;
>>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index 47f58cfb6a19..ef6b74938dd8 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>
> Hook description comment is missing.
>
>> @@ -1571,6 +1571,7 @@ union security_list_options {
>> int (*inode_getxattr)(struct dentry *dentry, const char *name);
>> int (*inode_listxattr)(struct dentry *dentry);
>> int (*inode_removexattr)(struct dentry *dentry, const char *name);
>> + int (*inode_notify)(struct inode *inode, u64 mask);
>> int (*inode_need_killpriv)(struct dentry *dentry);
>> int (*inode_killpriv)(struct dentry *dentry);
>> int (*inode_getsecurity)(struct inode *inode, const char *name,
>> @@ -1881,6 +1882,7 @@ struct security_hook_heads {
>> struct hlist_head inode_getxattr;
>> struct hlist_head inode_listxattr;
>> struct hlist_head inode_removexattr;
>> + struct hlist_head inode_notify;
>> struct hlist_head inode_need_killpriv;
>> struct hlist_head inode_killpriv;
>> struct hlist_head inode_getsecurity;
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 659071c2e57c..50106fb9eef9 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -301,6 +301,7 @@ int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer
>> void security_inode_getsecid(struct inode *inode, u32 *secid);
>> int security_inode_copy_up(struct dentry *src, struct cred **new);
>> int security_inode_copy_up_xattr(const char *name);
>> +int security_inode_notify(struct inode *inode, u64 mask);
>> int security_kernfs_init_security(struct kernfs_node *kn_dir,
>> struct kernfs_node *kn);
>> int security_file_permission(struct file *file, int mask);
>> @@ -392,6 +393,7 @@ void security_inode_invalidate_secctx(struct inode *inode);
>> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
>> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
>> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
>> +
>
> Please don't change whitespace unless it's directly adjacent to your code.
>
>> #else /* CONFIG_SECURITY */
>>
>> static inline int call_lsm_notifier(enum lsm_event event, void *data)
>> @@ -776,6 +778,11 @@ static inline int security_inode_removexattr(struct dentry *dentry,
>> return cap_inode_removexattr(dentry, name);
>> }
>>
>> +static inline int security_inode_notify(struct inode *inode, u64 mask)
>> +{
>> + return 0;
>> +}
>> +
>> static inline int security_inode_need_killpriv(struct dentry *dentry)
>> {
>> return cap_inode_need_killpriv(dentry);
>> diff --git a/security/security.c b/security/security.c
>> index 613a5c00e602..57b2a96c1991 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -1251,6 +1251,11 @@ int security_inode_removexattr(struct dentry *dentry, const char *name)
>> return evm_inode_removexattr(dentry, name);
>> }
>>
>> +int security_inode_notify(struct inode *inode, u64 mask)
>> +{
>> + return call_int_hook(inode_notify, 0, inode, mask);
>> +}
>> +
>> int security_inode_need_killpriv(struct dentry *dentry)
>> {
>> return call_int_hook(inode_need_killpriv, 0, dentry);
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index c61787b15f27..1a37966c2978 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -92,6 +92,7 @@
>> #include <linux/kernfs.h>
>> #include <linux/stringhash.h> /* for hashlen_string() */
>> #include <uapi/linux/mount.h>
>> +#include <linux/fsnotify.h>
>>
>> #include "avc.h"
>> #include "objsec.h"
>> @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
>> return -EACCES;
>> }
>>
>> +static int selinux_inode_notify(struct inode *inode, u64 mask)
>> +{
>> + u32 perm = FILE__WATCH; // basic permission, can a watch be set?
>
> We don't use // comments in the Linux kernel.
>
>> +
>> + struct common_audit_data ad;
>> +
>> + ad.type = LSM_AUDIT_DATA_INODE;
>> + ad.u.inode = inode;
>> +
>> + // check if the mask is requesting ability to set a blocking watch
>> + if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM))
>> + perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
>> +
>> + // is the mask asking to watch file reads?
>> + if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
>> + perm |= FILE__WATCH_READS; // check that permission as well
>> +
>> + return inode_has_perm(current_cred(), inode, perm, &ad);
>> +}
>> +
>> /*
>> * Copy the inode security context value to the user.
>> *
>> @@ -6797,6 +6818,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>> LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
>> LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
>> LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
>> + LSM_HOOK_INIT(inode_notify, selinux_inode_notify),
>>
>> LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
>>
>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
>> index 201f7e588a29..0654dd2fbebf 100644
>> --- a/security/selinux/include/classmap.h
>> +++ b/security/selinux/include/classmap.h
>> @@ -7,7 +7,7 @@
>>
>> #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
>> "rename", "execute", "quotaon", "mounton", "audit_access", \
>> - "open", "execmod"
>> + "open", "execmod", "watch", "watch_with_perm", "watch_reads"
>>
>> #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
>> "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
>
^ permalink raw reply
* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
From: Linus Torvalds @ 2019-07-10 18:35 UTC (permalink / raw)
To: David Howells
Cc: James Morris James Morris, keyrings, Netdev, linux-nfs, CIFS,
linux-afs, linux-fsdevel, linux-integrity, LSM List,
Linux List Kernel Mailing
In-Reply-To: <28477.1562362239@warthog.procyon.org.uk>
On Fri, Jul 5, 2019 at 2:30 PM David Howells <dhowells@redhat.com> wrote:
>
> Here's my fourth block of keyrings changes for the next merge window. They
> change the permissions model used by keys and keyrings to be based on an
> internal ACL by the following means:
It turns out that this is broken, and I'll probably have to revert the
merge entirely.
With this merge in place, I can't boot any of the machines that have
an encrypted disk setup. The boot just stops at
systemd[1]: Started Forward Password Requests to Plymouth Directory Watch.
systemd[1]: Reached target Paths.
and never gets any further. I never get the prompt for a passphrase
for the disk encryption.
Apparently not a lot of developers are using encrypted volumes for
their development machines.
I'm not sure if the only requirement is an encrypted volume, or if
this is also particular to a F30 install in case you need to be able
to reproduce. But considering that you have a redhat email address,
I'm sure you can find a F30 install somewhere with an encrypted disk.
David, if you can fix this quickly, I'll hold off on the revert of it
all, but I can wait only so long. I've stopped merging stuff since I
noticed my machines don't work (this merge window has not been
pleasant so far - in addition to this issue I had another entirely
unrelated boot failure which made bisecting this one even more fun).
So if I don't see a quick fix, I'll just revert in order to then
continue to do pull requests later today. Because I do not want to do
further pulls with something that I can't boot as a base.
Linus
^ permalink raw reply
* Re: [RFC PATCH v3 4/4] x86/sgx: Implement SGX specific hooks in SELinux
From: Xing, Cedric @ 2019-07-10 18:16 UTC (permalink / raw)
To: Jethro Beekman, Sean Christopherson
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org
In-Reply-To: <d6506d52-2da6-7667-aae4-e3eef7596e93@fortanix.com>
On 7/10/2019 9:08 AM, Jethro Beekman wrote:
> On 2019-07-10 08:49, Sean Christopherson wrote:
>> On Sun, Jul 07, 2019 at 04:41:34PM -0700, Cedric Xing wrote:
>>> selinux_enclave_init() determines if an enclave is allowed to launch,
>>> using the
>>> criteria described earlier. This implementation does NOT accept
>>> SIGSTRUCT in
>>> anonymous memory. The backing file is also cached in struct
>>> file_security_struct and will serve as the base for decisions for
>>> anonymous
>>> pages.
>>
>> Did we ever reach a consensus on whether sigstruct must reside in a file?
>
> This would be inconvenient for me, but I guess I can create a memfd?
No, sigstruct doesn't have to reside in a file.
But the current direction is, in SELinux, what the enclave can do
depends on permissions given to the file containing sigstruct. That
said, if SELinux is in effect, sigstruct has to reside in a real file
with FILE__EXECUTE permission for the enclave to launch. memfd wouldn't
work. To some extent, that serves the purpose of whitelisting.
> --
> Jethro Beekman | Fortanix
>
^ permalink raw reply
* Re: [RFC PATCH v3 4/4] x86/sgx: Implement SGX specific hooks in SELinux
From: Xing, Cedric @ 2019-07-10 17:54 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-sgx, linux-security-module, selinux
In-Reply-To: <20190710154915.GA4331@linux.intel.com>
On 7/10/2019 8:49 AM, Sean Christopherson wrote:
> On Sun, Jul 07, 2019 at 04:41:34PM -0700, Cedric Xing wrote:
>> selinux_enclave_init() determines if an enclave is allowed to launch, using the
>> criteria described earlier. This implementation does NOT accept SIGSTRUCT in
>> anonymous memory. The backing file is also cached in struct
>> file_security_struct and will serve as the base for decisions for anonymous
>> pages.
>
> Did we ever reach a consensus on whether sigstruct must reside in a file?
No. We reached the opposite agreement of *not* requiring sigstruct to
reside in a file at the interface level - i.e., security_enclave_init()
takes a VMA but *not* a file struct as input.
At the implementation level, an LSM may require sigstruct to reside in a
file. But that's a per-LSM decision.
>> + /* Store SIGSTRUCT file for future use */
>> + if (atomic_long_cmpxchg(&fsec->encl_ss, 0, (long)src->vm_file))
>> + return -EEXIST;
>> +
>> + get_file(src->vm_file);
>
> My understanding is that Andy is strongly against pinning a file for the
> duration of the enclave, has that changed?
I think everyone including Andy prefers not to pin any files. But it's a
trade-off among code simplicity, auditing accuracy and memory
consumption. I think the latest suggestion from Stephen was to keep
files open, for SELinux. Again, that's a per-LSM decision.
^ permalink raw reply
* Re: [RFC PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Randy Dunlap @ 2019-07-10 17:53 UTC (permalink / raw)
To: Joe Perches, Casey Schaufler, Aaron Goidel, paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel
In-Reply-To: <079745c94c232591453dcb01c9d9406b721bb6bf.camel@perches.com>
On 7/10/19 10:22 AM, Joe Perches wrote:
> On Wed, 2019-07-10 at 10:18 -0700, Joe Perches wrote:
>> On Wed, 2019-07-10 at 09:49 -0700, Randy Dunlap wrote:
>>> On 7/10/19 9:38 AM, Casey Schaufler wrote:
>>>> On 7/10/2019 6:34 AM, Aaron Goidel wrote:
>>>>> @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
>>>>> return -EACCES;
>>>>> }
>>>>>
>>>>> +static int selinux_inode_notify(struct inode *inode, u64 mask)
>>>>> +{
>>>>> + u32 perm = FILE__WATCH; // basic permission, can a watch be set?
>>>>
>>>> We don't use // comments in the Linux kernel.
>>>>
>>>
>>> I thought that we had recently moved into the 21st century on that issue,
>>> but I don't see it mentioned in coding-style.rst. Maybe we need a Doc update.
>>>
>>> checkpatch allows C99 comments by default.
>>> Joe, do you recall about this?
>>
>> My recollection is it was something I thought was
>> just simple and useful so I added it to checkpatch
>> without going through the negative of the nominal
>> approvals required by modifying CodingStyle.
>
> https://lkml.org/lkml/2016/7/8/625
>
Aha, thanks, I don't recall seeing that one.
--
~Randy
^ permalink raw reply
* Re: [RFC PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Joe Perches @ 2019-07-10 17:22 UTC (permalink / raw)
To: Randy Dunlap, Casey Schaufler, Aaron Goidel, paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel
In-Reply-To: <6ce2ce60b2435940bc8dfa07fa2553c4524d2db5.camel@perches.com>
On Wed, 2019-07-10 at 10:18 -0700, Joe Perches wrote:
> On Wed, 2019-07-10 at 09:49 -0700, Randy Dunlap wrote:
> > On 7/10/19 9:38 AM, Casey Schaufler wrote:
> > > On 7/10/2019 6:34 AM, Aaron Goidel wrote:
> > > > @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
> > > > return -EACCES;
> > > > }
> > > >
> > > > +static int selinux_inode_notify(struct inode *inode, u64 mask)
> > > > +{
> > > > + u32 perm = FILE__WATCH; // basic permission, can a watch be set?
> > >
> > > We don't use // comments in the Linux kernel.
> > >
> >
> > I thought that we had recently moved into the 21st century on that issue,
> > but I don't see it mentioned in coding-style.rst. Maybe we need a Doc update.
> >
> > checkpatch allows C99 comments by default.
> > Joe, do you recall about this?
>
> My recollection is it was something I thought was
> just simple and useful so I added it to checkpatch
> without going through the negative of the nominal
> approvals required by modifying CodingStyle.
https://lkml.org/lkml/2016/7/8/625
^ permalink raw reply
* Re: [RFC PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Joe Perches @ 2019-07-10 17:18 UTC (permalink / raw)
To: Randy Dunlap, Casey Schaufler, Aaron Goidel, paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel
In-Reply-To: <cb754dda-fbce-8169-4cd7-eef66e8d809e@infradead.org>
On Wed, 2019-07-10 at 09:49 -0700, Randy Dunlap wrote:
> On 7/10/19 9:38 AM, Casey Schaufler wrote:
> > On 7/10/2019 6:34 AM, Aaron Goidel wrote:
> > > @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
> > > return -EACCES;
> > > }
> > >
> > > +static int selinux_inode_notify(struct inode *inode, u64 mask)
> > > +{
> > > + u32 perm = FILE__WATCH; // basic permission, can a watch be set?
> >
> > We don't use // comments in the Linux kernel.
> >
>
> I thought that we had recently moved into the 21st century on that issue,
> but I don't see it mentioned in coding-style.rst. Maybe we need a Doc update.
>
> checkpatch allows C99 comments by default.
> Joe, do you recall about this?
My recollection is it was something I thought was
just simple and useful so I added it to checkpatch
without going through the negative of the nominal
approvals required by modifying CodingStyle.
^ permalink raw reply
* Re: [Non-DoD Source] Re: [RFC PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-07-10 17:25 UTC (permalink / raw)
To: Amir Goldstein
Cc: Paul Moore, selinux, LSM List, linux-fsdevel, David Howells,
Jan Kara, James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <CAOQ4uxhKP9AUHqYN24ELP5OcyaJQcpS9hdzuZOm5uJpokFAXvg@mail.gmail.com>
On 7/10/19 10:55 AM, Amir Goldstein wrote:
> On Wed, Jul 10, 2019 at 4:34 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>>
>> As of now, setting watches on filesystem objects has, at most, applied a
>> check for read access to the inode, and in the case of fanotify, requires
>> CAP_SYS_ADMIN. No specific security hook or permission check has been
>> provided to control the setting of watches. Using any of inotify, dnotify,
>> or fanotify, it is possible to observe, not only write-like operations, but
>> even read access to a file. Modeling the watch as being merely a read from
>> the file is insufficient. Furthermore, fanotify watches grant more power to
>> an application in the form of permission events. While notification events
>> are solely, unidirectional (i.e. they only pass information to the
>> receiving application), permission events are blocking. Permission events
>> make a request to the receiving application which will then reply with a
>> decision as to whether or not that action may be completed.
>>
>> In order to solve these issues, a new LSM hook is implemented and has been
>> placed within the system calls for marking filesystem objects with inotify,
>> fanotify, and dnotify watches. These calls to the hook are placed at the
>> point at which the target inode has been resolved and are provided with
>> both the inode and the mask of requested notification events. The mask has
>> already been translated into common FS_* values shared by the entirety of
>> the fs notification infrastructure.
>>
>> This only provides a hook at the point of setting a watch, and presumes
>> that permission to set a particular watch implies the ability to receive
>> all notification about that object which match the mask. This is all that
>> is required for SELinux. If other security modules require additional hooks
>> or infrastructure to control delivery of notification, these can be added
>> by them. It does not make sense for us to propose hooks for which we have
>> no implementation. The understanding that all notifications received by the
>> requesting application are all strictly of a type for which the application
>> has been granted permission shows that this implementation is sufficient in
>> its coverage.
>>
>> Fanotify further has the issue that it returns a file descriptor with the
>> file mode specified during fanotify_init() to the watching process on
>> event. This is already covered by the LSM security_file_open hook if the
>> security module implements checking of the requested file mode there.
>>
>> The selinux_inode_notify hook implementation works by adding three new
>> file permissions: watch, watch_reads, and watch_with_perm (descriptions
>> about which will follow). The hook then decides which subset of these
>> permissions must be held by the requesting application based on the
>> contents of the provided mask. The selinux_file_open hook already checks
>> the requested file mode and therefore ensures that a watching process
>> cannot escalate its access through fanotify.
>>
>> The watch permission is the baseline permission for setting a watch on an
>> object and is a requirement for any watch to be set whatsoever. It should
>> be noted that having either of the other two permissions (watch_reads and
>> watch_with_perm) does not imply the watch permission, though this could be
>> changed if need be.
>>
>> The watch_reads permission is required to receive notifications from
>> read-exclusive events on filesystem objects. These events include accessing
>> a file for the purpose of reading and closing a file which has been opened
>> read-only. This distinction has been drawn in order to provide a direct
>> indication in the policy for this otherwise not obvious capability. Read
>> access to a file should not necessarily imply the ability to observe read
>> events on a file.
>>
>> Finally, watch_with_perm only applies to fanotify masks since it is the
>> only way to set a mask which allows for the blocking, permission event.
>> This permission is needed for any watch which is of this type. Though
>> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
>> trust to root, which we do not do, and does not support least privilege.
>>
>> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
>> ---
>> fs/notify/dnotify/dnotify.c | 14 +++++++++++---
>> fs/notify/fanotify/fanotify_user.c | 11 +++++++++--
>> fs/notify/inotify/inotify_user.c | 12 ++++++++++--
>> include/linux/lsm_hooks.h | 2 ++
>> include/linux/security.h | 7 +++++++
>> security/security.c | 5 +++++
>> security/selinux/hooks.c | 22 ++++++++++++++++++++++
>> security/selinux/include/classmap.h | 2 +-
>> 8 files changed, 67 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
>> index 250369d6901d..e91ce092efb1 100644
>> --- a/fs/notify/dnotify/dnotify.c
>> +++ b/fs/notify/dnotify/dnotify.c
>> @@ -22,6 +22,7 @@
>> #include <linux/sched/signal.h>
>> #include <linux/dnotify.h>
>> #include <linux/init.h>
>> +#include <linux/security.h>
>> #include <linux/spinlock.h>
>> #include <linux/slab.h>
>> #include <linux/fdtable.h>
>> @@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>> goto out_err;
>> }
>>
>> + /*
>> + * convert the userspace DN_* "arg" to the internal FS_*
>> + * defined in fsnotify
>> + */
>> + mask = convert_arg(arg);
>> +
>> + error = security_inode_notify(inode, mask);
>> + if (error)
>> + goto out_err;
>> +
>> /* expect most fcntl to add new rather than augment old */
>> dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
>> if (!dn) {
>> @@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>> goto out_err;
>> }
>>
>> - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
>> - mask = convert_arg(arg);
>> -
>> /* set up the new_fsn_mark and new_dn_mark */
>> new_fsn_mark = &new_dn_mark->fsn_mark;
>> fsnotify_init_mark(new_fsn_mark, dnotify_group);
>> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
>> index a90bb19dcfa2..c0d9fa998377 100644
>> --- a/fs/notify/fanotify/fanotify_user.c
>> +++ b/fs/notify/fanotify/fanotify_user.c
>> @@ -528,7 +528,7 @@ static const struct file_operations fanotify_fops = {
>> };
>>
>> static int fanotify_find_path(int dfd, const char __user *filename,
>> - struct path *path, unsigned int flags)
>> + struct path *path, unsigned int flags, __u64 mask)
>> {
>> int ret;
>>
>> @@ -567,8 +567,15 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>>
>> /* you can only watch an inode if you have read permissions on it */
>> ret = inode_permission(path->dentry->d_inode, MAY_READ);
>> + if (ret) {
>> + path_put(path);
>> + goto out;
>> + }
>> +
>> + ret = security_inode_notify(path->dentry->d_inode, mask);
>> if (ret)
>> path_put(path);
>> +
>> out:
>> return ret;
>> }
>> @@ -1014,7 +1021,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>> goto fput_and_out;
>> }
>>
>> - ret = fanotify_find_path(dfd, pathname, &path, flags);
>> + ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
>> if (ret)
>> goto fput_and_out;
>>
>
> So the mark_type doesn't matter to SELinux?
Good catch regarding the mark_type, I overlooked that as an issue. The
recursive setting of watches--for everything under the mount, in the
case of the mount type, and for all the contents of the
superblock--warrants further security checks.
> You have no need for mount_noitify and sb_notify hooks?
While I fully agree that there needs to be separate logic to handle
security checks for inode, mount, and superblock notifications, I don't
see the need for additional hooks. I believe that it would be sufficient
to pass down the mark_type into the existing hook and create two new
permissions--one for watching sb's and the other for mounts--which will
be checked based on the type.
> A watch permission on the mount/sb root inode implies permission
> (as CAP_SYS_ADMIN) to watch all events in mount/sb?
This would be true for the new watch_mount and watch_sb permissions. In
the case of sb's we can also apply check against the sb security label.
For mounts there is no other security information, so there's likely no
reason to pass it to the hook. Hence, my reasoning for editing the
existing hook in favor of creating new ones.
>
> [...]
>
>> +static int selinux_inode_notify(struct inode *inode, u64 mask)
>> +{
>> + u32 perm = FILE__WATCH; // basic permission, can a watch be set?
>> +
>> + struct common_audit_data ad;
>> +
>> + ad.type = LSM_AUDIT_DATA_INODE;
>> + ad.u.inode = inode;
>> +
>> + // check if the mask is requesting ability to set a blocking watch
>> + if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM))
>
> Better ALL_FSNOTIFY_PERM_EVENTS
Thanks, you will see this in v2!
>
> Thanks,
> Amir.
--
Aaron
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Sean Christopherson @ 2019-07-10 16:57 UTC (permalink / raw)
To: Xing, Cedric
Cc: Jarkko Sakkinen, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <35b9dd93-4988-d998-056c-aeae36076bc0@intel.com>
On Tue, Jul 09, 2019 at 04:11:08PM -0700, Xing, Cedric wrote:
> On 7/9/2019 3:25 PM, Sean Christopherson wrote:
> >On Tue, Jul 09, 2019 at 01:41:28PM -0700, Xing, Cedric wrote:
> >>On 7/9/2019 10:09 AM, Sean Christopherson wrote:
> >>>Translating those to SGX, with a lot of input from Stephen, I ended up
> >>>with the following:
> >>>
> >>> - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
> >>> on an enclave page loaded from a regular file
> >>>
> >>> - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
> >>> required to gain W->X on an enclave page
> >>
> >>EXECMOD basically indicates a file containing self-modifying code. Your
> >>ENCLAVE_EXECDIRTY is however a process permission, which is illogical.
> >
> >How is it illogical? If a PROCESS wants to EXECute a DIRTY ENCLAVE page,
> >then it needs PROCESS2__ENCLAVE_EXECDIRTY
> Just think of the purpose of FILE__EXECMOD. It indicates to LSM the file has
> self-modifying code, hence W->X transition should be considered "normal" and
> allowed, regardless which process that file is loaded into.
>
> The same thing for enclaves here. Whether an enclave contains self-modifying
> code is specific to that enclave, regardless which process it is loaded
> into.
>
> But what are you doing is quite the opposite, and that's I mean by
> "illogical".
Ah. My intent was to minimize the number of new labels, and because W->X
scenarios are not guaranteed to be backed by a file, I went with a per
process permission. Ditto for EXECANON. I'm not opposed to also having a
per file permission that can be used when possible.
Something like this? And maybe merge EXECANON and EXECDIRTY into a single
permission?
Depending on whether sigstruct is required to be in a pinned file, EAUG
pages would need either EXECDIRTY or EXECMOD.
static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
bool measured)
{
const struct cred *cred = current_cred();
u32 sid = cred_sid(cred);
int ret;
/* Currently supported only in noexec kernels. */
WARN_ON_ONCE(!default_noexec);
/* Only executable enclave pages are restricted in any way. */
if (!(prot & PROT_EXEC))
return 0;
if (!measured) {
ret = enclave_has_perm(sid, PROCESS2__ENCLAVE_EXECUNMR);
if (ret)
goto out;
}
if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file))) {
ret = enclave_has_perm(sid, PROCESS2__ENCLAVE_EXECANON);
if (ret)
goto out;
/* Ability to do W->X within the enclave. */
if (prot & PROT_WRITE)
ret = enclave_has_perm(sid,
PROCESS2__ENCLAVE_EXECDIRTY);
} else {
ret = file_has_perm(cred, vma->vm_file, FILE__ENCLAVE_EXECUTE);
if (ret)
goto out;
/*
* Load code from a modified private mapping, or from any file
* mapping with the ability to do W->X within the enclave.
*/
if (vma->anon_vma || (prot & PROT_WRITE))
ret = file_has_perm(cred, vma->vm_file,
FILE__ENCLAVE_EXECMOD);
}
out:
return ret;
}
^ permalink raw reply
* Re: [RFC PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Casey Schaufler @ 2019-07-10 16:55 UTC (permalink / raw)
To: Randy Dunlap, Aaron Goidel, paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel, Joe Perches, casey
In-Reply-To: <cb754dda-fbce-8169-4cd7-eef66e8d809e@infradead.org>
On 7/10/2019 9:49 AM, Randy Dunlap wrote:
> On 7/10/19 9:38 AM, Casey Schaufler wrote:
>> On 7/10/2019 6:34 AM, Aaron Goidel wrote:
>>> @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
>>> return -EACCES;
>>> }
>>>
>>> +static int selinux_inode_notify(struct inode *inode, u64 mask)
>>> +{
>>> + u32 perm = FILE__WATCH; // basic permission, can a watch be set?
>> We don't use // comments in the Linux kernel.
>>
> I thought that we had recently moved into the 21st century on that issue,
> but I don't see it mentioned in coding-style.rst. Maybe we need a Doc update.
Really? Yuck. Next thing you know M4 macros will be allowed.
>
> checkpatch allows C99 comments by default.
> Joe, do you recall about this?
>
> thanks.
^ permalink raw reply
* Re: [RFC PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Randy Dunlap @ 2019-07-10 16:49 UTC (permalink / raw)
To: Casey Schaufler, Aaron Goidel, paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel, Joe Perches
In-Reply-To: <4fd98c88-61a6-a155-5028-db22a778d3c1@schaufler-ca.com>
On 7/10/19 9:38 AM, Casey Schaufler wrote:
> On 7/10/2019 6:34 AM, Aaron Goidel wrote:
>> @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
>> return -EACCES;
>> }
>>
>> +static int selinux_inode_notify(struct inode *inode, u64 mask)
>> +{
>> + u32 perm = FILE__WATCH; // basic permission, can a watch be set?
>
> We don't use // comments in the Linux kernel.
>
I thought that we had recently moved into the 21st century on that issue,
but I don't see it mentioned in coding-style.rst. Maybe we need a Doc update.
checkpatch allows C99 comments by default.
Joe, do you recall about this?
thanks.
--
~Randy
^ permalink raw reply
* Re: [RFC PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Casey Schaufler @ 2019-07-10 16:38 UTC (permalink / raw)
To: Aaron Goidel, paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel, casey
In-Reply-To: <20190710133403.855-1-acgoide@tycho.nsa.gov>
On 7/10/2019 6:34 AM, Aaron Goidel wrote:
> As of now, setting watches on filesystem objects has, at most, applied a
> check for read access to the inode, and in the case of fanotify, requires
> CAP_SYS_ADMIN. No specific security hook or permission check has been
> provided to control the setting of watches. Using any of inotify, dnotify,
> or fanotify, it is possible to observe, not only write-like operations, but
> even read access to a file. Modeling the watch as being merely a read from
> the file is insufficient.
That's a very model-specific viewpoint. It is true for
a fine-grained model such as SELinux, but not necessarily
for a model with more traditional object definitions.
I'm not saying you're wrong, I'm saying that stating it
as a given assumes your model. You can do that all you want
within SELinux, but it doesn't hold when you're talking
about the LSM infrastructure.
Have you coordinated this with the work that David Howells
is doing on generic notifications?
> Furthermore, fanotify watches grant more power to
> an application in the form of permission events. While notification events
> are solely, unidirectional (i.e. they only pass information to the
> receiving application), permission events are blocking. Permission events
> make a request to the receiving application which will then reply with a
> decision as to whether or not that action may be completed.
You're not saying why this is an issue.
> In order to solve these issues, a new LSM hook is implemented and has been
> placed within the system calls for marking filesystem objects with inotify,
> fanotify, and dnotify watches. These calls to the hook are placed at the
> point at which the target inode has been resolved and are provided with
> both the inode and the mask of requested notification events. The mask has
> already been translated into common FS_* values shared by the entirety of
> the fs notification infrastructure.
>
> This only provides a hook at the point of setting a watch, and presumes
> that permission to set a particular watch implies the ability to receive
> all notification about that object which match the mask. This is all that
> is required for SELinux. If other security modules require additional hooks
> or infrastructure to control delivery of notification, these can be added
> by them. It does not make sense for us to propose hooks for which we have
> no implementation. The understanding that all notifications received by the
> requesting application are all strictly of a type for which the application
> has been granted permission shows that this implementation is sufficient in
> its coverage.
A reasonable approach. It would be *nice* if you had
a look at the other security modules to see what they
might need from such a hook or hook set.
> Fanotify further has the issue that it returns a file descriptor with the
> file mode specified during fanotify_init() to the watching process on
> event. This is already covered by the LSM security_file_open hook if the
> security module implements checking of the requested file mode there.
How is this relevant?
> The selinux_inode_notify hook implementation works by adding three new
> file permissions: watch, watch_reads, and watch_with_perm (descriptions
> about which will follow). The hook then decides which subset of these
> permissions must be held by the requesting application based on the
> contents of the provided mask. The selinux_file_open hook already checks
> the requested file mode and therefore ensures that a watching process
> cannot escalate its access through fanotify.
Thereby increasing the granularity of control available.
> The watch permission is the baseline permission for setting a watch on an
> object and is a requirement for any watch to be set whatsoever. It should
> be noted that having either of the other two permissions (watch_reads and
> watch_with_perm) does not imply the watch permission, though this could be
> changed if need be.
>
> The watch_reads permission is required to receive notifications from
> read-exclusive events on filesystem objects. These events include accessing
> a file for the purpose of reading and closing a file which has been opened
> read-only. This distinction has been drawn in order to provide a direct
> indication in the policy for this otherwise not obvious capability. Read
> access to a file should not necessarily imply the ability to observe read
> events on a file.
>
> Finally, watch_with_perm only applies to fanotify masks since it is the
> only way to set a mask which allows for the blocking, permission event.
> This permission is needed for any watch which is of this type. Though
> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
> trust to root, which we do not do, and does not support least privilege.
>
> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
> ---
> fs/notify/dnotify/dnotify.c | 14 +++++++++++---
> fs/notify/fanotify/fanotify_user.c | 11 +++++++++--
> fs/notify/inotify/inotify_user.c | 12 ++++++++++--
> include/linux/lsm_hooks.h | 2 ++
> include/linux/security.h | 7 +++++++
> security/security.c | 5 +++++
> security/selinux/hooks.c | 22 ++++++++++++++++++++++
> security/selinux/include/classmap.h | 2 +-
> 8 files changed, 67 insertions(+), 8 deletions(-)
>
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index 250369d6901d..e91ce092efb1 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -22,6 +22,7 @@
> #include <linux/sched/signal.h>
> #include <linux/dnotify.h>
> #include <linux/init.h>
> +#include <linux/security.h>
> #include <linux/spinlock.h>
> #include <linux/slab.h>
> #include <linux/fdtable.h>
> @@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> + /*
> + * convert the userspace DN_* "arg" to the internal FS_*
> + * defined in fsnotify
> + */
> + mask = convert_arg(arg);
> +
> + error = security_inode_notify(inode, mask);
> + if (error)
> + goto out_err;
> +
> /* expect most fcntl to add new rather than augment old */
> dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
> if (!dn) {
> @@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
> - mask = convert_arg(arg);
> -
> /* set up the new_fsn_mark and new_dn_mark */
> new_fsn_mark = &new_dn_mark->fsn_mark;
> fsnotify_init_mark(new_fsn_mark, dnotify_group);
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index a90bb19dcfa2..c0d9fa998377 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -528,7 +528,7 @@ static const struct file_operations fanotify_fops = {
> };
>
> static int fanotify_find_path(int dfd, const char __user *filename,
> - struct path *path, unsigned int flags)
> + struct path *path, unsigned int flags, __u64 mask)
> {
> int ret;
>
> @@ -567,8 +567,15 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>
> /* you can only watch an inode if you have read permissions on it */
> ret = inode_permission(path->dentry->d_inode, MAY_READ);
> + if (ret) {
> + path_put(path);
> + goto out;
> + }
> +
> + ret = security_inode_notify(path->dentry->d_inode, mask);
> if (ret)
> path_put(path);
> +
> out:
> return ret;
> }
> @@ -1014,7 +1021,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
> goto fput_and_out;
> }
>
> - ret = fanotify_find_path(dfd, pathname, &path, flags);
> + ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
> if (ret)
> goto fput_and_out;
>
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index 7b53598c8804..47b079f20aad 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -39,6 +39,7 @@
> #include <linux/poll.h>
> #include <linux/wait.h>
> #include <linux/memcontrol.h>
> +#include <linux/security.h>
>
> #include "inotify.h"
> #include "../fdinfo.h"
> @@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
> /*
> * find_inode - resolve a user-given path to a specific inode
> */
> -static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
> +static int inotify_find_inode(const char __user *dirname, struct path *path,
> + unsigned int flags, __u64 mask)
> {
> int error;
>
> @@ -351,8 +353,14 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
> return error;
> /* you can only watch an inode if you have read permissions on it */
> error = inode_permission(path->dentry->d_inode, MAY_READ);
> + if (error) {
> + path_put(path);
> + return error;
> + }
> + error = security_inode_notify(path->dentry->d_inode, mask);
> if (error)
> path_put(path);
> +
> return error;
> }
>
> @@ -744,7 +752,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
> if (mask & IN_ONLYDIR)
> flags |= LOOKUP_DIRECTORY;
>
> - ret = inotify_find_inode(pathname, &path, flags);
> + ret = inotify_find_inode(pathname, &path, flags, mask);
> if (ret)
> goto fput_and_out;
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..ef6b74938dd8 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
Hook description comment is missing.
> @@ -1571,6 +1571,7 @@ union security_list_options {
> int (*inode_getxattr)(struct dentry *dentry, const char *name);
> int (*inode_listxattr)(struct dentry *dentry);
> int (*inode_removexattr)(struct dentry *dentry, const char *name);
> + int (*inode_notify)(struct inode *inode, u64 mask);
> int (*inode_need_killpriv)(struct dentry *dentry);
> int (*inode_killpriv)(struct dentry *dentry);
> int (*inode_getsecurity)(struct inode *inode, const char *name,
> @@ -1881,6 +1882,7 @@ struct security_hook_heads {
> struct hlist_head inode_getxattr;
> struct hlist_head inode_listxattr;
> struct hlist_head inode_removexattr;
> + struct hlist_head inode_notify;
> struct hlist_head inode_need_killpriv;
> struct hlist_head inode_killpriv;
> struct hlist_head inode_getsecurity;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..50106fb9eef9 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -301,6 +301,7 @@ int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer
> void security_inode_getsecid(struct inode *inode, u32 *secid);
> int security_inode_copy_up(struct dentry *src, struct cred **new);
> int security_inode_copy_up_xattr(const char *name);
> +int security_inode_notify(struct inode *inode, u64 mask);
> int security_kernfs_init_security(struct kernfs_node *kn_dir,
> struct kernfs_node *kn);
> int security_file_permission(struct file *file, int mask);
> @@ -392,6 +393,7 @@ void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> +
Please don't change whitespace unless it's directly adjacent to your code.
> #else /* CONFIG_SECURITY */
>
> static inline int call_lsm_notifier(enum lsm_event event, void *data)
> @@ -776,6 +778,11 @@ static inline int security_inode_removexattr(struct dentry *dentry,
> return cap_inode_removexattr(dentry, name);
> }
>
> +static inline int security_inode_notify(struct inode *inode, u64 mask)
> +{
> + return 0;
> +}
> +
> static inline int security_inode_need_killpriv(struct dentry *dentry)
> {
> return cap_inode_need_killpriv(dentry);
> diff --git a/security/security.c b/security/security.c
> index 613a5c00e602..57b2a96c1991 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1251,6 +1251,11 @@ int security_inode_removexattr(struct dentry *dentry, const char *name)
> return evm_inode_removexattr(dentry, name);
> }
>
> +int security_inode_notify(struct inode *inode, u64 mask)
> +{
> + return call_int_hook(inode_notify, 0, inode, mask);
> +}
> +
> int security_inode_need_killpriv(struct dentry *dentry)
> {
> return call_int_hook(inode_need_killpriv, 0, dentry);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c61787b15f27..1a37966c2978 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -92,6 +92,7 @@
> #include <linux/kernfs.h>
> #include <linux/stringhash.h> /* for hashlen_string() */
> #include <uapi/linux/mount.h>
> +#include <linux/fsnotify.h>
>
> #include "avc.h"
> #include "objsec.h"
> @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
> return -EACCES;
> }
>
> +static int selinux_inode_notify(struct inode *inode, u64 mask)
> +{
> + u32 perm = FILE__WATCH; // basic permission, can a watch be set?
We don't use // comments in the Linux kernel.
> +
> + struct common_audit_data ad;
> +
> + ad.type = LSM_AUDIT_DATA_INODE;
> + ad.u.inode = inode;
> +
> + // check if the mask is requesting ability to set a blocking watch
> + if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM))
> + perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
> +
> + // is the mask asking to watch file reads?
> + if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
> + perm |= FILE__WATCH_READS; // check that permission as well
> +
> + return inode_has_perm(current_cred(), inode, perm, &ad);
> +}
> +
> /*
> * Copy the inode security context value to the user.
> *
> @@ -6797,6 +6818,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
> LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
> LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
> + LSM_HOOK_INIT(inode_notify, selinux_inode_notify),
>
> LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 201f7e588a29..0654dd2fbebf 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -7,7 +7,7 @@
>
> #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
> "rename", "execute", "quotaon", "mounton", "audit_access", \
> - "open", "execmod"
> + "open", "execmod", "watch", "watch_with_perm", "watch_reads"
>
> #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
> "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
^ permalink raw reply
* Re: [PATCH V34 10/29] hibernate: Disable when the kernel is locked down
From: Joey Lee @ 2019-07-10 15:26 UTC (permalink / raw)
To: Jiri Kosina
Cc: Pavel Machek, Matthew Garrett, jmorris@namei.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
Josh Boyer, David Howells, Matthew Garrett, rjw@rjwysocki.net,
linux-pm@vger.kernel.org
In-Reply-To: <nycvar.YFH.7.76.1906241520500.27227@cbobk.fhfr.pm>
Hi,
On Mon, Jun 24, 2019 at 03:21:23PM +0200, Jiri Kosina wrote:
> On Sat, 22 Jun 2019, Pavel Machek wrote:
>
> > > There is currently no way to verify the resume image when returning
> > > from hibernate. This might compromise the signed modules trust model,
> > > so until we can work with signed hibernate images we disable it when the
> > > kernel is locked down.
> >
> > I keep getting these...
> >
> > IIRC suse has patches to verify the images.
>
> Yeah, Joey Lee is taking care of those. CCing.
>
The last time that I sent for hibernation encryption and authentication is
here:
https://lkml.org/lkml/2019/1/3/281
It needs some big changes after review:
- Simplify the design: remove keyring dependency and trampoline.
- Encrypted whole snapshot image instead of only data pages.
- Using TPM:
- Direct use TPM API in hibernation instead of keyring
- Localities (suggested by James Bottomley)
I am still finding enough time to implement those changes, especial TPM
parts.
Thanks
Joey Lee
^ permalink raw reply
* Re: [RFC PATCH v3 4/4] x86/sgx: Implement SGX specific hooks in SELinux
From: Jethro Beekman @ 2019-07-10 16:08 UTC (permalink / raw)
To: Sean Christopherson, Cedric Xing
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org
In-Reply-To: <20190710154915.GA4331@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 621 bytes --]
On 2019-07-10 08:49, Sean Christopherson wrote:
> On Sun, Jul 07, 2019 at 04:41:34PM -0700, Cedric Xing wrote:
>> selinux_enclave_init() determines if an enclave is allowed to launch, using the
>> criteria described earlier. This implementation does NOT accept SIGSTRUCT in
>> anonymous memory. The backing file is also cached in struct
>> file_security_struct and will serve as the base for decisions for anonymous
>> pages.
>
> Did we ever reach a consensus on whether sigstruct must reside in a file?
This would be inconvenient for me, but I guess I can create a memfd?
--
Jethro Beekman | Fortanix
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3990 bytes --]
^ permalink raw reply
* Re: [RFC PATCH v3 4/4] x86/sgx: Implement SGX specific hooks in SELinux
From: Sean Christopherson @ 2019-07-10 15:49 UTC (permalink / raw)
To: Cedric Xing; +Cc: linux-sgx, linux-security-module, selinux
In-Reply-To: <3a9efc8d3c27490dbcfe802ce3facddd62f47872.1562542383.git.cedric.xing@intel.com>
On Sun, Jul 07, 2019 at 04:41:34PM -0700, Cedric Xing wrote:
> selinux_enclave_init() determines if an enclave is allowed to launch, using the
> criteria described earlier. This implementation does NOT accept SIGSTRUCT in
> anonymous memory. The backing file is also cached in struct
> file_security_struct and will serve as the base for decisions for anonymous
> pages.
Did we ever reach a consensus on whether sigstruct must reside in a file?
> + /* Store SIGSTRUCT file for future use */
> + if (atomic_long_cmpxchg(&fsec->encl_ss, 0, (long)src->vm_file))
> + return -EEXIST;
> +
> + get_file(src->vm_file);
My understanding is that Andy is strongly against pinning a file for the
duration of the enclave, has that changed?
^ permalink raw reply
* Re: [RFC PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Amir Goldstein @ 2019-07-10 14:55 UTC (permalink / raw)
To: Aaron Goidel
Cc: Paul Moore, selinux, LSM List, linux-fsdevel, David Howells,
Jan Kara, James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <20190710133403.855-1-acgoide@tycho.nsa.gov>
On Wed, Jul 10, 2019 at 4:34 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>
> As of now, setting watches on filesystem objects has, at most, applied a
> check for read access to the inode, and in the case of fanotify, requires
> CAP_SYS_ADMIN. No specific security hook or permission check has been
> provided to control the setting of watches. Using any of inotify, dnotify,
> or fanotify, it is possible to observe, not only write-like operations, but
> even read access to a file. Modeling the watch as being merely a read from
> the file is insufficient. Furthermore, fanotify watches grant more power to
> an application in the form of permission events. While notification events
> are solely, unidirectional (i.e. they only pass information to the
> receiving application), permission events are blocking. Permission events
> make a request to the receiving application which will then reply with a
> decision as to whether or not that action may be completed.
>
> In order to solve these issues, a new LSM hook is implemented and has been
> placed within the system calls for marking filesystem objects with inotify,
> fanotify, and dnotify watches. These calls to the hook are placed at the
> point at which the target inode has been resolved and are provided with
> both the inode and the mask of requested notification events. The mask has
> already been translated into common FS_* values shared by the entirety of
> the fs notification infrastructure.
>
> This only provides a hook at the point of setting a watch, and presumes
> that permission to set a particular watch implies the ability to receive
> all notification about that object which match the mask. This is all that
> is required for SELinux. If other security modules require additional hooks
> or infrastructure to control delivery of notification, these can be added
> by them. It does not make sense for us to propose hooks for which we have
> no implementation. The understanding that all notifications received by the
> requesting application are all strictly of a type for which the application
> has been granted permission shows that this implementation is sufficient in
> its coverage.
>
> Fanotify further has the issue that it returns a file descriptor with the
> file mode specified during fanotify_init() to the watching process on
> event. This is already covered by the LSM security_file_open hook if the
> security module implements checking of the requested file mode there.
>
> The selinux_inode_notify hook implementation works by adding three new
> file permissions: watch, watch_reads, and watch_with_perm (descriptions
> about which will follow). The hook then decides which subset of these
> permissions must be held by the requesting application based on the
> contents of the provided mask. The selinux_file_open hook already checks
> the requested file mode and therefore ensures that a watching process
> cannot escalate its access through fanotify.
>
> The watch permission is the baseline permission for setting a watch on an
> object and is a requirement for any watch to be set whatsoever. It should
> be noted that having either of the other two permissions (watch_reads and
> watch_with_perm) does not imply the watch permission, though this could be
> changed if need be.
>
> The watch_reads permission is required to receive notifications from
> read-exclusive events on filesystem objects. These events include accessing
> a file for the purpose of reading and closing a file which has been opened
> read-only. This distinction has been drawn in order to provide a direct
> indication in the policy for this otherwise not obvious capability. Read
> access to a file should not necessarily imply the ability to observe read
> events on a file.
>
> Finally, watch_with_perm only applies to fanotify masks since it is the
> only way to set a mask which allows for the blocking, permission event.
> This permission is needed for any watch which is of this type. Though
> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
> trust to root, which we do not do, and does not support least privilege.
>
> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
> ---
> fs/notify/dnotify/dnotify.c | 14 +++++++++++---
> fs/notify/fanotify/fanotify_user.c | 11 +++++++++--
> fs/notify/inotify/inotify_user.c | 12 ++++++++++--
> include/linux/lsm_hooks.h | 2 ++
> include/linux/security.h | 7 +++++++
> security/security.c | 5 +++++
> security/selinux/hooks.c | 22 ++++++++++++++++++++++
> security/selinux/include/classmap.h | 2 +-
> 8 files changed, 67 insertions(+), 8 deletions(-)
>
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index 250369d6901d..e91ce092efb1 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -22,6 +22,7 @@
> #include <linux/sched/signal.h>
> #include <linux/dnotify.h>
> #include <linux/init.h>
> +#include <linux/security.h>
> #include <linux/spinlock.h>
> #include <linux/slab.h>
> #include <linux/fdtable.h>
> @@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> + /*
> + * convert the userspace DN_* "arg" to the internal FS_*
> + * defined in fsnotify
> + */
> + mask = convert_arg(arg);
> +
> + error = security_inode_notify(inode, mask);
> + if (error)
> + goto out_err;
> +
> /* expect most fcntl to add new rather than augment old */
> dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
> if (!dn) {
> @@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
> - mask = convert_arg(arg);
> -
> /* set up the new_fsn_mark and new_dn_mark */
> new_fsn_mark = &new_dn_mark->fsn_mark;
> fsnotify_init_mark(new_fsn_mark, dnotify_group);
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index a90bb19dcfa2..c0d9fa998377 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -528,7 +528,7 @@ static const struct file_operations fanotify_fops = {
> };
>
> static int fanotify_find_path(int dfd, const char __user *filename,
> - struct path *path, unsigned int flags)
> + struct path *path, unsigned int flags, __u64 mask)
> {
> int ret;
>
> @@ -567,8 +567,15 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>
> /* you can only watch an inode if you have read permissions on it */
> ret = inode_permission(path->dentry->d_inode, MAY_READ);
> + if (ret) {
> + path_put(path);
> + goto out;
> + }
> +
> + ret = security_inode_notify(path->dentry->d_inode, mask);
> if (ret)
> path_put(path);
> +
> out:
> return ret;
> }
> @@ -1014,7 +1021,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
> goto fput_and_out;
> }
>
> - ret = fanotify_find_path(dfd, pathname, &path, flags);
> + ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
> if (ret)
> goto fput_and_out;
>
So the mark_type doesn't matter to SELinux?
You have no need for mount_noitify and sb_notify hooks?
A watch permission on the mount/sb root inode implies permission
(as CAP_SYS_ADMIN) to watch all events in mount/sb?
[...]
> +static int selinux_inode_notify(struct inode *inode, u64 mask)
> +{
> + u32 perm = FILE__WATCH; // basic permission, can a watch be set?
> +
> + struct common_audit_data ad;
> +
> + ad.type = LSM_AUDIT_DATA_INODE;
> + ad.u.inode = inode;
> +
> + // check if the mask is requesting ability to set a blocking watch
> + if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM))
Better ALL_FSNOTIFY_PERM_EVENTS
Thanks,
Amir.
^ permalink raw reply
* [RFC PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-07-10 13:34 UTC (permalink / raw)
To: paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel, Aaron Goidel
As of now, setting watches on filesystem objects has, at most, applied a
check for read access to the inode, and in the case of fanotify, requires
CAP_SYS_ADMIN. No specific security hook or permission check has been
provided to control the setting of watches. Using any of inotify, dnotify,
or fanotify, it is possible to observe, not only write-like operations, but
even read access to a file. Modeling the watch as being merely a read from
the file is insufficient. Furthermore, fanotify watches grant more power to
an application in the form of permission events. While notification events
are solely, unidirectional (i.e. they only pass information to the
receiving application), permission events are blocking. Permission events
make a request to the receiving application which will then reply with a
decision as to whether or not that action may be completed.
In order to solve these issues, a new LSM hook is implemented and has been
placed within the system calls for marking filesystem objects with inotify,
fanotify, and dnotify watches. These calls to the hook are placed at the
point at which the target inode has been resolved and are provided with
both the inode and the mask of requested notification events. The mask has
already been translated into common FS_* values shared by the entirety of
the fs notification infrastructure.
This only provides a hook at the point of setting a watch, and presumes
that permission to set a particular watch implies the ability to receive
all notification about that object which match the mask. This is all that
is required for SELinux. If other security modules require additional hooks
or infrastructure to control delivery of notification, these can be added
by them. It does not make sense for us to propose hooks for which we have
no implementation. The understanding that all notifications received by the
requesting application are all strictly of a type for which the application
has been granted permission shows that this implementation is sufficient in
its coverage.
Fanotify further has the issue that it returns a file descriptor with the
file mode specified during fanotify_init() to the watching process on
event. This is already covered by the LSM security_file_open hook if the
security module implements checking of the requested file mode there.
The selinux_inode_notify hook implementation works by adding three new
file permissions: watch, watch_reads, and watch_with_perm (descriptions
about which will follow). The hook then decides which subset of these
permissions must be held by the requesting application based on the
contents of the provided mask. The selinux_file_open hook already checks
the requested file mode and therefore ensures that a watching process
cannot escalate its access through fanotify.
The watch permission is the baseline permission for setting a watch on an
object and is a requirement for any watch to be set whatsoever. It should
be noted that having either of the other two permissions (watch_reads and
watch_with_perm) does not imply the watch permission, though this could be
changed if need be.
The watch_reads permission is required to receive notifications from
read-exclusive events on filesystem objects. These events include accessing
a file for the purpose of reading and closing a file which has been opened
read-only. This distinction has been drawn in order to provide a direct
indication in the policy for this otherwise not obvious capability. Read
access to a file should not necessarily imply the ability to observe read
events on a file.
Finally, watch_with_perm only applies to fanotify masks since it is the
only way to set a mask which allows for the blocking, permission event.
This permission is needed for any watch which is of this type. Though
fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
trust to root, which we do not do, and does not support least privilege.
Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
---
fs/notify/dnotify/dnotify.c | 14 +++++++++++---
fs/notify/fanotify/fanotify_user.c | 11 +++++++++--
fs/notify/inotify/inotify_user.c | 12 ++++++++++--
include/linux/lsm_hooks.h | 2 ++
include/linux/security.h | 7 +++++++
security/security.c | 5 +++++
security/selinux/hooks.c | 22 ++++++++++++++++++++++
security/selinux/include/classmap.h | 2 +-
8 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 250369d6901d..e91ce092efb1 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -22,6 +22,7 @@
#include <linux/sched/signal.h>
#include <linux/dnotify.h>
#include <linux/init.h>
+#include <linux/security.h>
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/fdtable.h>
@@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
+ /*
+ * convert the userspace DN_* "arg" to the internal FS_*
+ * defined in fsnotify
+ */
+ mask = convert_arg(arg);
+
+ error = security_inode_notify(inode, mask);
+ if (error)
+ goto out_err;
+
/* expect most fcntl to add new rather than augment old */
dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
if (!dn) {
@@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
- /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
- mask = convert_arg(arg);
-
/* set up the new_fsn_mark and new_dn_mark */
new_fsn_mark = &new_dn_mark->fsn_mark;
fsnotify_init_mark(new_fsn_mark, dnotify_group);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index a90bb19dcfa2..c0d9fa998377 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -528,7 +528,7 @@ static const struct file_operations fanotify_fops = {
};
static int fanotify_find_path(int dfd, const char __user *filename,
- struct path *path, unsigned int flags)
+ struct path *path, unsigned int flags, __u64 mask)
{
int ret;
@@ -567,8 +567,15 @@ static int fanotify_find_path(int dfd, const char __user *filename,
/* you can only watch an inode if you have read permissions on it */
ret = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (ret) {
+ path_put(path);
+ goto out;
+ }
+
+ ret = security_inode_notify(path->dentry->d_inode, mask);
if (ret)
path_put(path);
+
out:
return ret;
}
@@ -1014,7 +1021,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
goto fput_and_out;
}
- ret = fanotify_find_path(dfd, pathname, &path, flags);
+ ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
if (ret)
goto fput_and_out;
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 7b53598c8804..47b079f20aad 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -39,6 +39,7 @@
#include <linux/poll.h>
#include <linux/wait.h>
#include <linux/memcontrol.h>
+#include <linux/security.h>
#include "inotify.h"
#include "../fdinfo.h"
@@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
/*
* find_inode - resolve a user-given path to a specific inode
*/
-static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
+static int inotify_find_inode(const char __user *dirname, struct path *path,
+ unsigned int flags, __u64 mask)
{
int error;
@@ -351,8 +353,14 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
return error;
/* you can only watch an inode if you have read permissions on it */
error = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (error) {
+ path_put(path);
+ return error;
+ }
+ error = security_inode_notify(path->dentry->d_inode, mask);
if (error)
path_put(path);
+
return error;
}
@@ -744,7 +752,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
if (mask & IN_ONLYDIR)
flags |= LOOKUP_DIRECTORY;
- ret = inotify_find_inode(pathname, &path, flags);
+ ret = inotify_find_inode(pathname, &path, flags, mask);
if (ret)
goto fput_and_out;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..ef6b74938dd8 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1571,6 +1571,7 @@ union security_list_options {
int (*inode_getxattr)(struct dentry *dentry, const char *name);
int (*inode_listxattr)(struct dentry *dentry);
int (*inode_removexattr)(struct dentry *dentry, const char *name);
+ int (*inode_notify)(struct inode *inode, u64 mask);
int (*inode_need_killpriv)(struct dentry *dentry);
int (*inode_killpriv)(struct dentry *dentry);
int (*inode_getsecurity)(struct inode *inode, const char *name,
@@ -1881,6 +1882,7 @@ struct security_hook_heads {
struct hlist_head inode_getxattr;
struct hlist_head inode_listxattr;
struct hlist_head inode_removexattr;
+ struct hlist_head inode_notify;
struct hlist_head inode_need_killpriv;
struct hlist_head inode_killpriv;
struct hlist_head inode_getsecurity;
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..50106fb9eef9 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -301,6 +301,7 @@ int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer
void security_inode_getsecid(struct inode *inode, u32 *secid);
int security_inode_copy_up(struct dentry *src, struct cred **new);
int security_inode_copy_up_xattr(const char *name);
+int security_inode_notify(struct inode *inode, u64 mask);
int security_kernfs_init_security(struct kernfs_node *kn_dir,
struct kernfs_node *kn);
int security_file_permission(struct file *file, int mask);
@@ -392,6 +393,7 @@ void security_inode_invalidate_secctx(struct inode *inode);
int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
+
#else /* CONFIG_SECURITY */
static inline int call_lsm_notifier(enum lsm_event event, void *data)
@@ -776,6 +778,11 @@ static inline int security_inode_removexattr(struct dentry *dentry,
return cap_inode_removexattr(dentry, name);
}
+static inline int security_inode_notify(struct inode *inode, u64 mask)
+{
+ return 0;
+}
+
static inline int security_inode_need_killpriv(struct dentry *dentry)
{
return cap_inode_need_killpriv(dentry);
diff --git a/security/security.c b/security/security.c
index 613a5c00e602..57b2a96c1991 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1251,6 +1251,11 @@ int security_inode_removexattr(struct dentry *dentry, const char *name)
return evm_inode_removexattr(dentry, name);
}
+int security_inode_notify(struct inode *inode, u64 mask)
+{
+ return call_int_hook(inode_notify, 0, inode, mask);
+}
+
int security_inode_need_killpriv(struct dentry *dentry)
{
return call_int_hook(inode_need_killpriv, 0, dentry);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c61787b15f27..1a37966c2978 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -92,6 +92,7 @@
#include <linux/kernfs.h>
#include <linux/stringhash.h> /* for hashlen_string() */
#include <uapi/linux/mount.h>
+#include <linux/fsnotify.h>
#include "avc.h"
#include "objsec.h"
@@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
return -EACCES;
}
+static int selinux_inode_notify(struct inode *inode, u64 mask)
+{
+ u32 perm = FILE__WATCH; // basic permission, can a watch be set?
+
+ struct common_audit_data ad;
+
+ ad.type = LSM_AUDIT_DATA_INODE;
+ ad.u.inode = inode;
+
+ // check if the mask is requesting ability to set a blocking watch
+ if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM))
+ perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
+
+ // is the mask asking to watch file reads?
+ if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
+ perm |= FILE__WATCH_READS; // check that permission as well
+
+ return inode_has_perm(current_cred(), inode, perm, &ad);
+}
+
/*
* Copy the inode security context value to the user.
*
@@ -6797,6 +6818,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
+ LSM_HOOK_INIT(inode_notify, selinux_inode_notify),
LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 201f7e588a29..0654dd2fbebf 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -7,7 +7,7 @@
#define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
"rename", "execute", "quotaon", "mounton", "audit_access", \
- "open", "execmod"
+ "open", "execmod", "watch", "watch_with_perm", "watch_reads"
#define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
"listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
--
2.21.0
^ permalink raw reply related
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Jethro Beekman @ 2019-07-10 3:21 UTC (permalink / raw)
To: Sean Christopherson, Jarkko Sakkinen
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Bill Roberts, Casey Schaufler,
James Morris, Dave Hansen, Cedric Xing, Andy Lutomirski,
Dr . Greg Wettstein, Stephen Smalley
In-Reply-To: <20190708172930.GA20791@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]
On 2019-07-08 10:29, Sean Christopherson wrote:
> On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
>> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
>>
>> I still don't get why we need this whole mess and do not simply admit
>> that there are two distinct roles:
>>
>> 1. Creator
>> 2. User
>
> Because SELinux has existing concepts of EXECMEM and EXECMOD.
>
>> In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but
>> User does not. It just gets the fd from the Creator. I'm sure that all
>> the SGX2 related functionality can be solved somehow in this role
>> playing game.
>>
>> An example would be the usual case where enclave is actually a loader
>> that loads the actual piece of software that one wants to run. Things
>> simply need to be designed in a way the Creator runs the loader part.
>> These are non-trivial problems but oddball security model is not going
>> to make them disappear - on the contrary it will make designing user
>> space only more complicated.
>>
>> I think this is classical example of when something overly complicated
>> is invented in the kernel only to realize that it should be solved in
>> the user space.
>>
>> It would not be like the only use case where some kind of privileged
>> daemon is used for managing some a kernel provided resource.
>>
>> I think a really good conclusion from this discussion that has taken two
>> months is to realize that nothing needs to be done in this area (except
>> *maybe* noexec check).
>
> Hmm, IMO we need to support at least equivalents to EXECMEM and EXECMOD.
>
> That being said, we can do so without functional changes to the SGX uapi,
> e.g. add reserved fields so that the initial uapi can be extended *if* we
> decide to go with the "userspace provides maximal protections" path, and
> use the EPCM permissions as the maximal protections for the initial
> upstreaming.
Why do you need to add reserved fields now? Isn't this what
incorporating the struct size in the ioctl number is for?
--
Jethro Beekman | Fortanix
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3990 bytes --]
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Xing, Cedric @ 2019-07-10 2:04 UTC (permalink / raw)
To: Dr. Greg, Sean Christopherson
Cc: Jarkko Sakkinen, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Stephen Smalley
In-Reply-To: <20190710012811.GA18755@wind.enjellic.com>
On 7/9/2019 6:28 PM, Dr. Greg wrote:
> On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
>
> Good evening to everyone.
>
>> That being said, we can do so without functional changes to the SGX
>> uapi, e.g. add reserved fields so that the initial uapi can be
>> extended *if* we decide to go with the "userspace provides maximal
>> protections" path, and use the EPCM permissions as the maximal
>> protections for the initial upstreaming.
>>
>> That'd give us a minimal implemenation for initial upstreaming and
>> would eliminate Cedric's blocking complaint. The "whole mess" of
>> whitelisting, blacklisting and SGX2 support would be deferred until
>> post-upstreaming.
>
> Are we convinced the 'mess' will be any easier to clean up after the
> driver is upstreamed?
>
> The primary problem is that we haven't addressed the issue of what
> this technology is designed to do and its implications with respect to
> the kernel. As a result we are attempting to implement controls which
> we are comfortable with and understand rather then those that are
> relevant.
I don't think it's about easier or harder to clean up the mess, but a
divide-and-conquer strategy. After all, SGX and LSM are kind of
orthogonal as long as SGX doesn't compromise the protection provided by LSM.
Let's step back and look at what started this lengthy discussion. The
primary problem of v20 was that the SGX module allows executable enclave
pages to be created from non-executable regular pages, which could be
exploited by adversaries to grant executable permissions to pages that
would otherwise be denied without SGX. And that could be fixed simply by
capping EPCM permissions to whatever allowed on the source page, without
touching LSM. Of course the drawback is loss of functionality - i.e.
self-modifying enclaves cannot be loaded unless the calling process has
EXECMEM. But that should suffice, as most SGX1 applications don't
contain self-modifying code anyway.
Then we could switch our focus back to LSM and work out what's relevant,
especially for SGX2 and beyond.
> Have a good evening.
>
> Dr. Greg
>
> As always,
> Dr. Greg Wettstein, Ph.D, Worker
> IDfusion, LLC Implementing SGX secured and modeled
> 4206 N. 19th Ave. intelligent network endpoints.
> Fargo, ND 58102
> PH: 701-281-1686 EMAIL: greg@idfusion.net
> ------------------------------------------------------------------------------
> "Courage is not the absence of fear, but rather the judgement that
> something else is more important than fear."
> -- Ambrose Redmoon
>
^ 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