* Re: [PATCH v6 0/7] capabilities: Introduce CAP_CHECKPOINT_RESTORE
From: Christian Brauner @ 2020-07-19 18:17 UTC (permalink / raw)
To: Adrian Reber, Nicolas Viennot
Cc: Eric Biederman, Pavel Emelyanov, Oleg Nesterov, Dmitry Safonov,
Andrei Vagin, Michał Cłapiński, Kamil Yurtsever,
Dirk Petersen, Christine Flood, Casey Schaufler, Mike Rapoport,
Radostin Stoyanov, Cyrill Gorcunov, Serge Hallyn, Stephen Smalley,
Sargun Dhillon, Arnd Bergmann, linux-security-module,
linux-kernel, selinux, Eric Paris, Jann Horn, linux-fsdevel
In-Reply-To: <20200719100418.2112740-1-areber@redhat.com>
On Sun, Jul 19, 2020 at 12:04:10PM +0200, Adrian Reber wrote:
> This is v6 of the 'Introduce CAP_CHECKPOINT_RESTORE' patchset. The
> changes to v5 are:
>
> * split patch dealing with /proc/self/exe into two patches:
> * first patch to enable changing it with CAP_CHECKPOINT_RESTORE
> and detailed history in the commit message
> * second patch changes -EINVAL to -EPERM
> * use kselftest_harness.h infrastructure for test
> * replace if (!capable(CAP_SYS_ADMIN) || !capable(CAP_CHECKPOINT_RESTORE))
> with if (!checkpoint_restore_ns_capable(&init_user_ns))
>
> Adrian Reber (5):
> capabilities: Introduce CAP_CHECKPOINT_RESTORE
> pid: use checkpoint_restore_ns_capable() for set_tid
> pid_namespace: use checkpoint_restore_ns_capable() for ns_last_pid
> proc: allow access in init userns for map_files with
> CAP_CHECKPOINT_RESTORE
> selftests: add clone3() CAP_CHECKPOINT_RESTORE test
>
> Nicolas Viennot (2):
> prctl: Allow local CAP_CHECKPOINT_RESTORE to change /proc/self/exe
> prctl: exe link permission error changed from -EINVAL to -EPERM
>
> fs/proc/base.c | 8 +-
> include/linux/capability.h | 6 +
> include/uapi/linux/capability.h | 9 +-
> kernel/pid.c | 2 +-
> kernel/pid_namespace.c | 2 +-
> kernel/sys.c | 13 +-
> security/selinux/include/classmap.h | 5 +-
> tools/testing/selftests/clone3/.gitignore | 1 +
> tools/testing/selftests/clone3/Makefile | 4 +-
> .../clone3/clone3_cap_checkpoint_restore.c | 177 ++++++++++++++++++
> 10 files changed, 212 insertions(+), 15 deletions(-)
> create mode 100644 tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
>
> base-commit: d31958b30ea3b7b6e522d6bf449427748ad45822
Adrian, Nicolas thank you!
I grabbed the series to run the various core test-suites we've added
over the last year and pushed it to
https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=cap_checkpoint_restore
for now to let kbuild/ltp chew on it for a bit.
Thanks!
Christian
^ permalink raw reply
* Re: [RFC PATCH 0/5] keys: Security changes, ACLs and Container keyring
From: Eric W. Biederman @ 2020-07-19 18:10 UTC (permalink / raw)
To: David Howells
Cc: Stephen Smalley, Casey Schaufler, keyrings, Jarkko Sakkinen,
Paul Moore, selinux, jlayton, christian, linux-afs, linux-nfs,
linux-cifs, linux-api, linux-fsdevel, linux-security-module,
linux-kernel, containers, Linus Torvalds
In-Reply-To: <159493167778.3249370.8145886688150701997.stgit@warthog.procyon.org.uk>
David Howells <dhowells@redhat.com> writes:
> Here are some patches to provide some security changes and some container
> support:
Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
There remain unfixed security issues in the new mount api. Those need
to get fixed before it is even worth anyones time reviewing new code.
Those issues came up in the review. I successfully demonstrated how to
address the security issues in the new mount api before the code was
merged. Yet the code was merged with the security issues present,
and I have not seem those issues addressed.
So far I have had to rewrite two filesystems because of bugs in the
mount API.
Enough is enough. Let's get the what has already been merged sorted
out before we had more.
Eric
^ permalink raw reply
* Re: [PATCH v6 6/7] prctl: exe link permission error changed from -EINVAL to -EPERM
From: Serge E. Hallyn @ 2020-07-19 17:05 UTC (permalink / raw)
To: Adrian Reber
Cc: Christian Brauner, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Nicolas Viennot,
Michał Cłapiński, Kamil Yurtsever, Dirk Petersen,
Christine Flood, Casey Schaufler, Mike Rapoport,
Radostin Stoyanov, Cyrill Gorcunov, Serge Hallyn, Stephen Smalley,
Sargun Dhillon, Arnd Bergmann, linux-security-module,
linux-kernel, selinux, Eric Paris, Jann Horn, linux-fsdevel
In-Reply-To: <20200719100418.2112740-7-areber@redhat.com>
On Sun, Jul 19, 2020 at 12:04:16PM +0200, Adrian Reber wrote:
> From: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
>
> This brings consistency with the rest of the prctl() syscall where
> -EPERM is returned when failing a capability check.
>
> Signed-off-by: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
> Signed-off-by: Adrian Reber <areber@redhat.com>
Ok, i see how EINVAL snuck its way in there through validate_prctl_map()s
evolution :)
Reviewed-by: Serge Hallyn <serge@hallyn.com>
> ---
> kernel/sys.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sys.c b/kernel/sys.c
> index a3f4ef0bbda3..ca11af9d815d 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2015,7 +2015,7 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data
> * This may have implications in the tomoyo subsystem.
> */
> if (!checkpoint_restore_ns_capable(current_user_ns()))
> - return -EINVAL;
> + return -EPERM;
>
> error = prctl_set_mm_exe_file(mm, prctl_map.exe_fd);
> if (error)
> --
> 2.26.2
^ permalink raw reply
* Re: [PATCH v6 4/7] proc: allow access in init userns for map_files with CAP_CHECKPOINT_RESTORE
From: Serge E. Hallyn @ 2020-07-19 16:50 UTC (permalink / raw)
To: Adrian Reber
Cc: Christian Brauner, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Nicolas Viennot,
Michał Cłapiński, Kamil Yurtsever, Dirk Petersen,
Christine Flood, Casey Schaufler, Mike Rapoport,
Radostin Stoyanov, Cyrill Gorcunov, Serge Hallyn, Stephen Smalley,
Sargun Dhillon, Arnd Bergmann, linux-security-module,
linux-kernel, selinux, Eric Paris, Jann Horn, linux-fsdevel
In-Reply-To: <20200719100418.2112740-5-areber@redhat.com>
On Sun, Jul 19, 2020 at 12:04:14PM +0200, Adrian Reber wrote:
> Opening files in /proc/pid/map_files when the current user is
> CAP_CHECKPOINT_RESTORE capable in the root namespace is useful for
> checkpointing and restoring to recover files that are unreachable via
> the file system such as deleted files, or memfd files.
>
> Signed-off-by: Adrian Reber <areber@redhat.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
> Signed-off-by: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
> Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> fs/proc/base.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 65893686d1f1..b824a8c89011 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2194,16 +2194,16 @@ struct map_files_info {
> };
>
> /*
> - * Only allow CAP_SYS_ADMIN to follow the links, due to concerns about how the
> - * symlinks may be used to bypass permissions on ancestor directories in the
> - * path to the file in question.
> + * Only allow CAP_SYS_ADMIN and CAP_CHECKPOINT_RESTORE to follow the links, due
> + * to concerns about how the symlinks may be used to bypass permissions on
> + * ancestor directories in the path to the file in question.
> */
> static const char *
> proc_map_files_get_link(struct dentry *dentry,
> struct inode *inode,
> struct delayed_call *done)
> {
> - if (!capable(CAP_SYS_ADMIN))
> + if (!checkpoint_restore_ns_capable(&init_user_ns))
> return ERR_PTR(-EPERM);
>
> return proc_pid_get_link(dentry, inode, done);
> --
> 2.26.2
^ permalink raw reply
* [PATCH for v5.9] netfilter: xtables: Replace HTTP links with HTTPS ones
From: Alexander A. Klimov @ 2020-07-19 12:02 UTC (permalink / raw)
To: paul, pablo, kadlec, fw, davem, kuba, netdev,
linux-security-module, netfilter-devel, coreteam, linux-kernel
Cc: Alexander A. Klimov
Rationale:
Reduces attack surface on kernel devs opening the links for MITM
as HTTPS traffic is much harder to manipulate.
Deterministic algorithm:
For each file:
If not .svg:
For each line:
If doesn't contain `\bxmlns\b`:
For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`:
If neither `\bgnu\.org/license`, nor `\bmozilla\.org/MPL\b`:
If both the HTTP and HTTPS versions
return 200 OK and serve the same content:
Replace HTTP with HTTPS.
Signed-off-by: Alexander A. Klimov <grandmaster@al2klimov.de>
---
Continuing my work started at 93431e0607e5.
See also: git log --oneline '--author=Alexander A. Klimov <grandmaster@al2klimov.de>' v5.7..master
(Actually letting a shell for loop submit all this stuff for me.)
If there are any URLs to be removed completely
or at least not (just) HTTPSified:
Just clearly say so and I'll *undo my change*.
See also: https://lkml.org/lkml/2020/6/27/64
If there are any valid, but yet not changed URLs:
See: https://lkml.org/lkml/2020/6/26/837
If you apply the patch, please let me know.
Sorry again to all maintainers who complained about subject lines.
Now I realized that you want an actually perfect prefixes,
not just subsystem ones.
I tried my best...
And yes, *I could* (at least half-)automate it.
Impossible is nothing! :)
net/netfilter/xt_CONNSECMARK.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netfilter/xt_CONNSECMARK.c b/net/netfilter/xt_CONNSECMARK.c
index a5c8b653476a..76acecf3e757 100644
--- a/net/netfilter/xt_CONNSECMARK.c
+++ b/net/netfilter/xt_CONNSECMARK.c
@@ -6,7 +6,7 @@
* with the SECMARK target and state match.
*
* Based somewhat on CONNMARK:
- * Copyright (C) 2002,2004 MARA Systems AB <http://www.marasystems.com>
+ * Copyright (C) 2002,2004 MARA Systems AB <https://www.marasystems.com>
* by Henrik Nordstrom <hno@marasystems.com>
*
* (C) 2006,2008 Red Hat, Inc., James Morris <jmorris@redhat.com>
--
2.27.0
^ permalink raw reply related
* Re: [PATCH v3 01/12] ima: Have the LSM free its audit rule
From: Mimi Zohar @ 2020-07-19 11:02 UTC (permalink / raw)
To: Tyler Hicks, Nayna
Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module, Janne Karhunen,
Casey Schaufler
In-Reply-To: <20200717192447.GO3673@sequoia>
On Fri, 2020-07-17 at 14:24 -0500, Tyler Hicks wrote:
> On 2020-07-17 15:20:22, Nayna wrote:
> >
> > On 7/9/20 2:19 AM, Tyler Hicks wrote:
> > > Ask the LSM to free its audit rule rather than directly calling kfree().
> >
> > Is it to be called audit rule or filter rule ? Likewise in subject line.
> gt
> The security hooks call this "audit rule" but Mimi explained the
> reasoning for IMA referring to this as an "audit filter" here:
>
> https://lore.kernel.org/lkml/1593466203.5085.62.camel@linux.ibm.com/
>
> I would be fine with her renaming/rewording this patch, accordingly, in
> next-integrity-testing.
Both here and "ima: AppArmor satisfies the audit rule requirements",
the subject is AppArmor/LSM, which do refer to the rules as "audit"
rules. In the "ima: Rename internal audit rule functions" case, the
rule rename is internal to IMA. Here it makes sense to replace
"audit" with "filter". Tyler, I've gone ahead and made the change.
Mimi
^ permalink raw reply
* [PATCH v6 7/7] selftests: add clone3() CAP_CHECKPOINT_RESTORE test
From: Adrian Reber @ 2020-07-19 10:04 UTC (permalink / raw)
To: Christian Brauner, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Nicolas Viennot,
Michał Cłapiński, Kamil Yurtsever, Dirk Petersen,
Christine Flood, Casey Schaufler
Cc: Mike Rapoport, Radostin Stoyanov, Adrian Reber, Cyrill Gorcunov,
Serge Hallyn, Stephen Smalley, Sargun Dhillon, Arnd Bergmann,
linux-security-module, linux-kernel, selinux, Eric Paris,
Jann Horn, linux-fsdevel
In-Reply-To: <20200719100418.2112740-1-areber@redhat.com>
This adds a test that changes its UID, uses capabilities to
get CAP_CHECKPOINT_RESTORE and uses clone3() with set_tid to
create a process with a given PID as non-root.
Signed-off-by: Adrian Reber <areber@redhat.com>
---
tools/testing/selftests/clone3/.gitignore | 1 +
tools/testing/selftests/clone3/Makefile | 4 +-
.../clone3/clone3_cap_checkpoint_restore.c | 177 ++++++++++++++++++
3 files changed, 181 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
diff --git a/tools/testing/selftests/clone3/.gitignore b/tools/testing/selftests/clone3/.gitignore
index a81085742d40..83c0f6246055 100644
--- a/tools/testing/selftests/clone3/.gitignore
+++ b/tools/testing/selftests/clone3/.gitignore
@@ -2,3 +2,4 @@
clone3
clone3_clear_sighand
clone3_set_tid
+clone3_cap_checkpoint_restore
diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile
index cf976c732906..ef7564cb7abe 100644
--- a/tools/testing/selftests/clone3/Makefile
+++ b/tools/testing/selftests/clone3/Makefile
@@ -1,6 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
CFLAGS += -g -I../../../../usr/include/
+LDLIBS += -lcap
-TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid
+TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \
+ clone3_cap_checkpoint_restore
include ../lib.mk
diff --git a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
new file mode 100644
index 000000000000..c0d83511cd28
--- /dev/null
+++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Based on Christian Brauner's clone3() example.
+ * These tests are assuming to be running in the host's
+ * PID namespace.
+ */
+
+/* capabilities related code based on selftests/bpf/test_verifier.c */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <linux/types.h>
+#include <linux/sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <sys/capability.h>
+#include <sys/prctl.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/un.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <sched.h>
+
+#include "../kselftest_harness.h"
+#include "clone3_selftests.h"
+
+#ifndef MAX_PID_NS_LEVEL
+#define MAX_PID_NS_LEVEL 32
+#endif
+
+static void child_exit(int ret)
+{
+ fflush(stdout);
+ fflush(stderr);
+ _exit(ret);
+}
+
+static int call_clone3_set_tid(pid_t *set_tid, size_t set_tid_size)
+{
+ int status;
+ pid_t pid = -1;
+
+ struct clone_args args = {
+ .exit_signal = SIGCHLD,
+ .set_tid = ptr_to_u64(set_tid),
+ .set_tid_size = set_tid_size,
+ };
+
+ pid = sys_clone3(&args, sizeof(struct clone_args));
+ if (pid < 0) {
+ ksft_print_msg("%s - Failed to create new process\n", strerror(errno));
+ return -errno;
+ }
+
+ if (pid == 0) {
+ int ret;
+ char tmp = 0;
+
+ ksft_print_msg
+ ("I am the child, my PID is %d (expected %d)\n", getpid(), set_tid[0]);
+
+ if (set_tid[0] != getpid())
+ child_exit(EXIT_FAILURE);
+ child_exit(EXIT_SUCCESS);
+ }
+
+ ksft_print_msg("I am the parent (%d). My child's pid is %d\n", getpid(), pid);
+
+ if (waitpid(pid, &status, 0) < 0) {
+ ksft_print_msg("Child returned %s\n", strerror(errno));
+ return -errno;
+ }
+
+ if (!WIFEXITED(status))
+ return -1;
+
+ return WEXITSTATUS(status);
+}
+
+static int test_clone3_set_tid(pid_t *set_tid, size_t set_tid_size)
+{
+ int ret;
+
+ ksft_print_msg("[%d] Trying clone3() with CLONE_SET_TID to %d\n", getpid(), set_tid[0]);
+ ret = call_clone3_set_tid(set_tid, set_tid_size);
+ ksft_print_msg("[%d] clone3() with CLONE_SET_TID %d says:%d\n", getpid(), set_tid[0], ret);
+ return ret;
+}
+
+struct libcap {
+ struct __user_cap_header_struct hdr;
+ struct __user_cap_data_struct data[2];
+};
+
+static int set_capability(void)
+{
+ cap_value_t cap_values[] = { CAP_SETUID, CAP_SETGID };
+ struct libcap *cap;
+ int ret = -1;
+ cap_t caps;
+
+ caps = cap_get_proc();
+ if (!caps) {
+ perror("cap_get_proc");
+ return -1;
+ }
+
+ /* Drop all capabilities */
+ if (cap_clear(caps)) {
+ perror("cap_clear");
+ goto out;
+ }
+
+ cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_values, CAP_SET);
+ cap_set_flag(caps, CAP_PERMITTED, 2, cap_values, CAP_SET);
+
+ cap = (struct libcap *) caps;
+
+ /* 40 -> CAP_CHECKPOINT_RESTORE */
+ cap->data[1].effective |= 1 << (40 - 32);
+ cap->data[1].permitted |= 1 << (40 - 32);
+
+ if (cap_set_proc(caps)) {
+ perror("cap_set_proc");
+ goto out;
+ }
+ ret = 0;
+out:
+ if (cap_free(caps))
+ perror("cap_free");
+ return ret;
+}
+
+TEST(clone3_cap_checkpoint_restore)
+{
+ pid_t pid;
+ int status;
+ int ret = 0;
+ pid_t set_tid[1];
+
+ test_clone3_supported();
+
+ EXPECT_EQ(getuid(), 0)
+ SKIP(return, "Skipping all tests as non-root\n");
+
+ memset(&set_tid, 0, sizeof(set_tid));
+
+ /* Find the current active PID */
+ pid = fork();
+ if (pid == 0) {
+ TH_LOG("Child has PID %d", getpid());
+ child_exit(EXIT_SUCCESS);
+ }
+ ASSERT_GT(waitpid(pid, &status, 0), 0)
+ TH_LOG("Waiting for child %d failed", pid);
+
+ /* After the child has finished, its PID should be free. */
+ set_tid[0] = pid;
+
+ ASSERT_EQ(set_capability(), 0)
+ TH_LOG("Could not set CAP_CHECKPOINT_RESTORE");
+ prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0);
+ setgid(1000);
+ setuid(1000);
+ set_tid[0] = pid;
+ /* This would fail without CAP_CHECKPOINT_RESTORE */
+ ASSERT_EQ(test_clone3_set_tid(set_tid, 1), -EPERM);
+ ASSERT_EQ(set_capability(), 0)
+ TH_LOG("Could not set CAP_CHECKPOINT_RESTORE");
+ /* This should work as we have CAP_CHECKPOINT_RESTORE as non-root */
+ ASSERT_EQ(test_clone3_set_tid(set_tid, 1), 0);
+}
+
+TEST_HARNESS_MAIN
--
2.26.2
^ permalink raw reply related
* [PATCH v6 6/7] prctl: exe link permission error changed from -EINVAL to -EPERM
From: Adrian Reber @ 2020-07-19 10:04 UTC (permalink / raw)
To: Christian Brauner, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Nicolas Viennot,
Michał Cłapiński, Kamil Yurtsever, Dirk Petersen,
Christine Flood, Casey Schaufler
Cc: Mike Rapoport, Radostin Stoyanov, Adrian Reber, Cyrill Gorcunov,
Serge Hallyn, Stephen Smalley, Sargun Dhillon, Arnd Bergmann,
linux-security-module, linux-kernel, selinux, Eric Paris,
Jann Horn, linux-fsdevel
In-Reply-To: <20200719100418.2112740-1-areber@redhat.com>
From: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
This brings consistency with the rest of the prctl() syscall where
-EPERM is returned when failing a capability check.
Signed-off-by: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
Signed-off-by: Adrian Reber <areber@redhat.com>
---
kernel/sys.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sys.c b/kernel/sys.c
index a3f4ef0bbda3..ca11af9d815d 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2015,7 +2015,7 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data
* This may have implications in the tomoyo subsystem.
*/
if (!checkpoint_restore_ns_capable(current_user_ns()))
- return -EINVAL;
+ return -EPERM;
error = prctl_set_mm_exe_file(mm, prctl_map.exe_fd);
if (error)
--
2.26.2
^ permalink raw reply related
* [PATCH v6 5/7] prctl: Allow local CAP_CHECKPOINT_RESTORE to change /proc/self/exe
From: Adrian Reber @ 2020-07-19 10:04 UTC (permalink / raw)
To: Christian Brauner, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Nicolas Viennot,
Michał Cłapiński, Kamil Yurtsever, Dirk Petersen,
Christine Flood, Casey Schaufler
Cc: Mike Rapoport, Radostin Stoyanov, Adrian Reber, Cyrill Gorcunov,
Serge Hallyn, Stephen Smalley, Sargun Dhillon, Arnd Bergmann,
linux-security-module, linux-kernel, selinux, Eric Paris,
Jann Horn, linux-fsdevel
In-Reply-To: <20200719100418.2112740-1-areber@redhat.com>
From: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
Originally, only a local CAP_SYS_ADMIN could change the exe link,
making it difficult for doing checkpoint/restore without CAP_SYS_ADMIN.
This commit adds CAP_CHECKPOINT_RESTORE in addition to CAP_SYS_ADMIN
for permitting changing the exe link.
The following describes the history of the /proc/self/exe permission
checks as it may be difficult to understand what decisions lead to this
point.
* [1] May 2012: This commit introduces the ability of changing
/proc/self/exe if the user is CAP_SYS_RESOURCE capable.
In the related discussion [2], no clear thread model is presented for
what could happen if the /proc/self/exe changes multiple times, or why
would the admin be at the mercy of userspace.
* [3] Oct 2014: This commit introduces a new API to change
/proc/self/exe. The permission no longer checks for CAP_SYS_RESOURCE,
but instead checks if the current user is root (uid=0) in its local
namespace. In the related discussion [4] it is said that "Controlling
exe_fd without privileges may turn out to be dangerous. At least
things like tomoyo examine it for making policy decisions (see
tomoyo_manager())."
* [5] Dec 2016: This commit removes the restriction to change
/proc/self/exe at most once. The related discussion [6] informs that
the audit subsystem relies on the exe symlink, presumably
audit_log_d_path_exe() in kernel/audit.c.
* [7] May 2017: This commit changed the check from uid==0 to local
CAP_SYS_ADMIN. No discussion.
* [8] July 2020: A PoC to spoof any program's /proc/self/exe via ptrace
is demonstrated
Overall, the concrete points that were made to retain capability checks
around changing the exe symlink is that tomoyo_manager() and
audit_log_d_path_exe() uses the exe_file path.
Christian Brauner said that relying on /proc/<pid>/exe being immutable (or
guarded by caps) in a sake of security is a bit misleading. It can only
be used as a hint without any guarantees of what code is being executed
once execve() returns to userspace. Christian suggested that in the
future, we could call audit_log() or similar to inform the admin of all
exe link changes, instead of attempting to provide security guarantees
via permission checks. However, this proposed change requires the
understanding of the security implications in the tomoyo/audit subsystems.
[1] b32dfe377102 ("c/r: prctl: add ability to set new mm_struct::exe_file")
[2] https://lore.kernel.org/patchwork/patch/292515/
[3] f606b77f1a9e ("prctl: PR_SET_MM -- introduce PR_SET_MM_MAP operation")
[4] https://lore.kernel.org/patchwork/patch/479359/
[5] 3fb4afd9a504 ("prctl: remove one-shot limitation for changing exe link")
[6] https://lore.kernel.org/patchwork/patch/697304/
[7] 4d28df6152aa ("prctl: Allow local CAP_SYS_ADMIN changing exe_file")
[8] https://github.com/nviennot/run_as_exe
Signed-off-by: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
Signed-off-by: Adrian Reber <areber@redhat.com>
---
kernel/sys.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/kernel/sys.c b/kernel/sys.c
index 00a96746e28a..a3f4ef0bbda3 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2007,11 +2007,14 @@ static int prctl_set_mm_map(int opt, const void __user *addr, unsigned long data
if (prctl_map.exe_fd != (u32)-1) {
/*
- * Make sure the caller has the rights to
- * change /proc/pid/exe link: only local sys admin should
- * be allowed to.
+ * Check if the current user is checkpoint/restore capable.
+ * At the time of this writing, it checks for CAP_SYS_ADMIN
+ * or CAP_CHECKPOINT_RESTORE.
+ * Note that a user with access to ptrace can masquerade an
+ * arbitrary program as any executable, even setuid ones.
+ * This may have implications in the tomoyo subsystem.
*/
- if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN))
+ if (!checkpoint_restore_ns_capable(current_user_ns()))
return -EINVAL;
error = prctl_set_mm_exe_file(mm, prctl_map.exe_fd);
--
2.26.2
^ permalink raw reply related
* [PATCH v6 4/7] proc: allow access in init userns for map_files with CAP_CHECKPOINT_RESTORE
From: Adrian Reber @ 2020-07-19 10:04 UTC (permalink / raw)
To: Christian Brauner, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Nicolas Viennot,
Michał Cłapiński, Kamil Yurtsever, Dirk Petersen,
Christine Flood, Casey Schaufler
Cc: Mike Rapoport, Radostin Stoyanov, Adrian Reber, Cyrill Gorcunov,
Serge Hallyn, Stephen Smalley, Sargun Dhillon, Arnd Bergmann,
linux-security-module, linux-kernel, selinux, Eric Paris,
Jann Horn, linux-fsdevel
In-Reply-To: <20200719100418.2112740-1-areber@redhat.com>
Opening files in /proc/pid/map_files when the current user is
CAP_CHECKPOINT_RESTORE capable in the root namespace is useful for
checkpointing and restoring to recover files that are unreachable via
the file system such as deleted files, or memfd files.
Signed-off-by: Adrian Reber <areber@redhat.com>
Signed-off-by: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
fs/proc/base.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 65893686d1f1..b824a8c89011 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2194,16 +2194,16 @@ struct map_files_info {
};
/*
- * Only allow CAP_SYS_ADMIN to follow the links, due to concerns about how the
- * symlinks may be used to bypass permissions on ancestor directories in the
- * path to the file in question.
+ * Only allow CAP_SYS_ADMIN and CAP_CHECKPOINT_RESTORE to follow the links, due
+ * to concerns about how the symlinks may be used to bypass permissions on
+ * ancestor directories in the path to the file in question.
*/
static const char *
proc_map_files_get_link(struct dentry *dentry,
struct inode *inode,
struct delayed_call *done)
{
- if (!capable(CAP_SYS_ADMIN))
+ if (!checkpoint_restore_ns_capable(&init_user_ns))
return ERR_PTR(-EPERM);
return proc_pid_get_link(dentry, inode, done);
--
2.26.2
^ permalink raw reply related
* [PATCH v6 3/7] pid_namespace: use checkpoint_restore_ns_capable() for ns_last_pid
From: Adrian Reber @ 2020-07-19 10:04 UTC (permalink / raw)
To: Christian Brauner, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Nicolas Viennot,
Michał Cłapiński, Kamil Yurtsever, Dirk Petersen,
Christine Flood, Casey Schaufler
Cc: Mike Rapoport, Radostin Stoyanov, Adrian Reber, Cyrill Gorcunov,
Serge Hallyn, Stephen Smalley, Sargun Dhillon, Arnd Bergmann,
linux-security-module, linux-kernel, selinux, Eric Paris,
Jann Horn, linux-fsdevel
In-Reply-To: <20200719100418.2112740-1-areber@redhat.com>
Use the newly introduced capability CAP_CHECKPOINT_RESTORE to allow
writing to ns_last_pid.
Signed-off-by: Adrian Reber <areber@redhat.com>
Signed-off-by: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
kernel/pid_namespace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 0e5ac162c3a8..ac135bd600eb 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -269,7 +269,7 @@ static int pid_ns_ctl_handler(struct ctl_table *table, int write,
struct ctl_table tmp = *table;
int ret, next;
- if (write && !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
+ if (write && !checkpoint_restore_ns_capable(pid_ns->user_ns))
return -EPERM;
/*
--
2.26.2
^ permalink raw reply related
* [PATCH v6 2/7] pid: use checkpoint_restore_ns_capable() for set_tid
From: Adrian Reber @ 2020-07-19 10:04 UTC (permalink / raw)
To: Christian Brauner, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Nicolas Viennot,
Michał Cłapiński, Kamil Yurtsever, Dirk Petersen,
Christine Flood, Casey Schaufler
Cc: Mike Rapoport, Radostin Stoyanov, Adrian Reber, Cyrill Gorcunov,
Serge Hallyn, Stephen Smalley, Sargun Dhillon, Arnd Bergmann,
linux-security-module, linux-kernel, selinux, Eric Paris,
Jann Horn, linux-fsdevel
In-Reply-To: <20200719100418.2112740-1-areber@redhat.com>
Use the newly introduced capability CAP_CHECKPOINT_RESTORE to allow
using clone3() with set_tid set.
Signed-off-by: Adrian Reber <areber@redhat.com>
Signed-off-by: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
kernel/pid.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index de9d29c41d77..a9cbab0194d9 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -199,7 +199,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
if (tid != 1 && !tmp->child_reaper)
goto out_free;
retval = -EPERM;
- if (!ns_capable(tmp->user_ns, CAP_SYS_ADMIN))
+ if (!checkpoint_restore_ns_capable(tmp->user_ns))
goto out_free;
set_tid_size--;
}
--
2.26.2
^ permalink raw reply related
* [PATCH v6 1/7] capabilities: Introduce CAP_CHECKPOINT_RESTORE
From: Adrian Reber @ 2020-07-19 10:04 UTC (permalink / raw)
To: Christian Brauner, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Nicolas Viennot,
Michał Cłapiński, Kamil Yurtsever, Dirk Petersen,
Christine Flood, Casey Schaufler
Cc: Mike Rapoport, Radostin Stoyanov, Adrian Reber, Cyrill Gorcunov,
Serge Hallyn, Stephen Smalley, Sargun Dhillon, Arnd Bergmann,
linux-security-module, linux-kernel, selinux, Eric Paris,
Jann Horn, linux-fsdevel
In-Reply-To: <20200719100418.2112740-1-areber@redhat.com>
This patch introduces CAP_CHECKPOINT_RESTORE, a new capability facilitating
checkpoint/restore for non-root users.
Over the last years, The CRIU (Checkpoint/Restore In Userspace) team has
been asked numerous times if it is possible to checkpoint/restore a
process as non-root. The answer usually was: 'almost'.
The main blocker to restore a process as non-root was to control the PID
of the restored process. This feature available via the clone3 system
call, or via /proc/sys/kernel/ns_last_pid is unfortunately guarded by
CAP_SYS_ADMIN.
In the past two years, requests for non-root checkpoint/restore have
increased due to the following use cases:
* Checkpoint/Restore in an HPC environment in combination with a
resource manager distributing jobs where users are always running as
non-root. There is a desire to provide a way to checkpoint and
restore long running jobs.
* Container migration as non-root
* We have been in contact with JVM developers who are integrating
CRIU into a Java VM to decrease the startup time. These
checkpoint/restore applications are not meant to be running with
CAP_SYS_ADMIN.
We have seen the following workarounds:
* Use a setuid wrapper around CRIU:
See https://github.com/FredHutch/slurm-examples/blob/master/checkpointer/lib/checkpointer/checkpointer-suid.c
* Use a setuid helper that writes to ns_last_pid.
Unfortunately, this helper delegation technique is impossible to use
with clone3, and is thus prone to races.
See https://github.com/twosigma/set_ns_last_pid
* Cycle through PIDs with fork() until the desired PID is reached:
This has been demonstrated to work with cycling rates of 100,000 PIDs/s
See https://github.com/twosigma/set_ns_last_pid
* Patch out the CAP_SYS_ADMIN check from the kernel
* Run the desired application in a new user and PID namespace to provide
a local CAP_SYS_ADMIN for controlling PIDs. This technique has limited
use in typical container environments (e.g., Kubernetes) as /proc is
typically protected with read-only layers (e.g., /proc/sys) for
hardening purposes. Read-only layers prevent additional /proc mounts
(due to proc's SB_I_USERNS_VISIBLE property), making the use of new
PID namespaces limited as certain applications need access to /proc
matching their PID namespace.
The introduced capability allows to:
* Control PIDs when the current user is CAP_CHECKPOINT_RESTORE capable
for the corresponding PID namespace via ns_last_pid/clone3.
* Open files in /proc/pid/map_files when the current user is
CAP_CHECKPOINT_RESTORE capable in the root namespace, useful for
recovering files that are unreachable via the file system such as
deleted files, or memfd files.
See corresponding selftest for an example with clone3().
Signed-off-by: Adrian Reber <areber@redhat.com>
Signed-off-by: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
include/linux/capability.h | 6 ++++++
include/uapi/linux/capability.h | 9 ++++++++-
security/selinux/include/classmap.h | 5 +++--
3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/include/linux/capability.h b/include/linux/capability.h
index b4345b38a6be..1e7fe311cabe 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -261,6 +261,12 @@ static inline bool bpf_capable(void)
return capable(CAP_BPF) || capable(CAP_SYS_ADMIN);
}
+static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns)
+{
+ return ns_capable(ns, CAP_CHECKPOINT_RESTORE) ||
+ ns_capable(ns, CAP_SYS_ADMIN);
+}
+
/* audit system wants to get cap info from files as well */
extern int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data *cpu_caps);
diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 48ff0757ae5e..395dd0df8d08 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -408,7 +408,14 @@ struct vfs_ns_cap_data {
*/
#define CAP_BPF 39
-#define CAP_LAST_CAP CAP_BPF
+
+/* Allow checkpoint/restore related operations */
+/* Allow PID selection during clone3() */
+/* Allow writing to ns_last_pid */
+
+#define CAP_CHECKPOINT_RESTORE 40
+
+#define CAP_LAST_CAP CAP_CHECKPOINT_RESTORE
#define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index e54d62d529f1..ba2e01a6955c 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -27,9 +27,10 @@
"audit_control", "setfcap"
#define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
- "wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf"
+ "wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf", \
+ "checkpoint_restore"
-#if CAP_LAST_CAP > CAP_BPF
+#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
#error New capability defined, please update COMMON_CAP2_PERMS.
#endif
--
2.26.2
^ permalink raw reply related
* [PATCH v6 0/7] capabilities: Introduce CAP_CHECKPOINT_RESTORE
From: Adrian Reber @ 2020-07-19 10:04 UTC (permalink / raw)
To: Christian Brauner, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Nicolas Viennot,
Michał Cłapiński, Kamil Yurtsever, Dirk Petersen,
Christine Flood, Casey Schaufler
Cc: Mike Rapoport, Radostin Stoyanov, Adrian Reber, Cyrill Gorcunov,
Serge Hallyn, Stephen Smalley, Sargun Dhillon, Arnd Bergmann,
linux-security-module, linux-kernel, selinux, Eric Paris,
Jann Horn, linux-fsdevel
This is v6 of the 'Introduce CAP_CHECKPOINT_RESTORE' patchset. The
changes to v5 are:
* split patch dealing with /proc/self/exe into two patches:
* first patch to enable changing it with CAP_CHECKPOINT_RESTORE
and detailed history in the commit message
* second patch changes -EINVAL to -EPERM
* use kselftest_harness.h infrastructure for test
* replace if (!capable(CAP_SYS_ADMIN) || !capable(CAP_CHECKPOINT_RESTORE))
with if (!checkpoint_restore_ns_capable(&init_user_ns))
Adrian Reber (5):
capabilities: Introduce CAP_CHECKPOINT_RESTORE
pid: use checkpoint_restore_ns_capable() for set_tid
pid_namespace: use checkpoint_restore_ns_capable() for ns_last_pid
proc: allow access in init userns for map_files with
CAP_CHECKPOINT_RESTORE
selftests: add clone3() CAP_CHECKPOINT_RESTORE test
Nicolas Viennot (2):
prctl: Allow local CAP_CHECKPOINT_RESTORE to change /proc/self/exe
prctl: exe link permission error changed from -EINVAL to -EPERM
fs/proc/base.c | 8 +-
include/linux/capability.h | 6 +
include/uapi/linux/capability.h | 9 +-
kernel/pid.c | 2 +-
kernel/pid_namespace.c | 2 +-
kernel/sys.c | 13 +-
security/selinux/include/classmap.h | 5 +-
tools/testing/selftests/clone3/.gitignore | 1 +
tools/testing/selftests/clone3/Makefile | 4 +-
.../clone3/clone3_cap_checkpoint_restore.c | 177 ++++++++++++++++++
10 files changed, 212 insertions(+), 15 deletions(-)
create mode 100644 tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
base-commit: d31958b30ea3b7b6e522d6bf449427748ad45822
--
2.26.2
^ permalink raw reply
* Re: [PATCH v5 0/6] capabilities: Introduce CAP_CHECKPOINT_RESTORE
From: Christian Brauner @ 2020-07-18 17:47 UTC (permalink / raw)
To: Serge E. Hallyn, Adrian Reber, Nicolas Viennot
Cc: Adrian Reber, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Michał Cłapiński,
Kamil Yurtsever, Dirk Petersen, Christine Flood, Casey Schaufler,
Mike Rapoport, Radostin Stoyanov, Cyrill Gorcunov,
Stephen Smalley, Sargun Dhillon, Arnd Bergmann,
linux-security-module, linux-kernel, selinux, Eric Paris,
Jann Horn, linux-fsdevel
In-Reply-To: <20200718032416.GC11982@mail.hallyn.com>
On Fri, Jul 17, 2020 at 10:24:16PM -0500, Serge Hallyn wrote:
> On Wed, Jul 15, 2020 at 04:49:48PM +0200, Adrian Reber wrote:
> > This is v5 of the 'Introduce CAP_CHECKPOINT_RESTORE' patchset. The
> > changes to v4 are:
> >
> > * split into more patches to have the introduction of
> > CAP_CHECKPOINT_RESTORE and the actual usage in different
> > patches
> > * reduce the /proc/self/exe patch to only be about
> > CAP_CHECKPOINT_RESTORE
> >
> > Adrian Reber (5):
> > capabilities: Introduce CAP_CHECKPOINT_RESTORE
> > pid: use checkpoint_restore_ns_capable() for set_tid
> > pid_namespace: use checkpoint_restore_ns_capable() for ns_last_pid
> > proc: allow access in init userns for map_files with CAP_CHECKPOINT_RESTORE
> > selftests: add clone3() CAP_CHECKPOINT_RESTORE test
> >
> > Nicolas Viennot (1):
> > prctl: Allow checkpoint/restore capable processes to change exe link
>
> (This is probably bad form, but) All
>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>
>
> Assuming you changes patches 4 and 6 per Christian's suggestions,
> I'd like to re-review those then.
Thanks, once Adrian has reposted the changes and you agree with them as
well, I'll pick them up though I might end up pushing this into the next
merge window...
Christian
^ permalink raw reply
* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: kernel test robot @ 2020-07-18 15:31 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, zohar, stephen.smalley.work, casey
Cc: kbuild-all, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <20200717222819.26198-5-nramas@linux.microsoft.com>
[-- Attachment #1: Type: text/plain, Size: 1417 bytes --]
Hi Lakshmi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on integrity/next-integrity]
[cannot apply to pcmoore-selinux/next security/next-testing linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Lakshmi-Ramasubramanian/LSM-Measure-security-module-state/20200718-063111
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: x86_64-randconfig-s022-20200717 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-49-g707c5017-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> security/selinux/ss/services.c:3737:5: sparse: sparse: symbol 'security_read_selinux_policy' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36178 bytes --]
^ permalink raw reply
* [RFC PATCH] LSM: security_read_selinux_policy() can be static
From: kernel test robot @ 2020-07-18 15:31 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, zohar, stephen.smalley.work, casey
Cc: kbuild-all, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <20200717222819.26198-5-nramas@linux.microsoft.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
services.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 79a6b462f1fe9..4374c75f91a21 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3734,8 +3734,8 @@ static int security_read_policy_len(struct selinux_state *state, size_t *len)
* @data: binary policy data
* @len: length of data in bytes
*/
-int security_read_selinux_policy(struct selinux_state *state,
- void **data, size_t *len)
+static int security_read_selinux_policy(struct selinux_state *state,
+ void **data, size_t *len)
{
struct policydb *policydb = &state->ss->policydb;
int rc;
^ permalink raw reply related
* Re: [PATCH 06/13] fs/kernel_read_file: Remove redundant size argument
From: Scott Branden @ 2020-07-18 5:44 UTC (permalink / raw)
To: Kees Cook
Cc: Mimi Zohar, Matthew Wilcox, James Morris, Luis Chamberlain,
Greg Kroah-Hartman, Rafael J. Wysocki, Alexander Viro, Jessica Yu,
Dmitry Kasatkin, Serge E. Hallyn, Casey Schaufler,
Eric W. Biederman, Peter Zijlstra, Matthew Garrett, David Howells,
Mauro Carvalho Chehab, Randy Dunlap, Joel Fernandes (Google),
KP Singh, Dave Olsthoorn, Hans de Goede, Peter Jones,
Andrew Morton, Stephen Boyd, Paul Moore, Stephen Smalley,
linux-security-module, linux-integrity, selinux, linux-fsdevel,
kexec, linux-kernel
In-Reply-To: <202007171502.22E12A4E9@keescook>
Hi Kees,
On 2020-07-17 3:06 p.m., Kees Cook wrote:
> On Fri, Jul 17, 2020 at 12:04:18PM -0700, Scott Branden wrote:
>> On 2020-07-17 10:43 a.m., Kees Cook wrote:
>>> In preparation for refactoring kernel_read_file*(), remove the redundant
>>> "size" argument which is not needed: it can be included in the return
>> I don't think the size argument is redundant though.
>> The existing kernel_read_file functions always read the whole file.
>> Now, what happens if the file is bigger than the buffer.
>> How does kernel_read_file know it read the whole file by looking at the
>> return value?
> Yes; an entirely reasonable concern. This is why I add the file_size
> output argument later in the series.
There is something wrong with this patch. I apply patches 1-5 and these
pass the kernel self test.
Patch 6 does not pass the kernel-selftest/firmware/fw_run_tests.sh
>>> code, with callers adjusted. (VFS reads already cannot be larger than
>>> INT_MAX.)
>>> [...]
>>> - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
>>> + if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
>> Should this be SSIZE_MAX?
> No, for two reasons: then we need to change the return value and likely
> the callers need more careful checks, and more importantly, because the
> VFS already limits single read actions to INT_MAX, so limits above this
> make no sense. Win win! :)
>
^ permalink raw reply
* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: kernel test robot @ 2020-07-18 3:38 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, zohar, stephen.smalley.work, casey
Cc: kbuild-all, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <20200717222819.26198-5-nramas@linux.microsoft.com>
[-- Attachment #1: Type: text/plain, Size: 4885 bytes --]
Hi Lakshmi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on integrity/next-integrity]
[cannot apply to pcmoore-selinux/next security/next-testing linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Lakshmi-Ramasubramanian/LSM-Measure-security-module-state/20200718-063111
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
security/selinux/measure.c: In function 'selinux_hash_policy':
>> security/selinux/measure.c:57:8: error: implicit declaration of function 'crypto_alloc_shash' [-Werror=implicit-function-declaration]
57 | tfm = crypto_alloc_shash(hash_alg_name, 0, 0);
| ^~~~~~~~~~~~~~~~~~
>> security/selinux/measure.c:57:6: warning: assignment to 'struct crypto_shash *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
57 | tfm = crypto_alloc_shash(hash_alg_name, 0, 0);
| ^
>> security/selinux/measure.c:61:14: error: implicit declaration of function 'crypto_shash_descsize' [-Werror=implicit-function-declaration]
61 | desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
| ^~~~~~~~~~~~~~~~~~~~~
>> security/selinux/measure.c:61:50: error: dereferencing pointer to incomplete type 'struct shash_desc'
61 | desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
| ^~~~~
>> security/selinux/measure.c:62:16: error: implicit declaration of function 'crypto_shash_digestsize' [-Werror=implicit-function-declaration]
62 | digest_size = crypto_shash_digestsize(tfm);
| ^~~~~~~~~~~~~~~~~~~~~~~
>> security/selinux/measure.c:78:8: error: implicit declaration of function 'crypto_shash_digest' [-Werror=implicit-function-declaration]
78 | ret = crypto_shash_digest(desc, policy, policy_len, digest);
| ^~~~~~~~~~~~~~~~~~~
>> security/selinux/measure.c:90:2: error: implicit declaration of function 'crypto_free_shash' [-Werror=implicit-function-declaration]
90 | crypto_free_shash(tfm);
| ^~~~~~~~~~~~~~~~~
security/selinux/measure.c: In function 'selinux_measure_state':
security/selinux/measure.c:132:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
132 | if (curr >= 0 && curr < selinux_state_string_len)
| ^~
security/selinux/measure.c:148:2: error: implicit declaration of function 'vfree'; did you mean 'kvfree'? [-Werror=implicit-function-declaration]
148 | vfree(policy);
| ^~~~~
| kvfree
cc1: some warnings being treated as errors
vim +/crypto_alloc_shash +57 security/selinux/measure.c
45
46 static int selinux_hash_policy(const char *hash_alg_name,
47 void *policy, size_t policy_len,
48 void **policy_hash, int *policy_hash_len)
49 {
50 struct crypto_shash *tfm;
51 struct shash_desc *desc = NULL;
52 void *digest = NULL;
53 int desc_size;
54 int digest_size;
55 int ret = 0;
56
> 57 tfm = crypto_alloc_shash(hash_alg_name, 0, 0);
58 if (IS_ERR(tfm))
59 return PTR_ERR(tfm);
60
> 61 desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> 62 digest_size = crypto_shash_digestsize(tfm);
63
64 digest = kmalloc(digest_size, GFP_KERNEL);
65 if (!digest) {
66 ret = -ENOMEM;
67 goto error;
68 }
69
70 desc = kzalloc(desc_size, GFP_KERNEL);
71 if (!desc) {
72 ret = -ENOMEM;
73 goto error;
74 }
75
76 desc->tfm = tfm;
77
> 78 ret = crypto_shash_digest(desc, policy, policy_len, digest);
79 if (ret < 0)
80 goto error;
81
82 *policy_hash_len = digest_size;
83 *policy_hash = digest;
84 digest = NULL;
85
86 error:
87 kfree(desc);
88 kfree(digest);
89
> 90 crypto_free_shash(tfm);
91
92 return ret;
93 }
94
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66232 bytes --]
^ permalink raw reply
* Re: [PATCH v5 0/6] capabilities: Introduce CAP_CHECKPOINT_RESTORE
From: Serge E. Hallyn @ 2020-07-18 3:24 UTC (permalink / raw)
To: Adrian Reber
Cc: Christian Brauner, Eric Biederman, Pavel Emelyanov, Oleg Nesterov,
Dmitry Safonov, Andrei Vagin, Nicolas Viennot,
Michał Cłapiński, Kamil Yurtsever, Dirk Petersen,
Christine Flood, Casey Schaufler, Mike Rapoport,
Radostin Stoyanov, Cyrill Gorcunov, Serge Hallyn, Stephen Smalley,
Sargun Dhillon, Arnd Bergmann, linux-security-module,
linux-kernel, selinux, Eric Paris, Jann Horn, linux-fsdevel
In-Reply-To: <20200715144954.1387760-1-areber@redhat.com>
On Wed, Jul 15, 2020 at 04:49:48PM +0200, Adrian Reber wrote:
> This is v5 of the 'Introduce CAP_CHECKPOINT_RESTORE' patchset. The
> changes to v4 are:
>
> * split into more patches to have the introduction of
> CAP_CHECKPOINT_RESTORE and the actual usage in different
> patches
> * reduce the /proc/self/exe patch to only be about
> CAP_CHECKPOINT_RESTORE
>
> Adrian Reber (5):
> capabilities: Introduce CAP_CHECKPOINT_RESTORE
> pid: use checkpoint_restore_ns_capable() for set_tid
> pid_namespace: use checkpoint_restore_ns_capable() for ns_last_pid
> proc: allow access in init userns for map_files with CAP_CHECKPOINT_RESTORE
> selftests: add clone3() CAP_CHECKPOINT_RESTORE test
>
> Nicolas Viennot (1):
> prctl: Allow checkpoint/restore capable processes to change exe link
(This is probably bad form, but) All
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Assuming you changes patches 4 and 6 per Christian's suggestions,
I'd like to re-review those then.
>
> fs/proc/base.c | 8 +-
> include/linux/capability.h | 6 +
> include/uapi/linux/capability.h | 9 +-
> kernel/pid.c | 2 +-
> kernel/pid_namespace.c | 2 +-
> kernel/sys.c | 12 +-
> security/selinux/include/classmap.h | 5 +-
> tools/testing/selftests/clone3/Makefile | 4 +-
> .../clone3/clone3_cap_checkpoint_restore.c | 203 ++++++++++++++++++
> 9 files changed, 236 insertions(+), 15 deletions(-)
> create mode 100644 tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
>
>
> base-commit: d31958b30ea3b7b6e522d6bf449427748ad45822
> --
> 2.26.2
^ permalink raw reply
* Re: [PATCH v3 4/5] LSM: Define SELinux function to measure security state
From: kernel test robot @ 2020-07-18 3:14 UTC (permalink / raw)
To: Lakshmi Ramasubramanian, zohar, stephen.smalley.work, casey
Cc: kbuild-all, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <20200717222819.26198-5-nramas@linux.microsoft.com>
[-- Attachment #1: Type: text/plain, Size: 3792 bytes --]
Hi Lakshmi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on integrity/next-integrity]
[cannot apply to pcmoore-selinux/next security/next-testing linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Lakshmi-Ramasubramanian/LSM-Measure-security-module-state/20200718-063111
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
security/selinux/measure.c: In function 'selinux_measure_state':
security/selinux/measure.c:132:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
132 | if (curr >= 0 && curr < selinux_state_string_len)
| ^~
>> security/selinux/measure.c:148:2: error: implicit declaration of function 'vfree'; did you mean 'kvfree'? [-Werror=implicit-function-declaration]
148 | vfree(policy);
| ^~~~~
| kvfree
cc1: some warnings being treated as errors
vim +148 security/selinux/measure.c
94
95 void selinux_measure_state(struct selinux_state *selinux_state)
96 {
97 void *policy = NULL;
98 void *policy_hash = NULL;
99 size_t curr, buflen;
100 int i, policy_hash_len, rc = 0;
101
102 if (!selinux_initialized(selinux_state)) {
103 pr_warn("%s: SELinux not yet initialized.\n", __func__);
104 return;
105 }
106
107 if (!selinux_state_string) {
108 pr_warn("%s: Buffer for state not allocated.\n", __func__);
109 return;
110 }
111
112 curr = snprintf(selinux_state_string, selinux_state_string_len,
113 str_format, "enabled",
114 !selinux_disabled(selinux_state));
115 curr += snprintf((selinux_state_string + curr),
116 (selinux_state_string_len - curr),
117 str_format, "enforcing",
118 enforcing_enabled(selinux_state));
119 curr += snprintf((selinux_state_string + curr),
120 (selinux_state_string_len - curr),
121 str_format, "checkreqprot",
122 selinux_checkreqprot(selinux_state));
123
124 for (i = 3; i < selinux_state_count; i++) {
125 curr += snprintf((selinux_state_string + curr),
126 (selinux_state_string_len - curr),
127 str_format,
128 selinux_policycap_names[i - 3],
129 selinux_state->policycap[i - 3]);
130 }
131
> 132 if (curr >= 0 && curr < selinux_state_string_len)
133 ima_lsm_state("selinux-state", selinux_state_string, curr);
134 else {
135 rc = -EINVAL;
136 goto out;
137 }
138
139 rc = security_read_policy_kernel(selinux_state, &policy, &buflen);
140 if (!rc)
141 rc = selinux_hash_policy("sha256", policy, buflen,
142 &policy_hash, &policy_hash_len);
143 if (!rc)
144 ima_lsm_state("selinux-policy-hash", policy_hash,
145 policy_hash_len);
146
147 out:
> 148 vfree(policy);
149 kfree(policy_hash);
150 }
151
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65334 bytes --]
^ permalink raw reply
* Re: [PATCH] LSM: drop duplicated words in header file comments
From: Serge E. Hallyn @ 2020-07-18 2:59 UTC (permalink / raw)
To: Randy Dunlap; +Cc: LKML, James Morris, Serge E. Hallyn, linux-security-module
In-Reply-To: <9299abf4-75e3-6d73-a8b8-c2617208a990@infradead.org>
On Fri, Jul 17, 2020 at 04:36:40PM -0700, Randy Dunlap wrote:
> From: Randy Dunlap <rdunlap@infradead.org>
>
> Drop the doubled words "the" and "and" in comments.
>
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
> Cc: linux-security-module@vger.kernel.org
> ---
> include/linux/lsm_hook_defs.h | 2 +-
> include/linux/lsm_hooks.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> --- linux-next-20200714.orig/include/linux/lsm_hook_defs.h
> +++ linux-next-20200714/include/linux/lsm_hook_defs.h
> @@ -15,7 +15,7 @@
> */
>
> /*
> - * The macro LSM_HOOK is used to define the data structures required by the
> + * The macro LSM_HOOK is used to define the data structures required by
> * the LSM framework using the pattern:
> *
> * LSM_HOOK(<return_type>, <default_value>, <hook_name>, args...)
> --- linux-next-20200714.orig/include/linux/lsm_hooks.h
> +++ linux-next-20200714/include/linux/lsm_hooks.h
> @@ -822,7 +822,7 @@
> * structure. Note that the security field was not added directly to the
> * socket structure, but rather, the socket security information is stored
> * in the associated inode. Typically, the inode alloc_security hook will
> - * allocate and and attach security information to
> + * allocate and attach security information to
> * SOCK_INODE(sock)->i_security. This hook may be used to update the
> * SOCK_INODE(sock)->i_security field with additional information that
> * wasn't available when the inode was allocated.
>
^ permalink raw reply
* Re: [PATCH] capabilities: Replace HTTP links with HTTPS ones
From: Serge E. Hallyn @ 2020-07-18 2:54 UTC (permalink / raw)
To: Alexander A. Klimov; +Cc: serge, linux-security-module, linux-kernel
In-Reply-To: <20200713103428.33342-1-grandmaster@al2klimov.de>
On Mon, Jul 13, 2020 at 12:34:28PM +0200, Alexander A. Klimov wrote:
> Rationale:
> Reduces attack surface on kernel devs opening the links for MITM
> as HTTPS traffic is much harder to manipulate.
>
> Deterministic algorithm:
> For each file:
> If not .svg:
> For each line:
> If doesn't contain `\bxmlns\b`:
> For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`:
> If neither `\bgnu\.org/license`, nor `\bmozilla\.org/MPL\b`:
> If both the HTTP and HTTPS versions
> return 200 OK and serve the same content:
> Replace HTTP with HTTPS.
>
> Signed-off-by: Alexander A. Klimov <grandmaster@al2klimov.de>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
> ---
> Continuing my work started at 93431e0607e5.
> See also: git log --oneline '--author=Alexander A. Klimov <grandmaster@al2klimov.de>' v5.7..master
> (Actually letting a shell for loop submit all this stuff for me.)
>
> If there are any URLs to be removed completely or at least not just HTTPSified:
> Just clearly say so and I'll *undo my change*.
> See also: https://lkml.org/lkml/2020/6/27/64
>
> If there are any valid, but yet not changed URLs:
> See: https://lkml.org/lkml/2020/6/26/837
>
> If you apply the patch, please let me know.
>
> Sorry again to all maintainers who complained about subject lines.
> Now I realized that you want an actually perfect prefixes,
> not just subsystem ones.
> I tried my best...
> And yes, *I could* (at least half-)automate it.
> Impossible is nothing! :)
>
>
> kernel/capability.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 1444f3954d75..a8a20ebc43ee 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -40,7 +40,7 @@ __setup("no_file_caps", file_caps_disable);
> /*
> * More recent versions of libcap are available from:
> *
> - * http://www.kernel.org/pub/linux/libs/security/linux-privs/
> + * https://www.kernel.org/pub/linux/libs/security/linux-privs/
> */
>
> static void warn_legacy_capability_use(void)
> --
> 2.27.0
^ permalink raw reply
* Re: [RFC PATCH v4 05/12] fs: add security blob and hooks for block_device
From: Casey Schaufler @ 2020-07-18 0:14 UTC (permalink / raw)
To: Deven Bowers, agk, axboe, snitzer, jmorris, serge, zohar, viro,
paul, eparis, jannh, dm-devel, linux-integrity,
linux-security-module, linux-fsdevel, linux-block, linux-audit
Cc: tyhicks, linux-kernel, corbet, sashal, jaskarankhurana, mdsakib,
nramas, pasha.tatshin, Casey Schaufler
In-Reply-To: <20200717230941.1190744-6-deven.desai@linux.microsoft.com>
On 7/17/2020 4:09 PM, Deven Bowers wrote:
> Add a security blob and associated allocation, deallocation and set hooks
> for a block_device structure.
>
> Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com>
> ---
> fs/block_dev.c | 8 +++++
> include/linux/fs.h | 1 +
> include/linux/lsm_hook_defs.h | 5 +++
> include/linux/lsm_hooks.h | 11 +++++++
> include/linux/security.h | 22 +++++++++++++
> security/security.c | 61 +++++++++++++++++++++++++++++++++++
> 6 files changed, 108 insertions(+)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 0ae656e022fd..8602dd62c3e2 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -34,6 +34,7 @@
> #include <linux/falloc.h>
> #include <linux/uaccess.h>
> #include <linux/suspend.h>
> +#include <linux/security.h>
> #include "internal.h"
>
> struct bdev_inode {
> @@ -768,11 +769,18 @@ static struct inode *bdev_alloc_inode(struct super_block *sb)
> struct bdev_inode *ei = kmem_cache_alloc(bdev_cachep, GFP_KERNEL);
> if (!ei)
> return NULL;
> +
> + if (unlikely(security_bdev_alloc(&ei->bdev))) {
> + kmem_cache_free(bdev_cachep, ei);
> + return NULL;
> + }
> +
> return &ei->vfs_inode;
> }
>
> static void bdev_free_inode(struct inode *inode)
> {
> + security_bdev_free(&BDEV_I(inode)->bdev);
> kmem_cache_free(bdev_cachep, BDEV_I(inode));
> }
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index f5abba86107d..42d7e3ce7712 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -509,6 +509,7 @@ struct block_device {
> int bd_fsfreeze_count;
> /* Mutex for freeze */
> struct mutex bd_fsfreeze_mutex;
> + void *security;
> } __randomize_layout;
>
> /* XArray tags, for tagging dirty and writeback pages in the pagecache. */
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index af998f93d256..f3c0da0db4e8 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -391,3 +391,8 @@ LSM_HOOK(void, LSM_RET_VOID, perf_event_free, struct perf_event *event)
> LSM_HOOK(int, 0, perf_event_read, struct perf_event *event)
> LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
> #endif /* CONFIG_PERF_EVENTS */
> +
> +LSM_HOOK(int, 0, bdev_alloc_security, struct block_device *bdev)
> +LSM_HOOK(void, LSM_RET_VOID, bdev_free_security, struct block_device *bdev)
> +LSM_HOOK(int, 0, bdev_setsecurity, struct block_device *bdev, const char *name,
> + const void *value, size_t size)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 95b7c1d32062..8a728b7dd32d 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1507,6 +1507,16 @@
> *
> * @what: kernel feature being accessed
> *
> + * @bdev_alloc_security:
> + * Initialize the security field inside a block_device structure.
> + *
> + * @bdev_free_security:
> + * Cleanup the security information stored inside a block_device structure.
> + *
> + * @bdev_setsecurity:
> + * Set the security property associated with @name for @bdev with
> + * value @value. @size indicates the size of the @value in bytes.
> + *
> * Security hooks for perf events
> *
> * @perf_event_open:
> @@ -1553,6 +1563,7 @@ struct lsm_blob_sizes {
> int lbs_ipc;
> int lbs_msg_msg;
> int lbs_task;
> + int lbs_bdev;
> };
>
> /*
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 0a0a03b36a3b..8f83fdc6c65d 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -451,6 +451,11 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> int security_locked_down(enum lockdown_reason what);
> +int security_bdev_alloc(struct block_device *bdev);
> +void security_bdev_free(struct block_device *bdev);
> +int security_bdev_setsecurity(struct block_device *bdev,
> + const char *name, const void *value,
> + size_t size);
> #else /* CONFIG_SECURITY */
>
> static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
> @@ -1291,6 +1296,23 @@ static inline int security_locked_down(enum lockdown_reason what)
> {
> return 0;
> }
> +
> +static inline int security_bdev_alloc(struct block_device *bdev)
> +{
> + return 0;
> +}
> +
> +static inline void security_bdev_free(struct block_device *bdev)
> +{
> +}
> +
> +static inline int security_bdev_setsecurity(struct block_device *bdev,
> + const char *name,
> + const void *value, size_t size)
> +{
> + return 0;
> +}
> +
> #endif /* CONFIG_SECURITY */
>
> #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
> diff --git a/security/security.c b/security/security.c
> index 70a7ad357bc6..8e61b7e6adfc 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -28,6 +28,7 @@
> #include <linux/string.h>
> #include <linux/msg.h>
> #include <net/flow.h>
> +#include <linux/fs.h>
>
> #define MAX_LSM_EVM_XATTR 2
>
> @@ -202,6 +203,7 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
> lsm_set_blob_size(&needed->lbs_ipc, &blob_sizes.lbs_ipc);
> lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
> lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
> + lsm_set_blob_size(&needed->lbs_bdev, &blob_sizes.lbs_bdev);
> }
>
> /* Prepare LSM for initialization. */
> @@ -337,6 +339,7 @@ static void __init ordered_lsm_init(void)
> init_debug("ipc blob size = %d\n", blob_sizes.lbs_ipc);
> init_debug("msg_msg blob size = %d\n", blob_sizes.lbs_msg_msg);
> init_debug("task blob size = %d\n", blob_sizes.lbs_task);
> + init_debug("bdev blob size = %d\n", blob_sizes.lbs_bdev);
>
> /*
> * Create any kmem_caches needed for blobs
> @@ -654,6 +657,28 @@ static int lsm_msg_msg_alloc(struct msg_msg *mp)
> return 0;
> }
>
> +/**
> + * lsm_bdev_alloc - allocate a composite block_device blob
> + * @bdev: the block_device that needs a blob
> + *
> + * Allocate the block_device blob for all the modules
> + *
> + * Returns 0, or -ENOMEM if memory can't be allocated.
> + */
> +static int lsm_bdev_alloc(struct block_device *bdev)
> +{
> + if (blob_sizes.lbs_bdev == 0) {
> + bdev->security = NULL;
> + return 0;
> + }
> +
> + bdev->security = kzalloc(blob_sizes.lbs_bdev, GFP_KERNEL);
> + if (!bdev->security)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> /**
> * lsm_early_task - during initialization allocate a composite task blob
> * @task: the task that needs a blob
> @@ -2516,6 +2541,42 @@ int security_locked_down(enum lockdown_reason what)
> }
> EXPORT_SYMBOL(security_locked_down);
>
> +int security_bdev_alloc(struct block_device *bdev)
> +{
> + int rc = 0;
> +
> + rc = lsm_bdev_alloc(bdev);
> + if (unlikely(rc))
> + return rc;
> +
> + rc = call_int_hook(bdev_alloc_security, 0, bdev);
> + if (unlikely(rc))
> + security_bdev_free(bdev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(security_bdev_alloc);
> +
> +void security_bdev_free(struct block_device *bdev)
> +{
> + if (!bdev->security)
> + return;
> +
> + call_void_hook(bdev_free_security, bdev);
> +
> + kfree(bdev->security);
> + bdev->security = NULL;
> +}
> +EXPORT_SYMBOL(security_bdev_free);
> +
> +int security_bdev_setsecurity(struct block_device *bdev,
> + const char *name, const void *value,
> + size_t size)
> +{
> + return call_int_hook(bdev_setsecurity, 0, bdev, name, value, size);
> +}
What is your expectation regarding multiple security modules using the
same @name? What do you expect a security module to do if it does not
support a particular @name? You may have a case where SELinux supports
a @name that AppArmor (or KSRI) doesn't. -ENOSYS may be you friend here.
> +EXPORT_SYMBOL(security_bdev_setsecurity);
> +
> #ifdef CONFIG_PERF_EVENTS
> int security_perf_event_open(struct perf_event_attr *attr, int type)
> {
^ permalink raw reply
* Re: [PATCH v3 06/12] ima: Fail rule parsing when the KEY_CHECK hook is combined with an invalid cond
From: Tyler Hicks @ 2020-07-17 23:39 UTC (permalink / raw)
To: Nayna, Lakshmi Ramasubramanian, Mimi Zohar
Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200717191858.GN3673@sequoia>
On 2020-07-17 14:19:03, Tyler Hicks wrote:
> On 2020-07-17 14:56:46, Nayna wrote:
> >
> > On 7/9/20 2:19 AM, Tyler Hicks wrote:
> > > The KEY_CHECK function only supports the uid, pcr, and keyrings
> > > conditionals. Make this clear at policy load so that IMA policy authors
> > > don't assume that other conditionals are supported.
> > >
> > > Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
> > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > > Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> > > ---
> > >
> > > * v3
> > > - Added Lakshmi's Reviewed-by
> > > - Adjust for the indentation change introduced in patch #4
> > > * v2
> > > - No change
> > >
> > > security/integrity/ima/ima_policy.c | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > index 1c64bd6f1728..81da02071d41 100644
> > > --- a/security/integrity/ima/ima_policy.c
> > > +++ b/security/integrity/ima/ima_policy.c
> > > @@ -1023,6 +1023,13 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> > > if (entry->action & ~(MEASURE | DONT_MEASURE))
> > > return false;
> > >
> > > + if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
> > > + IMA_KEYRINGS))
> > > + return false;
> > > +
> > > + if (ima_rule_contains_lsm_cond(entry))
> > > + return false;
> > > +
> > > break;
> > > default:
> > > return false;
> >
> > Should there be a check for IMA_MEASURE_ASYMMETRIC_KEYS in Opt_keyrings in
> > ima_parse_rule() to return immediately if not enabled ?
>
> I didn't notice that "keyrings=" could be disabled at build time. I
> think you're right that something like what I have below would be a good idea.
>
> @Lakshmi, do you agree?
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 81da02071d41..bd687560f88e 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1212,6 +1212,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> case Opt_keyrings:
> ima_log_string(ab, "keyrings", args[0].from);
>
> + if (!IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS)) {
> + result = -EINVAL;
> + break;
> + }
> +
> keyrings_len = strlen(args[0].from) + 1;
>
> if ((entry->keyrings) ||
>
Actually, this change introduces a new compiler warning in another part
of the code that I need to think some more about. I'd like to leave this
patch as-is for now and work on the !CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
case in a separate, later patch when I have some more time to think
about it and test properly.
Tyler
> Tyler
>
> >
> > Thanks & Regards,
> >
> > - Nayna
> >
^ 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