* Re: [PATCH v5 1/2] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-20 13:56 UTC (permalink / raw)
To: David Howells
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <3251035.1584692419@warthog.procyon.org.uk>
On 3/20/20 4:20 AM, David Howells wrote:
> Waiman Long <longman@redhat.com> wrote:
>
>> + if ((ret > 0) && (ret <= buflen)) {
> That's a bit excessive on the bracketage, btw, but don't worry about it unless
> you respin the patches.
Got it.
Thanks,
Longman
^ permalink raw reply
* Re: [PATCH v1] perf tool: make Perf tool aware of SELinux access control
From: Arnaldo Carvalho de Melo @ 2020-03-20 13:48 UTC (permalink / raw)
To: Alexey Budankov
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim,
Alexander Shishkin, Peter Zijlstra, Ingo Molnar, Andi Kleen,
linux-kernel, selinux@vger.kernel.org,
linux-security-module@vger.kernel.org
In-Reply-To: <d521a22d-9fa7-1bca-fa60-f23b55953c91@linux.intel.com>
Em Fri, Mar 20, 2020 at 03:24:47PM +0300, Alexey Budankov escreveu:
>
> On 19.03.2020 22:05, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Mar 19, 2020 at 04:01:26PM -0300, Arnaldo Carvalho de Melo escreveu:
> <SNIP>
> >
> > So I'll try the steps below with/without your patch, and then... what
> > are the steps that a tester needs to go thru to have that refpolicy in?
> > Install some new SELinux package or library, spelling out in detail the
> > steps one needs to go thru helps reviewing/testing,
>
> Yes, sure. Steps to extend FC31 Targeted policy for testing perf_events access control:
Thanks a lot! This is the level of detail I was talking about, good job!
- Arnaldo
> * download selinux-policy srpm [1]: selinux-policy-3.14.4-48.fc31.src.rpm on my FC31
>
> * install srpm - it creates rpmbuild dir:
> [root@host ~]# rpm -Uhv selinux-policy-3.14.4-48.fc31.src.rpm
>
> * get into rpmbuild/SPECS dir and unpack sources:
> [root@host ~]# rpmbuild -bp selinux-policy.spec
>
> * Place patch below at rpmbuild/BUILD/selinux-policy-b86eaaf4dbcf2d51dd4432df7185c0eaf3cbcc02
> dir and apply it:
> [root@host ~]# patch -p1 < selinux-policy-perf-events-perfmon.patch
> patching file policy/flask/access_vectors
> patching file policy/flask/security_classes
> [root@host ~]# cat selinux-policy-perf-events-perfmon.patch
> diff -Nura a/policy/flask/access_vectors b/policy/flask/access_vectors
> --- a/policy/flask/access_vectors 2020-02-04 18:19:53.000000000 +0300
> +++ b/policy/flask/access_vectors 2020-02-28 23:37:25.000000000 +0300
> @@ -174,6 +174,7 @@
> wake_alarm
> block_suspend
> audit_read
> + perfmon
> }
>
> #
> @@ -1099,3 +1100,15 @@
>
> class xdp_socket
> inherits socket
> +
> +class perf_event
> +{
> + open
> + cpu
> + kernel
> + tracepoint
> + read
> + write
> +}
> +
> +
> diff -Nura a/policy/flask/security_classes b/policy/flask/security_classes
> --- a/policy/flask/security_classes 2020-02-04 18:19:53.000000000 +0300
> +++ b/policy/flask/security_classes 2020-02-28 21:35:17.000000000 +0300
> @@ -200,4 +200,6 @@
>
> class xdp_socket
>
> +class perf_event
> +
> # FLASK
>
> [root@host ~]#
>
> * get into rpmbuild/SPECS dir and build policy packages from patched sources:
> [root@host ~]# rpmbuild --noclean --noprep -ba selinux-policy.spec
> so you have this:
> [root@host ~]# ls -alh rpmbuild/RPMS/noarch/
> total 33M
> drwxr-xr-x. 2 root root 4.0K Mar 20 12:16 .
> drwxr-xr-x. 3 root root 4.0K Mar 20 12:16 ..
> -rw-r--r--. 1 root root 112K Mar 20 12:16 selinux-policy-3.14.4-48.fc31.noarch.rpm
> -rw-r--r--. 1 root root 1.2M Mar 20 12:17 selinux-policy-devel-3.14.4-48.fc31.noarch.rpm
> -rw-r--r--. 1 root root 2.3M Mar 20 12:17 selinux-policy-doc-3.14.4-48.fc31.noarch.rpm
> -rw-r--r--. 1 root root 12M Mar 20 12:17 selinux-policy-minimum-3.14.4-48.fc31.noarch.rpm
> -rw-r--r--. 1 root root 4.5M Mar 20 12:16 selinux-policy-mls-3.14.4-48.fc31.noarch.rpm
> -rw-r--r--. 1 root root 111K Mar 20 12:16 selinux-policy-sandbox-3.14.4-48.fc31.noarch.rpm
> -rw-r--r--. 1 root root 14M Mar 20 12:17 selinux-policy-targeted-3.14.4-48.fc31.noarch.rpm
>
> * install SELinux packages from FC repo [2], if not already done so, and
> update with the patched rpms above:
> [root@host ~]# rpm -Uhv rpmbuild/RPMS/noarch/selinux-policy-*
>
> * there are also packages providing GUI interface and visualizing SELinux management
> [root@host ~]# dnf install policycoreutils-gui
>
> * enable SELinux Permissive mode for Targeted policy, if not already done so:
> [root@host ~]# cat /etc/selinux/config
> # This file controls the state of SELinux on the system.
> # SELINUX= can take one of these three values:
> # enforcing - SELinux security policy is enforced.
> # permissive - SELinux prints warnings instead of enforcing.
> # disabled - No SELinux policy is loaded.
> SELINUX=permissive
> # SELINUXTYPE= can take one of these three values:
> # targeted - Targeted processes are protected,
> # minimum - Modification of targeted policy. Only selected processes are protected.
> # mls - Multi Level Security protection.
> SELINUXTYPE=targeted
>
> * enable filesystem SELinux labeling at the next reboot
> [root@host ~]# touch /.autorelabel
>
> * reboot machine and it will label filesystems and load Targeted policy into the kernel
>
> * login and check that dmesg output doesn't mention that perf_event class is unknown to SELinux subsystem
>
> * check that SELinux is enabled and in Permissive mode
> [root@host ~]# getenforce
> Permissive
>
> * turn SELinux into Enforcing mode:
> [root@host ~]# setenforce 1
> [root@host ~]# getenforce
> Enforcing
>
> * Now the machine is enabled to test the patch
>
> --- If something went wrong ---
>
> * To turn SELinux into Permissive mode: setenforce 0
> * To fully disable SELinux during kernel boot [3] set kernel command line parameter: selinux=0
> * To remove SELinux labeling from local filesystems: find / -mount -print0 | xargs -0 setfattr -h -x security.selinux
> * To fully turn SELinux off a machine set SELINUX=disabled at /etc/selinux/config file and reboot
>
> ~Alexey
>
> [1] https://download-ib01.fedoraproject.org/pub/fedora/linux/updates/31/Everything/SRPMS/Packages/s/selinux-policy-3.14.4-49.fc31.src.rpm
> [2] https://docs.fedoraproject.org/en-US/Fedora/11/html/Security-Enhanced_Linux/sect-Security-Enhanced_Linux-Working_with_SELinux-Enabling_and_Disabling_SELinux.html
> [3] https://danwalsh.livejournal.com/10972.html
>
--
- Arnaldo
^ permalink raw reply
* Re: [PATCH v5 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-20 13:27 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200320020717.GC183331@linux.intel.com>
On 3/19/20 10:07 PM, Jarkko Sakkinen wrote:
> On Thu, Mar 19, 2020 at 08:07:55PM -0400, Waiman Long wrote:
>> On 3/19/20 3:46 PM, Jarkko Sakkinen wrote:
>>> On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote:
>>>> + * It is possible, though unlikely, that the key
>>>> + * changes in between the up_read->down_read period.
>>>> + * If the key becomes longer, we will have to
>>>> + * allocate a larger buffer and redo the key read
>>>> + * again.
>>>> + */
>>>> + if (!tmpbuf || unlikely(ret > tmpbuflen)) {
>>> Shouldn't you check that tmpbuflen stays below buflen (why else
>>> you had made copy of buflen otherwise)?
>> The check above this thunk:
>>
>> if ((ret > 0) && (ret <= buflen)) {
>>
>> will make sure that ret will not be larger than buflen. So tmpbuflen
>> will never be bigger than buflen.
> Ah right, of course, thanks.
>
> What would go wrong if the condition was instead
> ((ret > 0) && (ret <= tmpbuflen))?
That if statement is a check to see if the actual key length is longer
than the user-supplied buffer (buflen). If that is the case, it will
just return the expected length without storing anything into the user
buffer. For the case that buflen >= ret > tmpbuflen, the revised check
above will incorrectly skip the storing step causing the caller to
incorrectly think the key is there in the buffer.
Maybe I should clarify that a bit more in the comment.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v1] perf tool: make Perf tool aware of SELinux access control
From: Alexey Budankov @ 2020-03-20 12:24 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra,
Ingo Molnar, Andi Kleen, linux-kernel, selinux@vger.kernel.org,
linux-security-module@vger.kernel.org
In-Reply-To: <20200319190504.GG14841@kernel.org>
On 19.03.2020 22:05, Arnaldo Carvalho de Melo wrote:
> Em Thu, Mar 19, 2020 at 04:01:26PM -0300, Arnaldo Carvalho de Melo escreveu:
<SNIP>
>
> So I'll try the steps below with/without your patch, and then... what
> are the steps that a tester needs to go thru to have that refpolicy in?
> Install some new SELinux package or library, spelling out in detail the
> steps one needs to go thru helps reviewing/testing,
Yes, sure. Steps to extend FC31 Targeted policy for testing perf_events access control:
* download selinux-policy srpm [1]: selinux-policy-3.14.4-48.fc31.src.rpm on my FC31
* install srpm - it creates rpmbuild dir:
[root@host ~]# rpm -Uhv selinux-policy-3.14.4-48.fc31.src.rpm
* get into rpmbuild/SPECS dir and unpack sources:
[root@host ~]# rpmbuild -bp selinux-policy.spec
* Place patch below at rpmbuild/BUILD/selinux-policy-b86eaaf4dbcf2d51dd4432df7185c0eaf3cbcc02
dir and apply it:
[root@host ~]# patch -p1 < selinux-policy-perf-events-perfmon.patch
patching file policy/flask/access_vectors
patching file policy/flask/security_classes
[root@host ~]# cat selinux-policy-perf-events-perfmon.patch
diff -Nura a/policy/flask/access_vectors b/policy/flask/access_vectors
--- a/policy/flask/access_vectors 2020-02-04 18:19:53.000000000 +0300
+++ b/policy/flask/access_vectors 2020-02-28 23:37:25.000000000 +0300
@@ -174,6 +174,7 @@
wake_alarm
block_suspend
audit_read
+ perfmon
}
#
@@ -1099,3 +1100,15 @@
class xdp_socket
inherits socket
+
+class perf_event
+{
+ open
+ cpu
+ kernel
+ tracepoint
+ read
+ write
+}
+
+
diff -Nura a/policy/flask/security_classes b/policy/flask/security_classes
--- a/policy/flask/security_classes 2020-02-04 18:19:53.000000000 +0300
+++ b/policy/flask/security_classes 2020-02-28 21:35:17.000000000 +0300
@@ -200,4 +200,6 @@
class xdp_socket
+class perf_event
+
# FLASK
[root@host ~]#
* get into rpmbuild/SPECS dir and build policy packages from patched sources:
[root@host ~]# rpmbuild --noclean --noprep -ba selinux-policy.spec
so you have this:
[root@host ~]# ls -alh rpmbuild/RPMS/noarch/
total 33M
drwxr-xr-x. 2 root root 4.0K Mar 20 12:16 .
drwxr-xr-x. 3 root root 4.0K Mar 20 12:16 ..
-rw-r--r--. 1 root root 112K Mar 20 12:16 selinux-policy-3.14.4-48.fc31.noarch.rpm
-rw-r--r--. 1 root root 1.2M Mar 20 12:17 selinux-policy-devel-3.14.4-48.fc31.noarch.rpm
-rw-r--r--. 1 root root 2.3M Mar 20 12:17 selinux-policy-doc-3.14.4-48.fc31.noarch.rpm
-rw-r--r--. 1 root root 12M Mar 20 12:17 selinux-policy-minimum-3.14.4-48.fc31.noarch.rpm
-rw-r--r--. 1 root root 4.5M Mar 20 12:16 selinux-policy-mls-3.14.4-48.fc31.noarch.rpm
-rw-r--r--. 1 root root 111K Mar 20 12:16 selinux-policy-sandbox-3.14.4-48.fc31.noarch.rpm
-rw-r--r--. 1 root root 14M Mar 20 12:17 selinux-policy-targeted-3.14.4-48.fc31.noarch.rpm
* install SELinux packages from FC repo [2], if not already done so, and
update with the patched rpms above:
[root@host ~]# rpm -Uhv rpmbuild/RPMS/noarch/selinux-policy-*
* there are also packages providing GUI interface and visualizing SELinux management
[root@host ~]# dnf install policycoreutils-gui
* enable SELinux Permissive mode for Targeted policy, if not already done so:
[root@host ~]# cat /etc/selinux/config
# This file controls the state of SELinux on the system.
# SELINUX= can take one of these three values:
# enforcing - SELinux security policy is enforced.
# permissive - SELinux prints warnings instead of enforcing.
# disabled - No SELinux policy is loaded.
SELINUX=permissive
# SELINUXTYPE= can take one of these three values:
# targeted - Targeted processes are protected,
# minimum - Modification of targeted policy. Only selected processes are protected.
# mls - Multi Level Security protection.
SELINUXTYPE=targeted
* enable filesystem SELinux labeling at the next reboot
[root@host ~]# touch /.autorelabel
* reboot machine and it will label filesystems and load Targeted policy into the kernel
* login and check that dmesg output doesn't mention that perf_event class is unknown to SELinux subsystem
* check that SELinux is enabled and in Permissive mode
[root@host ~]# getenforce
Permissive
* turn SELinux into Enforcing mode:
[root@host ~]# setenforce 1
[root@host ~]# getenforce
Enforcing
* Now the machine is enabled to test the patch
--- If something went wrong ---
* To turn SELinux into Permissive mode: setenforce 0
* To fully disable SELinux during kernel boot [3] set kernel command line parameter: selinux=0
* To remove SELinux labeling from local filesystems: find / -mount -print0 | xargs -0 setfattr -h -x security.selinux
* To fully turn SELinux off a machine set SELINUX=disabled at /etc/selinux/config file and reboot
~Alexey
[1] https://download-ib01.fedoraproject.org/pub/fedora/linux/updates/31/Everything/SRPMS/Packages/s/selinux-policy-3.14.4-49.fc31.src.rpm
[2] https://docs.fedoraproject.org/en-US/Fedora/11/html/Security-Enhanced_Linux/sect-Security-Enhanced_Linux-Working_with_SELinux-Enabling_and_Disabling_SELinux.html
[3] https://danwalsh.livejournal.com/10972.html
^ permalink raw reply
* Re: [PATCH v5 1/2] KEYS: Don't write out to userspace while holding key semaphore
From: David Howells @ 2020-03-20 8:20 UTC (permalink / raw)
To: Waiman Long
Cc: dhowells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski, keyrings,
linux-kernel, linux-security-module, linux-integrity, netdev,
linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen
In-Reply-To: <20200318221457.1330-2-longman@redhat.com>
Waiman Long <longman@redhat.com> wrote:
> + if ((ret > 0) && (ret <= buflen)) {
That's a bit excessive on the bracketage, btw, but don't worry about it unless
you respin the patches.
David
^ permalink raw reply
* Re: [PATCH v5 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Jarkko Sakkinen @ 2020-03-20 2:07 UTC (permalink / raw)
To: Waiman Long
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <f22757ad-4d6f-ffd2-eed5-6b9bd1621b10@redhat.com>
On Thu, Mar 19, 2020 at 08:07:55PM -0400, Waiman Long wrote:
> On 3/19/20 3:46 PM, Jarkko Sakkinen wrote:
> > On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote:
> >> + * It is possible, though unlikely, that the key
> >> + * changes in between the up_read->down_read period.
> >> + * If the key becomes longer, we will have to
> >> + * allocate a larger buffer and redo the key read
> >> + * again.
> >> + */
> >> + if (!tmpbuf || unlikely(ret > tmpbuflen)) {
> > Shouldn't you check that tmpbuflen stays below buflen (why else
> > you had made copy of buflen otherwise)?
>
> The check above this thunk:
>
> if ((ret > 0) && (ret <= buflen)) {
>
> will make sure that ret will not be larger than buflen. So tmpbuflen
> will never be bigger than buflen.
Ah right, of course, thanks.
What would go wrong if the condition was instead
((ret > 0) && (ret <= tmpbuflen))?
/Jarkko
^ permalink raw reply
* Re: [PATCH v5 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-20 0:07 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200319194650.GA24804@linux.intel.com>
On 3/19/20 3:46 PM, Jarkko Sakkinen wrote:
> On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote:
>> + * It is possible, though unlikely, that the key
>> + * changes in between the up_read->down_read period.
>> + * If the key becomes longer, we will have to
>> + * allocate a larger buffer and redo the key read
>> + * again.
>> + */
>> + if (!tmpbuf || unlikely(ret > tmpbuflen)) {
> Shouldn't you check that tmpbuflen stays below buflen (why else
> you had made copy of buflen otherwise)?
The check above this thunk:
if ((ret > 0) && (ret <= buflen)) {
will make sure that ret will not be larger than buflen. So tmpbuflen
will never be bigger than buflen.
Cheers,
Longman
^ permalink raw reply
* Re: [RFC PATCH v14 00/10] Landlock LSM
From: Jann Horn @ 2020-03-19 21:17 UTC (permalink / raw)
To: Mickaël Salaün
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <2d48e3e3-e7b2-ec33-91c5-be6a308a12d4@digikod.net>
On Thu, Mar 19, 2020 at 5:58 PM Mickaël Salaün <mic@digikod.net> wrote:
> On 19/03/2020 00:33, Jann Horn wrote:
> > On Wed, Mar 18, 2020 at 1:06 PM Mickaël Salaün <mic@digikod.net> wrote:
[...]
> >> As I understand your proposition, we need to build the required_bits
> >> when adding a rule or enforcing/merging a ruleset with a domain. The
> >> issue is that a rule only refers to a struct inode, not a struct path.
> >> For your proposition to work, we would need to walk through the file
> >> path when adding a rule to a ruleset, which means that we need to depend
> >> of the current view of the process (i.e. its mount namespace), and its
> >> Landlock domain.
> >
> > I don't see why that is necessary. Why would we have to walk the file
> > path when adding a rule?
> >
> >> If the required_bits field is set when the ruleset is
> >> merged with the domain, it is not possible anymore to walk through the
> >> corresponding initial file path, which makes the enforcement step too
> >> late to check for such consistency. The important point is that a
> >> ruleset/domain doesn't have a notion of file hierarchy, a ruleset is
> >> only a set of tagged inodes.
> >>
> >> I'm not sure I got your proposition right, though. When and how would
> >> you generate the required_bits?
> >
> > Using your terminology:
> > A domain is a collection of N layers, which are assigned indices 0..N-1.
> > For each possible access type, a domain has a bitmask containing N
> > bits that stores which layers control that access type. (Basically a
> > per-layer version of fs_access_mask.)
>
> OK, so there is a bit for each domain, which means that you get a limit
> of, let's say 64 layers? Knowing that each layer can be created by a
> standalone application, potentially nested in a bunch of layers, this
> seems artificially limiting.
Yes, that is a downside of my approach.
> > To validate an access, you start by ORing together the bitmasks for
> > the requested access types; that gives you the required_bits mask,
> > which lists all layers that want to control the access.
> > Then you set seen_policy_bits=0, then do the
> > check_access_path_continue() loop while keeping track of which layers
> > you've seen with "seen_policy_bits |= access->contributing_policies",
> > or something like that.
> > And in the end, you check that seen_policy_bits is a superset of
> > required_bits - something like `(~seen_policy_bits) & required_bits ==
> > 0`.
> >
> > AFAICS to create a new domain from a bunch of layers, you wouldn't
> > have to do any path walking.
>
> Right, I misunderstood your previous email.
>
> >
> >> Here is my updated proposition: add a layer level and a depth to each
> >> rule (once enforced/merged with a domain), and a top layer level for a
> >> domain. When enforcing a ruleset (i.e. merging a ruleset into the
> >> current domain), the layer level of a new rule would be the incremented
> >> top layer level.
> >> If there is no rule (from this domain) tied to the same
> >> inode, then the depth of the new rule is 1. However, if there is already
> >> a rule tied to the same inode and if this rule's layer level is the
> >> previous top layer level, then the depth and the layer level are both
> >> incremented and the rule is updated with the new access rights (boolean
> >> AND).
> >>
> >> The policy looks like this:
> >> domain top_layer=2
> >> /a RW policy_bitmask=0x00000003 layer=1 depth=1
> >> /a/b R policy_bitmask=0x00000002 layer=2 depth=1
> >>
> >> The path walk access check walks through all inodes and start with a
> >> layer counter equal to the top layer of the current domain. For each
> >> encountered inode tied to a rule, the access rights are checked and a
> >> new check ensures that the layer of the matching rule is the same as the
> >> counter (this may be a merged ruleset containing rules pertaining to the
> >> same hierarchy, which is fine) or equal to the decremented counter (i.e.
> >> the path walk just reached the underlying layer). If the path walk
> >> encounter a rule with a layer strictly less than the counter minus one,
> >> there is a whole in the layers which means that the ruleset
> >> hierarchy/subset does not match, and the access must be denied.
> >>
> >> When accessing a file at /private/b/foo for a read access:
> >> /private/b/foo <no rules>
> >> allowed_access=unknown layer_counter=2
> >> /private/b <access: R, policy_bitmask=0x00000002, layer=2, depth=1>
> >> allowed_access=allowed layer_counter=2
> >> /private <no rules>
> >> allowed_access=allowed layer_counter=2
> >> / <no rules>
> >> allowed_access=allowed layer_counter=2
> >>
> >> Because the layer_counter didn't reach 1, the access request is then denied.
> >>
> >> This proposition enables not to rely on a parent ruleset at first, only
> >> when enforcing/merging a ruleset with a domain. This also solves the
> >> issue with multiple inherited/nested rules on the same inode (in which
> >> case the depth just grows). Moreover, this enables to safely stop the
> >> path walk as soon as we reach the layer 1.
> >
> > (FWIW, you could do the same optimization with the seen_policy_bits approach.)
> >
> > I guess the difference between your proposal and mine is that in my
> > proposal, the following would work, in effect permitting W access to
> > /foo/bar/baz (and nothing else)?
> >
> > first ruleset:
> > /foo W
> > second ruleset:
> > /foo/bar/baz W
> > third ruleset:
> > /foo/bar W
> >
> > whereas in your proposal, IIUC it wouldn't be valid for a new ruleset
> > to whitelist a superset of what was whitelisted in a previous ruleset?
> >
>
> This behavior seems dangerous because a process which sandbox itself to
> only access /foo/bar W can bypass the restrictions from one of its
> parent domains (i.e. only access /foo/bar/baz W). Indeed, each layer is
> (most of the time) a different and standalone security policy.
It isn't actually bypassing the restriction: You still can't actually
access files like /foo/bar/blah, because a path walk from there
doesn't encounter any rules from the second ruleset.
> To sum up, the bitmask approach doesn't have the notion of layers
> ordering. It is then not possible to check that a rule comes from a
> domain which is the direct ancestor of a child's domain. I want each
> policy/layer to be really nested in the sense that a process sandboxing
> itself can only add more restriction to itself with regard to its parent
> domain (and the whole hierarchy). This is a similar approach to
> seccomp-bpf (with chained filters), except there is almost no overhead
> to nest several policies/layers together because they are flattened.
> Using the layer level and depth approach enables to implement this.
^ permalink raw reply
* Re: [PATCH v5 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Jarkko Sakkinen @ 2020-03-19 19:46 UTC (permalink / raw)
To: Waiman Long
Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
linux-security-module, linux-integrity, netdev, linux-afs,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <20200318221457.1330-3-longman@redhat.com>
On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote:
> + * It is possible, though unlikely, that the key
> + * changes in between the up_read->down_read period.
> + * If the key becomes longer, we will have to
> + * allocate a larger buffer and redo the key read
> + * again.
> + */
> + if (!tmpbuf || unlikely(ret > tmpbuflen)) {
Shouldn't you check that tmpbuflen stays below buflen (why else
you had made copy of buflen otherwise)?
/Jarkko
^ permalink raw reply
* Re: [PATCH v1] perf tool: make Perf tool aware of SELinux access control
From: Arnaldo Carvalho de Melo @ 2020-03-19 19:05 UTC (permalink / raw)
To: Alexey Budankov
Cc: Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra,
Ingo Molnar, Andi Kleen, linux-kernel, selinux@vger.kernel.org,
linux-security-module@vger.kernel.org
In-Reply-To: <20200319190126.GF14841@kernel.org>
Em Thu, Mar 19, 2020 at 04:01:26PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Mar 19, 2020 at 09:23:30AM +0300, Alexey Budankov escreveu:
> > Is there any thougts, comments or questions so far?
> > Please share you mind.
> From a quick look, seems ok, I'll do some testing now,
> > On 13.03.2020 20:27, Alexey Budankov wrote:
> > > Extend Perf tool with the check of /sys/fs/selinux/enforce value and notify
> > > in case access to perf_event_open() syscall is restricted by the enforced
> > > SELinux policy settings.
> > >
> > > Testing and evaluation (Fedora 31 x86_64 with enforced Targeted policy extended
> > > by perf_event class (see refpolicy [1] master branch)):
So I'll try the steps below with/without your patch, and then... what
are the steps that a tester needs to go thru to have that refpolicy in?
Install some new SELinux package or library, spelling out in detail the
steps one needs to go thru helps reviewing/testing,
- Arnaldo
> > > [root@host ~]# ps -Z
> > > LABEL PID TTY TIME CMD
> > > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 3960 pts/1 00:00:00 bash
> > > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4167 pts/1 00:00:00 ps
> > >
> > > [root@host ~]# ls -alhZ /usr/local/bin/
> > > total 56M
> > > drwxr-xr-x. 2 root root system_u:object_r:bin_t:s0 4.0K Mar 4 12:27 .
> > > drwxr-xr-x. 12 root root system_u:object_r:usr_t:s0 4.0K Jul 25 2019 ..
> > > -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 4.1M Jan 23 2017 bash
> > > -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 4.1M Jan 23 2017 bash.before_shellshock_patch
> > > ...
> > > -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 372 May 14 2019 flask
> > > -rwxr-xr-x. 1 root root unconfined_u:object_r:bin_t:s0 24M Mar 4 12:15 perf <== unprivileged users (perf_event_paranoid)
> > > -rwxr-x---. 1 root perf_users unconfined_u:object_r:bin_t:s0 24M Mar 4 12:19 perf.cap <== perf_users (CAP_SYS_ADMIN)
> > > -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 44K Dec 8 2016 spiff
> > > ...
> > > lrwxrwxrwx. 1 root root system_u:object_r:bin_t:s0 4 Aug 21 2018 zstdmt -> zstd
> > >
> > > [root@host ~]# getenforce
> > > Enforcing
> > >
> > > === Access by unprivileged user ===
> > >
> > > [user@host ~]$ ps -Z
> > > LABEL PID TTY TIME CMD
> > > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4043 pts/2 00:00:00 bash
> > > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4168 pts/2 00:00:00 ps
> > >
> > > [user@host ~]$ /usr/local/bin/perf stat -- ls
> > > Error:
> > > Access to performance monitoring and observability operations is limited.
> > > SELinux Enforcing mode is enabled and can limit access to performance
> > > monitoring and observability operations. Inspect system audit records
> > > for more perf_event access control information and adjusting the policy.
> > > Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open
> > > access to performance monitoring and observability operations for users
> > > without CAP_SYS_ADMIN capability. perf_event_paranoid setting is -1:
> > > -1: Allow use of (almost) all events by all users
> > > Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
> > >> = 0: Disallow raw and ftrace function tracepoint access
> > >> = 1: Disallow CPU event access
> > >> = 2: Disallow kernel profiling
> > > To make the adjusted perf_event_paranoid setting permanent preserve it
> > > in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)
> > >
> > > [root@host ~]# journalctl --follow
> > > ... audit[4186]: AVC avc: denied { open } for pid=4186 comm="perf" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
> > > ... audit[4186]: AVC avc: denied { open } for pid=4186 comm="perf" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
> > > ... setroubleshoot[4194]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t. For complete SELinux messages run: sealert -l 9a6f3db2-3d8f-461e-afad-0b5c3a9c3b9d
> > > ... python3[4194]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t.
> > >
> > > ***** Plugin catchall (100. confidence) suggests **************************
> > >
> > > If you believe that perf should be allowed open access on perf_event labeled unconfined_t by default.
> > > Then you should report this as a bug.
> > > You can generate a local policy module to allow this access.
> > > Do
> > > allow this access for now by executing:
> > > # ausearch -c 'perf' --raw | audit2allow -M my-perf
> > > # semodule -X 300 -i my-perf.pp
> > >
> > > === Access by perf privileged user ===
> > >
> > > [user@host ~]$ ps -Z
> > > LABEL PID TTY TIME CMD
> > > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4043 pts/2 00:00:00 bash
> > > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4168 pts/2 00:00:00 ps
> > >
> > > [user@host ~]$ libcap/progs/getcap /usr/local/bin/perf.cap
> > > /usr/local/bin/perf.cap = cap_sys_ptrace,cap_syslog,cap_sys_admin+ep
> > >
> > > [user@host ~]$ /usr/local/bin/perf.cap stat -- ls
> > > Error:
> > > Access to performance monitoring and observability operations is limited.
> > > SELinux Enforcing mode is enabled and can limit access to performance
> > > monitoring and observability operations. Inspect system audit records
> > > for more perf_event access control information and adjusting the policy.
> > > Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open
> > > access to performance monitoring and observability operations for users
> > > without CAP_SYS_ADMIN capability. perf_event_paranoid setting is -1:
> > > -1: Allow use of (almost) all events by all users
> > > Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
> > >> = 0: Disallow raw and ftrace function tracepoint access
> > >> = 1: Disallow CPU event access
> > >> = 2: Disallow kernel profiling
> > > To make the adjusted perf_event_paranoid setting permanent preserve it
> > > in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)
> > >
> > > [root@host ~]# journalctl --follow
> > >
> > > ... audit[3926]: AVC avc: denied { open } for pid=3926 comm="perf.cap" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
> > > ... audit[3926]: AVC avc: denied { open } for pid=3926 comm="perf.cap" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
> > >
> > > ... setroubleshoot[3934]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t. For complete SELinux messages run: sealert -l 9a6f3db2-3d8f-461e-afad-0b5c3a9c3b9d
> > > ... python3[3934]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t.
> > >
> > > ***** Plugin catchall (100. confidence) suggests **************************
> > >
> > > If you believe that perf should be allowed open access on perf_event labeled unconfined_t by default.
> > > Then you should report this as a bug.
> > > You can generate a local policy module to allow this access.
> > > Do
> > > allow this access for now by executing:
> > > # ausearch -c 'perf' --raw | audit2allow -M my-perf
> > > # semodule -X 300 -i my-perf.pp
> > >
> > > === Open access to performance monitoring and observability operations in unconfined_t domain ===
> > >
> > > [root@host ~]# ausearch -c 'perf' --raw | audit2allow -M my-perf && cat my-perf.te
> > >
> > > module my-perf 1.0;
> > >
> > > require {
> > > type unconfined_t;
> > > class perf_event { cpu kernel open read tracepoint write };
> > > }
> > >
> > > #============= unconfined_t ==============
> > > allow unconfined_t self:perf_event { cpu kernel open read tracepoint write };
> > >
> > > [root@host ~]# semodule -X 300 -i my-perf.pp
> > >
> > > [user@host ~]$ ps -Z
> > > LABEL PID TTY TIME CMD
> > > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4043 pts/2 00:00:00 bash
> > > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4168 pts/2 00:00:00 ps
> > >
> > > [user@host ~]$ /usr/local/bin/perf stat -- ls
> > > Desktop Documents Downloads intel Music perf.data perf.data.old Pictures Public Templates Videos
> > >
> > > Performance counter stats for 'ls':
> > >
> > > 0.72 msec task-clock:u # 0.655 CPUs utilized
> > > 0 context-switches:u # 0.000 K/sec
> > > 0 cpu-migrations:u # 0.000 K/sec
> > > 98 page-faults:u # 0.137 M/sec
> > > 908,356 cycles:u # 1.266 GHz
> > > 729,984 instructions:u # 0.80 insn per cycle
> > > 142,774 branches:u # 198.968 M/sec
> > > 8,238 branch-misses:u # 5.77% of all branches
> > >
> > > 0.001095239 seconds time elapsed
> > >
> > > 0.001147000 seconds user
> > > 0.000000000 seconds sys
> > >
> > > [user@host ~]$ /usr/local/bin/perf stat -a
> > > Error:
> > > Access to performance monitoring and observability operations is limited.
> > > SELinux Enforcing mode is enabled and can limit access to performance
> > > monitoring and observability operations. Inspect system audit records
> > > for more perf_event access control information and adjusting the policy.
> > > Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open
> > > access to performance monitoring and observability operations for users
> > > without CAP_SYS_ADMIN capability. perf_event_paranoid setting is -1:
> > > -1: Allow use of (almost) all events by all users
> > > Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
> > >> = 0: Disallow raw and ftrace function tracepoint access
> > >> = 1: Disallow CPU event access
> > >> = 2: Disallow kernel profiling
> > > To make the adjusted perf_event_paranoid setting permanent preserve it
> > > in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)
> > >
> > > [user@host ~]$ /usr/local/bin/perf.cap stat -a
> > > ^C
> > > Performance counter stats for 'system wide':
> > >
> > > 13,427.05 msec cpu-clock # 7.997 CPUs utilized
> > > 783 context-switches # 0.058 K/sec
> > > 29 cpu-migrations # 0.002 K/sec
> > > 6 page-faults # 0.000 K/sec
> > > 161,084,874 cycles # 0.012 GHz
> > > 146,823,131 instructions # 0.91 insn per cycle
> > > 12,164,802 branches # 0.906 M/sec
> > > 380,350 branch-misses # 3.13% of all branches
> > >
> > > 1.678938906 seconds time elapsed
> > >
> > > [1] https://github.com/SELinuxProject/refpolicy
> > >
> > > Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> > > ---
> > > tools/perf/util/cloexec.c | 4 ++--
> > > tools/perf/util/evsel.c | 40 +++++++++++++++++++++++----------------
> > > 2 files changed, 26 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/tools/perf/util/cloexec.c b/tools/perf/util/cloexec.c
> > > index a12872f2856a..9c8ec816261b 100644
> > > --- a/tools/perf/util/cloexec.c
> > > +++ b/tools/perf/util/cloexec.c
> > > @@ -65,7 +65,7 @@ static int perf_flag_probe(void)
> > > return 1;
> > > }
> > >
> > > - WARN_ONCE(err != EINVAL && err != EBUSY,
> > > + WARN_ONCE(err != EINVAL && err != EBUSY && err != EACCES,
> > > "perf_event_open(..., PERF_FLAG_FD_CLOEXEC) failed with unexpected error %d (%s)\n",
> > > err, str_error_r(err, sbuf, sizeof(sbuf)));
> > >
> > > @@ -83,7 +83,7 @@ static int perf_flag_probe(void)
> > > if (fd >= 0)
> > > close(fd);
> > >
> > > - if (WARN_ONCE(fd < 0 && err != EBUSY,
> > > + if (WARN_ONCE(fd < 0 && err != EBUSY && err != EACCES,
> > > "perf_event_open(..., 0) failed unexpectedly with error %d (%s)\n",
> > > err, str_error_r(err, sbuf, sizeof(sbuf))))
> > > return -1;
> > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > index 816d930d774e..f03ce1d362d3 100644
> > > --- a/tools/perf/util/evsel.c
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -2493,32 +2493,40 @@ int perf_evsel__open_strerror(struct evsel *evsel, struct target *target,
> > > int err, char *msg, size_t size)
> > > {
> > > char sbuf[STRERR_BUFSIZE];
> > > - int printed = 0;
> > > + int printed = 0, enforced = 0;
> > >
> > > switch (err) {
> > > case EPERM:
> > > case EACCES:
> > > + printed += scnprintf(msg + printed, size - printed,
> > > + "Access to performance monitoring and observability operations is limited.\n");
> > > +
> > > + if (!sysfs__read_int("fs/selinux/enforce", &enforced)) {
> > > + if (enforced) {
> > > + printed += scnprintf(msg + printed, size - printed,
> > > + "SELinux Enforcing mode is enabled and can limit access to performance\n"
> > > + "monitoring and observability operations. Inspect system audit records\n"
> > > + "for more perf_event access control information and adjusting the policy.\n");
> > > + }
> > > + }
> > > +
> > > if (err == EPERM)
> > > - printed = scnprintf(msg, size,
> > > - "No permission to enable %s event.\n\n",
> > > + printed += scnprintf(msg, size,
> > > + "No permission to enable %s event.\n",
> > > perf_evsel__name(evsel));
> > >
> > > return scnprintf(msg + printed, size - printed,
> > > - "You may not have permission to collect %sstats.\n\n"
> > > - "Consider tweaking /proc/sys/kernel/perf_event_paranoid,\n"
> > > - "which controls use of the performance events system by\n"
> > > - "unprivileged users (without CAP_SYS_ADMIN).\n\n"
> > > - "The current value is %d:\n\n"
> > > + "Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open\n"
> > > + "access to performance monitoring and observability operations for users\n"
> > > + "without CAP_SYS_ADMIN capability. perf_event_paranoid setting is %d:\n"
> > > " -1: Allow use of (almost) all events by all users\n"
> > > " Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK\n"
> > > - ">= 0: Disallow ftrace function tracepoint by users without CAP_SYS_ADMIN\n"
> > > - " Disallow raw tracepoint access by users without CAP_SYS_ADMIN\n"
> > > - ">= 1: Disallow CPU event access by users without CAP_SYS_ADMIN\n"
> > > - ">= 2: Disallow kernel profiling by users without CAP_SYS_ADMIN\n\n"
> > > - "To make this setting permanent, edit /etc/sysctl.conf too, e.g.:\n\n"
> > > - " kernel.perf_event_paranoid = -1\n" ,
> > > - target->system_wide ? "system-wide " : "",
> > > - perf_event_paranoid());
> > > + ">= 0: Disallow raw and ftrace function tracepoint access\n"
> > > + ">= 1: Disallow CPU event access\n"
> > > + ">= 2: Disallow kernel profiling\n"
> > > + "To make the adjusted perf_event_paranoid setting permanent preserve it\n"
> > > + "in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)",
> > > + perf_event_paranoid());
> > > case ENOENT:
> > > return scnprintf(msg, size, "The %s event is not supported.",
> > > perf_evsel__name(evsel));
> > >
>
> --
>
> - Arnaldo
--
- Arnaldo
^ permalink raw reply
* Re: [PATCH v1] perf tool: make Perf tool aware of SELinux access control
From: Arnaldo Carvalho de Melo @ 2020-03-19 19:01 UTC (permalink / raw)
To: Alexey Budankov
Cc: Jiri Olsa, Namhyung Kim, Alexander Shishkin, Peter Zijlstra,
Ingo Molnar, Andi Kleen, linux-kernel, selinux@vger.kernel.org,
linux-security-module@vger.kernel.org
In-Reply-To: <f5ed60b2-4a61-dc72-bfd5-6d0af74bc152@linux.intel.com>
Em Thu, Mar 19, 2020 at 09:23:30AM +0300, Alexey Budankov escreveu:
> Hi,
>
> Is there any thougts, comments or questions so far?
> Please share you mind.
From a quick look, seems ok, I'll do some testing now,
- Arnaldo
> Thanks,
> Alexey
>
> On 13.03.2020 20:27, Alexey Budankov wrote:
> >
> > Extend Perf tool with the check of /sys/fs/selinux/enforce value and notify
> > in case access to perf_event_open() syscall is restricted by the enforced
> > SELinux policy settings.
> >
> > Testing and evaluation (Fedora 31 x86_64 with enforced Targeted policy extended
> > by perf_event class (see refpolicy [1] master branch)):
> >
> > [root@host ~]# ps -Z
> > LABEL PID TTY TIME CMD
> > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 3960 pts/1 00:00:00 bash
> > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4167 pts/1 00:00:00 ps
> >
> > [root@host ~]# ls -alhZ /usr/local/bin/
> > total 56M
> > drwxr-xr-x. 2 root root system_u:object_r:bin_t:s0 4.0K Mar 4 12:27 .
> > drwxr-xr-x. 12 root root system_u:object_r:usr_t:s0 4.0K Jul 25 2019 ..
> > -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 4.1M Jan 23 2017 bash
> > -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 4.1M Jan 23 2017 bash.before_shellshock_patch
> > ...
> > -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 372 May 14 2019 flask
> > -rwxr-xr-x. 1 root root unconfined_u:object_r:bin_t:s0 24M Mar 4 12:15 perf <== unprivileged users (perf_event_paranoid)
> > -rwxr-x---. 1 root perf_users unconfined_u:object_r:bin_t:s0 24M Mar 4 12:19 perf.cap <== perf_users (CAP_SYS_ADMIN)
> > -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 44K Dec 8 2016 spiff
> > ...
> > lrwxrwxrwx. 1 root root system_u:object_r:bin_t:s0 4 Aug 21 2018 zstdmt -> zstd
> >
> > [root@host ~]# getenforce
> > Enforcing
> >
> > === Access by unprivileged user ===
> >
> > [user@host ~]$ ps -Z
> > LABEL PID TTY TIME CMD
> > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4043 pts/2 00:00:00 bash
> > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4168 pts/2 00:00:00 ps
> >
> > [user@host ~]$ /usr/local/bin/perf stat -- ls
> > Error:
> > Access to performance monitoring and observability operations is limited.
> > SELinux Enforcing mode is enabled and can limit access to performance
> > monitoring and observability operations. Inspect system audit records
> > for more perf_event access control information and adjusting the policy.
> > Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open
> > access to performance monitoring and observability operations for users
> > without CAP_SYS_ADMIN capability. perf_event_paranoid setting is -1:
> > -1: Allow use of (almost) all events by all users
> > Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
> >> = 0: Disallow raw and ftrace function tracepoint access
> >> = 1: Disallow CPU event access
> >> = 2: Disallow kernel profiling
> > To make the adjusted perf_event_paranoid setting permanent preserve it
> > in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)
> >
> > [root@host ~]# journalctl --follow
> > ... audit[4186]: AVC avc: denied { open } for pid=4186 comm="perf" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
> > ... audit[4186]: AVC avc: denied { open } for pid=4186 comm="perf" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
> > ... setroubleshoot[4194]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t. For complete SELinux messages run: sealert -l 9a6f3db2-3d8f-461e-afad-0b5c3a9c3b9d
> > ... python3[4194]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t.
> >
> > ***** Plugin catchall (100. confidence) suggests **************************
> >
> > If you believe that perf should be allowed open access on perf_event labeled unconfined_t by default.
> > Then you should report this as a bug.
> > You can generate a local policy module to allow this access.
> > Do
> > allow this access for now by executing:
> > # ausearch -c 'perf' --raw | audit2allow -M my-perf
> > # semodule -X 300 -i my-perf.pp
> >
> > === Access by perf privileged user ===
> >
> > [user@host ~]$ ps -Z
> > LABEL PID TTY TIME CMD
> > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4043 pts/2 00:00:00 bash
> > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4168 pts/2 00:00:00 ps
> >
> > [user@host ~]$ libcap/progs/getcap /usr/local/bin/perf.cap
> > /usr/local/bin/perf.cap = cap_sys_ptrace,cap_syslog,cap_sys_admin+ep
> >
> > [user@host ~]$ /usr/local/bin/perf.cap stat -- ls
> > Error:
> > Access to performance monitoring and observability operations is limited.
> > SELinux Enforcing mode is enabled and can limit access to performance
> > monitoring and observability operations. Inspect system audit records
> > for more perf_event access control information and adjusting the policy.
> > Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open
> > access to performance monitoring and observability operations for users
> > without CAP_SYS_ADMIN capability. perf_event_paranoid setting is -1:
> > -1: Allow use of (almost) all events by all users
> > Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
> >> = 0: Disallow raw and ftrace function tracepoint access
> >> = 1: Disallow CPU event access
> >> = 2: Disallow kernel profiling
> > To make the adjusted perf_event_paranoid setting permanent preserve it
> > in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)
> >
> > [root@host ~]# journalctl --follow
> >
> > ... audit[3926]: AVC avc: denied { open } for pid=3926 comm="perf.cap" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
> > ... audit[3926]: AVC avc: denied { open } for pid=3926 comm="perf.cap" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
> >
> > ... setroubleshoot[3934]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t. For complete SELinux messages run: sealert -l 9a6f3db2-3d8f-461e-afad-0b5c3a9c3b9d
> > ... python3[3934]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t.
> >
> > ***** Plugin catchall (100. confidence) suggests **************************
> >
> > If you believe that perf should be allowed open access on perf_event labeled unconfined_t by default.
> > Then you should report this as a bug.
> > You can generate a local policy module to allow this access.
> > Do
> > allow this access for now by executing:
> > # ausearch -c 'perf' --raw | audit2allow -M my-perf
> > # semodule -X 300 -i my-perf.pp
> >
> > === Open access to performance monitoring and observability operations in unconfined_t domain ===
> >
> > [root@host ~]# ausearch -c 'perf' --raw | audit2allow -M my-perf && cat my-perf.te
> >
> > module my-perf 1.0;
> >
> > require {
> > type unconfined_t;
> > class perf_event { cpu kernel open read tracepoint write };
> > }
> >
> > #============= unconfined_t ==============
> > allow unconfined_t self:perf_event { cpu kernel open read tracepoint write };
> >
> > [root@host ~]# semodule -X 300 -i my-perf.pp
> >
> > [user@host ~]$ ps -Z
> > LABEL PID TTY TIME CMD
> > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4043 pts/2 00:00:00 bash
> > unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4168 pts/2 00:00:00 ps
> >
> > [user@host ~]$ /usr/local/bin/perf stat -- ls
> > Desktop Documents Downloads intel Music perf.data perf.data.old Pictures Public Templates Videos
> >
> > Performance counter stats for 'ls':
> >
> > 0.72 msec task-clock:u # 0.655 CPUs utilized
> > 0 context-switches:u # 0.000 K/sec
> > 0 cpu-migrations:u # 0.000 K/sec
> > 98 page-faults:u # 0.137 M/sec
> > 908,356 cycles:u # 1.266 GHz
> > 729,984 instructions:u # 0.80 insn per cycle
> > 142,774 branches:u # 198.968 M/sec
> > 8,238 branch-misses:u # 5.77% of all branches
> >
> > 0.001095239 seconds time elapsed
> >
> > 0.001147000 seconds user
> > 0.000000000 seconds sys
> >
> > [user@host ~]$ /usr/local/bin/perf stat -a
> > Error:
> > Access to performance monitoring and observability operations is limited.
> > SELinux Enforcing mode is enabled and can limit access to performance
> > monitoring and observability operations. Inspect system audit records
> > for more perf_event access control information and adjusting the policy.
> > Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open
> > access to performance monitoring and observability operations for users
> > without CAP_SYS_ADMIN capability. perf_event_paranoid setting is -1:
> > -1: Allow use of (almost) all events by all users
> > Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
> >> = 0: Disallow raw and ftrace function tracepoint access
> >> = 1: Disallow CPU event access
> >> = 2: Disallow kernel profiling
> > To make the adjusted perf_event_paranoid setting permanent preserve it
> > in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)
> >
> > [user@host ~]$ /usr/local/bin/perf.cap stat -a
> > ^C
> > Performance counter stats for 'system wide':
> >
> > 13,427.05 msec cpu-clock # 7.997 CPUs utilized
> > 783 context-switches # 0.058 K/sec
> > 29 cpu-migrations # 0.002 K/sec
> > 6 page-faults # 0.000 K/sec
> > 161,084,874 cycles # 0.012 GHz
> > 146,823,131 instructions # 0.91 insn per cycle
> > 12,164,802 branches # 0.906 M/sec
> > 380,350 branch-misses # 3.13% of all branches
> >
> > 1.678938906 seconds time elapsed
> >
> > [1] https://github.com/SELinuxProject/refpolicy
> >
> > Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> > ---
> > tools/perf/util/cloexec.c | 4 ++--
> > tools/perf/util/evsel.c | 40 +++++++++++++++++++++++----------------
> > 2 files changed, 26 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/perf/util/cloexec.c b/tools/perf/util/cloexec.c
> > index a12872f2856a..9c8ec816261b 100644
> > --- a/tools/perf/util/cloexec.c
> > +++ b/tools/perf/util/cloexec.c
> > @@ -65,7 +65,7 @@ static int perf_flag_probe(void)
> > return 1;
> > }
> >
> > - WARN_ONCE(err != EINVAL && err != EBUSY,
> > + WARN_ONCE(err != EINVAL && err != EBUSY && err != EACCES,
> > "perf_event_open(..., PERF_FLAG_FD_CLOEXEC) failed with unexpected error %d (%s)\n",
> > err, str_error_r(err, sbuf, sizeof(sbuf)));
> >
> > @@ -83,7 +83,7 @@ static int perf_flag_probe(void)
> > if (fd >= 0)
> > close(fd);
> >
> > - if (WARN_ONCE(fd < 0 && err != EBUSY,
> > + if (WARN_ONCE(fd < 0 && err != EBUSY && err != EACCES,
> > "perf_event_open(..., 0) failed unexpectedly with error %d (%s)\n",
> > err, str_error_r(err, sbuf, sizeof(sbuf))))
> > return -1;
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index 816d930d774e..f03ce1d362d3 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -2493,32 +2493,40 @@ int perf_evsel__open_strerror(struct evsel *evsel, struct target *target,
> > int err, char *msg, size_t size)
> > {
> > char sbuf[STRERR_BUFSIZE];
> > - int printed = 0;
> > + int printed = 0, enforced = 0;
> >
> > switch (err) {
> > case EPERM:
> > case EACCES:
> > + printed += scnprintf(msg + printed, size - printed,
> > + "Access to performance monitoring and observability operations is limited.\n");
> > +
> > + if (!sysfs__read_int("fs/selinux/enforce", &enforced)) {
> > + if (enforced) {
> > + printed += scnprintf(msg + printed, size - printed,
> > + "SELinux Enforcing mode is enabled and can limit access to performance\n"
> > + "monitoring and observability operations. Inspect system audit records\n"
> > + "for more perf_event access control information and adjusting the policy.\n");
> > + }
> > + }
> > +
> > if (err == EPERM)
> > - printed = scnprintf(msg, size,
> > - "No permission to enable %s event.\n\n",
> > + printed += scnprintf(msg, size,
> > + "No permission to enable %s event.\n",
> > perf_evsel__name(evsel));
> >
> > return scnprintf(msg + printed, size - printed,
> > - "You may not have permission to collect %sstats.\n\n"
> > - "Consider tweaking /proc/sys/kernel/perf_event_paranoid,\n"
> > - "which controls use of the performance events system by\n"
> > - "unprivileged users (without CAP_SYS_ADMIN).\n\n"
> > - "The current value is %d:\n\n"
> > + "Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open\n"
> > + "access to performance monitoring and observability operations for users\n"
> > + "without CAP_SYS_ADMIN capability. perf_event_paranoid setting is %d:\n"
> > " -1: Allow use of (almost) all events by all users\n"
> > " Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK\n"
> > - ">= 0: Disallow ftrace function tracepoint by users without CAP_SYS_ADMIN\n"
> > - " Disallow raw tracepoint access by users without CAP_SYS_ADMIN\n"
> > - ">= 1: Disallow CPU event access by users without CAP_SYS_ADMIN\n"
> > - ">= 2: Disallow kernel profiling by users without CAP_SYS_ADMIN\n\n"
> > - "To make this setting permanent, edit /etc/sysctl.conf too, e.g.:\n\n"
> > - " kernel.perf_event_paranoid = -1\n" ,
> > - target->system_wide ? "system-wide " : "",
> > - perf_event_paranoid());
> > + ">= 0: Disallow raw and ftrace function tracepoint access\n"
> > + ">= 1: Disallow CPU event access\n"
> > + ">= 2: Disallow kernel profiling\n"
> > + "To make the adjusted perf_event_paranoid setting permanent preserve it\n"
> > + "in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)",
> > + perf_event_paranoid());
> > case ENOENT:
> > return scnprintf(msg, size, "The %s event is not supported.",
> > perf_evsel__name(evsel));
> >
--
- Arnaldo
^ permalink raw reply
* Re: [PATCH v28 11/22] x86/sgx: Linux Enclave Driver
From: Dr. Greg @ 2020-03-19 18:22 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Jethro Beekman, Sean Christopherson, linux-kernel, x86, linux-sgx,
akpm, dave.hansen, nhorman, npmccallum, haitao.huang,
andriy.shevchenko, tglx, kai.svahn, bp, josh, luto, kai.huang,
rientjes, cedric.xing, puiterwijk, linux-security-module,
Suresh Siddha, Haitao Huang, Chunyang Hui
In-Reply-To: <20200306190003.GF7472@linux.intel.com>
On Fri, Mar 06, 2020 at 09:00:03PM +0200, Jarkko Sakkinen wrote:
Good afternoon, I hope the day is going well for everyone.
> > >> +/**
> > >> + * struct sgx_enclave_create - parameter structure for the
> > >> + * %SGX_IOC_ENCLAVE_CREATE ioctl
> > >> + * @src: address for the SECS page data
> > >> + */
> > >> +struct sgx_enclave_create {
> > >> + __u64 src;
> > >
> > > Would it make sense to add reserved fields to the structs so that new
> > > features can be added in a backwards compatible way? E.g. if we want to
> > > allow userspace to control the backing store by passing in a file
> > > descriptor ENCLAVE_CREATE.
> > Reserving space for future fields is not necessary because the
> > size of the struct is encoded in the ioctl number. The kernel can
> > use this to differentiate between different call versions from
> > userspace.
> Sure but I still would never change the signature once a ioctl is
> added.
These reflections cause me to call, once again, what will be an
unpopular question, but if we pride ourselves on intellectual honesty
in our engineering practices we need to address it.
If we are locking ioctl's and ABI's down, then we need to address,
before the driver goes in, how we are going to handle support for
those of us that have engineering needs around launch token
technology.
On an FLC (unlocked) platform, which is the only thing the driver will
initialize on, there is no reason not to preserve the initialization
IOCTL argument structure that accepts a pointer to a launch token or
to disallow an enclave that has the attribute bit/mask set that allows
the enclave to have access to the launch key. When presented with an
enclave with the launch bit set the driver can simply set the launch
control registers to the MRSIGNER value of that enclave.
It is a straight forward process to provide a root based mechanism
that allows the owner of the platform to control that behavior, if
there are concerns about how the platform is to be used, much like
what is currently being done with access to the provision key.
The ideological purity statement is made by the decision to limit what
hardware the driver will run on. Once that statement is made, and the
driver firmly implements that decision, the owner of that platform
should be able to decide how they want to use and implement the
technology from there.
Unless someone else chirps up there are essentially only two complete
runtimes that are concerned with this issue. Intel can enforce
enhanced ideological purity, if they desire, by simply disallowing
anyone to pass a non-null EINIT structure pointer through their
runtime to the driver, which leaves us to implement the behavior we
want without ABI concerns.
There is a fairly fundamental question here regarding 'software
freedom', maybe Jonathan Corbet and company will even pick up on
it.... :-)
> /Jarkko
Hopefully all of this makes sense and is reasonable.
Have a good remainder of the week, wherever you happen to be locked in
at.
We now return you to your regularly scheduled programming on the
register dance surrounding enclave entry.
Dr. Greg
As always,
Dr. Greg Wettstein, Ph.D, Worker SGX secured infrastructure and
Enjellic Systems Development, LLC autonomously self-defensive
4206 N. 19th Ave. platforms.
Fargo, ND 58102
PH: 701-281-1686 EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"More people are killed every year by pigs than by sharks, which shows
you how good we are at evaluating risk."
-- Bruce Schneier
Beyond Fear
^ permalink raw reply
* Re: [RFC PATCH v14 00/10] Landlock LSM
From: Mickaël Salaün @ 2020-03-19 16:58 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <CAG48ez1K-7Lq2Ep_p9fOvXQ-fwj_8dA1CFd5SVDbT4ccqejDzA@mail.gmail.com>
On 19/03/2020 00:33, Jann Horn wrote:
> On Wed, Mar 18, 2020 at 1:06 PM Mickaël Salaün <mic@digikod.net> wrote:
>> On 17/03/2020 20:45, Jann Horn wrote:
>>> On Tue, Mar 17, 2020 at 6:50 PM Mickaël Salaün <mic@digikod.net> wrote:
>>>> On 17/03/2020 17:19, Jann Horn wrote:
>>>>> On Thu, Mar 12, 2020 at 12:38 AM Mickaël Salaün <mic@digikod.net> wrote:
>>>>>> On 10/03/2020 00:44, Jann Horn wrote:
>>>>>>> On Mon, Feb 24, 2020 at 5:03 PM Mickaël Salaün <mic@digikod.net> wrote:
>>>>
>>>> [...]
>>>>
>>>>>>> Aside from those things, there is also a major correctness issue where
>>>>>>> I'm not sure how to solve it properly:
>>>>>>>
>>>>>>> Let's say a process installs a filter on itself like this:
>>>>>>>
>>>>>>> struct landlock_attr_ruleset ruleset = { .handled_access_fs =
>>>>>>> ACCESS_FS_ROUGHLY_WRITE};
>>>>>>> int ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
>>>>>>> LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
>>>>>>> struct landlock_attr_path_beneath path_beneath = {
>>>>>>> .ruleset_fd = ruleset_fd,
>>>>>>> .allowed_access = ACCESS_FS_ROUGHLY_WRITE,
>>>>>>> .parent_fd = open("/tmp/foobar", O_PATH),
>>>>>>> };
>>>>>>> landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
>>>>>>> sizeof(path_beneath), &path_beneath);
>>>>>>> prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
>>>>>>> struct landlock_attr_enforce attr_enforce = { .ruleset_fd = ruleset_fd };
>>>>>>> landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
>>>>>>> sizeof(attr_enforce), &attr_enforce);
>>>>>>>
>>>>>>> At this point, the process is not supposed to be able to write to
>>>>>>> anything outside /tmp/foobar, right? But what happens if the process
>>>>>>> does the following next?
>>>>>>>
>>>>>>> struct landlock_attr_ruleset ruleset = { .handled_access_fs =
>>>>>>> ACCESS_FS_ROUGHLY_WRITE};
>>>>>>> int ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
>>>>>>> LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
>>>>>>> struct landlock_attr_path_beneath path_beneath = {
>>>>>>> .ruleset_fd = ruleset_fd,
>>>>>>> .allowed_access = ACCESS_FS_ROUGHLY_WRITE,
>>>>>>> .parent_fd = open("/", O_PATH),
>>>>>>> };
>>>>>>> landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
>>>>>>> sizeof(path_beneath), &path_beneath);
>>>>>>> prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
>>>>>>> struct landlock_attr_enforce attr_enforce = { .ruleset_fd = ruleset_fd };
>>>>>>> landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
>>>>>>> sizeof(attr_enforce), &attr_enforce);
>>>>>>>
>>>>>>> As far as I can tell from looking at the source, after this, you will
>>>>>>> have write access to the entire filesystem again. I think the idea is
>>>>>>> that LANDLOCK_CMD_ENFORCE_RULESET should only let you drop privileges,
>>>>>>> not increase them, right?
>>>>>>
>>>>>> There is an additionnal check in syscall.c:get_path_from_fd(): it is
>>>>>> forbidden to add a rule with a path which is not accessible (according
>>>>>> to LANDLOCK_ACCESS_FS_OPEN) thanks to a call to security_file_open(),
>>>>>> but this is definitely not perfect.
>>>>>
>>>>> Ah, I missed that.
>>>>>
>>>>>>> I think the easy way to fix this would be to add a bitmask to each
>>>>>>> rule that says from which ruleset it originally comes, and then let
>>>>>>> check_access_path() collect these bitmasks from each rule with OR, and
>>>>>>> check at the end whether the resulting bitmask is full - if not, at
>>>>>>> least one of the rulesets did not permit the access, and it should be
>>>>>>> denied.
>>>>>>>
>>>>>>> But maybe it would make more sense to change how the API works
>>>>>>> instead, and get rid of the concept of "merging" two rulesets
>>>>>>> together? Instead, we could make the API work like this:
>>>>>>>
>>>>>>> - LANDLOCK_CMD_CREATE_RULESET gives you a file descriptor whose
>>>>>>> ->private_data contains a pointer to the old ruleset of the process,
>>>>>>> as well as a pointer to a new empty ruleset.
>>>>>>> - LANDLOCK_CMD_ADD_RULE fails if the specified rule would not be
>>>>>>> permitted by the old ruleset, then adds the rule to the new ruleset
>>>>>>> - LANDLOCK_CMD_ENFORCE_RULESET fails if the old ruleset pointer in
>>>>>>> ->private_data doesn't match the current ruleset of the process, then
>>>>>>> replaces the old ruleset with the new ruleset.
>>>>>>>
>>>>>>> With this, the new ruleset is guaranteed to be a subset of the old
>>>>>>> ruleset because each of the new ruleset's rules is permitted by the
>>>>>>> old ruleset. (Unless the directory hierarchy rotates, but in that case
>>>>>>> the inaccuracy isn't much worse than what would've been possible
>>>>>>> through RCU path walk anyway AFAIK.)
>>>>>>>
>>>>>>> What do you think?
>>>>>>>
>>>>>>
>>>>>> I would prefer to add the same checks you described at first (with
>>>>>> check_access_path), but only when creating a new ruleset with
>>>>>> merge_ruleset() (which should probably be renamed). This enables not to
>>>>>> rely on a parent ruleset/domain until the enforcement, which is the case
>>>>>> anyway.
>>>>>> Unfortunately this doesn't work for some cases with bind mounts. Because
>>>>>> check_access_path() goes through one path, another (bind mounted) path
>>>>>> could be illegitimately allowed.
>>>>>
>>>>> Hmm... I'm not sure what you mean. At the moment, landlock doesn't
>>>>> allow any sandboxed process to change the mount hierarchy, right? Can
>>>>> you give an example where this would go wrong?
>>>>
>>>> Indeed, a Landlocked process must no be able to change its mount
>>>> namespace layout. However, bind mounts may already exist.
>>>> Let's say a process sandbox itself to only access /a in a read-write
>>>> way.
>>>
>>> So, first policy:
>>>
>>> /a RW
>>>
>>>> Then, this process (or one of its children) add a new restriction
>>>> on /a/b to only be able to read this hierarchy.
>>>
>>> You mean with the second policy looking like this?
>>
>> Right.
>>
>>>
>>> /a RW
>>> /a/b R
>>>
>>> Then the resulting policy would be:
>>>
>>> /a RW policy_bitmask=0x00000003 (bits 0 and 1 set)
>>> /a/b R policy_bitmask=0x00000002 (bit 1 set)
>>> required_bits=0x00000003 (bits 0 and 1 set)
>>>
>>>> The check at insertion
>>>> time would allow this because this access right is a subset of the
>>>> access right allowed with the parent directory. However, If /a/b is bind
>>>> mounted somewhere else, let's say in /private/b, then the second
>>>> enforcement just gave new access rights to this hierarchy too.
>>>
>>> But with the solution I proposed, landlock's path walk would see
>>> something like this when accessing a file at /private/b/foo:
>>> /private/b/foo <no rules>
>>> policies seen until now: 0x00000000
>>> /private/b <access: R, policy_bitmask=0x00000002>
>>> policies seen until now: 0x00000002
>>> /private <no rules>
>>> policies seen until now: 0x00000002
>>> / <no rules>
>>> policies seen until now: 0x00000002
>>>
>>> It wouldn't encounter any rule from the first policy, so the OR of the
>>> seen policy bitmasks would be 0x00000002, which is not the required
>>> value 0x00000003, and so the access would be denied.
>> As I understand your proposition, we need to build the required_bits
>> when adding a rule or enforcing/merging a ruleset with a domain. The
>> issue is that a rule only refers to a struct inode, not a struct path.
>> For your proposition to work, we would need to walk through the file
>> path when adding a rule to a ruleset, which means that we need to depend
>> of the current view of the process (i.e. its mount namespace), and its
>> Landlock domain.
>
> I don't see why that is necessary. Why would we have to walk the file
> path when adding a rule?
>
>> If the required_bits field is set when the ruleset is
>> merged with the domain, it is not possible anymore to walk through the
>> corresponding initial file path, which makes the enforcement step too
>> late to check for such consistency. The important point is that a
>> ruleset/domain doesn't have a notion of file hierarchy, a ruleset is
>> only a set of tagged inodes.
>>
>> I'm not sure I got your proposition right, though. When and how would
>> you generate the required_bits?
>
> Using your terminology:
> A domain is a collection of N layers, which are assigned indices 0..N-1.
> For each possible access type, a domain has a bitmask containing N
> bits that stores which layers control that access type. (Basically a
> per-layer version of fs_access_mask.)
OK, so there is a bit for each domain, which means that you get a limit
of, let's say 64 layers? Knowing that each layer can be created by a
standalone application, potentially nested in a bunch of layers, this
seems artificially limiting.
> To validate an access, you start by ORing together the bitmasks for
> the requested access types; that gives you the required_bits mask,
> which lists all layers that want to control the access.
> Then you set seen_policy_bits=0, then do the
> check_access_path_continue() loop while keeping track of which layers
> you've seen with "seen_policy_bits |= access->contributing_policies",
> or something like that.
> And in the end, you check that seen_policy_bits is a superset of
> required_bits - something like `(~seen_policy_bits) & required_bits ==
> 0`.
>
> AFAICS to create a new domain from a bunch of layers, you wouldn't
> have to do any path walking.
Right, I misunderstood your previous email.
>
>> Here is my updated proposition: add a layer level and a depth to each
>> rule (once enforced/merged with a domain), and a top layer level for a
>> domain. When enforcing a ruleset (i.e. merging a ruleset into the
>> current domain), the layer level of a new rule would be the incremented
>> top layer level.
>> If there is no rule (from this domain) tied to the same
>> inode, then the depth of the new rule is 1. However, if there is already
>> a rule tied to the same inode and if this rule's layer level is the
>> previous top layer level, then the depth and the layer level are both
>> incremented and the rule is updated with the new access rights (boolean
>> AND).
>>
>> The policy looks like this:
>> domain top_layer=2
>> /a RW policy_bitmask=0x00000003 layer=1 depth=1
>> /a/b R policy_bitmask=0x00000002 layer=2 depth=1
>>
>> The path walk access check walks through all inodes and start with a
>> layer counter equal to the top layer of the current domain. For each
>> encountered inode tied to a rule, the access rights are checked and a
>> new check ensures that the layer of the matching rule is the same as the
>> counter (this may be a merged ruleset containing rules pertaining to the
>> same hierarchy, which is fine) or equal to the decremented counter (i.e.
>> the path walk just reached the underlying layer). If the path walk
>> encounter a rule with a layer strictly less than the counter minus one,
>> there is a whole in the layers which means that the ruleset
>> hierarchy/subset does not match, and the access must be denied.
>>
>> When accessing a file at /private/b/foo for a read access:
>> /private/b/foo <no rules>
>> allowed_access=unknown layer_counter=2
>> /private/b <access: R, policy_bitmask=0x00000002, layer=2, depth=1>
>> allowed_access=allowed layer_counter=2
>> /private <no rules>
>> allowed_access=allowed layer_counter=2
>> / <no rules>
>> allowed_access=allowed layer_counter=2
>>
>> Because the layer_counter didn't reach 1, the access request is then denied.
>>
>> This proposition enables not to rely on a parent ruleset at first, only
>> when enforcing/merging a ruleset with a domain. This also solves the
>> issue with multiple inherited/nested rules on the same inode (in which
>> case the depth just grows). Moreover, this enables to safely stop the
>> path walk as soon as we reach the layer 1.
>
> (FWIW, you could do the same optimization with the seen_policy_bits approach.)
>
> I guess the difference between your proposal and mine is that in my
> proposal, the following would work, in effect permitting W access to
> /foo/bar/baz (and nothing else)?
>
> first ruleset:
> /foo W
> second ruleset:
> /foo/bar/baz W
> third ruleset:
> /foo/bar W
>
> whereas in your proposal, IIUC it wouldn't be valid for a new ruleset
> to whitelist a superset of what was whitelisted in a previous ruleset?
>
This behavior seems dangerous because a process which sandbox itself to
only access /foo/bar W can bypass the restrictions from one of its
parent domains (i.e. only access /foo/bar/baz W). Indeed, each layer is
(most of the time) a different and standalone security policy.
To sum up, the bitmask approach doesn't have the notion of layers
ordering. It is then not possible to check that a rule comes from a
domain which is the direct ancestor of a child's domain. I want each
policy/layer to be really nested in the sense that a process sandboxing
itself can only add more restriction to itself with regard to its parent
domain (and the whole hierarchy). This is a similar approach to
seccomp-bpf (with chained filters), except there is almost no overhead
to nest several policies/layers together because they are flattened.
Using the layer level and depth approach enables to implement this.
^ permalink raw reply
* Re: [PATCH 00/13] VFS: Filesystem information [ver #19]
From: Miklos Szeredi @ 2020-03-19 12:36 UTC (permalink / raw)
To: David Howells
Cc: Linus Torvalds, Al Viro, Linux NFS list, Andreas Dilger,
Anna Schumaker, Theodore Ts'o, Linux API, linux-ext4,
Trond Myklebust, Ian Kent, Miklos Szeredi, Christian Brauner,
Jann Horn, Darrick J. Wong, Karel Zak, Jeff Layton, linux-fsdevel,
LSM, linux-kernel
In-Reply-To: <3085880.1584614257@warthog.procyon.org.uk>
On Thu, Mar 19, 2020 at 11:37 AM David Howells <dhowells@redhat.com> wrote:
>
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> > > (2) It's more efficient as we can return specific binary data rather than
> > > making huge text dumps. Granted, sysfs and procfs could present the
> > > same data, though as lots of little files which have to be
> > > individually opened, read, closed and parsed.
> >
> > Asked this a number of times, but you haven't answered yet: what
> > application would require such a high efficiency?
>
> Low efficiency means more time doing this when that time could be spent doing
> other things - or even putting the CPU in a powersaving state. Using an
> open/read/close render-to-text-and-parse interface *will* be slower and less
> efficient as there are more things you have to do to use it.
>
> Then consider doing a walk over all the mounts in the case where there are
> 10000 of them - we have issues with /proc/mounts for such. fsinfo() will end
> up doing a lot less work.
Current /proc/mounts problems arise from the fact that mount info can
only be queried for the whole namespace, and hence changes related to
a single mount will require rescanning the complete mount list. If
mount info can be queried for individual mounts, then the need to scan
the complete list will be rare. That's *the* point of this change.
> > > (3) We wouldn't have the overhead of open and close (even adding a
> > > self-contained readfile() syscall has to do that internally
> >
> > Busted: add f_op->readfile() and be done with all that. For example
> > DEFINE_SHOW_ATTRIBUTE() could be trivially moved to that interface.
>
> Look at your example. "f_op->". That's "file->f_op->" I presume.
>
> You would have to make it "i_op->" to avoid the open and the close - and for
> things like procfs and sysfs, that's probably entirely reasonable - but bear
> in mind that you still have to apply all the LSM file security controls, just
> in case the backing filesystem is, say, ext4 rather than procfs.
>
> > We could optimize existing proc, sys, etc. interfaces, but it's not
> > been an issue, apparently.
>
> You can't get rid of or change many of the existing interfaces. A lot of them
> are effectively indirect system calls and are, as such, part of the fixed
> UAPI. You'd have to add a parallel optimised set.
Sure.
We already have the single_open() internal API that is basically a
->readfile() wrapper. Moving this up to the f_op level (no, it's not
an i_op, and yes, we do need struct file, but it can be simply
allocated on the stack) is a trivial optimization that would let a
readfile(2) syscall access that level. No new complexity in that
case. Same generally goes for seq_file: seq_readfile() is trivial
to implement without messing with current implementation or any
existing APIs.
>
> > > (6) Don't have to create/delete a bunch of sysfs/procfs nodes each time a
> > > mount happens or is removed - and since systemd makes much use of
> > > mount namespaces and mount propagation, this will create a lot of
> > > nodes.
> >
> > Not true.
>
> This may not be true if you roll your own special filesystem. It *is* true if
> you do it in procfs or sysfs. The files don't exist if you don't create nodes
> or attribute tables for them.
That's one of the reasons why I opted to roll my own. But the ideas
therein could be applied to kernfs, if found to be generally useful.
Nothing magic about that.
>
> > > The argument for doing this through procfs/sysfs/somemagicfs is that
> > > someone using a shell can just query the magic files using ordinary text
> > > tools, such as cat - and that has merit - but it doesn't solve the
> > > query-by-pathname problem.
> > >
> > > The suggested way around the query-by-pathname problem is to open the
> > > target file O_PATH and then look in a magic directory under procfs
> > > corresponding to the fd number to see a set of attribute files[*] laid out.
> > > Bash, however, can't open by O_PATH or O_NOFOLLOW as things stand...
> >
> > Bash doesn't have fsinfo(2) either, so that's not really a good argument.
>
> I never claimed that fsinfo() could be accessed directly from the shell. For
> you proposal, you claimed "immediately usable from all programming languages,
> including scripts".
You are right. Note however: only special files need the O_PATH
handling, regular files are directories can be opened by the shell
without side effects.
In any case, I think neither of us can be convinced of the other's
right, so I guess It's up to Al and Linus to make a decision.
Thanks,
Miklos
^ permalink raw reply
* Re: [PATCH 00/13] VFS: Filesystem information [ver #19]
From: David Howells @ 2020-03-19 10:37 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dhowells, Linus Torvalds, Al Viro, Linux NFS list, Andreas Dilger,
Anna Schumaker, Theodore Ts'o, Linux API, linux-ext4,
Trond Myklebust, Ian Kent, Miklos Szeredi, Christian Brauner,
Jann Horn, Darrick J. Wong, Karel Zak, Jeff Layton, linux-fsdevel,
LSM, linux-kernel
In-Reply-To: <CAJfpeguaiicjS2StY5m=8H7BCjq6PLxMsWE3Mx_jYR1foDWVTg@mail.gmail.com>
Miklos Szeredi <miklos@szeredi.hu> wrote:
> > (2) It's more efficient as we can return specific binary data rather than
> > making huge text dumps. Granted, sysfs and procfs could present the
> > same data, though as lots of little files which have to be
> > individually opened, read, closed and parsed.
>
> Asked this a number of times, but you haven't answered yet: what
> application would require such a high efficiency?
Low efficiency means more time doing this when that time could be spent doing
other things - or even putting the CPU in a powersaving state. Using an
open/read/close render-to-text-and-parse interface *will* be slower and less
efficient as there are more things you have to do to use it.
Then consider doing a walk over all the mounts in the case where there are
10000 of them - we have issues with /proc/mounts for such. fsinfo() will end
up doing a lot less work.
> I strongly feel that mount info belongs in the latter category
I feel strongly that a lot of stuff done through /proc or /sys shouldn't be.
Yes, it's nice that you can explore it with cat and poke it with echo, but it
has a number of problems: security, atomiticity, efficiency and providing an
round-the-back way to pin stuff if not done right.
> > (3) We wouldn't have the overhead of open and close (even adding a
> > self-contained readfile() syscall has to do that internally
>
> Busted: add f_op->readfile() and be done with all that. For example
> DEFINE_SHOW_ATTRIBUTE() could be trivially moved to that interface.
Look at your example. "f_op->". That's "file->f_op->" I presume.
You would have to make it "i_op->" to avoid the open and the close - and for
things like procfs and sysfs, that's probably entirely reasonable - but bear
in mind that you still have to apply all the LSM file security controls, just
in case the backing filesystem is, say, ext4 rather than procfs.
> We could optimize existing proc, sys, etc. interfaces, but it's not
> been an issue, apparently.
You can't get rid of or change many of the existing interfaces. A lot of them
are effectively indirect system calls and are, as such, part of the fixed
UAPI. You'd have to add a parallel optimised set.
> > (6) Don't have to create/delete a bunch of sysfs/procfs nodes each time a
> > mount happens or is removed - and since systemd makes much use of
> > mount namespaces and mount propagation, this will create a lot of
> > nodes.
>
> Not true.
This may not be true if you roll your own special filesystem. It *is* true if
you do it in procfs or sysfs. The files don't exist if you don't create nodes
or attribute tables for them.
> > The argument for doing this through procfs/sysfs/somemagicfs is that
> > someone using a shell can just query the magic files using ordinary text
> > tools, such as cat - and that has merit - but it doesn't solve the
> > query-by-pathname problem.
> >
> > The suggested way around the query-by-pathname problem is to open the
> > target file O_PATH and then look in a magic directory under procfs
> > corresponding to the fd number to see a set of attribute files[*] laid out.
> > Bash, however, can't open by O_PATH or O_NOFOLLOW as things stand...
>
> Bash doesn't have fsinfo(2) either, so that's not really a good argument.
I never claimed that fsinfo() could be accessed directly from the shell. For
you proposal, you claimed "immediately usable from all programming languages,
including scripts".
> Implementing a utility to show mount attribute(s) by path is trivial
> for the file based interface, while it would need to be updated for
> each extension of fsinfo(2). Same goes for libc, language bindings,
> etc.
That's not precisely true. If you aren't using an extension to an fsinfo()
attribute, you wouldn't need to change anything[*].
If you want to use an extension - *even* through a file based interface - you
*would* have to change your code and your parser.
And, no, extending an fsinfo() attribute would not require any changes to libc
unless libc is using that attribute[*] and wants to access the extension.
[*] I assume that in C/C++ at least, you'd use linux/fsinfo.h rather than some
libc version.
[*] statfs() could be emulated this way, but I'm not sure what else libc
specifically is going to look at. This is more aimed at libmount amongst
other things.
David
^ permalink raw reply
* Re: [PATCH v3 7/8] ima: Calculate and extend PCR with digests in ima_template_entry
From: Mimi Zohar @ 2020-03-19 9:45 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <7df041fd4cd64a5bb61beb4eb8276819@huawei.com>
On Thu, 2020-03-19 at 08:31 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> > owner@vger.kernel.org] On Behalf Of Mimi Zohar
> > Sent: Wednesday, March 18, 2020 10:55 PM
> > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > James.Bottomley@HansenPartnership.com;
> > jarkko.sakkinen@linux.intel.com
> > Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Silviu Vlasceanu
> > <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH v3 7/8] ima: Calculate and extend PCR with digests in
> > ima_template_entry
> >
> > On Wed, 2020-03-18 at 12:42 +0000, Roberto Sassu wrote:
> > > > -----Original Message-----
> > > > From: owner-linux-security-module@vger.kernel.org [mailto:owner-
> > linux-
> > > > security-module@vger.kernel.org] On Behalf Of Mimi Zohar
> > > > Sent: Tuesday, March 3, 2020 5:04 AM
> > > > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > > > James.Bottomley@HansenPartnership.com;
> > > > jarkko.sakkinen@linux.intel.com
> > > > Cc: linux-integrity@vger.kernel.org; linux-security-
> > module@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; Silviu Vlasceanu
> > > > <Silviu.Vlasceanu@huawei.com>
> > > > Subject: Re: [PATCH v3 7/8] ima: Calculate and extend PCR with digests
> > in
> > > > ima_template_entry
> > > >
> > > > On Mon, 2020-02-10 at 11:04 +0100, Roberto Sassu wrote:
> > > >
> > > > > @@ -219,6 +214,8 @@ int ima_restore_measurement_entry(struct
> > > > ima_template_entry *entry)
> > > > >
> > > > > int __init ima_init_digests(void)
> > > > > {
> > > > > + u16 digest_size;
> > > > > + u16 crypto_id;
> > > > > int i;
> > > > >
> > > > > if (!ima_tpm_chip)
> > > > > @@ -229,8 +226,17 @@ int __init ima_init_digests(void)
> > > > > if (!digests)
> > > > > return -ENOMEM;
> > > > >
> > > > > - for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
> > > > > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> > > > > digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
> > > > > + digest_size = ima_tpm_chip->allocated_banks[i].digest_size;
> > > > > + crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
> > > > > +
> > > > > + /* for unmapped TPM algorithms digest is still a padded
> > > > SHA1 */
> > > > > + if (crypto_id == HASH_ALGO__LAST)
> > > > > + digest_size = SHA1_DIGEST_SIZE;
> > > > > +
> > > > > + memset(digests[i].digest, 0xff, digest_size);
> > > >
> > > > Shouldn't the memset here be of the actual digest size even for
> > > > unmapped TPM algorithms.
> > >
> > > This is consistent with ima_calc_field_array_hash(), so that a verifier
> > > will always pad the SHA1 digest with zeros to obtain the final PCR value.
> > >
> > > I can set all bytes if you prefer.
> >
> > My concern is with violations. The measurement list will be padded
> > with 0's, but the value being extended into the TPM will only
> > partially be 0xFF's. When verifying the measurement list, replacing
> > all 0x00's with all 0xFF's is simpler.
>
> If the TPM algorithm is unknown, the starting point is the SHA1 digest.
> If there is a violation, this should be the one to be modified. Then, after
> that, padding is done for all entries in the same way, regardless of
> whether the entry is a violation or not.
Ok. In the case that the verifier supports the hash algorithm and
calculates the template hash, walking the measurement list will fail
anyway. In the case that the verifier does not support the hash
algorithm, then it will pad/truncate the SHA1 hash consistently. That
works for now with the SHA1 based measurement list and should work
with a hash agile measurement list.
thanks,
Mimi
^ permalink raw reply
* RE: [PATCH v3 7/8] ima: Calculate and extend PCR with digests in ima_template_entry
From: Roberto Sassu @ 2020-03-19 8:31 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <1584568492.5188.200.camel@linux.ibm.com>
> -----Original Message-----
> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> owner@vger.kernel.org] On Behalf Of Mimi Zohar
> Sent: Wednesday, March 18, 2020 10:55 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>
> Subject: Re: [PATCH v3 7/8] ima: Calculate and extend PCR with digests in
> ima_template_entry
>
> On Wed, 2020-03-18 at 12:42 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> > > From: owner-linux-security-module@vger.kernel.org [mailto:owner-
> linux-
> > > security-module@vger.kernel.org] On Behalf Of Mimi Zohar
> > > Sent: Tuesday, March 3, 2020 5:04 AM
> > > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > > James.Bottomley@HansenPartnership.com;
> > > jarkko.sakkinen@linux.intel.com
> > > Cc: linux-integrity@vger.kernel.org; linux-security-
> module@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; Silviu Vlasceanu
> > > <Silviu.Vlasceanu@huawei.com>
> > > Subject: Re: [PATCH v3 7/8] ima: Calculate and extend PCR with digests
> in
> > > ima_template_entry
> > >
> > > On Mon, 2020-02-10 at 11:04 +0100, Roberto Sassu wrote:
> > >
> > > > @@ -219,6 +214,8 @@ int ima_restore_measurement_entry(struct
> > > ima_template_entry *entry)
> > > >
> > > > int __init ima_init_digests(void)
> > > > {
> > > > + u16 digest_size;
> > > > + u16 crypto_id;
> > > > int i;
> > > >
> > > > if (!ima_tpm_chip)
> > > > @@ -229,8 +226,17 @@ int __init ima_init_digests(void)
> > > > if (!digests)
> > > > return -ENOMEM;
> > > >
> > > > - for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
> > > > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> > > > digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
> > > > + digest_size = ima_tpm_chip->allocated_banks[i].digest_size;
> > > > + crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
> > > > +
> > > > + /* for unmapped TPM algorithms digest is still a padded
> > > SHA1 */
> > > > + if (crypto_id == HASH_ALGO__LAST)
> > > > + digest_size = SHA1_DIGEST_SIZE;
> > > > +
> > > > + memset(digests[i].digest, 0xff, digest_size);
> > >
> > > Shouldn't the memset here be of the actual digest size even for
> > > unmapped TPM algorithms.
> >
> > This is consistent with ima_calc_field_array_hash(), so that a verifier
> > will always pad the SHA1 digest with zeros to obtain the final PCR value.
> >
> > I can set all bytes if you prefer.
>
> My concern is with violations. The measurement list will be padded
> with 0's, but the value being extended into the TPM will only
> partially be 0xFF's. When verifying the measurement list, replacing
> all 0x00's with all 0xFF's is simpler.
If the TPM algorithm is unknown, the starting point is the SHA1 digest.
If there is a violation, this should be the one to be modified. Then, after
that, padding is done for all entries in the same way, regardless of
whether the entry is a violation or not.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
* Re: [PATCH 01/14] VFS: Add additional RESOLVE_* flags [ver #18]
From: Aleksa Sarai @ 2020-03-16 14:20 UTC (permalink / raw)
To: Al Viro
Cc: Stefan Metzmacher, Linus Torvalds, David Howells, Ian Kent,
Miklos Szeredi, Christian Brauner, Jann Horn, Darrick J. Wong,
Karel Zak, jlayton, Linux API, linux-fsdevel, LSM List,
Linux Kernel Mailing List, Jeremy Allison, Ralph Böhme,
Volker Lendecke
In-Reply-To: <20200313182844.GO23230@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2196 bytes --]
On 2020-03-13, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Mar 13, 2020 at 08:59:01PM +1100, Aleksa Sarai wrote:
> > On 2020-03-12, Stefan Metzmacher <metze@samba.org> wrote:
> > > Am 12.03.20 um 17:24 schrieb Linus Torvalds:
> > > > But yes, if we have a major package like samba use it, then by all
> > > > means let's add linkat2(). How many things are we talking about? We
> > > > have a number of system calls that do *not* take flags, but do do
> > > > pathname walking. I'm thinking things like "mkdirat()"?)
> > >
> > > I haven't looked them up in detail yet.
> > > Jeremy can you provide a list?
> > >
> > > Do you think we could route some of them like mkdirat() and mknodat()
> > > via openat2() instead of creating new syscalls?
> >
> > I have heard some folks asking for a way to create a directory and get a
> > handle to it atomically -- so arguably this is something that could be
> > inside openat2()'s feature set (O_MKDIR?). But I'm not sure how popular
> > of an idea this is.
>
> For fuck sake, *NO*!
>
> We don't need any more multiplexors from hell. mkdir() and open() have
> deeply different interpretation of pathnames (and anyone who asks for
> e.g. traversals of dangling symlinks on mkdir() is insane). Don't try to
> mix those; even O_TMPFILE had been a mistake.
I agree that O_TMPFILE is a mess, and you're right that it wouldn't be a
good idea to fold it into open*(). But what is your opinion on a
hypothetical mkdirat2() which would let you get an fd to the directory
that was just created?
> We really don't need openat2() turning into another one. Syscall table
> slots are not in a short supply, and the level of review one gets from
> "new syscall added" is higher than from "make fubar(2) recognize a new
> member in options->union_full_of_crap if it has RESOLVE_TO_WANK_WITH_RIGHT_HAND
> set in options->flags, affecting its behaviour in some odd ways".
> Which is a good thing, damnit.
You're quite right, and I don't intend openat2() to become another
ioctl-but-with-even-more-fun-semantics.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v1] perf tool: make Perf tool aware of SELinux access control
From: Alexey Budankov @ 2020-03-19 6:23 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Namhyung Kim, Alexander Shishkin, Peter Zijlstra, Ingo Molnar,
Andi Kleen, linux-kernel, selinux@vger.kernel.org,
linux-security-module@vger.kernel.org
In-Reply-To: <b8a0669e-36e4-a0e8-fd35-3dbd890d2170@linux.intel.com>
Hi,
Is there any thougts, comments or questions so far?
Please share you mind.
Thanks,
Alexey
On 13.03.2020 20:27, Alexey Budankov wrote:
>
> Extend Perf tool with the check of /sys/fs/selinux/enforce value and notify
> in case access to perf_event_open() syscall is restricted by the enforced
> SELinux policy settings.
>
> Testing and evaluation (Fedora 31 x86_64 with enforced Targeted policy extended
> by perf_event class (see refpolicy [1] master branch)):
>
> [root@host ~]# ps -Z
> LABEL PID TTY TIME CMD
> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 3960 pts/1 00:00:00 bash
> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4167 pts/1 00:00:00 ps
>
> [root@host ~]# ls -alhZ /usr/local/bin/
> total 56M
> drwxr-xr-x. 2 root root system_u:object_r:bin_t:s0 4.0K Mar 4 12:27 .
> drwxr-xr-x. 12 root root system_u:object_r:usr_t:s0 4.0K Jul 25 2019 ..
> -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 4.1M Jan 23 2017 bash
> -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 4.1M Jan 23 2017 bash.before_shellshock_patch
> ...
> -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 372 May 14 2019 flask
> -rwxr-xr-x. 1 root root unconfined_u:object_r:bin_t:s0 24M Mar 4 12:15 perf <== unprivileged users (perf_event_paranoid)
> -rwxr-x---. 1 root perf_users unconfined_u:object_r:bin_t:s0 24M Mar 4 12:19 perf.cap <== perf_users (CAP_SYS_ADMIN)
> -rwxr-xr-x. 1 root root system_u:object_r:bin_t:s0 44K Dec 8 2016 spiff
> ...
> lrwxrwxrwx. 1 root root system_u:object_r:bin_t:s0 4 Aug 21 2018 zstdmt -> zstd
>
> [root@host ~]# getenforce
> Enforcing
>
> === Access by unprivileged user ===
>
> [user@host ~]$ ps -Z
> LABEL PID TTY TIME CMD
> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4043 pts/2 00:00:00 bash
> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4168 pts/2 00:00:00 ps
>
> [user@host ~]$ /usr/local/bin/perf stat -- ls
> Error:
> Access to performance monitoring and observability operations is limited.
> SELinux Enforcing mode is enabled and can limit access to performance
> monitoring and observability operations. Inspect system audit records
> for more perf_event access control information and adjusting the policy.
> Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open
> access to performance monitoring and observability operations for users
> without CAP_SYS_ADMIN capability. perf_event_paranoid setting is -1:
> -1: Allow use of (almost) all events by all users
> Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
>> = 0: Disallow raw and ftrace function tracepoint access
>> = 1: Disallow CPU event access
>> = 2: Disallow kernel profiling
> To make the adjusted perf_event_paranoid setting permanent preserve it
> in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)
>
> [root@host ~]# journalctl --follow
> ... audit[4186]: AVC avc: denied { open } for pid=4186 comm="perf" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
> ... audit[4186]: AVC avc: denied { open } for pid=4186 comm="perf" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
> ... setroubleshoot[4194]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t. For complete SELinux messages run: sealert -l 9a6f3db2-3d8f-461e-afad-0b5c3a9c3b9d
> ... python3[4194]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t.
>
> ***** Plugin catchall (100. confidence) suggests **************************
>
> If you believe that perf should be allowed open access on perf_event labeled unconfined_t by default.
> Then you should report this as a bug.
> You can generate a local policy module to allow this access.
> Do
> allow this access for now by executing:
> # ausearch -c 'perf' --raw | audit2allow -M my-perf
> # semodule -X 300 -i my-perf.pp
>
> === Access by perf privileged user ===
>
> [user@host ~]$ ps -Z
> LABEL PID TTY TIME CMD
> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4043 pts/2 00:00:00 bash
> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4168 pts/2 00:00:00 ps
>
> [user@host ~]$ libcap/progs/getcap /usr/local/bin/perf.cap
> /usr/local/bin/perf.cap = cap_sys_ptrace,cap_syslog,cap_sys_admin+ep
>
> [user@host ~]$ /usr/local/bin/perf.cap stat -- ls
> Error:
> Access to performance monitoring and observability operations is limited.
> SELinux Enforcing mode is enabled and can limit access to performance
> monitoring and observability operations. Inspect system audit records
> for more perf_event access control information and adjusting the policy.
> Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open
> access to performance monitoring and observability operations for users
> without CAP_SYS_ADMIN capability. perf_event_paranoid setting is -1:
> -1: Allow use of (almost) all events by all users
> Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
>> = 0: Disallow raw and ftrace function tracepoint access
>> = 1: Disallow CPU event access
>> = 2: Disallow kernel profiling
> To make the adjusted perf_event_paranoid setting permanent preserve it
> in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)
>
> [root@host ~]# journalctl --follow
>
> ... audit[3926]: AVC avc: denied { open } for pid=3926 comm="perf.cap" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
> ... audit[3926]: AVC avc: denied { open } for pid=3926 comm="perf.cap" scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=perf_event permissive=0
>
> ... setroubleshoot[3934]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t. For complete SELinux messages run: sealert -l 9a6f3db2-3d8f-461e-afad-0b5c3a9c3b9d
> ... python3[3934]: SELinux is preventing perf from open access on the perf_event labeled unconfined_t.
>
> ***** Plugin catchall (100. confidence) suggests **************************
>
> If you believe that perf should be allowed open access on perf_event labeled unconfined_t by default.
> Then you should report this as a bug.
> You can generate a local policy module to allow this access.
> Do
> allow this access for now by executing:
> # ausearch -c 'perf' --raw | audit2allow -M my-perf
> # semodule -X 300 -i my-perf.pp
>
> === Open access to performance monitoring and observability operations in unconfined_t domain ===
>
> [root@host ~]# ausearch -c 'perf' --raw | audit2allow -M my-perf && cat my-perf.te
>
> module my-perf 1.0;
>
> require {
> type unconfined_t;
> class perf_event { cpu kernel open read tracepoint write };
> }
>
> #============= unconfined_t ==============
> allow unconfined_t self:perf_event { cpu kernel open read tracepoint write };
>
> [root@host ~]# semodule -X 300 -i my-perf.pp
>
> [user@host ~]$ ps -Z
> LABEL PID TTY TIME CMD
> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4043 pts/2 00:00:00 bash
> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 4168 pts/2 00:00:00 ps
>
> [user@host ~]$ /usr/local/bin/perf stat -- ls
> Desktop Documents Downloads intel Music perf.data perf.data.old Pictures Public Templates Videos
>
> Performance counter stats for 'ls':
>
> 0.72 msec task-clock:u # 0.655 CPUs utilized
> 0 context-switches:u # 0.000 K/sec
> 0 cpu-migrations:u # 0.000 K/sec
> 98 page-faults:u # 0.137 M/sec
> 908,356 cycles:u # 1.266 GHz
> 729,984 instructions:u # 0.80 insn per cycle
> 142,774 branches:u # 198.968 M/sec
> 8,238 branch-misses:u # 5.77% of all branches
>
> 0.001095239 seconds time elapsed
>
> 0.001147000 seconds user
> 0.000000000 seconds sys
>
> [user@host ~]$ /usr/local/bin/perf stat -a
> Error:
> Access to performance monitoring and observability operations is limited.
> SELinux Enforcing mode is enabled and can limit access to performance
> monitoring and observability operations. Inspect system audit records
> for more perf_event access control information and adjusting the policy.
> Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open
> access to performance monitoring and observability operations for users
> without CAP_SYS_ADMIN capability. perf_event_paranoid setting is -1:
> -1: Allow use of (almost) all events by all users
> Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
>> = 0: Disallow raw and ftrace function tracepoint access
>> = 1: Disallow CPU event access
>> = 2: Disallow kernel profiling
> To make the adjusted perf_event_paranoid setting permanent preserve it
> in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)
>
> [user@host ~]$ /usr/local/bin/perf.cap stat -a
> ^C
> Performance counter stats for 'system wide':
>
> 13,427.05 msec cpu-clock # 7.997 CPUs utilized
> 783 context-switches # 0.058 K/sec
> 29 cpu-migrations # 0.002 K/sec
> 6 page-faults # 0.000 K/sec
> 161,084,874 cycles # 0.012 GHz
> 146,823,131 instructions # 0.91 insn per cycle
> 12,164,802 branches # 0.906 M/sec
> 380,350 branch-misses # 3.13% of all branches
>
> 1.678938906 seconds time elapsed
>
> [1] https://github.com/SELinuxProject/refpolicy
>
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> ---
> tools/perf/util/cloexec.c | 4 ++--
> tools/perf/util/evsel.c | 40 +++++++++++++++++++++++----------------
> 2 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/tools/perf/util/cloexec.c b/tools/perf/util/cloexec.c
> index a12872f2856a..9c8ec816261b 100644
> --- a/tools/perf/util/cloexec.c
> +++ b/tools/perf/util/cloexec.c
> @@ -65,7 +65,7 @@ static int perf_flag_probe(void)
> return 1;
> }
>
> - WARN_ONCE(err != EINVAL && err != EBUSY,
> + WARN_ONCE(err != EINVAL && err != EBUSY && err != EACCES,
> "perf_event_open(..., PERF_FLAG_FD_CLOEXEC) failed with unexpected error %d (%s)\n",
> err, str_error_r(err, sbuf, sizeof(sbuf)));
>
> @@ -83,7 +83,7 @@ static int perf_flag_probe(void)
> if (fd >= 0)
> close(fd);
>
> - if (WARN_ONCE(fd < 0 && err != EBUSY,
> + if (WARN_ONCE(fd < 0 && err != EBUSY && err != EACCES,
> "perf_event_open(..., 0) failed unexpectedly with error %d (%s)\n",
> err, str_error_r(err, sbuf, sizeof(sbuf))))
> return -1;
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 816d930d774e..f03ce1d362d3 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2493,32 +2493,40 @@ int perf_evsel__open_strerror(struct evsel *evsel, struct target *target,
> int err, char *msg, size_t size)
> {
> char sbuf[STRERR_BUFSIZE];
> - int printed = 0;
> + int printed = 0, enforced = 0;
>
> switch (err) {
> case EPERM:
> case EACCES:
> + printed += scnprintf(msg + printed, size - printed,
> + "Access to performance monitoring and observability operations is limited.\n");
> +
> + if (!sysfs__read_int("fs/selinux/enforce", &enforced)) {
> + if (enforced) {
> + printed += scnprintf(msg + printed, size - printed,
> + "SELinux Enforcing mode is enabled and can limit access to performance\n"
> + "monitoring and observability operations. Inspect system audit records\n"
> + "for more perf_event access control information and adjusting the policy.\n");
> + }
> + }
> +
> if (err == EPERM)
> - printed = scnprintf(msg, size,
> - "No permission to enable %s event.\n\n",
> + printed += scnprintf(msg, size,
> + "No permission to enable %s event.\n",
> perf_evsel__name(evsel));
>
> return scnprintf(msg + printed, size - printed,
> - "You may not have permission to collect %sstats.\n\n"
> - "Consider tweaking /proc/sys/kernel/perf_event_paranoid,\n"
> - "which controls use of the performance events system by\n"
> - "unprivileged users (without CAP_SYS_ADMIN).\n\n"
> - "The current value is %d:\n\n"
> + "Consider adjusting /proc/sys/kernel/perf_event_paranoid setting to open\n"
> + "access to performance monitoring and observability operations for users\n"
> + "without CAP_SYS_ADMIN capability. perf_event_paranoid setting is %d:\n"
> " -1: Allow use of (almost) all events by all users\n"
> " Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK\n"
> - ">= 0: Disallow ftrace function tracepoint by users without CAP_SYS_ADMIN\n"
> - " Disallow raw tracepoint access by users without CAP_SYS_ADMIN\n"
> - ">= 1: Disallow CPU event access by users without CAP_SYS_ADMIN\n"
> - ">= 2: Disallow kernel profiling by users without CAP_SYS_ADMIN\n\n"
> - "To make this setting permanent, edit /etc/sysctl.conf too, e.g.:\n\n"
> - " kernel.perf_event_paranoid = -1\n" ,
> - target->system_wide ? "system-wide " : "",
> - perf_event_paranoid());
> + ">= 0: Disallow raw and ftrace function tracepoint access\n"
> + ">= 1: Disallow CPU event access\n"
> + ">= 2: Disallow kernel profiling\n"
> + "To make the adjusted perf_event_paranoid setting permanent preserve it\n"
> + "in /etc/sysctl.conf (e.g. kernel.perf_event_paranoid = <setting>)",
> + perf_event_paranoid());
> case ENOENT:
> return scnprintf(msg, size, "The %s event is not supported.",
> perf_evsel__name(evsel));
>
^ permalink raw reply
* Re: [RFC PATCH v14 00/10] Landlock LSM
From: Jann Horn @ 2020-03-18 23:33 UTC (permalink / raw)
To: Mickaël Salaün
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <e8530226-f295-a897-1132-7e6970dad49f@digikod.net>
On Wed, Mar 18, 2020 at 1:06 PM Mickaël Salaün <mic@digikod.net> wrote:
> On 17/03/2020 20:45, Jann Horn wrote:
> > On Tue, Mar 17, 2020 at 6:50 PM Mickaël Salaün <mic@digikod.net> wrote:
> >> On 17/03/2020 17:19, Jann Horn wrote:
> >>> On Thu, Mar 12, 2020 at 12:38 AM Mickaël Salaün <mic@digikod.net> wrote:
> >>>> On 10/03/2020 00:44, Jann Horn wrote:
> >>>>> On Mon, Feb 24, 2020 at 5:03 PM Mickaël Salaün <mic@digikod.net> wrote:
> >>
> >> [...]
> >>
> >>>>> Aside from those things, there is also a major correctness issue where
> >>>>> I'm not sure how to solve it properly:
> >>>>>
> >>>>> Let's say a process installs a filter on itself like this:
> >>>>>
> >>>>> struct landlock_attr_ruleset ruleset = { .handled_access_fs =
> >>>>> ACCESS_FS_ROUGHLY_WRITE};
> >>>>> int ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
> >>>>> LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
> >>>>> struct landlock_attr_path_beneath path_beneath = {
> >>>>> .ruleset_fd = ruleset_fd,
> >>>>> .allowed_access = ACCESS_FS_ROUGHLY_WRITE,
> >>>>> .parent_fd = open("/tmp/foobar", O_PATH),
> >>>>> };
> >>>>> landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
> >>>>> sizeof(path_beneath), &path_beneath);
> >>>>> prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> >>>>> struct landlock_attr_enforce attr_enforce = { .ruleset_fd = ruleset_fd };
> >>>>> landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
> >>>>> sizeof(attr_enforce), &attr_enforce);
> >>>>>
> >>>>> At this point, the process is not supposed to be able to write to
> >>>>> anything outside /tmp/foobar, right? But what happens if the process
> >>>>> does the following next?
> >>>>>
> >>>>> struct landlock_attr_ruleset ruleset = { .handled_access_fs =
> >>>>> ACCESS_FS_ROUGHLY_WRITE};
> >>>>> int ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
> >>>>> LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
> >>>>> struct landlock_attr_path_beneath path_beneath = {
> >>>>> .ruleset_fd = ruleset_fd,
> >>>>> .allowed_access = ACCESS_FS_ROUGHLY_WRITE,
> >>>>> .parent_fd = open("/", O_PATH),
> >>>>> };
> >>>>> landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
> >>>>> sizeof(path_beneath), &path_beneath);
> >>>>> prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> >>>>> struct landlock_attr_enforce attr_enforce = { .ruleset_fd = ruleset_fd };
> >>>>> landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
> >>>>> sizeof(attr_enforce), &attr_enforce);
> >>>>>
> >>>>> As far as I can tell from looking at the source, after this, you will
> >>>>> have write access to the entire filesystem again. I think the idea is
> >>>>> that LANDLOCK_CMD_ENFORCE_RULESET should only let you drop privileges,
> >>>>> not increase them, right?
> >>>>
> >>>> There is an additionnal check in syscall.c:get_path_from_fd(): it is
> >>>> forbidden to add a rule with a path which is not accessible (according
> >>>> to LANDLOCK_ACCESS_FS_OPEN) thanks to a call to security_file_open(),
> >>>> but this is definitely not perfect.
> >>>
> >>> Ah, I missed that.
> >>>
> >>>>> I think the easy way to fix this would be to add a bitmask to each
> >>>>> rule that says from which ruleset it originally comes, and then let
> >>>>> check_access_path() collect these bitmasks from each rule with OR, and
> >>>>> check at the end whether the resulting bitmask is full - if not, at
> >>>>> least one of the rulesets did not permit the access, and it should be
> >>>>> denied.
> >>>>>
> >>>>> But maybe it would make more sense to change how the API works
> >>>>> instead, and get rid of the concept of "merging" two rulesets
> >>>>> together? Instead, we could make the API work like this:
> >>>>>
> >>>>> - LANDLOCK_CMD_CREATE_RULESET gives you a file descriptor whose
> >>>>> ->private_data contains a pointer to the old ruleset of the process,
> >>>>> as well as a pointer to a new empty ruleset.
> >>>>> - LANDLOCK_CMD_ADD_RULE fails if the specified rule would not be
> >>>>> permitted by the old ruleset, then adds the rule to the new ruleset
> >>>>> - LANDLOCK_CMD_ENFORCE_RULESET fails if the old ruleset pointer in
> >>>>> ->private_data doesn't match the current ruleset of the process, then
> >>>>> replaces the old ruleset with the new ruleset.
> >>>>>
> >>>>> With this, the new ruleset is guaranteed to be a subset of the old
> >>>>> ruleset because each of the new ruleset's rules is permitted by the
> >>>>> old ruleset. (Unless the directory hierarchy rotates, but in that case
> >>>>> the inaccuracy isn't much worse than what would've been possible
> >>>>> through RCU path walk anyway AFAIK.)
> >>>>>
> >>>>> What do you think?
> >>>>>
> >>>>
> >>>> I would prefer to add the same checks you described at first (with
> >>>> check_access_path), but only when creating a new ruleset with
> >>>> merge_ruleset() (which should probably be renamed). This enables not to
> >>>> rely on a parent ruleset/domain until the enforcement, which is the case
> >>>> anyway.
> >>>> Unfortunately this doesn't work for some cases with bind mounts. Because
> >>>> check_access_path() goes through one path, another (bind mounted) path
> >>>> could be illegitimately allowed.
> >>>
> >>> Hmm... I'm not sure what you mean. At the moment, landlock doesn't
> >>> allow any sandboxed process to change the mount hierarchy, right? Can
> >>> you give an example where this would go wrong?
> >>
> >> Indeed, a Landlocked process must no be able to change its mount
> >> namespace layout. However, bind mounts may already exist.
> >> Let's say a process sandbox itself to only access /a in a read-write
> >> way.
> >
> > So, first policy:
> >
> > /a RW
> >
> >> Then, this process (or one of its children) add a new restriction
> >> on /a/b to only be able to read this hierarchy.
> >
> > You mean with the second policy looking like this?
>
> Right.
>
> >
> > /a RW
> > /a/b R
> >
> > Then the resulting policy would be:
> >
> > /a RW policy_bitmask=0x00000003 (bits 0 and 1 set)
> > /a/b R policy_bitmask=0x00000002 (bit 1 set)
> > required_bits=0x00000003 (bits 0 and 1 set)
> >
> >> The check at insertion
> >> time would allow this because this access right is a subset of the
> >> access right allowed with the parent directory. However, If /a/b is bind
> >> mounted somewhere else, let's say in /private/b, then the second
> >> enforcement just gave new access rights to this hierarchy too.
> >
> > But with the solution I proposed, landlock's path walk would see
> > something like this when accessing a file at /private/b/foo:
> > /private/b/foo <no rules>
> > policies seen until now: 0x00000000
> > /private/b <access: R, policy_bitmask=0x00000002>
> > policies seen until now: 0x00000002
> > /private <no rules>
> > policies seen until now: 0x00000002
> > / <no rules>
> > policies seen until now: 0x00000002
> >
> > It wouldn't encounter any rule from the first policy, so the OR of the
> > seen policy bitmasks would be 0x00000002, which is not the required
> > value 0x00000003, and so the access would be denied.
> As I understand your proposition, we need to build the required_bits
> when adding a rule or enforcing/merging a ruleset with a domain. The
> issue is that a rule only refers to a struct inode, not a struct path.
> For your proposition to work, we would need to walk through the file
> path when adding a rule to a ruleset, which means that we need to depend
> of the current view of the process (i.e. its mount namespace), and its
> Landlock domain.
I don't see why that is necessary. Why would we have to walk the file
path when adding a rule?
> If the required_bits field is set when the ruleset is
> merged with the domain, it is not possible anymore to walk through the
> corresponding initial file path, which makes the enforcement step too
> late to check for such consistency. The important point is that a
> ruleset/domain doesn't have a notion of file hierarchy, a ruleset is
> only a set of tagged inodes.
>
> I'm not sure I got your proposition right, though. When and how would
> you generate the required_bits?
Using your terminology:
A domain is a collection of N layers, which are assigned indices 0..N-1.
For each possible access type, a domain has a bitmask containing N
bits that stores which layers control that access type. (Basically a
per-layer version of fs_access_mask.)
To validate an access, you start by ORing together the bitmasks for
the requested access types; that gives you the required_bits mask,
which lists all layers that want to control the access.
Then you set seen_policy_bits=0, then do the
check_access_path_continue() loop while keeping track of which layers
you've seen with "seen_policy_bits |= access->contributing_policies",
or something like that.
And in the end, you check that seen_policy_bits is a superset of
required_bits - something like `(~seen_policy_bits) & required_bits ==
0`.
AFAICS to create a new domain from a bunch of layers, you wouldn't
have to do any path walking.
> Here is my updated proposition: add a layer level and a depth to each
> rule (once enforced/merged with a domain), and a top layer level for a
> domain. When enforcing a ruleset (i.e. merging a ruleset into the
> current domain), the layer level of a new rule would be the incremented
> top layer level.
> If there is no rule (from this domain) tied to the same
> inode, then the depth of the new rule is 1. However, if there is already
> a rule tied to the same inode and if this rule's layer level is the
> previous top layer level, then the depth and the layer level are both
> incremented and the rule is updated with the new access rights (boolean
> AND).
>
> The policy looks like this:
> domain top_layer=2
> /a RW policy_bitmask=0x00000003 layer=1 depth=1
> /a/b R policy_bitmask=0x00000002 layer=2 depth=1
>
> The path walk access check walks through all inodes and start with a
> layer counter equal to the top layer of the current domain. For each
> encountered inode tied to a rule, the access rights are checked and a
> new check ensures that the layer of the matching rule is the same as the
> counter (this may be a merged ruleset containing rules pertaining to the
> same hierarchy, which is fine) or equal to the decremented counter (i.e.
> the path walk just reached the underlying layer). If the path walk
> encounter a rule with a layer strictly less than the counter minus one,
> there is a whole in the layers which means that the ruleset
> hierarchy/subset does not match, and the access must be denied.
>
> When accessing a file at /private/b/foo for a read access:
> /private/b/foo <no rules>
> allowed_access=unknown layer_counter=2
> /private/b <access: R, policy_bitmask=0x00000002, layer=2, depth=1>
> allowed_access=allowed layer_counter=2
> /private <no rules>
> allowed_access=allowed layer_counter=2
> / <no rules>
> allowed_access=allowed layer_counter=2
>
> Because the layer_counter didn't reach 1, the access request is then denied.
>
> This proposition enables not to rely on a parent ruleset at first, only
> when enforcing/merging a ruleset with a domain. This also solves the
> issue with multiple inherited/nested rules on the same inode (in which
> case the depth just grows). Moreover, this enables to safely stop the
> path walk as soon as we reach the layer 1.
(FWIW, you could do the same optimization with the seen_policy_bits approach.)
I guess the difference between your proposal and mine is that in my
proposal, the following would work, in effect permitting W access to
/foo/bar/baz (and nothing else)?
first ruleset:
/foo W
second ruleset:
/foo/bar/baz W
third ruleset:
/foo/bar W
whereas in your proposal, IIUC it wouldn't be valid for a new ruleset
to whitelist a superset of what was whitelisted in a previous ruleset?
^ permalink raw reply
* [PATCH v5 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-18 22:14 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200318221457.1330-1-longman@redhat.com>
By allocating a kernel buffer with a user-supplied buffer length, it
is possible that a false positive ENOMEM error may be returned because
the user-supplied length is just too large even if the system do have
enough memory to hold the actual key data.
Moreover, if the buffer length is larger than the maximum amount of
memory that can be returned by kmalloc() (2^(MAX_ORDER-1) number of
pages), a warning message will also be printed.
To reduce this possibility, we set a threshold (page size) over which we
do check the actual key length first before allocating a buffer of the
right size to hold it. The threshold is arbitrary, it is just used to
trigger a buffer length check. It does not limit the actual key length
as long as there is enough memory to satisfy the memory request.
To further avoid large buffer allocation failure due to page
fragmentation, kvmalloc() is used to allocate the buffer so that vmapped
pages can be used when there is not a large enough contiguous set of
pages available for allocation.
Signed-off-by: Waiman Long <longman@redhat.com>
---
security/keys/internal.h | 12 ++++++++++++
security/keys/keyctl.c | 41 ++++++++++++++++++++++++++++++++--------
2 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/security/keys/internal.h b/security/keys/internal.h
index ba3e2da14cef..6d0ca48ae9a5 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -16,6 +16,8 @@
#include <linux/keyctl.h>
#include <linux/refcount.h>
#include <linux/compat.h>
+#include <linux/mm.h>
+#include <linux/vmalloc.h>
struct iovec;
@@ -349,4 +351,14 @@ static inline void key_check(const struct key *key)
#endif
+/*
+ * Helper function to clear and free a kvmalloc'ed memory object.
+ */
+static inline void __kvzfree(const void *addr, size_t len)
+{
+ if (addr) {
+ memset((void *)addr, 0, len);
+ kvfree(addr);
+ }
+}
#endif /* _INTERNAL_H */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 81f68e434b9f..07eaa46d344c 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -339,7 +339,7 @@ long keyctl_update_key(key_serial_t id,
payload = NULL;
if (plen) {
ret = -ENOMEM;
- payload = kmalloc(plen, GFP_KERNEL);
+ payload = kvmalloc(plen, GFP_KERNEL);
if (!payload)
goto error;
@@ -360,7 +360,7 @@ long keyctl_update_key(key_serial_t id,
key_ref_put(key_ref);
error2:
- kzfree(payload);
+ __kvzfree(payload, plen);
error:
return ret;
}
@@ -877,13 +877,24 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
* transferring them to user buffer to avoid potential
* deadlock involving page fault and mmap_sem.
*/
- char *tmpbuf = kmalloc(buflen, GFP_KERNEL);
+ char *tmpbuf = NULL;
+ size_t tmpbuflen = buflen;
- if (!tmpbuf) {
- ret = -ENOMEM;
- goto error2;
+ /*
+ * To prevent memory allocation failure with an arbitrary
+ * large user-supplied buflen, we do a key length check
+ * before allocating a buffer of the right size to hold
+ * key data if it exceeds a threshold (PAGE_SIZE).
+ */
+ if (buflen <= PAGE_SIZE) {
+allocbuf:
+ tmpbuf = kvmalloc(tmpbuflen, GFP_KERNEL);
+ if (!tmpbuf) {
+ ret = -ENOMEM;
+ goto error2;
+ }
}
- ret = __keyctl_read_key(key, tmpbuf, buflen);
+ ret = __keyctl_read_key(key, tmpbuf, tmpbuflen);
/*
* Read methods will just return the required length
@@ -891,10 +902,24 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
* enough.
*/
if ((ret > 0) && (ret <= buflen)) {
+ /*
+ * It is possible, though unlikely, that the key
+ * changes in between the up_read->down_read period.
+ * If the key becomes longer, we will have to
+ * allocate a larger buffer and redo the key read
+ * again.
+ */
+ if (!tmpbuf || unlikely(ret > tmpbuflen)) {
+ if (unlikely(tmpbuf))
+ __kvzfree(tmpbuf, tmpbuflen);
+ tmpbuflen = ret;
+ goto allocbuf;
+ }
+
if (copy_to_user(buffer, tmpbuf, ret))
ret = -EFAULT;
}
- kzfree(tmpbuf);
+ __kvzfree(tmpbuf, tmpbuflen);
}
error2:
--
2.18.1
^ permalink raw reply related
* [PATCH v5 1/2] KEYS: Don't write out to userspace while holding key semaphore
From: Waiman Long @ 2020-03-18 22:14 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
In-Reply-To: <20200318221457.1330-1-longman@redhat.com>
A lockdep circular locking dependency report was seen when running a
keyutils test:
[12537.027242] ======================================================
[12537.059309] WARNING: possible circular locking dependency detected
[12537.088148] 4.18.0-147.7.1.el8_1.x86_64+debug #1 Tainted: G OE --------- - -
[12537.125253] ------------------------------------------------------
[12537.153189] keyctl/25598 is trying to acquire lock:
[12537.175087] 000000007c39f96c (&mm->mmap_sem){++++}, at: __might_fault+0xc4/0x1b0
[12537.208365]
[12537.208365] but task is already holding lock:
[12537.234507] 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12537.270476]
[12537.270476] which lock already depends on the new lock.
[12537.270476]
[12537.307209]
[12537.307209] the existing dependency chain (in reverse order) is:
[12537.340754]
[12537.340754] -> #3 (&type->lock_class){++++}:
[12537.367434] down_write+0x4d/0x110
[12537.385202] __key_link_begin+0x87/0x280
[12537.405232] request_key_and_link+0x483/0xf70
[12537.427221] request_key+0x3c/0x80
[12537.444839] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.468445] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.496731] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.519418] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.546263] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.573551] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.601045] kthread+0x30c/0x3d0
[12537.617906] ret_from_fork+0x3a/0x50
[12537.636225]
[12537.636225] -> #2 (root_key_user.cons_lock){+.+.}:
[12537.664525] __mutex_lock+0x105/0x11f0
[12537.683734] request_key_and_link+0x35a/0xf70
[12537.705640] request_key+0x3c/0x80
[12537.723304] dns_query+0x1db/0x5a5 [dns_resolver]
[12537.746773] dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.775607] cifs_reconnect+0xe04/0x2500 [cifs]
[12537.798322] cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.823369] cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.847262] cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.873477] kthread+0x30c/0x3d0
[12537.890281] ret_from_fork+0x3a/0x50
[12537.908649]
[12537.908649] -> #1 (&tcp_ses->srv_mutex){+.+.}:
[12537.935225] __mutex_lock+0x105/0x11f0
[12537.954450] cifs_call_async+0x102/0x7f0 [cifs]
[12537.977250] smb2_async_readv+0x6c3/0xc90 [cifs]
[12538.000659] cifs_readpages+0x120a/0x1e50 [cifs]
[12538.023920] read_pages+0xf5/0x560
[12538.041583] __do_page_cache_readahead+0x41d/0x4b0
[12538.067047] ondemand_readahead+0x44c/0xc10
[12538.092069] filemap_fault+0xec1/0x1830
[12538.111637] __do_fault+0x82/0x260
[12538.129216] do_fault+0x419/0xfb0
[12538.146390] __handle_mm_fault+0x862/0xdf0
[12538.167408] handle_mm_fault+0x154/0x550
[12538.187401] __do_page_fault+0x42f/0xa60
[12538.207395] do_page_fault+0x38/0x5e0
[12538.225777] page_fault+0x1e/0x30
[12538.243010]
[12538.243010] -> #0 (&mm->mmap_sem){++++}:
[12538.267875] lock_acquire+0x14c/0x420
[12538.286848] __might_fault+0x119/0x1b0
[12538.306006] keyring_read_iterator+0x7e/0x170
[12538.327936] assoc_array_subtree_iterate+0x97/0x280
[12538.352154] keyring_read+0xe9/0x110
[12538.370558] keyctl_read_key+0x1b9/0x220
[12538.391470] do_syscall_64+0xa5/0x4b0
[12538.410511] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
[12538.435535]
[12538.435535] other info that might help us debug this:
[12538.435535]
[12538.472829] Chain exists of:
[12538.472829] &mm->mmap_sem --> root_key_user.cons_lock --> &type->lock_class
[12538.472829]
[12538.524820] Possible unsafe locking scenario:
[12538.524820]
[12538.551431] CPU0 CPU1
[12538.572654] ---- ----
[12538.595865] lock(&type->lock_class);
[12538.613737] lock(root_key_user.cons_lock);
[12538.644234] lock(&type->lock_class);
[12538.672410] lock(&mm->mmap_sem);
[12538.687758]
[12538.687758] *** DEADLOCK ***
[12538.687758]
[12538.714455] 1 lock held by keyctl/25598:
[12538.732097] #0: 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12538.770573]
[12538.770573] stack backtrace:
[12538.790136] CPU: 2 PID: 25598 Comm: keyctl Kdump: loaded Tainted: G
[12538.844855] Hardware name: HP ProLiant DL360 Gen9/ProLiant DL360 Gen9, BIOS P89 12/27/2015
[12538.881963] Call Trace:
[12538.892897] dump_stack+0x9a/0xf0
[12538.907908] print_circular_bug.isra.25.cold.50+0x1bc/0x279
[12538.932891] ? save_trace+0xd6/0x250
[12538.948979] check_prev_add.constprop.32+0xc36/0x14f0
[12538.971643] ? keyring_compare_object+0x104/0x190
[12538.992738] ? check_usage+0x550/0x550
[12539.009845] ? sched_clock+0x5/0x10
[12539.025484] ? sched_clock_cpu+0x18/0x1e0
[12539.043555] __lock_acquire+0x1f12/0x38d0
[12539.061551] ? trace_hardirqs_on+0x10/0x10
[12539.080554] lock_acquire+0x14c/0x420
[12539.100330] ? __might_fault+0xc4/0x1b0
[12539.119079] __might_fault+0x119/0x1b0
[12539.135869] ? __might_fault+0xc4/0x1b0
[12539.153234] keyring_read_iterator+0x7e/0x170
[12539.172787] ? keyring_read+0x110/0x110
[12539.190059] assoc_array_subtree_iterate+0x97/0x280
[12539.211526] keyring_read+0xe9/0x110
[12539.227561] ? keyring_gc_check_iterator+0xc0/0xc0
[12539.249076] keyctl_read_key+0x1b9/0x220
[12539.266660] do_syscall_64+0xa5/0x4b0
[12539.283091] entry_SYSCALL_64_after_hwframe+0x6a/0xdf
One way to prevent this deadlock scenario from happening is to not
allow writing to userspace while holding the key semaphore. Instead,
an internal buffer is allocated for getting the keys out from the
read method first before copying them out to userspace without holding
the lock.
That requires taking out the __user modifier from all the relevant
read methods as well as additional changes to not use any userspace
write helpers. That is,
1) The put_user() call is replaced by a direct copy.
2) The copy_to_user() call is replaced by memcpy().
3) All the fault handling code is removed.
Compiling on a x86-64 system, the size of the rxrpc_read() function is
reduced from 3795 bytes to 2384 bytes with this patch.
Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
include/keys/big_key-type.h | 2 +-
include/keys/user-type.h | 3 +-
include/linux/key-type.h | 2 +-
net/dns_resolver/dns_key.c | 2 +-
net/rxrpc/key.c | 27 ++++-------
security/keys/big_key.c | 11 ++---
security/keys/encrypted-keys/encrypted.c | 7 ++-
security/keys/keyctl.c | 57 +++++++++++++++++++----
security/keys/keyring.c | 6 +--
security/keys/request_key_auth.c | 7 ++-
security/keys/trusted-keys/trusted_tpm1.c | 14 +-----
security/keys/user_defined.c | 5 +-
12 files changed, 77 insertions(+), 66 deletions(-)
diff --git a/include/keys/big_key-type.h b/include/keys/big_key-type.h
index f6a7ba4dccd4..3fee04f81439 100644
--- a/include/keys/big_key-type.h
+++ b/include/keys/big_key-type.h
@@ -17,6 +17,6 @@ extern void big_key_free_preparse(struct key_preparsed_payload *prep);
extern void big_key_revoke(struct key *key);
extern void big_key_destroy(struct key *key);
extern void big_key_describe(const struct key *big_key, struct seq_file *m);
-extern long big_key_read(const struct key *key, char __user *buffer, size_t buflen);
+extern long big_key_read(const struct key *key, char *buffer, size_t buflen);
#endif /* _KEYS_BIG_KEY_TYPE_H */
diff --git a/include/keys/user-type.h b/include/keys/user-type.h
index d5e73266a81a..be61fcddc02a 100644
--- a/include/keys/user-type.h
+++ b/include/keys/user-type.h
@@ -41,8 +41,7 @@ extern int user_update(struct key *key, struct key_preparsed_payload *prep);
extern void user_revoke(struct key *key);
extern void user_destroy(struct key *key);
extern void user_describe(const struct key *user, struct seq_file *m);
-extern long user_read(const struct key *key,
- char __user *buffer, size_t buflen);
+extern long user_read(const struct key *key, char *buffer, size_t buflen);
static inline const struct user_key_payload *user_key_payload_rcu(const struct key *key)
{
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index 4ded94bcf274..2ab2d6d6aeab 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -127,7 +127,7 @@ struct key_type {
* much is copied into the buffer
* - shouldn't do the copy if the buffer is NULL
*/
- long (*read)(const struct key *key, char __user *buffer, size_t buflen);
+ long (*read)(const struct key *key, char *buffer, size_t buflen);
/* handle request_key() for this type instead of invoking
* /sbin/request-key (optional)
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index 3e1a90669006..ad53eb31d40f 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -302,7 +302,7 @@ static void dns_resolver_describe(const struct key *key, struct seq_file *m)
* - the key's semaphore is read-locked
*/
static long dns_resolver_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
int err = PTR_ERR(key->payload.data[dns_key_error]);
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 6c3f35fac42d..0c98313dd7a8 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -31,7 +31,7 @@ static void rxrpc_free_preparse_s(struct key_preparsed_payload *);
static void rxrpc_destroy(struct key *);
static void rxrpc_destroy_s(struct key *);
static void rxrpc_describe(const struct key *, struct seq_file *);
-static long rxrpc_read(const struct key *, char __user *, size_t);
+static long rxrpc_read(const struct key *, char *, size_t);
/*
* rxrpc defined keys take an arbitrary string as the description and an
@@ -1042,12 +1042,12 @@ EXPORT_SYMBOL(rxrpc_get_null_key);
* - this returns the result in XDR form
*/
static long rxrpc_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
const struct rxrpc_key_token *token;
const struct krb5_principal *princ;
size_t size;
- __be32 __user *xdr, *oldxdr;
+ __be32 *xdr, *oldxdr;
u32 cnlen, toksize, ntoks, tok, zero;
u16 toksizes[AFSTOKEN_MAX];
int loop;
@@ -1124,30 +1124,25 @@ static long rxrpc_read(const struct key *key,
if (!buffer || buflen < size)
return size;
- xdr = (__be32 __user *) buffer;
+ xdr = (__be32 *)buffer;
zero = 0;
#define ENCODE(x) \
do { \
- __be32 y = htonl(x); \
- if (put_user(y, xdr++) < 0) \
- goto fault; \
+ *xdr++ = htonl(x); \
} while(0)
#define ENCODE_DATA(l, s) \
do { \
u32 _l = (l); \
ENCODE(l); \
- if (copy_to_user(xdr, (s), _l) != 0) \
- goto fault; \
- if (_l & 3 && \
- copy_to_user((u8 __user *)xdr + _l, &zero, 4 - (_l & 3)) != 0) \
- goto fault; \
+ memcpy(xdr, (s), _l); \
+ if (_l & 3) \
+ memcpy((u8 *)xdr + _l, &zero, 4 - (_l & 3)); \
xdr += (_l + 3) >> 2; \
} while(0)
#define ENCODE64(x) \
do { \
__be64 y = cpu_to_be64(x); \
- if (copy_to_user(xdr, &y, 8) != 0) \
- goto fault; \
+ memcpy(xdr, &y, 8); \
xdr += 8 >> 2; \
} while(0)
#define ENCODE_STR(s) \
@@ -1238,8 +1233,4 @@ static long rxrpc_read(const struct key *key,
ASSERTCMP((char __user *) xdr - buffer, ==, size);
_leave(" = %zu", size);
return size;
-
-fault:
- _leave(" = -EFAULT");
- return -EFAULT;
}
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 001abe530a0d..82008f900930 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -352,7 +352,7 @@ void big_key_describe(const struct key *key, struct seq_file *m)
* read the key data
* - the key's semaphore is read-locked
*/
-long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
+long big_key_read(const struct key *key, char *buffer, size_t buflen)
{
size_t datalen = (size_t)key->payload.data[big_key_len];
long ret;
@@ -391,9 +391,8 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
ret = datalen;
- /* copy decrypted data to user */
- if (copy_to_user(buffer, buf->virt, datalen) != 0)
- ret = -EFAULT;
+ /* copy out decrypted data */
+ memcpy(buffer, buf->virt, datalen);
err_fput:
fput(file);
@@ -401,9 +400,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
big_key_free_buffer(buf);
} else {
ret = datalen;
- if (copy_to_user(buffer, key->payload.data[big_key_data],
- datalen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, key->payload.data[big_key_data], datalen);
}
return ret;
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 60720f58cbe0..f6797ba44bf7 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -902,14 +902,14 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
}
/*
- * encrypted_read - format and copy the encrypted data to userspace
+ * encrypted_read - format and copy out the encrypted data
*
* The resulting datablob format is:
* <master-key name> <decrypted data length> <encrypted iv> <encrypted data>
*
* On success, return to userspace the encrypted key datablob size.
*/
-static long encrypted_read(const struct key *key, char __user *buffer,
+static long encrypted_read(const struct key *key, char *buffer,
size_t buflen)
{
struct encrypted_key_payload *epayload;
@@ -957,8 +957,7 @@ static long encrypted_read(const struct key *key, char __user *buffer,
key_put(mkey);
memzero_explicit(derived_key, sizeof(derived_key));
- if (copy_to_user(buffer, ascii_buf, asciiblob_len) != 0)
- ret = -EFAULT;
+ memcpy(buffer, ascii_buf, asciiblob_len);
kzfree(ascii_buf);
return asciiblob_len;
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 9b898c969558..81f68e434b9f 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -797,6 +797,21 @@ long keyctl_keyring_search(key_serial_t ringid,
return ret;
}
+/*
+ * Call the read method
+ */
+static long __keyctl_read_key(struct key *key, char *buffer, size_t buflen)
+{
+ long ret;
+
+ down_read(&key->sem);
+ ret = key_validate(key);
+ if (ret == 0)
+ ret = key->type->read(key, buffer, buflen);
+ up_read(&key->sem);
+ return ret;
+}
+
/*
* Read a key's payload.
*
@@ -844,16 +859,42 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
/* the key is probably readable - now try to read it */
can_read_key:
- ret = -EOPNOTSUPP;
- if (key->type->read) {
- /* Read the data with the semaphore held (since we might sleep)
+ if (!key->type->read) {
+ ret = -EOPNOTSUPP;
+ goto error2;
+ }
+
+ if (!buffer || !buflen) {
+ /* Get the key length from the read method */
+ ret = __keyctl_read_key(key, NULL, 0);
+ } else {
+
+ /*
+ * Read the data with the semaphore held (since we might sleep)
* to protect against the key being updated or revoked.
+ *
+ * Allocating a temporary buffer to hold the keys before
+ * transferring them to user buffer to avoid potential
+ * deadlock involving page fault and mmap_sem.
+ */
+ char *tmpbuf = kmalloc(buflen, GFP_KERNEL);
+
+ if (!tmpbuf) {
+ ret = -ENOMEM;
+ goto error2;
+ }
+ ret = __keyctl_read_key(key, tmpbuf, buflen);
+
+ /*
+ * Read methods will just return the required length
+ * without any copying if the provided length isn't big
+ * enough.
*/
- down_read(&key->sem);
- ret = key_validate(key);
- if (ret == 0)
- ret = key->type->read(key, buffer, buflen);
- up_read(&key->sem);
+ if ((ret > 0) && (ret <= buflen)) {
+ if (copy_to_user(buffer, tmpbuf, ret))
+ ret = -EFAULT;
+ }
+ kzfree(tmpbuf);
}
error2:
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index febf36c6ddc5..5ca620d31cd3 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -459,7 +459,6 @@ static int keyring_read_iterator(const void *object, void *data)
{
struct keyring_read_iterator_context *ctx = data;
const struct key *key = keyring_ptr_to_key(object);
- int ret;
kenter("{%s,%d},,{%zu/%zu}",
key->type->name, key->serial, ctx->count, ctx->buflen);
@@ -467,10 +466,7 @@ static int keyring_read_iterator(const void *object, void *data)
if (ctx->count >= ctx->buflen)
return 1;
- ret = put_user(key->serial, ctx->buffer);
- if (ret < 0)
- return ret;
- ctx->buffer++;
+ *ctx->buffer++ = key->serial;
ctx->count += sizeof(key->serial);
return 0;
}
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index ecba39c93fd9..41e9735006d0 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -22,7 +22,7 @@ static int request_key_auth_instantiate(struct key *,
static void request_key_auth_describe(const struct key *, struct seq_file *);
static void request_key_auth_revoke(struct key *);
static void request_key_auth_destroy(struct key *);
-static long request_key_auth_read(const struct key *, char __user *, size_t);
+static long request_key_auth_read(const struct key *, char *, size_t);
/*
* The request-key authorisation key type definition.
@@ -80,7 +80,7 @@ static void request_key_auth_describe(const struct key *key,
* - the key's semaphore is read-locked
*/
static long request_key_auth_read(const struct key *key,
- char __user *buffer, size_t buflen)
+ char *buffer, size_t buflen)
{
struct request_key_auth *rka = dereference_key_locked(key);
size_t datalen;
@@ -97,8 +97,7 @@ static long request_key_auth_read(const struct key *key,
if (buflen > datalen)
buflen = datalen;
- if (copy_to_user(buffer, rka->callout_info, buflen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, rka->callout_info, buflen);
}
return ret;
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index d2c5ec1e040b..8001ab07e63b 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -1130,11 +1130,10 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
* trusted_read - copy the sealed blob data to userspace in hex.
* On success, return to userspace the trusted key datablob size.
*/
-static long trusted_read(const struct key *key, char __user *buffer,
+static long trusted_read(const struct key *key, char *buffer,
size_t buflen)
{
const struct trusted_key_payload *p;
- char *ascii_buf;
char *bufp;
int i;
@@ -1143,18 +1142,9 @@ static long trusted_read(const struct key *key, char __user *buffer,
return -EINVAL;
if (buffer && buflen >= 2 * p->blob_len) {
- ascii_buf = kmalloc_array(2, p->blob_len, GFP_KERNEL);
- if (!ascii_buf)
- return -ENOMEM;
-
- bufp = ascii_buf;
+ bufp = buffer;
for (i = 0; i < p->blob_len; i++)
bufp = hex_byte_pack(bufp, p->blob[i]);
- if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
- kzfree(ascii_buf);
- return -EFAULT;
- }
- kzfree(ascii_buf);
}
return 2 * p->blob_len;
}
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 6f12de4ce549..07d4287e9084 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -168,7 +168,7 @@ EXPORT_SYMBOL_GPL(user_describe);
* read the key data
* - the key's semaphore is read-locked
*/
-long user_read(const struct key *key, char __user *buffer, size_t buflen)
+long user_read(const struct key *key, char *buffer, size_t buflen)
{
const struct user_key_payload *upayload;
long ret;
@@ -181,8 +181,7 @@ long user_read(const struct key *key, char __user *buffer, size_t buflen)
if (buflen > upayload->datalen)
buflen = upayload->datalen;
- if (copy_to_user(buffer, upayload->data, buflen) != 0)
- ret = -EFAULT;
+ memcpy(buffer, upayload->data, buflen);
}
return ret;
--
2.18.1
^ permalink raw reply related
* [PATCH v5 0/2] KEYS: Read keys to internal buffer & then copy to userspace
From: Waiman Long @ 2020-03-18 22:14 UTC (permalink / raw)
To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, David S. Miller, Jakub Kicinski
Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen, Waiman Long
v5:
- Merge v4 patches 2 and 3 into 1 to avoid sparse warning. Merge some of
commit logs into patch 1 as well. There is no further change.
v4:
- Remove the __user annotation from big_key_read() and user_read() in
patch 1.
- Add a new patch 2 to remove __user annotation from rxrpc_read().
- Add a new patch 3 to remove __user annotation from dns_resolver_read().
- Merge the original patches 2 and 3 into a single patch 4 and refactor
it as suggested by Jarkko and Eric.
v3:
- Reorganize the keyctl_read_key() code to make it more readable as
suggested by Jarkko Sakkinen.
- Add patch 3 to use kvmalloc() for safer large buffer allocation as
suggested by David Howells.
v2:
- Handle NULL buffer and buflen properly in patch 1.
- Fix a bug in big_key.c.
- Add patch 2 to handle arbitrary large user-supplied buflen.
The current security key read methods are called with the key semaphore
held. The methods then copy out the key data to userspace which is
subjected to page fault and may acquire the mmap semaphore. That can
result in circular lock dependency and hence a chance to get into
deadlock.
To avoid such a deadlock, an internal buffer is now allocated for getting
out the necessary data first. After releasing the key semaphore, the
key data are then copied out to userspace sidestepping the circular
lock dependency.
The keyutils test suite was run and the test passed with these patchset
applied without any falure.
Waiman Long (2):
KEYS: Don't write out to userspace while holding key semaphore
KEYS: Avoid false positive ENOMEM error on key read
include/keys/big_key-type.h | 2 +-
include/keys/user-type.h | 3 +-
include/linux/key-type.h | 2 +-
net/dns_resolver/dns_key.c | 2 +-
net/rxrpc/key.c | 27 +++----
security/keys/big_key.c | 11 ++-
security/keys/encrypted-keys/encrypted.c | 7 +-
security/keys/internal.h | 12 ++++
security/keys/keyctl.c | 86 ++++++++++++++++++++---
security/keys/keyring.c | 6 +-
security/keys/request_key_auth.c | 7 +-
security/keys/trusted-keys/trusted_tpm1.c | 14 +---
security/keys/user_defined.c | 5 +-
13 files changed, 116 insertions(+), 68 deletions(-)
--
2.18.1
^ permalink raw reply
* Re: [PATCH v3 7/8] ima: Calculate and extend PCR with digests in ima_template_entry
From: Mimi Zohar @ 2020-03-18 21:54 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <fecf59c1880045769bfecc17b5670ac5@huawei.com>
On Wed, 2020-03-18 at 12:42 +0000, Roberto Sassu wrote:
> > -----Original Message-----
> > From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux-
> > security-module@vger.kernel.org] On Behalf Of Mimi Zohar
> > Sent: Tuesday, March 3, 2020 5:04 AM
> > To: Roberto Sassu <roberto.sassu@huawei.com>;
> > James.Bottomley@HansenPartnership.com;
> > jarkko.sakkinen@linux.intel.com
> > Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Silviu Vlasceanu
> > <Silviu.Vlasceanu@huawei.com>
> > Subject: Re: [PATCH v3 7/8] ima: Calculate and extend PCR with digests in
> > ima_template_entry
> >
> > On Mon, 2020-02-10 at 11:04 +0100, Roberto Sassu wrote:
> >
> > > @@ -219,6 +214,8 @@ int ima_restore_measurement_entry(struct
> > ima_template_entry *entry)
> > >
> > > int __init ima_init_digests(void)
> > > {
> > > + u16 digest_size;
> > > + u16 crypto_id;
> > > int i;
> > >
> > > if (!ima_tpm_chip)
> > > @@ -229,8 +226,17 @@ int __init ima_init_digests(void)
> > > if (!digests)
> > > return -ENOMEM;
> > >
> > > - for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++)
> > > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> > > digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
> > > + digest_size = ima_tpm_chip->allocated_banks[i].digest_size;
> > > + crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
> > > +
> > > + /* for unmapped TPM algorithms digest is still a padded
> > SHA1 */
> > > + if (crypto_id == HASH_ALGO__LAST)
> > > + digest_size = SHA1_DIGEST_SIZE;
> > > +
> > > + memset(digests[i].digest, 0xff, digest_size);
> >
> > Shouldn't the memset here be of the actual digest size even for
> > unmapped TPM algorithms.
>
> This is consistent with ima_calc_field_array_hash(), so that a verifier
> will always pad the SHA1 digest with zeros to obtain the final PCR value.
>
> I can set all bytes if you prefer.
My concern is with violations. The measurement list will be padded
with 0's, but the value being extended into the TPM will only
partially be 0xFF's. When verifying the measurement list, replacing
all 0x00's with all 0xFF's is simpler.
Mimi
^ permalink raw reply
* Re: [PATCH 12/17] watch_queue: Add security hooks to rule on setting mount and sb watches [ver #5]
From: James Morris @ 2020-03-18 19:07 UTC (permalink / raw)
To: David Howells
Cc: torvalds, viro, Casey Schaufler, Stephen Smalley,
linux-security-module, Casey Schaufler, Stephen Smalley,
nicolas.dichtel, raven, christian, andres, jlayton, dray, kzak,
keyrings, linux-api, linux-fsdevel, linux-security-module,
linux-kernel
In-Reply-To: <158454390389.2863966.16459187265972715098.stgit@warthog.procyon.org.uk>
On Wed, 18 Mar 2020, David Howells wrote:
> Add security hooks that will allow an LSM to rule on whether or not a watch
> may be set on a mount or on a superblock. More than one hook is required
> as the watches watch different types of object.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Casey Schaufler <casey@schaufler-ca.com>
> cc: Stephen Smalley <sds@tycho.nsa.gov>
> cc: linux-security-module@vger.kernel.org
Acked-by: James Morris <jamorris@linux.microsoft.com>
--
James Morris
<jmorris@namei.org>
^ 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