* [PATCH v21 1/4] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: Jason A. Donenfeld @ 2024-07-07 0:26 UTC (permalink / raw)
To: linux-kernel, patches, tglx
Cc: Jason A. Donenfeld, linux-crypto, linux-api, x86, Linus Torvalds,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand, linux-mm
In-Reply-To: <20240707002658.1917440-1-Jason@zx2c4.com>
The vDSO getrandom() implementation works with a buffer allocated with a
new system call that has certain requirements:
- It shouldn't be written to core dumps.
* Easy: VM_DONTDUMP.
- It should be zeroed on fork.
* Easy: VM_WIPEONFORK.
- It shouldn't be written to swap.
* Uh-oh: mlock is rlimited.
* Uh-oh: mlock isn't inherited by forks.
It turns out that the vDSO getrandom() function has three really nice
characteristics that we can exploit to solve this problem:
1) Due to being wiped during fork(), the vDSO code is already robust to
having the contents of the pages it reads zeroed out midway through
the function's execution.
2) In the absolute worst case of whatever contingency we're coding for,
we have the option to fallback to the getrandom() syscall, and
everything is fine.
3) The buffers the function uses are only ever useful for a maximum of
60 seconds -- a sort of cache, rather than a long term allocation.
These characteristics mean that we can introduce VM_DROPPABLE, which
has the following semantics:
a) It never is written out to swap.
b) Under memory pressure, mm can just drop the pages (so that they're
zero when read back again).
c) It is inherited by fork.
d) It doesn't count against the mlock budget, since nothing is locked.
This is fairly simple to implement, with the one snag that we have to
use 64-bit VM_* flags, but this shouldn't be a problem, since the only
consumers will probably be 64-bit anyway.
This way, allocations used by vDSO getrandom() can use:
VM_DROPPABLE | VM_DONTDUMP | VM_WIPEONFORK | VM_NORESERVE
And there will be no problem with using memory when not in use, not
wiping on fork(), coredumps, or writing out to swap.
In order to let vDSO getrandom() use this, expose these via mmap(2) as
well, giving MAP_WIPEONFORK, MAP_DONTDUMP, and MAP_DROPPABLE.
Finally, the provided self test ensures that this is working as desired.
Cc: linux-mm@kvack.org
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
arch/alpha/include/uapi/asm/mman.h | 3 ++
arch/mips/include/uapi/asm/mman.h | 3 ++
arch/parisc/include/uapi/asm/mman.h | 3 ++
arch/xtensa/include/uapi/asm/mman.h | 3 ++
fs/proc/task_mmu.c | 3 ++
include/linux/mm.h | 8 +++
include/trace/events/mmflags.h | 7 +++
include/uapi/asm-generic/mman-common.h | 4 ++
mm/Kconfig | 3 ++
mm/mmap.c | 15 ++++++
mm/mprotect.c | 2 +-
mm/rmap.c | 16 ++++--
tools/include/uapi/asm-generic/mman-common.h | 4 ++
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
tools/testing/selftests/mm/droppable.c | 53 ++++++++++++++++++++
16 files changed, 125 insertions(+), 4 deletions(-)
create mode 100644 tools/testing/selftests/mm/droppable.c
diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
index 763929e814e9..951c54a45676 100644
--- a/arch/alpha/include/uapi/asm/mman.h
+++ b/arch/alpha/include/uapi/asm/mman.h
@@ -31,6 +31,9 @@
#define MAP_STACK 0x80000 /* give out an address that is best suited for process/thread stacks */
#define MAP_HUGETLB 0x100000 /* create a huge page mapping */
#define MAP_FIXED_NOREPLACE 0x200000/* MAP_FIXED which doesn't unmap underlying mapping */
+#define MAP_WIPEONFORK 0x08000000 /* Zero memory in child forks. */
+#define MAP_DONTDUMP 0x10000000 /* Do not write to coredumps. */
+#define MAP_DROPPABLE 0x20000000 /* Zero memory under memory pressure. */
#define MS_ASYNC 1 /* sync memory asynchronously */
#define MS_SYNC 2 /* synchronous memory sync */
diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h
index 9c48d9a21aa0..7490a28ec960 100644
--- a/arch/mips/include/uapi/asm/mman.h
+++ b/arch/mips/include/uapi/asm/mman.h
@@ -49,6 +49,9 @@
#define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */
#define MAP_HUGETLB 0x80000 /* create a huge page mapping */
#define MAP_FIXED_NOREPLACE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */
+#define MAP_WIPEONFORK 0x08000000 /* Zero memory in child forks. */
+#define MAP_DONTDUMP 0x10000000 /* Do not write to coredumps. */
+#define MAP_DROPPABLE 0x20000000 /* Zero memory under memory pressure. */
/*
* Flags for msync
diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h
index 68c44f99bc93..ed03f1d7d06c 100644
--- a/arch/parisc/include/uapi/asm/mman.h
+++ b/arch/parisc/include/uapi/asm/mman.h
@@ -26,6 +26,9 @@
#define MAP_HUGETLB 0x80000 /* create a huge page mapping */
#define MAP_FIXED_NOREPLACE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */
#define MAP_UNINITIALIZED 0 /* uninitialized anonymous mmap */
+#define MAP_WIPEONFORK 0x08000000 /* Zero memory in child forks. */
+#define MAP_DONTDUMP 0x10000000 /* Do not write to coredumps. */
+#define MAP_DROPPABLE 0x20000000 /* Zero memory under memory pressure. */
#define MS_SYNC 1 /* synchronous memory sync */
#define MS_ASYNC 2 /* sync memory asynchronously */
diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h
index 1ff0c858544f..2e777670b7fa 100644
--- a/arch/xtensa/include/uapi/asm/mman.h
+++ b/arch/xtensa/include/uapi/asm/mman.h
@@ -58,6 +58,9 @@
#define MAP_FIXED_NOREPLACE 0x100000 /* MAP_FIXED which doesn't unmap underlying mapping */
#define MAP_UNINITIALIZED 0x4000000 /* For anonymous mmap, memory could be
* uninitialized */
+#define MAP_WIPEONFORK 0x08000000 /* Zero memory in child forks. */
+#define MAP_DONTDUMP 0x10000000 /* Do not write to coredumps. */
+#define MAP_DROPPABLE 0x20000000 /* Zero memory under memory pressure. */
/*
* Flags for msync
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 71e5039d940d..b3bd8432f869 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -709,6 +709,9 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
#endif
#ifdef CONFIG_64BIT
[ilog2(VM_SEALED)] = "sl",
+#endif
+#ifdef CONFIG_NEED_VM_DROPPABLE
+ [ilog2(VM_DROPPABLE)] = "dp",
#endif
};
size_t i;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index eb7c96d24ac0..92454a0272ce 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -321,12 +321,14 @@ extern unsigned int kobjsize(const void *objp);
#define VM_HIGH_ARCH_BIT_3 35 /* bit only usable on 64-bit architectures */
#define VM_HIGH_ARCH_BIT_4 36 /* bit only usable on 64-bit architectures */
#define VM_HIGH_ARCH_BIT_5 37 /* bit only usable on 64-bit architectures */
+#define VM_HIGH_ARCH_BIT_6 38 /* bit only usable on 64-bit architectures */
#define VM_HIGH_ARCH_0 BIT(VM_HIGH_ARCH_BIT_0)
#define VM_HIGH_ARCH_1 BIT(VM_HIGH_ARCH_BIT_1)
#define VM_HIGH_ARCH_2 BIT(VM_HIGH_ARCH_BIT_2)
#define VM_HIGH_ARCH_3 BIT(VM_HIGH_ARCH_BIT_3)
#define VM_HIGH_ARCH_4 BIT(VM_HIGH_ARCH_BIT_4)
#define VM_HIGH_ARCH_5 BIT(VM_HIGH_ARCH_BIT_5)
+#define VM_HIGH_ARCH_6 BIT(VM_HIGH_ARCH_BIT_6)
#endif /* CONFIG_ARCH_USES_HIGH_VMA_FLAGS */
#ifdef CONFIG_ARCH_HAS_PKEYS
@@ -357,6 +359,12 @@ extern unsigned int kobjsize(const void *objp);
# define VM_SHADOW_STACK VM_NONE
#endif
+#ifdef CONFIG_NEED_VM_DROPPABLE
+# define VM_DROPPABLE VM_HIGH_ARCH_6
+#else
+# define VM_DROPPABLE VM_NONE
+#endif
+
#if defined(CONFIG_X86)
# define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
#elif defined(CONFIG_PPC)
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index e46d6e82765e..fab7848df50a 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -165,6 +165,12 @@ IF_HAVE_PG_ARCH_X(arch_3)
# define IF_HAVE_UFFD_MINOR(flag, name)
#endif
+#ifdef CONFIG_NEED_VM_DROPPABLE
+# define IF_HAVE_VM_DROPPABLE(flag, name) {flag, name},
+#else
+# define IF_HAVE_VM_DROPPABLE(flag, name)
+#endif
+
#define __def_vmaflag_names \
{VM_READ, "read" }, \
{VM_WRITE, "write" }, \
@@ -197,6 +203,7 @@ IF_HAVE_VM_SOFTDIRTY(VM_SOFTDIRTY, "softdirty" ) \
{VM_MIXEDMAP, "mixedmap" }, \
{VM_HUGEPAGE, "hugepage" }, \
{VM_NOHUGEPAGE, "nohugepage" }, \
+IF_HAVE_VM_DROPPABLE(VM_DROPPABLE, "droppable" ) \
{VM_MERGEABLE, "mergeable" } \
#define show_vma_flags(flags) \
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index 6ce1f1ceb432..65a3069462a8 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -33,6 +33,10 @@
#define MAP_UNINITIALIZED 0x4000000 /* For anonymous mmap, memory could be
* uninitialized */
+#define MAP_WIPEONFORK 0x08000000 /* Zero memory in child forks. */
+#define MAP_DONTDUMP 0x10000000 /* Do not write to coredumps. */
+#define MAP_DROPPABLE 0x20000000 /* Zero memory under memory pressure. */
+
/*
* Flags for mlock
*/
diff --git a/mm/Kconfig b/mm/Kconfig
index b4cb45255a54..6cd65ea4b3ad 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -1056,6 +1056,9 @@ config ARCH_USES_HIGH_VMA_FLAGS
bool
config ARCH_HAS_PKEYS
bool
+config NEED_VM_DROPPABLE
+ select ARCH_USES_HIGH_VMA_FLAGS
+ bool
config ARCH_USES_PG_ARCH_X
bool
diff --git a/mm/mmap.c b/mm/mmap.c
index 83b4682ec85c..e361f6750201 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1278,6 +1278,21 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
vm_flags |= calc_vm_prot_bits(prot, pkey) | calc_vm_flag_bits(flags) |
mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
+ if (flags & MAP_WIPEONFORK) {
+ /* MAP_WIPEONFORK is only supported on anonymous memory. */
+ if (file || !(flags & MAP_PRIVATE))
+ return -EINVAL;
+ vm_flags |= VM_WIPEONFORK;
+ }
+ if (flags & MAP_DONTDUMP)
+ vm_flags |= VM_DONTDUMP;
+ if (flags & MAP_DROPPABLE) {
+ /* MAP_DROPPABLE is only supported on anonymous memory. */
+ if (file || !(flags & MAP_PRIVATE))
+ return -EINVAL;
+ vm_flags |= VM_DROPPABLE;
+ }
+
/* Obtain the address to map to. we verify (or select) it and ensure
* that it represents a valid section of the address space.
*/
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 8c6cd8825273..57b8dad9adcc 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -623,7 +623,7 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
may_expand_vm(mm, oldflags, nrpages))
return -ENOMEM;
if (!(oldflags & (VM_ACCOUNT|VM_WRITE|VM_HUGETLB|
- VM_SHARED|VM_NORESERVE))) {
+ VM_SHARED|VM_NORESERVE|VM_DROPPABLE))) {
charged = nrpages;
if (security_vm_enough_memory_mm(mm, charged))
return -ENOMEM;
diff --git a/mm/rmap.c b/mm/rmap.c
index e8fc5ecb59b2..56d7535d5cf6 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1397,7 +1397,10 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
VM_WARN_ON_FOLIO(folio_test_hugetlb(folio), folio);
VM_BUG_ON_VMA(address < vma->vm_start ||
address + (nr << PAGE_SHIFT) > vma->vm_end, vma);
- __folio_set_swapbacked(folio);
+ /* VM_DROPPABLE mappings don't swap; instead they're just dropped when
+ * under memory pressure. */
+ if (!(vma->vm_flags & VM_DROPPABLE))
+ __folio_set_swapbacked(folio);
__folio_set_anon(folio, vma, address, true);
if (likely(!folio_test_large(folio))) {
@@ -1841,7 +1844,11 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
* plus the rmap(s) (dropped by discard:).
*/
if (ref_count == 1 + map_count &&
- !folio_test_dirty(folio)) {
+ (!folio_test_dirty(folio) ||
+ /* Unlike MADV_FREE mappings, VM_DROPPABLE
+ * ones can be dropped even if they've
+ * been dirtied. */
+ (vma->vm_flags & VM_DROPPABLE))) {
dec_mm_counter(mm, MM_ANONPAGES);
goto discard;
}
@@ -1851,7 +1858,10 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
* discarded. Remap the page to page table.
*/
set_pte_at(mm, address, pvmw.pte, pteval);
- folio_set_swapbacked(folio);
+ /* Unlike MADV_FREE mappings, VM_DROPPABLE ones
+ * never get swap backed on failure to drop. */
+ if (!(vma->vm_flags & VM_DROPPABLE))
+ folio_set_swapbacked(folio);
ret = false;
page_vma_mapped_walk_done(&pvmw);
break;
diff --git a/tools/include/uapi/asm-generic/mman-common.h b/tools/include/uapi/asm-generic/mman-common.h
index 6ce1f1ceb432..65a3069462a8 100644
--- a/tools/include/uapi/asm-generic/mman-common.h
+++ b/tools/include/uapi/asm-generic/mman-common.h
@@ -33,6 +33,10 @@
#define MAP_UNINITIALIZED 0x4000000 /* For anonymous mmap, memory could be
* uninitialized */
+#define MAP_WIPEONFORK 0x08000000 /* Zero memory in child forks. */
+#define MAP_DONTDUMP 0x10000000 /* Do not write to coredumps. */
+#define MAP_DROPPABLE 0x20000000 /* Zero memory under memory pressure. */
+
/*
* Flags for mlock
*/
diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
index 0b9ab987601c..a8beeb43c2b5 100644
--- a/tools/testing/selftests/mm/.gitignore
+++ b/tools/testing/selftests/mm/.gitignore
@@ -49,3 +49,4 @@ hugetlb_fault_after_madv
hugetlb_madv_vs_map
mseal_test
seal_elf
+droppable
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 3b49bc3d0a3b..e3e5740e13e1 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -73,6 +73,7 @@ TEST_GEN_FILES += ksm_functional_tests
TEST_GEN_FILES += mdwe_test
TEST_GEN_FILES += hugetlb_fault_after_madv
TEST_GEN_FILES += hugetlb_madv_vs_map
+TEST_GEN_FILES += droppable
ifneq ($(ARCH),arm64)
TEST_GEN_FILES += soft-dirty
diff --git a/tools/testing/selftests/mm/droppable.c b/tools/testing/selftests/mm/droppable.c
new file mode 100644
index 000000000000..846fb9aea4d1
--- /dev/null
+++ b/tools/testing/selftests/mm/droppable.c
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ */
+
+#include <assert.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <signal.h>
+#include <sys/mman.h>
+#include <linux/mman.h>
+
+#include "../kselftest.h"
+
+int main(int argc, char *argv[])
+{
+ size_t alloc_size = 134217728;
+ size_t page_size = getpagesize();
+ void *alloc;
+ pid_t child;
+
+ ksft_print_header();
+ ksft_set_plan(1);
+
+ alloc = mmap(0, alloc_size, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE | MAP_DROPPABLE, -1, 0);
+ assert(alloc != MAP_FAILED);
+ memset(alloc, 'A', alloc_size);
+ for (size_t i = 0; i < alloc_size; i += page_size)
+ assert(*(uint8_t *)(alloc + i));
+
+ child = fork();
+ assert(child >= 0);
+ if (!child) {
+ for (;;)
+ memset(malloc(page_size), 'A', page_size);
+ }
+
+ for (bool done = false; !done;) {
+ for (size_t i = 0; i < alloc_size; i += page_size) {
+ if (!*(uint8_t *)(alloc + i)) {
+ done = true;
+ break;
+ }
+ }
+ }
+ kill(child, SIGTERM);
+
+ ksft_test_result_pass("VM_DROPPABLE: PASS\n");
+ exit(KSFT_PASS);
+}
--
2.45.2
^ permalink raw reply related
* [PATCH v21 0/4] implement getrandom() in vDSO
From: Jason A. Donenfeld @ 2024-07-07 0:26 UTC (permalink / raw)
To: linux-kernel, patches, tglx
Cc: Jason A. Donenfeld, linux-crypto, linux-api, x86, Linus Torvalds,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
The plan for this series is to take it through my random.git tree for 6.11.
It's cooking in linux-next now.
Changes v20->v21:
- After extensive conversation with Linus, we're nixing the entire
vgetrandom_alloc() syscall, in favor of just exposing the functionality
needed through mmap() and having the kernel communicate to the caller what
arguments/sizes it should pass to mmap(). This simplifies the series
considerably. It also means that the first commit adds some new MAP_*
constants for mmap().
- Separate vDSO selftests out into separate commit.
--------------
Useful links:
- This series:
- https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/log/
- In case you're actually interested in the v≤14 design where faults were
non-fatal and instructions were skipped (which I think is more coherent, even
if the implementation is controversial), this lives in my branch here:
- https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/log/?h=jd/vdso-skip-insn
Note that I'm *not* actually proposing this for upstream at this time. But it
may be of conversational interest.
-------------
Two statements:
1) Userspace wants faster cryptographically secure random numbers of
arbitrary size, big or small.
2) Userspace is currently unable to safely roll its own RNG with the
same security profile as getrandom().
Statement (1) has been debated for years, with arguments ranging from
"we need faster cryptographically secure card shuffling!" to "the only
things that actually need good randomness are keys, which are few and
far between" to "actually, TLS CBC nonces are frequent" and so on. I
don't intend to wade into that debate substantially, except to note that
recently glibc added arc4random(), whose goal is to return a
cryptographically secure uint32_t, and there are real user reports of it
being too slow. So here we are.
Statement (2) is more interesting. The kernel is the nexus of all
entropic inputs that influence the RNG. It is in the best position, and
probably the only position, to decide anything at all about the current
state of the RNG and of its entropy. One of the things it uniquely knows
about is when reseeding is necessary.
For example, when a virtual machine is forked, restored, or duplicated,
it's imparative that the RNG doesn't generate the same outputs. For this
reason, there's a small protocol between hypervisors and the kernel that
indicates this has happened, alongside some ID, which the RNG uses to
immediately reseed, so as not to return the same numbers. Were userspace
to expand a getrandom() seed from time T1 for the next hour, and at some
point T2 < hour, the virtual machine forked, userspace would continue to
provide the same numbers to two (or more) different virtual machines,
resulting in potential cryptographic catastrophe. Something similar
happens on resuming from hibernation (or even suspend), with various
compromise scenarios there in mind.
There's a more general reason why userspace rolling its own RNG from a
getrandom() seed is fraught. There's a lot of attention paid to this
particular Linuxism we have of the RNG being initialized and thus
non-blocking or uninitialized and thus blocking until it is initialized.
These are our Two Big States that many hold to be the holy
differentiating factor between safe and not safe, between
cryptographically secure and garbage. The fact is, however, that the
distinction between these two states is a hand-wavy wishy-washy inexact
approximation. Outside of a few exceptional cases (e.g. a HW RNG is
available), we actually don't really ever know with any rigor at all
when the RNG is safe and ready (nor when it's compromised). We do the
best we can to "estimate" it, but entropy estimation is fundamentally
impossible in the general case. So really, we're just doing guess work,
and hoping it's good and conservative enough. Let's then assume that
there's always some potential error involved in this differentiator.
In fact, under the surface, the RNG is engineered around a different
principle, and that is trying to *use* new entropic inputs regularly and
at the right specific moments in time. For example, close to boot time,
the RNG reseeds itself more often than later. At certain events, like VM
fork, the RNG reseeds itself immediately. The various heuristics for
when the RNG will use new entropy and how often is really a core aspect
of what the RNG has some potential to do decently enough (and something
that will probably continue to improve in the future from random.c's
present set of algorithms). So in your mind, put away the metal
attachment to the Two Big States, which represent an approximation with
a potential margin of error. Instead keep in mind that the RNG's primary
operating heuristic is how often and exactly when it's going to reseed.
So, if userspace takes a seed from getrandom() at point T1, and uses it
for the next hour (or N megabytes or some other meaningless metric),
during that time, potential errors in the Two Big States approximation
are amplified. During that time potential reseeds are being lost,
forgotten, not reflected in the output stream. That's not good.
The simplest statement you could make is that userspace RNGs that expand
a getrandom() seed at some point T1 are nearly always *worse*, in some
way, than just calling getrandom() every time a random number is
desired.
For those reasons, after some discussion on libc-alpha, glibc's
arc4random() now just calls getrandom() on each invocation. That's
trivially safe, and gives us latitude to then make the safe thing faster
without becoming unsafe at our leasure. Card shuffling isn't
particularly fast, however.
How do we rectify this? By putting a safe implementation of getrandom()
in the vDSO, which has access to whatever information a
particular iteration of random.c is using to make its decisions. I use
that careful language of "particular iteration of random.c", because the
set of things that a vDSO getrandom() implementation might need for making
decisions as good as the kernel's will likely change over time. This
isn't just a matter of exporting certain *data* to userspace. We're not
going to commit to a "data API" where the various heuristics used are
exposed, locking in how the kernel works for decades to come, and then
leave it to various userspaces to roll something on top and shoot
themselves in the foot and have all sorts of complexity disasters.
Rather, vDSO getrandom() is supposed to be the *same exact algorithm*
that runs in the kernel, except it's been hoisted into userspace as
much as possible. And so vDSO getrandom() and kernel getrandom() will
always mirror each other hermetically.
API-wise, the vDSO gains this function:
ssize_t vgetrandom(void *buffer, size_t len, unsigned int flags,
void *opaque_state, size_t opaque_len);
The return value and the first 3 arguments are the same as ordinary
getrandom(), while the penultimate argument is a pointer to some state
allocated with the right flags passed to mmap(2), explained below. Were all
five arguments passed to the getrandom syscall, nothing different would happen,
and the functions would have the exact same behavior.
If vgetrandom(NULL, 0, 0, ¶ms, ~0UL) is called, then params gets populated
with information about what flags and prot fields to pass to mmap(2), as well
as how big each state should be, so that the caller can slice up returned
memory from mmap(2) into chunks for passing to vgetrandom().
Libc is expected to allocate a chunk of these on first use, and then
dole them out to threads as they're created, allocating more when
needed.
The interesting meat of the implementation is in lib/vdso/getrandom.c,
as generic C code, and it aims to mainly follow random.c's buffered fast
key erasure logic. Before the RNG is initialized, it falls back to the
syscall. Right now it uses a simple generation counter to make its decisions
on reseeding (though this could be made more extensive over time).
The actual place that has the most work to do is in all of the other
files. Most of the vDSO shared page infrastructure is centered around
gettimeofday, and so the main structs are all in arrays for different
timestamp types, and attached to time namespaces, and so forth. I've
done the best I could to add onto this in an unintrusive way.
In my test results, performance is pretty stellar (around 15x for uint32_t
generation), and it seems to be working. There's an extended example in the
last commit of this series, showing how the syscall and the vDSO function
are meant to be used together.
Cc: linux-crypto@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: x86@kernel.org
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
Cc: Carlos O'Donell <carlos@redhat.com>
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jann Horn <jannh@google.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: David Hildenbrand <dhildenb@redhat.com>
Jason A. Donenfeld (4):
mm: add VM_DROPPABLE for designating always lazily freeable mappings
random: introduce generic vDSO getrandom() implementation
x86: vdso: Wire up getrandom() vDSO implementation
selftests/vDSO: add tests for vgetrandom
MAINTAINERS | 4 +
arch/alpha/include/uapi/asm/mman.h | 3 +
arch/mips/include/uapi/asm/mman.h | 3 +
arch/parisc/include/uapi/asm/mman.h | 3 +
arch/x86/Kconfig | 1 +
arch/x86/entry/vdso/Makefile | 3 +-
arch/x86/entry/vdso/vdso.lds.S | 2 +
arch/x86/entry/vdso/vgetrandom-chacha.S | 178 +++++++++++
arch/x86/entry/vdso/vgetrandom.c | 17 ++
arch/x86/include/asm/vdso/getrandom.h | 55 ++++
arch/x86/include/asm/vdso/vsyscall.h | 2 +
arch/x86/include/asm/vvar.h | 16 +
arch/xtensa/include/uapi/asm/mman.h | 3 +
drivers/char/random.c | 18 +-
fs/proc/task_mmu.c | 3 +
include/linux/mm.h | 8 +
include/trace/events/mmflags.h | 7 +
include/uapi/asm-generic/mman-common.h | 4 +
include/uapi/linux/random.h | 15 +
include/vdso/datapage.h | 11 +
include/vdso/getrandom.h | 46 +++
lib/vdso/Kconfig | 6 +
lib/vdso/getrandom.c | 252 +++++++++++++++
mm/Kconfig | 3 +
mm/mmap.c | 15 +
mm/mprotect.c | 2 +-
mm/rmap.c | 16 +-
tools/include/asm/rwonce.h | 0
tools/include/uapi/asm-generic/mman-common.h | 4 +
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
tools/testing/selftests/mm/droppable.c | 53 ++++
tools/testing/selftests/vDSO/.gitignore | 2 +
tools/testing/selftests/vDSO/Makefile | 15 +
.../testing/selftests/vDSO/vdso_test_chacha.c | 43 +++
.../selftests/vDSO/vdso_test_getrandom.c | 288 ++++++++++++++++++
36 files changed, 1097 insertions(+), 6 deletions(-)
create mode 100644 arch/x86/entry/vdso/vgetrandom-chacha.S
create mode 100644 arch/x86/entry/vdso/vgetrandom.c
create mode 100644 arch/x86/include/asm/vdso/getrandom.h
create mode 100644 include/vdso/getrandom.h
create mode 100644 lib/vdso/getrandom.c
create mode 100644 tools/include/asm/rwonce.h
create mode 100644 tools/testing/selftests/mm/droppable.c
create mode 100644 tools/testing/selftests/vDSO/vdso_test_chacha.c
create mode 100644 tools/testing/selftests/vDSO/vdso_test_getrandom.c
base-commit: 22a40d14b572deb80c0648557f4bd502d7e83826
--
2.45.2
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Jarkko Sakkinen @ 2024-07-06 17:28 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Kees Cook, Al Viro, Christian Brauner, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240706.SieHeiMie8fa@digikod.net>
On Sat Jul 6, 2024 at 5:56 PM EEST, Mickaël Salaün wrote:
> On Sat, Jul 06, 2024 at 01:22:06AM +0300, Jarkko Sakkinen wrote:
> > On Sat Jul 6, 2024 at 12:44 AM EEST, Kees Cook wrote:
> > > > As explained in the UAPI comments, all parent processes need to be
> > > > trusted. This meeans that their code is trusted, their seccomp filters
> > > > are trusted, and that they are patched, if needed, to check file
> > > > executability.
> > >
> > > But we have launchers that apply arbitrary seccomp policy, e.g. minijail
> > > on Chrome OS, or even systemd on regular distros. In theory, this should
> > > be handled via other ACLs.
> >
> > Or a regular web browser? AFAIK seccomp filtering was the tool to make
> > secure browser tabs in the first place.
>
> Yes, and that't OK. Web browsers embedded their own seccomp filters and
> they are then as trusted as the browser code.
I'd recommend to slice of tech detail from cover letter, as long as
those details are in the commit messages.
Then, in the cover letter I'd go through maybe two familiar scenarios,
with interactions to this functionality.
A desktop web browser could be perhaps one of them...
BR, Jarkko
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Florian Weimer @ 2024-07-06 15:32 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240706.poo9ahd3La9b@digikod.net>
* Mickaël Salaün:
> On Fri, Jul 05, 2024 at 08:03:14PM +0200, Florian Weimer wrote:
>> * Mickaël Salaün:
>>
>> > Add a new AT_CHECK flag to execveat(2) to check if a file would be
>> > allowed for execution. The main use case is for script interpreters and
>> > dynamic linkers to check execution permission according to the kernel's
>> > security policy. Another use case is to add context to access logs e.g.,
>> > which script (instead of interpreter) accessed a file. As any
>> > executable code, scripts could also use this check [1].
>>
>> Some distributions no longer set executable bits on most shared objects,
>> which I assume would interfere with AT_CHECK probing for shared objects.
>
> A file without the execute permission is not considered as executable by
> the kernel. The AT_CHECK flag doesn't change this semantic. Please
> note that this is just a check, not a restriction. See the next patch
> for the optional policy enforcement.
>
> Anyway, we need to define the policy, and for Linux this is done with
> the file permission bits. So for systems willing to have a consistent
> execution policy, we need to rely on the same bits.
Yes, that makes complete sense. I just wanted to point out the odd
interaction with the old binutils bug and the (sadly still current)
kernel bug.
>> Removing the executable bit is attractive because of a combination of
>> two bugs: a binutils wart which until recently always set the entry
>> point address in the ELF header to zero, and the kernel not checking for
>> a zero entry point (maybe in combination with an absent program
>> interpreter) and failing the execve with ELIBEXEC, instead of doing the
>> execve and then faulting at virtual address zero. Removing the
>> executable bit is currently the only way to avoid these confusing
>> crashes, so I understand the temptation.
>
> Interesting. Can you please point to the bug report and the fix? I
> don't see any ELIBEXEC in the kernel.
The kernel hasn't been fixed yet. I do think this should be fixed, so
that distributions can bring back the executable bit.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-07-06 14:55 UTC (permalink / raw)
To: Florian Weimer
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <87bk3bvhr1.fsf@oldenburg.str.redhat.com>
On Fri, Jul 05, 2024 at 08:03:14PM +0200, Florian Weimer wrote:
> * Mickaël Salaün:
>
> > Add a new AT_CHECK flag to execveat(2) to check if a file would be
> > allowed for execution. The main use case is for script interpreters and
> > dynamic linkers to check execution permission according to the kernel's
> > security policy. Another use case is to add context to access logs e.g.,
> > which script (instead of interpreter) accessed a file. As any
> > executable code, scripts could also use this check [1].
>
> Some distributions no longer set executable bits on most shared objects,
> which I assume would interfere with AT_CHECK probing for shared objects.
A file without the execute permission is not considered as executable by
the kernel. The AT_CHECK flag doesn't change this semantic. Please
note that this is just a check, not a restriction. See the next patch
for the optional policy enforcement.
Anyway, we need to define the policy, and for Linux this is done with
the file permission bits. So for systems willing to have a consistent
execution policy, we need to rely on the same bits.
> Removing the executable bit is attractive because of a combination of
> two bugs: a binutils wart which until recently always set the entry
> point address in the ELF header to zero, and the kernel not checking for
> a zero entry point (maybe in combination with an absent program
> interpreter) and failing the execve with ELIBEXEC, instead of doing the
> execve and then faulting at virtual address zero. Removing the
> executable bit is currently the only way to avoid these confusing
> crashes, so I understand the temptation.
Interesting. Can you please point to the bug report and the fix? I
don't see any ELIBEXEC in the kernel.
FYI, AT_CHECK doesn't check the content of the file (unlike a full
execve call).
Anyway, I think we should not design a new kernel interface to work
around a current user space bug.
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Mickaël Salaün @ 2024-07-06 14:56 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Kees Cook, Al Viro, Christian Brauner, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <D2HYFLLXVYLS.ORASE7L62L3N@kernel.org>
On Sat, Jul 06, 2024 at 01:22:06AM +0300, Jarkko Sakkinen wrote:
> On Sat Jul 6, 2024 at 12:44 AM EEST, Kees Cook wrote:
> > > As explained in the UAPI comments, all parent processes need to be
> > > trusted. This meeans that their code is trusted, their seccomp filters
> > > are trusted, and that they are patched, if needed, to check file
> > > executability.
> >
> > But we have launchers that apply arbitrary seccomp policy, e.g. minijail
> > on Chrome OS, or even systemd on regular distros. In theory, this should
> > be handled via other ACLs.
>
> Or a regular web browser? AFAIK seccomp filtering was the tool to make
> secure browser tabs in the first place.
Yes, and that't OK. Web browsers embedded their own seccomp filters and
they are then as trusted as the browser code.
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Mickaël Salaün @ 2024-07-06 14:56 UTC (permalink / raw)
To: Kees Cook
Cc: Al Viro, Christian Brauner, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <202407051425.32AF9D2@keescook>
On Fri, Jul 05, 2024 at 02:44:03PM -0700, Kees Cook wrote:
> On Fri, Jul 05, 2024 at 07:54:16PM +0200, Mickaël Salaün wrote:
> > On Thu, Jul 04, 2024 at 05:18:04PM -0700, Kees Cook wrote:
> > > On Thu, Jul 04, 2024 at 09:01:34PM +0200, Mickaël Salaün wrote:
> > > > Such a secure environment can be achieved with an appropriate access
> > > > control policy (e.g. mount's noexec option, file access rights, LSM
> > > > configuration) and an enlighten ld.so checking that libraries are
> > > > allowed for execution e.g., to protect against illegitimate use of
> > > > LD_PRELOAD.
> > > >
> > > > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > > > environment variables), but that is outside the scope of the kernel.
> > >
> > > If the threat model includes an attacker sitting at a shell prompt, we
> > > need to be very careful about how process perform enforcement. E.g. even
> > > on a locked down system, if an attacker has access to LD_PRELOAD or a
> >
> > LD_PRELOAD should be OK once ld.so will be patched to check the
> > libraries. We can still imagine a debug library used to bypass security
> > checks, but in this case the issue would be that this library is
> > executable in the first place.
>
> Ah yes, that's fair: the shell would discover the malicious library
> while using AT_CHECK during resolution of the LD_PRELOAD.
That's the idea, but it would be checked by ld.so, not the shell.
>
> > > seccomp wrapper (which you both mention here), it would be possible to
> > > run commands where the resulting process is tricked into thinking it
> > > doesn't have the bits set.
> >
> > As explained in the UAPI comments, all parent processes need to be
> > trusted. This meeans that their code is trusted, their seccomp filters
> > are trusted, and that they are patched, if needed, to check file
> > executability.
>
> But we have launchers that apply arbitrary seccomp policy, e.g. minijail
> on Chrome OS, or even systemd on regular distros. In theory, this should
> be handled via other ACLs.
Processes running with untrusted seccomp filter should be considered
untrusted. It would then make sense for these seccomp filters/programs
to be considered executable code, and then for minijail and systemd to
check them with AT_CHECK (according to the securebits policy).
>
> > > But this would be exactly true for calling execveat(): LD_PRELOAD or
> > > seccomp policy could have it just return 0.
> >
> > If an attacker is allowed/able to load an arbitrary seccomp filter on a
> > process, we cannot trust this process.
> >
> > >
> > > While I like AT_CHECK, I do wonder if it's better to do the checks via
> > > open(), as was originally designed with O_MAYEXEC. Because then
> > > enforcement is gated by the kernel -- the process does not get a file
> > > descriptor _at all_, no matter what LD_PRELOAD or seccomp tricks it into
> > > doing.
> >
> > Being able to check a path name or a file descriptor (with the same
> > syscall) is more flexible and cover more use cases.
>
> If flexibility costs us reliability, I think that flexibility is not
> a benefit.
Well, it's a matter of letting user space do what they think is best,
and I think there are legitimate and safe uses of path names, even if I
agree that this should not be used in most use cases. Would we want
faccessat2(2) to only take file descriptor as argument and not file
path? I don't think so but I'd defer to the VFS maintainers.
Christian, Al, Linus?
Steve, could you share a use case with file paths?
>
> > The execveat(2)
> > interface, including current and future flags, is dedicated to file
> > execution. I then think that using execveat(2) for this kind of check
> > makes more sense, and will easily evolve with this syscall.
>
> Yeah, I do recognize that is feels much more natural, but I remain
> unhappy about how difficult it will become to audit a system for safety
> when the check is strictly per-process opt-in, and not enforced by the
> kernel for a given process tree. But, I think this may have always been
> a fiction in my mind. :)
Hmm, I'm not sure to follow. Securebits are inherited, so process tree.
And we need the parent processes to be trusted anyway.
>
> > > And this thinking also applies to faccessat() too: if a process can be
> > > tricked into thinking the access check passed, it'll happily interpret
> > > whatever. :( But not being able to open the fd _at all_ when O_MAYEXEC
> > > is being checked seems substantially safer to me...
> >
> > If attackers can filter execveat(2), they can also filter open(2) and
> > any other syscalls. In all cases, that would mean an issue in the
> > security policy.
>
> Hm, as in, make a separate call to open(2) without O_MAYEXEC, and pass
> that fd back to the filtered open(2) that did have O_MAYEXEC. Yes, true.
>
> I guess it does become morally equivalent.
>
> Okay. Well, let me ask about usability. Right now, a process will need
> to do:
>
> - should I use AT_CHECK? (check secbit)
> - if yes: perform execveat(AT_CHECK)
>
> Why not leave the secbit test up to the kernel, and then the program can
> just unconditionally call execveat(AT_CHECK)?
That was kind of the approach of the previous patch series and Linus
wanted the new interface to follow the kernel semantic. Enforcing this
kind of restriction will always be the duty of user space anyway, so I
think it's simpler (i.e. no mix of policy definition, access check, and
policy enforcement, but a standalone execveat feature), more flexible,
and it fully delegates the policy enforcement to user space instead of
trying to enforce some part in the kernel which would only give the
illusion of security/policy enforcement.
>
> Though perhaps the issue here is that an execveat() EINVAL doesn't
> tell the program if AT_CHECK is unimplemented or if something else
> went wrong, and the secbit prctl() will give the correct signal about
> AT_CHECK availability?
This kind of check could indeed help to identify the issue.
^ permalink raw reply
* Re: [PATCH v3] platform/x86: asus-wmi: support the disable camera LED on F10 of Zenbook 2023
From: Ilpo Järvinen @ 2024-07-06 13:44 UTC (permalink / raw)
To: Devin Bayer
Cc: corentin.chary, luke, Hans de Goede, platform-driver-x86, LKML,
linux-api
In-Reply-To: <20240628084603.217106-1-dev@doubly.so>
On Fri, 28 Jun 2024, Devin Bayer wrote:
> Adds a sysfs entry for the LED on F10 above the crossed out camera icon on 2023 Zenbooks.
Thanks for the update, I've applied this into review-ilpo branch with
small (non-code) edits.
In future, please fold the changelog to 72 characters and include patch
history only after the first --- line so tools don't put it into the
commit message. I've handled these for you this time around so there's no
need to send another version because of those problems.
--
i.
> v3
> - add docs for WMI devices
> - remove duplicate #define
>
> v2
> - Changed name from `platform::camera` to `asus::camera`
> - Separated patch from patchset
>
> v1
> - https://lore.kernel.org/platform-driver-x86/20240620082223.20178-1-dev@doubly.so/
>
> Signed-off-by: Devin Bayer <dev@doubly.so>
> ---
> drivers/platform/x86/asus-wmi.c | 35 ++++++++++++++++++++++
> include/linux/platform_data/x86/asus-wmi.h | 4 +++
> 2 files changed, 39 insertions(+)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 3f07bbf809ef..4a9ad8b313e6 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -227,6 +227,7 @@ struct asus_wmi {
> struct led_classdev lightbar_led;
> int lightbar_led_wk;
> struct led_classdev micmute_led;
> + struct led_classdev camera_led;
> struct workqueue_struct *led_workqueue;
> struct work_struct tpd_led_work;
> struct work_struct wlan_led_work;
> @@ -1533,6 +1534,27 @@ static int micmute_led_set(struct led_classdev *led_cdev,
> return err < 0 ? err : 0;
> }
>
> +static enum led_brightness camera_led_get(struct led_classdev *led_cdev)
> +{
> + struct asus_wmi *asus;
> + u32 result;
> +
> + asus = container_of(led_cdev, struct asus_wmi, camera_led);
> + asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_CAMERA_LED, &result);
> +
> + return result & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
> +}
> +
> +static int camera_led_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + int state = brightness != LED_OFF;
> + int err;
> +
> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_CAMERA_LED, state, NULL);
> + return err < 0 ? err : 0;
> +}
> +
> static void asus_wmi_led_exit(struct asus_wmi *asus)
> {
> led_classdev_unregister(&asus->kbd_led);
> @@ -1540,6 +1562,7 @@ static void asus_wmi_led_exit(struct asus_wmi *asus)
> led_classdev_unregister(&asus->wlan_led);
> led_classdev_unregister(&asus->lightbar_led);
> led_classdev_unregister(&asus->micmute_led);
> + led_classdev_unregister(&asus->camera_led);
>
> if (asus->led_workqueue)
> destroy_workqueue(asus->led_workqueue);
> @@ -1631,6 +1654,18 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
> goto error;
> }
>
> + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CAMERA_LED)) {
> + asus->camera_led.name = "asus::camera";
> + asus->camera_led.max_brightness = 1;
> + asus->camera_led.brightness_get = camera_led_get;
> + asus->camera_led.brightness_set_blocking = camera_led_set;
> +
> + rv = led_classdev_register(&asus->platform_device->dev,
> + &asus->camera_led);
> + if (rv)
> + goto error;
> + }
> +
> error:
> if (rv)
> asus_wmi_led_exit(asus);
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index ab1c7deff118..d020fcbbcfb7 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -51,6 +51,10 @@
> #define ASUS_WMI_DEVID_LED6 0x00020016
> #define ASUS_WMI_DEVID_MICMUTE_LED 0x00040017
>
> +/* Disable Camera LED */
> +#define ASUS_WMI_DEVID_CAMERA_LED_NEG 0x00060078 /* 0 = on (unused) */
> +#define ASUS_WMI_DEVID_CAMERA_LED 0x00060079 /* 1 = on */
> +
> /* Backlight and Brightness */
> #define ASUS_WMI_DEVID_ALS_ENABLE 0x00050001 /* Ambient Light Sensor */
> #define ASUS_WMI_DEVID_BACKLIGHT 0x00050011
>
> base-commit: 103a4e4a4351d3d5214c4f54fdf89f0f81b692ef
>
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Andy Lutomirski @ 2024-07-06 8:52 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240704190137.696169-2-mic@digikod.net>
On Fri, Jul 5, 2024 at 3:03 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> Add a new AT_CHECK flag to execveat(2) to check if a file would be
> allowed for execution. The main use case is for script interpreters and
> dynamic linkers to check execution permission according to the kernel's
> security policy. Another use case is to add context to access logs e.g.,
> which script (instead of interpreter) accessed a file. As any
> executable code, scripts could also use this check [1].
>
Can you give a worked-out example of how this is useful?
I assume the idea is that a program could open a file, then pass the
fd to execveat() to get the kernel's idea of whether it's permissible
to execute it. And then the program would interpret the file, which
is morally like executing it. And there would be a big warning in the
manpage that passing a *path* is subject to a TOCTOU race.
This type of usage will do the wrong thing if LSM policy intends to
lock down the task if the task were to actually exec the file. I
personally think this is a mis-design (let the program doing the
exec-ing lock itself down, possibly by querying a policy, but having
magic happen on exec seems likely to do the wrong thing more often
that it does the wright thing), but that ship sailed a long time ago.
So maybe what's actually needed is a rather different API: a way to
check *and perform* the security transition for an exec without
actually execing. This would need to be done NO_NEW_PRIVS style for
reasons that are hopefully obvious, but it would permit:
fd = open(some script);
if (do_exec_transition_without_exec(fd) != 0)
return; // don't actually do it
// OK, we may have just lost privileges. But that's okay, because we
meant to do that.
// Make sure we've munmapped anything sensitive and erased any secrets
from memory,
// and then interpret the script!
I think this would actually be straightforward to implement in the
kernel -- one would need to make sure that all the relevant
no_new_privs checks are looking in the right place (as the task might
not actually have no_new_privs set, but LSM_UNSAFE_NO_NEW_PRIVS would
still be set), but I don't see any reason this would be
insurmountable, nor do I expect there would be any fundamental
problems.
> This is different than faccessat(2) which only checks file access
> rights, but not the full context e.g. mount point's noexec, stack limit,
> and all potential LSM extra checks (e.g. argv, envp, credentials).
> Since the use of AT_CHECK follows the exact kernel semantic as for a
> real execution, user space gets the same error codes.
>
> With the information that a script interpreter is about to interpret a
> script, an LSM security policy can adjust caller's access rights or log
> execution request as for native script execution (e.g. role transition).
> This is possible thanks to the call to security_bprm_creds_for_exec().
>
> Because LSMs may only change bprm's credentials, use of AT_CHECK with
> current kernel code should not be a security issue (e.g. unexpected role
> transition). LSMs willing to update the caller's credential could now
> do so when bprm->is_check is set. Of course, such policy change should
> be in line with the new user space code.
>
> Because AT_CHECK is dedicated to user space interpreters, it doesn't
> make sense for the kernel to parse the checked files, look for
> interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> if the format is unknown. Because of that, security_bprm_check() is
> never called when AT_CHECK is used.
>
> It should be noted that script interpreters cannot directly use
> execveat(2) (without this new AT_CHECK flag) because this could lead to
> unexpected behaviors e.g., `python script.sh` could lead to Bash being
> executed to interpret the script. Unlike the kernel, script
> interpreters may just interpret the shebang as a simple comment, which
> should not change for backward compatibility reasons.
>
> Because scripts or libraries files might not currently have the
> executable permission set, or because we might want specific users to be
> allowed to run arbitrary scripts, the following patch provides a dynamic
> configuration mechanism with the SECBIT_SHOULD_EXEC_CHECK and
> SECBIT_SHOULD_EXEC_RESTRICT securebits.
Can you explain what those bits do? And why they're useful?
>
> This is a redesign of the CLIP OS 4's O_MAYEXEC:
> https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> This patch has been used for more than a decade with customized script
> interpreters. Some examples can be found here:
> https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
This one at least returns an fd, so it looks less likely to get
misused in a way that adds a TOCTOU race.
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Jarkko Sakkinen @ 2024-07-05 22:22 UTC (permalink / raw)
To: Kees Cook, Mickaël Salaün
Cc: Al Viro, Christian Brauner, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <202407051425.32AF9D2@keescook>
On Sat Jul 6, 2024 at 12:44 AM EEST, Kees Cook wrote:
> > As explained in the UAPI comments, all parent processes need to be
> > trusted. This meeans that their code is trusted, their seccomp filters
> > are trusted, and that they are patched, if needed, to check file
> > executability.
>
> But we have launchers that apply arbitrary seccomp policy, e.g. minijail
> on Chrome OS, or even systemd on regular distros. In theory, this should
> be handled via other ACLs.
Or a regular web browser? AFAIK seccomp filtering was the tool to make
secure browser tabs in the first place.
BR, Jarkko
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Kees Cook @ 2024-07-05 21:44 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240705.IeTheequ7Ooj@digikod.net>
On Fri, Jul 05, 2024 at 07:54:16PM +0200, Mickaël Salaün wrote:
> On Thu, Jul 04, 2024 at 05:18:04PM -0700, Kees Cook wrote:
> > On Thu, Jul 04, 2024 at 09:01:34PM +0200, Mickaël Salaün wrote:
> > > Such a secure environment can be achieved with an appropriate access
> > > control policy (e.g. mount's noexec option, file access rights, LSM
> > > configuration) and an enlighten ld.so checking that libraries are
> > > allowed for execution e.g., to protect against illegitimate use of
> > > LD_PRELOAD.
> > >
> > > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > > environment variables), but that is outside the scope of the kernel.
> >
> > If the threat model includes an attacker sitting at a shell prompt, we
> > need to be very careful about how process perform enforcement. E.g. even
> > on a locked down system, if an attacker has access to LD_PRELOAD or a
>
> LD_PRELOAD should be OK once ld.so will be patched to check the
> libraries. We can still imagine a debug library used to bypass security
> checks, but in this case the issue would be that this library is
> executable in the first place.
Ah yes, that's fair: the shell would discover the malicious library
while using AT_CHECK during resolution of the LD_PRELOAD.
> > seccomp wrapper (which you both mention here), it would be possible to
> > run commands where the resulting process is tricked into thinking it
> > doesn't have the bits set.
>
> As explained in the UAPI comments, all parent processes need to be
> trusted. This meeans that their code is trusted, their seccomp filters
> are trusted, and that they are patched, if needed, to check file
> executability.
But we have launchers that apply arbitrary seccomp policy, e.g. minijail
on Chrome OS, or even systemd on regular distros. In theory, this should
be handled via other ACLs.
> > But this would be exactly true for calling execveat(): LD_PRELOAD or
> > seccomp policy could have it just return 0.
>
> If an attacker is allowed/able to load an arbitrary seccomp filter on a
> process, we cannot trust this process.
>
> >
> > While I like AT_CHECK, I do wonder if it's better to do the checks via
> > open(), as was originally designed with O_MAYEXEC. Because then
> > enforcement is gated by the kernel -- the process does not get a file
> > descriptor _at all_, no matter what LD_PRELOAD or seccomp tricks it into
> > doing.
>
> Being able to check a path name or a file descriptor (with the same
> syscall) is more flexible and cover more use cases.
If flexibility costs us reliability, I think that flexibility is not
a benefit.
> The execveat(2)
> interface, including current and future flags, is dedicated to file
> execution. I then think that using execveat(2) for this kind of check
> makes more sense, and will easily evolve with this syscall.
Yeah, I do recognize that is feels much more natural, but I remain
unhappy about how difficult it will become to audit a system for safety
when the check is strictly per-process opt-in, and not enforced by the
kernel for a given process tree. But, I think this may have always been
a fiction in my mind. :)
> > And this thinking also applies to faccessat() too: if a process can be
> > tricked into thinking the access check passed, it'll happily interpret
> > whatever. :( But not being able to open the fd _at all_ when O_MAYEXEC
> > is being checked seems substantially safer to me...
>
> If attackers can filter execveat(2), they can also filter open(2) and
> any other syscalls. In all cases, that would mean an issue in the
> security policy.
Hm, as in, make a separate call to open(2) without O_MAYEXEC, and pass
that fd back to the filtered open(2) that did have O_MAYEXEC. Yes, true.
I guess it does become morally equivalent.
Okay. Well, let me ask about usability. Right now, a process will need
to do:
- should I use AT_CHECK? (check secbit)
- if yes: perform execveat(AT_CHECK)
Why not leave the secbit test up to the kernel, and then the program can
just unconditionally call execveat(AT_CHECK)?
Though perhaps the issue here is that an execveat() EINVAL doesn't
tell the program if AT_CHECK is unimplemented or if something else
went wrong, and the secbit prctl() will give the correct signal about
AT_CHECK availability?
--
Kees Cook
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Florian Weimer @ 2024-07-05 18:03 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240704190137.696169-2-mic@digikod.net>
* Mickaël Salaün:
> Add a new AT_CHECK flag to execveat(2) to check if a file would be
> allowed for execution. The main use case is for script interpreters and
> dynamic linkers to check execution permission according to the kernel's
> security policy. Another use case is to add context to access logs e.g.,
> which script (instead of interpreter) accessed a file. As any
> executable code, scripts could also use this check [1].
Some distributions no longer set executable bits on most shared objects,
which I assume would interfere with AT_CHECK probing for shared objects.
Removing the executable bit is attractive because of a combination of
two bugs: a binutils wart which until recently always set the entry
point address in the ELF header to zero, and the kernel not checking for
a zero entry point (maybe in combination with an absent program
interpreter) and failing the execve with ELIBEXEC, instead of doing the
execve and then faulting at virtual address zero. Removing the
executable bit is currently the only way to avoid these confusing
crashes, so I understand the temptation.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-07-05 17:53 UTC (permalink / raw)
To: Kees Cook
Cc: Al Viro, Christian Brauner, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <202407041656.3A05153@keescook>
On Thu, Jul 04, 2024 at 05:04:03PM -0700, Kees Cook wrote:
> On Thu, Jul 04, 2024 at 09:01:33PM +0200, Mickaël Salaün wrote:
> > Add a new AT_CHECK flag to execveat(2) to check if a file would be
> > allowed for execution. The main use case is for script interpreters and
> > dynamic linkers to check execution permission according to the kernel's
> > security policy. Another use case is to add context to access logs e.g.,
> > which script (instead of interpreter) accessed a file. As any
> > executable code, scripts could also use this check [1].
> >
> > This is different than faccessat(2) which only checks file access
> > rights, but not the full context e.g. mount point's noexec, stack limit,
> > and all potential LSM extra checks (e.g. argv, envp, credentials).
> > Since the use of AT_CHECK follows the exact kernel semantic as for a
> > real execution, user space gets the same error codes.
>
> Nice! I much prefer this method of going through the exec machinery so
> we always have a single code path for these kinds of checks.
>
> > Because AT_CHECK is dedicated to user space interpreters, it doesn't
> > make sense for the kernel to parse the checked files, look for
> > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > if the format is unknown. Because of that, security_bprm_check() is
> > never called when AT_CHECK is used.
>
> I'd like some additional comments in the code that reminds us that
> access control checks have finished past a certain point.
Where in the code? Just before the bprm->is_check assignment?
>
> [...]
> > diff --git a/fs/exec.c b/fs/exec.c
> > index 40073142288f..ea2a1867afdc 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -931,7 +931,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > .lookup_flags = LOOKUP_FOLLOW,
> > };
> >
> > - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > + if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_CHECK)) != 0)
> > return ERR_PTR(-EINVAL);
> > if (flags & AT_SYMLINK_NOFOLLOW)
> > open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> [...]
> > + * To avoid race conditions leading to time-of-check to time-of-use issues,
> > + * AT_CHECK should be used with AT_EMPTY_PATH to check against a file
> > + * descriptor instead of a path.
>
> I want this enforced by the kernel. Let's not leave trivial ToCToU
> foot-guns around. i.e.:
>
> if ((flags & AT_CHECK) == AT_CHECK && (flags & AT_EMPTY_PATH) == 0)
> return ERR_PTR(-EBADF);
There are valid use cases relying on pathnames. See Linus's comment:
https://lore.kernel.org/r/CAHk-=whb=XuU=LGKnJWaa7LOYQz9VwHs8SLfgLbT5sf2VAbX1A@mail.gmail.com
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Mickaël Salaün @ 2024-07-05 17:54 UTC (permalink / raw)
To: Kees Cook
Cc: Al Viro, Christian Brauner, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <202407041711.B7CD16B2@keescook>
On Thu, Jul 04, 2024 at 05:18:04PM -0700, Kees Cook wrote:
> On Thu, Jul 04, 2024 at 09:01:34PM +0200, Mickaël Salaün wrote:
> > Such a secure environment can be achieved with an appropriate access
> > control policy (e.g. mount's noexec option, file access rights, LSM
> > configuration) and an enlighten ld.so checking that libraries are
> > allowed for execution e.g., to protect against illegitimate use of
> > LD_PRELOAD.
> >
> > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > environment variables), but that is outside the scope of the kernel.
>
> If the threat model includes an attacker sitting at a shell prompt, we
> need to be very careful about how process perform enforcement. E.g. even
> on a locked down system, if an attacker has access to LD_PRELOAD or a
LD_PRELOAD should be OK once ld.so will be patched to check the
libraries. We can still imagine a debug library used to bypass security
checks, but in this case the issue would be that this library is
executable in the first place.
> seccomp wrapper (which you both mention here), it would be possible to
> run commands where the resulting process is tricked into thinking it
> doesn't have the bits set.
As explained in the UAPI comments, all parent processes need to be
trusted. This meeans that their code is trusted, their seccomp filters
are trusted, and that they are patched, if needed, to check file
executability.
>
> But this would be exactly true for calling execveat(): LD_PRELOAD or
> seccomp policy could have it just return 0.
If an attacker is allowed/able to load an arbitrary seccomp filter on a
process, we cannot trust this process.
>
> While I like AT_CHECK, I do wonder if it's better to do the checks via
> open(), as was originally designed with O_MAYEXEC. Because then
> enforcement is gated by the kernel -- the process does not get a file
> descriptor _at all_, no matter what LD_PRELOAD or seccomp tricks it into
> doing.
Being able to check a path name or a file descriptor (with the same
syscall) is more flexible and cover more use cases. The execveat(2)
interface, including current and future flags, is dedicated to file
execution. I then think that using execveat(2) for this kind of check
makes more sense, and will easily evolve with this syscall.
>
> And this thinking also applies to faccessat() too: if a process can be
> tricked into thinking the access check passed, it'll happily interpret
> whatever. :( But not being able to open the fd _at all_ when O_MAYEXEC
> is being checked seems substantially safer to me...
If attackers can filter execveat(2), they can also filter open(2) and
any other syscalls. In all cases, that would mean an issue in the
security policy.
^ permalink raw reply
* Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Kees Cook @ 2024-07-05 0:18 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240704190137.696169-3-mic@digikod.net>
On Thu, Jul 04, 2024 at 09:01:34PM +0200, Mickaël Salaün wrote:
> Such a secure environment can be achieved with an appropriate access
> control policy (e.g. mount's noexec option, file access rights, LSM
> configuration) and an enlighten ld.so checking that libraries are
> allowed for execution e.g., to protect against illegitimate use of
> LD_PRELOAD.
>
> Scripts may need some changes to deal with untrusted data (e.g. stdin,
> environment variables), but that is outside the scope of the kernel.
If the threat model includes an attacker sitting at a shell prompt, we
need to be very careful about how process perform enforcement. E.g. even
on a locked down system, if an attacker has access to LD_PRELOAD or a
seccomp wrapper (which you both mention here), it would be possible to
run commands where the resulting process is tricked into thinking it
doesn't have the bits set.
But this would be exactly true for calling execveat(): LD_PRELOAD or
seccomp policy could have it just return 0.
While I like AT_CHECK, I do wonder if it's better to do the checks via
open(), as was originally designed with O_MAYEXEC. Because then
enforcement is gated by the kernel -- the process does not get a file
descriptor _at all_, no matter what LD_PRELOAD or seccomp tricks it into
doing.
And this thinking also applies to faccessat() too: if a process can be
tricked into thinking the access check passed, it'll happily interpret
whatever. :( But not being able to open the fd _at all_ when O_MAYEXEC
is being checked seems substantially safer to me...
--
Kees Cook
^ permalink raw reply
* Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Kees Cook @ 2024-07-05 0:04 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Linus Torvalds, Paul Moore,
Theodore Ts'o, Alejandro Colomar, Aleksa Sarai, Andrew Morton,
Andy Lutomirski, Arnd Bergmann, Casey Schaufler, Christian Heimes,
Dmitry Vyukov, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20240704190137.696169-2-mic@digikod.net>
On Thu, Jul 04, 2024 at 09:01:33PM +0200, Mickaël Salaün wrote:
> Add a new AT_CHECK flag to execveat(2) to check if a file would be
> allowed for execution. The main use case is for script interpreters and
> dynamic linkers to check execution permission according to the kernel's
> security policy. Another use case is to add context to access logs e.g.,
> which script (instead of interpreter) accessed a file. As any
> executable code, scripts could also use this check [1].
>
> This is different than faccessat(2) which only checks file access
> rights, but not the full context e.g. mount point's noexec, stack limit,
> and all potential LSM extra checks (e.g. argv, envp, credentials).
> Since the use of AT_CHECK follows the exact kernel semantic as for a
> real execution, user space gets the same error codes.
Nice! I much prefer this method of going through the exec machinery so
we always have a single code path for these kinds of checks.
> Because AT_CHECK is dedicated to user space interpreters, it doesn't
> make sense for the kernel to parse the checked files, look for
> interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> if the format is unknown. Because of that, security_bprm_check() is
> never called when AT_CHECK is used.
I'd like some additional comments in the code that reminds us that
access control checks have finished past a certain point.
[...]
> diff --git a/fs/exec.c b/fs/exec.c
> index 40073142288f..ea2a1867afdc 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -931,7 +931,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> .lookup_flags = LOOKUP_FOLLOW,
> };
>
> - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_CHECK)) != 0)
> return ERR_PTR(-EINVAL);
> if (flags & AT_SYMLINK_NOFOLLOW)
> open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
[...]
> + * To avoid race conditions leading to time-of-check to time-of-use issues,
> + * AT_CHECK should be used with AT_EMPTY_PATH to check against a file
> + * descriptor instead of a path.
I want this enforced by the kernel. Let's not leave trivial ToCToU
foot-guns around. i.e.:
if ((flags & AT_CHECK) == AT_CHECK && (flags & AT_EMPTY_PATH) == 0)
return ERR_PTR(-EBADF);
--
Kees Cook
^ permalink raw reply
* [RFC PATCH v19 3/5] selftests/exec: Add tests for AT_CHECK and related securebits
From: Mickaël Salaün @ 2024-07-04 19:01 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o
Cc: Mickaël Salaün, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Eric Biggers, Eric Chiang,
Fan Wu, Florian Weimer, Geert Uytterhoeven, James Morris,
Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module
In-Reply-To: <20240704190137.696169-1-mic@digikod.net>
Test that checks performed by execveat(..., AT_CHECK) are consistent
with noexec mount points and file execute permissions.
Test that SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT are
inherited by child processes and that they can be pinned with the
appropriate SECBIT_SHOULD_EXEC_CHECK_LOCKED and
SECBIT_SHOULD_EXEC_RESTRICT_LOCKED bits.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20240704190137.696169-4-mic@digikod.net
---
Changes since v18:
* Rewrite tests with the new design: execveat/AT_CHECK and securebits.
* Simplify the capability dropping and improve it with the NOROOT
securebits.
* Replace most ASSERT with EXPECT.
* Fix NULL execve's argv to avoid kernel warning.
* Move tests to exec/
* Build a "false" static binary to test full execution path.
Changes since v14:
* Add Reviewed-by Kees Cook.
Changes since v13:
* Move -I to CFLAGS (suggested by Kees Cook).
* Update sysctl name.
Changes since v12:
* Fix Makefile's license.
Changes since v10:
* Update selftest Makefile.
Changes since v9:
* Rename the syscall and the sysctl.
* Update tests for enum trusted_for_usage
Changes since v8:
* Update with the dedicated syscall introspect_access(2) and the renamed
fs.introspection_policy sysctl.
* Remove check symlink which can't be use as is anymore.
* Use socketpair(2) to test UNIX socket.
Changes since v7:
* Update tests with faccessat2/AT_INTERPRETED, including new ones to
check that setting R_OK or W_OK returns EINVAL.
* Add tests for memfd, pipefs and nsfs.
* Rename and move back tests to a standalone directory.
Changes since v6:
* Add full combination tests for all file types, including block
devices, character devices, fifos, sockets and symlinks.
* Properly save and restore initial sysctl value for all tests.
Changes since v5:
* Refactor with FIXTURE_VARIANT, which make the tests much more easy to
read and maintain.
* Save and restore initial sysctl value (suggested by Kees Cook).
* Test with a sysctl value of 0.
* Check errno in sysctl_access_write test.
* Update tests for the CAP_SYS_ADMIN switch.
* Update tests to check -EISDIR (replacing -EACCES).
* Replace FIXTURE_DATA() with FIXTURE() (spotted by Kees Cook).
* Use global const strings.
Changes since v3:
* Replace RESOLVE_MAYEXEC with O_MAYEXEC.
* Add tests to check that O_MAYEXEC is ignored by open(2) and openat(2).
Changes since v2:
* Move tests from exec/ to openat2/ .
* Replace O_MAYEXEC with RESOLVE_MAYEXEC from openat2(2).
* Cleanup tests.
Changes since v1:
* Move tests from yama/ to exec/ .
* Fix _GNU_SOURCE in kselftest_harness.h .
* Add a new test sysctl_access_write to check if CAP_MAC_ADMIN is taken
into account.
* Test directory execution which is always forbidden since commit
73601ea5b7b1 ("fs/open.c: allow opening only regular files during
execve()"), and also check that even the root user can not bypass file
execution checks.
* Make sure delete_workspace() always as enough right to succeed.
* Cosmetic cleanup.
---
tools/testing/selftests/exec/.gitignore | 2 +
tools/testing/selftests/exec/Makefile | 8 +
tools/testing/selftests/exec/config | 2 +
tools/testing/selftests/exec/false.c | 5 +
tools/testing/selftests/exec/should-exec.c | 449 +++++++++++++++++++++
5 files changed, 466 insertions(+)
create mode 100644 tools/testing/selftests/exec/config
create mode 100644 tools/testing/selftests/exec/false.c
create mode 100644 tools/testing/selftests/exec/should-exec.c
diff --git a/tools/testing/selftests/exec/.gitignore b/tools/testing/selftests/exec/.gitignore
index 90c238ba6a4b..20e965dcc98e 100644
--- a/tools/testing/selftests/exec/.gitignore
+++ b/tools/testing/selftests/exec/.gitignore
@@ -9,8 +9,10 @@ execveat.ephemeral
execveat.denatured
non-regular
null-argv
+/false
/load_address_*
/recursion-depth
+/should-exec
xxxxxxxx*
pipe
S_I*.test
diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile
index fb4472ddffd8..fc0cb8925b02 100644
--- a/tools/testing/selftests/exec/Makefile
+++ b/tools/testing/selftests/exec/Makefile
@@ -2,15 +2,20 @@
CFLAGS = -Wall
CFLAGS += -Wno-nonnull
CFLAGS += -D_GNU_SOURCE
+CFLAGS += $(KHDR_INCLUDES)
+
+LDLIBS += -lcap
TEST_PROGS := binfmt_script.py
TEST_GEN_PROGS := execveat load_address_4096 load_address_2097152 load_address_16777216 non-regular
+TEST_GEN_PROGS_EXTENDED := false
TEST_GEN_FILES := execveat.symlink execveat.denatured script subdir
# Makefile is a run-time dependency, since it's accessed by the execveat test
TEST_FILES := Makefile
TEST_GEN_PROGS += recursion-depth
TEST_GEN_PROGS += null-argv
+TEST_GEN_PROGS += should-exec
EXTRA_CLEAN := $(OUTPUT)/subdir.moved $(OUTPUT)/execveat.moved $(OUTPUT)/xxxxx* \
$(OUTPUT)/S_I*.test
@@ -34,3 +39,6 @@ $(OUTPUT)/load_address_2097152: load_address.c
$(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=0x200000 -pie -static $< -o $@
$(OUTPUT)/load_address_16777216: load_address.c
$(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=0x1000000 -pie -static $< -o $@
+
+$(OUTPUT)/false: false.c
+ $(CC) $(CFLAGS) $(LDFLAGS) -static $< -o $@
diff --git a/tools/testing/selftests/exec/config b/tools/testing/selftests/exec/config
new file mode 100644
index 000000000000..c308079867b3
--- /dev/null
+++ b/tools/testing/selftests/exec/config
@@ -0,0 +1,2 @@
+CONFIG_BLK_DEV=y
+CONFIG_BLK_DEV_LOOP=y
diff --git a/tools/testing/selftests/exec/false.c b/tools/testing/selftests/exec/false.c
new file mode 100644
index 000000000000..104383ec3a79
--- /dev/null
+++ b/tools/testing/selftests/exec/false.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+int main(void)
+{
+ return 1;
+}
diff --git a/tools/testing/selftests/exec/should-exec.c b/tools/testing/selftests/exec/should-exec.c
new file mode 100644
index 000000000000..166276a39b4e
--- /dev/null
+++ b/tools/testing/selftests/exec/should-exec.c
@@ -0,0 +1,449 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test execveat(2) with AT_CHECK, and prctl(2) with SECBIT_SHOULD_EXEC_CHECK,
+ * SECBIT_SHOULD_EXEC_RESTRIC, and their locked counterparts.
+ *
+ * Copyright © 2018-2020 ANSSI
+ * Copyright © 2024 Microsoft Corporation
+ *
+ * Author: Mickaël Salaün <mic@digikod.net>
+ */
+
+#include <asm-generic/unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/prctl.h>
+#include <linux/securebits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/capability.h>
+#include <sys/mount.h>
+#include <sys/prctl.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/sysmacros.h>
+#include <unistd.h>
+
+/* Defines AT_CHECK without type conflicts. */
+#define _ASM_GENERIC_FCNTL_H
+#include <linux/fcntl.h>
+
+#include "../kselftest_harness.h"
+
+static void drop_privileges(struct __test_metadata *const _metadata)
+{
+ const unsigned int noroot = SECBIT_NOROOT | SECBIT_NOROOT_LOCKED;
+ cap_t cap_p;
+
+ if ((cap_get_secbits() & noroot) != noroot)
+ EXPECT_EQ(0, cap_set_secbits(noroot));
+
+ cap_p = cap_get_proc();
+ EXPECT_NE(NULL, cap_p);
+ EXPECT_NE(-1, cap_clear(cap_p));
+
+ /*
+ * Drops everything, especially CAP_SETPCAP, CAP_DAC_OVERRIDE, and
+ * CAP_DAC_READ_SEARCH.
+ */
+ EXPECT_NE(-1, cap_set_proc(cap_p));
+ EXPECT_NE(-1, cap_free(cap_p));
+}
+
+static int test_secbits_set(const unsigned int secbits)
+{
+ int err;
+
+ err = prctl(PR_SET_SECUREBITS, secbits);
+ if (err)
+ return errno;
+ return 0;
+}
+
+FIXTURE(access)
+{
+ int memfd, pipefd;
+ int pipe_fds[2], socket_fds[2];
+};
+
+FIXTURE_VARIANT(access)
+{
+ const bool mount_exec;
+ const bool file_exec;
+};
+
+FIXTURE_VARIANT_ADD(access, mount_exec_file_exec){
+ .mount_exec = true,
+ .file_exec = true,
+};
+
+FIXTURE_VARIANT_ADD(access, mount_exec_file_noexec){
+ .mount_exec = true,
+ .file_exec = false,
+};
+
+FIXTURE_VARIANT_ADD(access, mount_noexec_file_exec){
+ .mount_exec = false,
+ .file_exec = true,
+};
+
+FIXTURE_VARIANT_ADD(access, mount_noexec_file_noexec){
+ .mount_exec = false,
+ .file_exec = false,
+};
+
+static const char binary_path[] = "./false";
+static const char workdir_path[] = "./test-mount";
+static const char reg_file_path[] = "./test-mount/regular_file";
+static const char dir_path[] = "./test-mount/directory";
+static const char block_dev_path[] = "./test-mount/block_device";
+static const char char_dev_path[] = "./test-mount/character_device";
+static const char fifo_path[] = "./test-mount/fifo";
+
+FIXTURE_SETUP(access)
+{
+ int procfd_path_size;
+ static const char path_template[] = "/proc/self/fd/%d";
+ char procfd_path[sizeof(path_template) + 10];
+
+ /* Makes sure we are not already restricted nor locked. */
+ EXPECT_EQ(0, test_secbits_set(0));
+
+ /*
+ * Cleans previous workspace if any error previously happened (don't
+ * check errors).
+ */
+ umount(workdir_path);
+ rmdir(workdir_path);
+
+ /* Creates a clean mount point. */
+ ASSERT_EQ(0, mkdir(workdir_path, 00700));
+ ASSERT_EQ(0, mount("test", workdir_path, "tmpfs",
+ MS_MGC_VAL | (variant->mount_exec ? 0 : MS_NOEXEC),
+ "mode=0700,size=9m"));
+
+ /* Creates a regular file. */
+ ASSERT_EQ(0, mknod(reg_file_path,
+ S_IFREG | (variant->file_exec ? 0700 : 0600), 0));
+ /* Creates a directory. */
+ ASSERT_EQ(0, mkdir(dir_path, variant->file_exec ? 0700 : 0600));
+ /* Creates a character device: /dev/null. */
+ ASSERT_EQ(0, mknod(char_dev_path, S_IFCHR | 0400, makedev(1, 3)));
+ /* Creates a block device: /dev/loop0 */
+ ASSERT_EQ(0, mknod(block_dev_path, S_IFBLK | 0400, makedev(7, 0)));
+ /* Creates a fifo. */
+ ASSERT_EQ(0, mknod(fifo_path, S_IFIFO | 0600, 0));
+
+ /* Creates a regular file without user mount point. */
+ self->memfd = memfd_create("test-exec-probe", MFD_CLOEXEC);
+ ASSERT_LE(0, self->memfd);
+ /* Sets mode, which must be ignored by the exec check. */
+ ASSERT_EQ(0, fchmod(self->memfd, variant->file_exec ? 0700 : 0600));
+
+ /* Creates a pipefs file descriptor. */
+ ASSERT_EQ(0, pipe(self->pipe_fds));
+ procfd_path_size = snprintf(procfd_path, sizeof(procfd_path),
+ path_template, self->pipe_fds[0]);
+ ASSERT_LT(procfd_path_size, sizeof(procfd_path));
+ self->pipefd = open(procfd_path, O_RDWR | O_CLOEXEC);
+ ASSERT_LE(0, self->pipefd);
+ ASSERT_EQ(0, fchmod(self->pipefd, variant->file_exec ? 0700 : 0600));
+
+ /* Creates a socket file descriptor. */
+ ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0,
+ self->socket_fds));
+}
+
+FIXTURE_TEARDOWN_PARENT(access)
+{
+ /* There is no need to unlink the test files. */
+ EXPECT_EQ(0, umount(workdir_path));
+ EXPECT_EQ(0, rmdir(workdir_path));
+}
+
+static void fill_exec_fd(struct __test_metadata *_metadata, const int fd_out)
+{
+ char buf[1024];
+ size_t len;
+ int fd_in;
+
+ fd_in = open(binary_path, O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd_in);
+ /* Cannot use copy_file_range(2) because of EXDEV. */
+ len = read(fd_in, buf, sizeof(buf));
+ EXPECT_LE(0, len);
+ while (len > 0) {
+ EXPECT_EQ(len, write(fd_out, buf, len))
+ {
+ TH_LOG("Failed to write: %s (%d)", strerror(errno),
+ errno);
+ }
+ len = read(fd_in, buf, sizeof(buf));
+ EXPECT_LE(0, len);
+ }
+ EXPECT_EQ(0, close(fd_in));
+}
+
+static void fill_exec_path(struct __test_metadata *_metadata,
+ const char *const path)
+{
+ int fd_out;
+
+ fd_out = open(path, O_CLOEXEC | O_WRONLY);
+ ASSERT_LE(0, fd_out)
+ {
+ TH_LOG("Failed to open %s: %s", path, strerror(errno));
+ }
+ fill_exec_fd(_metadata, fd_out);
+ EXPECT_EQ(0, close(fd_out));
+}
+
+static void test_exec_fd(struct __test_metadata *_metadata, const int fd,
+ const int err_code)
+{
+ char *const argv[] = { "", NULL };
+ int access_ret, access_errno;
+
+ /*
+ * If we really execute fd, filled with the "false" binary, the current
+ * thread will exits with an error, which will be interpreted by the
+ * test framework as an error. With AT_CHECK, we only check a
+ * potential successful execution.
+ */
+ access_ret = execveat(fd, "", argv, NULL, AT_EMPTY_PATH | AT_CHECK);
+ access_errno = errno;
+ if (err_code) {
+ EXPECT_EQ(-1, access_ret);
+ EXPECT_EQ(err_code, access_errno)
+ {
+ TH_LOG("Wrong error for execveat(2): %s (%d)",
+ strerror(access_errno), errno);
+ }
+ } else {
+ EXPECT_EQ(0, access_ret)
+ {
+ TH_LOG("Access denied: %s", strerror(access_errno));
+ }
+ }
+}
+
+static void test_exec_path(struct __test_metadata *_metadata,
+ const char *const path, const int err_code)
+{
+ int flags = O_CLOEXEC;
+ int fd;
+
+ /* Do not block on pipes. */
+ if (path == fifo_path)
+ flags |= O_NONBLOCK;
+
+ fd = open(path, flags | O_RDONLY);
+ ASSERT_LE(0, fd)
+ {
+ TH_LOG("Failed to open %s: %s", path, strerror(errno));
+ }
+ test_exec_fd(_metadata, fd, err_code);
+ EXPECT_EQ(0, close(fd));
+}
+
+/* Tests that we don't get ENOEXEC. */
+TEST_F(access, regular_file_empty)
+{
+ const int exec = variant->mount_exec && variant->file_exec;
+
+ test_exec_path(_metadata, reg_file_path, exec ? 0 : EACCES);
+
+ drop_privileges(_metadata);
+ test_exec_path(_metadata, reg_file_path, exec ? 0 : EACCES);
+}
+
+TEST_F(access, regular_file_elf)
+{
+ const int exec = variant->mount_exec && variant->file_exec;
+
+ fill_exec_path(_metadata, reg_file_path);
+
+ test_exec_path(_metadata, reg_file_path, exec ? 0 : EACCES);
+
+ drop_privileges(_metadata);
+ test_exec_path(_metadata, reg_file_path, exec ? 0 : EACCES);
+}
+
+/* Tests that we don't get ENOEXEC. */
+TEST_F(access, memfd_empty)
+{
+ const int exec = variant->file_exec;
+
+ test_exec_fd(_metadata, self->memfd, exec ? 0 : EACCES);
+
+ drop_privileges(_metadata);
+ test_exec_fd(_metadata, self->memfd, exec ? 0 : EACCES);
+}
+
+TEST_F(access, memfd_elf)
+{
+ const int exec = variant->file_exec;
+
+ fill_exec_fd(_metadata, self->memfd);
+
+ test_exec_fd(_metadata, self->memfd, exec ? 0 : EACCES);
+
+ drop_privileges(_metadata);
+ test_exec_fd(_metadata, self->memfd, exec ? 0 : EACCES);
+}
+
+TEST_F(access, non_regular_files)
+{
+ test_exec_path(_metadata, dir_path, EACCES);
+ test_exec_path(_metadata, block_dev_path, EACCES);
+ test_exec_path(_metadata, char_dev_path, EACCES);
+ test_exec_path(_metadata, fifo_path, EACCES);
+ test_exec_fd(_metadata, self->socket_fds[0], EACCES);
+ test_exec_fd(_metadata, self->pipefd, EACCES);
+}
+
+
+/* clang-format off */
+FIXTURE(secbits) {};
+/* clang-format on */
+
+FIXTURE_VARIANT(secbits)
+{
+ const bool is_privileged;
+ const int error;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(secbits, priv) {
+ /* clang-format on */
+ .is_privileged = true,
+ .error = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(secbits, unpriv) {
+ /* clang-format on */
+ .is_privileged = false,
+ .error = EPERM,
+};
+
+FIXTURE_SETUP(secbits)
+{
+ /* Makes sure no should-exec bits are set. */
+ EXPECT_EQ(0, test_secbits_set(0));
+ EXPECT_EQ(0, prctl(PR_GET_SECUREBITS));
+
+ if (!variant->is_privileged)
+ drop_privileges(_metadata);
+}
+
+FIXTURE_TEARDOWN(secbits)
+{
+}
+
+TEST_F(secbits, legacy)
+{
+ EXPECT_EQ(variant->error, test_secbits_set(0));
+}
+
+#define CHILD(...) \
+ do { \
+ pid_t child = vfork(); \
+ EXPECT_LE(0, child); \
+ if (child == 0) { \
+ __VA_ARGS__; \
+ _exit(0); \
+ } \
+ } while (0)
+
+TEST_F(secbits, should_exec)
+{
+ unsigned int secbits = prctl(PR_GET_SECUREBITS);
+
+ secbits |= SECBIT_SHOULD_EXEC_CHECK;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+ EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS));
+ CHILD(EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS)));
+
+ secbits |= SECBIT_SHOULD_EXEC_RESTRICT;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+ EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS));
+ CHILD(EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS)));
+
+ secbits &= ~(SECBIT_SHOULD_EXEC_CHECK | SECBIT_SHOULD_EXEC_RESTRICT);
+ EXPECT_EQ(0, test_secbits_set(secbits));
+ EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS));
+ CHILD(EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS)));
+}
+
+TEST_F(secbits, check_locked_set)
+{
+ unsigned int secbits = prctl(PR_GET_SECUREBITS);
+
+ secbits |= SECBIT_SHOULD_EXEC_CHECK;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+ secbits |= SECBIT_SHOULD_EXEC_CHECK_LOCKED;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+
+ /* Checks lock set but unchanged. */
+ EXPECT_EQ(variant->error, test_secbits_set(secbits));
+ CHILD(EXPECT_EQ(variant->error, test_secbits_set(secbits)));
+
+ secbits &= ~SECBIT_SHOULD_EXEC_CHECK;
+ EXPECT_EQ(EPERM, test_secbits_set(0));
+ CHILD(EXPECT_EQ(EPERM, test_secbits_set(0)));
+}
+
+TEST_F(secbits, check_locked_unset)
+{
+ unsigned int secbits = prctl(PR_GET_SECUREBITS);
+
+ secbits |= SECBIT_SHOULD_EXEC_CHECK_LOCKED;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+
+ /* Checks lock unset but unchanged. */
+ EXPECT_EQ(variant->error, test_secbits_set(secbits));
+ CHILD(EXPECT_EQ(variant->error, test_secbits_set(secbits)));
+
+ secbits &= ~SECBIT_SHOULD_EXEC_CHECK;
+ EXPECT_EQ(EPERM, test_secbits_set(0));
+ CHILD(EXPECT_EQ(EPERM, test_secbits_set(0)));
+}
+
+TEST_F(secbits, restrict_locked_set)
+{
+ unsigned int secbits = prctl(PR_GET_SECUREBITS);
+
+ secbits |= SECBIT_SHOULD_EXEC_RESTRICT;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+ secbits |= SECBIT_SHOULD_EXEC_RESTRICT_LOCKED;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+
+ /* Checks lock set but unchanged. */
+ EXPECT_EQ(variant->error, test_secbits_set(secbits));
+ CHILD(EXPECT_EQ(variant->error, test_secbits_set(secbits)));
+
+ secbits &= ~SECBIT_SHOULD_EXEC_RESTRICT;
+ EXPECT_EQ(EPERM, test_secbits_set(0));
+ CHILD(EXPECT_EQ(EPERM, test_secbits_set(0)));
+}
+
+TEST_F(secbits, restrict_locked_unset)
+{
+ unsigned int secbits = prctl(PR_GET_SECUREBITS);
+
+ secbits |= SECBIT_SHOULD_EXEC_RESTRICT_LOCKED;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+
+ /* Checks lock unset but unchanged. */
+ EXPECT_EQ(variant->error, test_secbits_set(secbits));
+ CHILD(EXPECT_EQ(variant->error, test_secbits_set(secbits)));
+
+ secbits &= ~SECBIT_SHOULD_EXEC_RESTRICT;
+ EXPECT_EQ(EPERM, test_secbits_set(0));
+ CHILD(EXPECT_EQ(EPERM, test_secbits_set(0)));
+}
+
+/* TODO: Add ptrace tests */
+
+TEST_HARNESS_MAIN
--
2.45.2
^ permalink raw reply related
* [RFC PATCH v19 4/5] selftests/landlock: Add tests for execveat + AT_CHECK
From: Mickaël Salaün @ 2024-07-04 19:01 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o
Cc: Mickaël Salaün, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Eric Biggers, Eric Chiang,
Fan Wu, Florian Weimer, Geert Uytterhoeven, James Morris,
Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module,
Günther Noack
In-Reply-To: <20240704190137.696169-1-mic@digikod.net>
Extend layout1.execute with the new AT_CHECK flag. The semantic with
AT_CHECK is the same as with a simple execve(2),
LANDLOCK_ACCESS_FS_EXECUTE is enforced the same way.
Cc: Günther Noack <gnoack@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20240704190137.696169-5-mic@digikod.net
---
tools/testing/selftests/landlock/fs_test.c | 26 ++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 7d063c652be1..85ef36b09a37 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -37,6 +37,10 @@
#include <linux/fs.h>
#include <linux/mount.h>
+/* Defines AT_CHECK without type conflicts. */
+#define _ASM_GENERIC_FCNTL_H
+#include <linux/fcntl.h>
+
#include "common.h"
#ifndef renameat2
@@ -2009,6 +2013,21 @@ static void test_execute(struct __test_metadata *const _metadata, const int err,
};
}
+static void test_check_exec(struct __test_metadata *const _metadata,
+ const int err, const char *const path)
+{
+ int ret;
+ char *const argv[] = { (char *)path, NULL };
+
+ ret = execveat(AT_FDCWD, path, argv, NULL, AT_EMPTY_PATH | AT_CHECK);
+ if (err) {
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(errno, err);
+ } else {
+ EXPECT_EQ(0, ret);
+ }
+}
+
TEST_F_FORK(layout1, execute)
{
const struct rule rules[] = {
@@ -2026,20 +2045,27 @@ TEST_F_FORK(layout1, execute)
copy_binary(_metadata, file1_s1d2);
copy_binary(_metadata, file1_s1d3);
+ /* Checks before file1_s1d1 being denied. */
+ test_execute(_metadata, 0, file1_s1d1);
+ test_check_exec(_metadata, 0, file1_s1d1);
+
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));
ASSERT_EQ(0, test_open(dir_s1d1, O_RDONLY));
ASSERT_EQ(0, test_open(file1_s1d1, O_RDONLY));
test_execute(_metadata, EACCES, file1_s1d1);
+ test_check_exec(_metadata, EACCES, file1_s1d1);
ASSERT_EQ(0, test_open(dir_s1d2, O_RDONLY));
ASSERT_EQ(0, test_open(file1_s1d2, O_RDONLY));
test_execute(_metadata, 0, file1_s1d2);
+ test_check_exec(_metadata, 0, file1_s1d2);
ASSERT_EQ(0, test_open(dir_s1d3, O_RDONLY));
ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
test_execute(_metadata, 0, file1_s1d3);
+ test_check_exec(_metadata, 0, file1_s1d3);
}
TEST_F_FORK(layout1, link)
--
2.45.2
^ permalink raw reply related
* [RFC PATCH v19 5/5] samples/should-exec: Add set-should-exec
From: Mickaël Salaün @ 2024-07-04 19:01 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o
Cc: Mickaël Salaün, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Eric Biggers, Eric Chiang,
Fan Wu, Florian Weimer, Geert Uytterhoeven, James Morris,
Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module
In-Reply-To: <20240704190137.696169-1-mic@digikod.net>
Add a simple tool to set SECBIT_SHOULD_EXEC_CHECK,
SECBIT_SHOULD_EXEC_RESTRICT, and their lock counterparts before
executing a command. This should be useful to easily test against
script interpreters.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20240704190137.696169-6-mic@digikod.net
---
samples/Kconfig | 7 +++
samples/Makefile | 1 +
samples/should-exec/.gitignore | 1 +
samples/should-exec/Makefile | 13 ++++
samples/should-exec/set-should-exec.c | 88 +++++++++++++++++++++++++++
5 files changed, 110 insertions(+)
create mode 100644 samples/should-exec/.gitignore
create mode 100644 samples/should-exec/Makefile
create mode 100644 samples/should-exec/set-should-exec.c
diff --git a/samples/Kconfig b/samples/Kconfig
index b288d9991d27..d8f2639bc830 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -180,6 +180,13 @@ config SAMPLE_SECCOMP
Build samples of seccomp filters using various methods of
BPF filter construction.
+config SAMPLE_SHOULD_EXEC
+ bool "Should-exec secure bits examples"
+ depends on CC_CAN_LINK && HEADERS_INSTALL
+ help
+ Build a tool to easily configure SECBIT_SHOULD_EXEC_CHECK,
+ SECBIT_SHOULD_EXEC_RESTRICT and their lock counterparts.
+
config SAMPLE_TIMER
bool "Timer sample"
depends on CC_CAN_LINK && HEADERS_INSTALL
diff --git a/samples/Makefile b/samples/Makefile
index b85fa64390c5..0e7a97fb222d 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -19,6 +19,7 @@ subdir-$(CONFIG_SAMPLE_PIDFD) += pidfd
obj-$(CONFIG_SAMPLE_QMI_CLIENT) += qmi/
obj-$(CONFIG_SAMPLE_RPMSG_CLIENT) += rpmsg/
subdir-$(CONFIG_SAMPLE_SECCOMP) += seccomp
+subdir-$(CONFIG_SAMPLE_SHOULD_EXEC) += should-exec
subdir-$(CONFIG_SAMPLE_TIMER) += timers
obj-$(CONFIG_SAMPLE_TRACE_EVENTS) += trace_events/
obj-$(CONFIG_SAMPLE_TRACE_CUSTOM_EVENTS) += trace_events/
diff --git a/samples/should-exec/.gitignore b/samples/should-exec/.gitignore
new file mode 100644
index 000000000000..ac46c614ec80
--- /dev/null
+++ b/samples/should-exec/.gitignore
@@ -0,0 +1 @@
+/set-should-exec
diff --git a/samples/should-exec/Makefile b/samples/should-exec/Makefile
new file mode 100644
index 000000000000..c4294278dd07
--- /dev/null
+++ b/samples/should-exec/Makefile
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: BSD-3-Clause
+
+userprogs-always-y := set-should-exec
+
+userccflags += -I usr/include
+
+.PHONY: all clean
+
+all:
+ $(MAKE) -C ../.. samples/should-exec/
+
+clean:
+ $(MAKE) -C ../.. M=samples/should-exec/ clean
diff --git a/samples/should-exec/set-should-exec.c b/samples/should-exec/set-should-exec.c
new file mode 100644
index 000000000000..b3c31106d916
--- /dev/null
+++ b/samples/should-exec/set-should-exec.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Simple tool to set SECBIT_SHOULD_EXEC_CHECK, SECBIT_SHOULD_EXEC_RESTRICT,
+ * and their lock counterparts before executing a command.
+ *
+ * Copyright © 2024 Microsoft Corporation
+ */
+
+#define _GNU_SOURCE
+#define __SANE_USERSPACE_TYPES__
+#include <errno.h>
+#include <linux/prctl.h>
+#include <linux/securebits.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/prctl.h>
+#include <unistd.h>
+
+static void print_usage(const char *argv0)
+{
+ fprintf(stderr, "usage: %s -c|-r [-l] -- <cmd> [args]...\n\n", argv0);
+ fprintf(stderr, "Execute a command with\n");
+ fprintf(stderr, "- SECBIT_SHOULD_EXEC_CHECK set: -c\n");
+ fprintf(stderr, "- SECBIT_SHOULD_EXEC_RESTRICT set: -r\n");
+ fprintf(stderr, "- SECBIT_SHOULD_EXEC_*_LOCKED set: -l\n");
+}
+
+int main(const int argc, char *const argv[], char *const *const envp)
+{
+ const char *cmd_path;
+ char *const *cmd_argv;
+ int opt, secbits, err;
+ bool has_policy = false;
+
+ secbits = prctl(PR_GET_SECUREBITS);
+
+ while ((opt = getopt(argc, argv, "crl")) != -1) {
+ switch (opt) {
+ case 'c':
+ secbits |= SECBIT_SHOULD_EXEC_CHECK;
+ has_policy = true;
+ break;
+ case 'r':
+ secbits |= SECBIT_SHOULD_EXEC_RESTRICT;
+ has_policy = true;
+ break;
+ case 'l':
+ secbits |= SECBIT_SHOULD_EXEC_CHECK_LOCKED;
+ secbits |= SECBIT_SHOULD_EXEC_RESTRICT_LOCKED;
+ break;
+ default:
+ print_usage(argv[0]);
+ return 1;
+ }
+ }
+
+ if (!argv[optind] || !has_policy) {
+ print_usage(argv[0]);
+ return 1;
+ }
+
+ err = prctl(PR_SET_SECUREBITS, secbits);
+ if (err) {
+ perror("Failed to set secure bit(s).");
+ fprintf(stderr,
+ "Hint: The running kernel may not support this feature.\n");
+ return 1;
+ }
+
+ fprintf(stderr, "SECBIT_SHOULD_EXEC_CHECK: %d\n",
+ !!(secbits & SECBIT_SHOULD_EXEC_CHECK));
+ fprintf(stderr, "SECBIT_SHOULD_EXEC_CHECK_LOCKED: %d\n",
+ !!(secbits & SECBIT_SHOULD_EXEC_CHECK_LOCKED));
+ fprintf(stderr, "SECBIT_SHOULD_EXEC_RESTRICT: %d\n",
+ !!(secbits & SECBIT_SHOULD_EXEC_RESTRICT));
+ fprintf(stderr, "SECBIT_SHOULD_EXEC_RESTRICT_LOCKED: %d\n",
+ !!(secbits & SECBIT_SHOULD_EXEC_RESTRICT_LOCKED));
+
+ cmd_path = argv[optind];
+ cmd_argv = argv + optind;
+ fprintf(stderr, "Executing command...\n");
+ execvpe(cmd_path, cmd_argv, envp);
+ fprintf(stderr, "Failed to execute \"%s\": %s\n", cmd_path,
+ strerror(errno));
+ return 1;
+}
--
2.45.2
^ permalink raw reply related
* [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-07-04 19:01 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o
Cc: Mickaël Salaün, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Eric Biggers, Eric Chiang,
Fan Wu, Florian Weimer, Geert Uytterhoeven, James Morris,
Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module
In-Reply-To: <20240704190137.696169-1-mic@digikod.net>
Add a new AT_CHECK flag to execveat(2) to check if a file would be
allowed for execution. The main use case is for script interpreters and
dynamic linkers to check execution permission according to the kernel's
security policy. Another use case is to add context to access logs e.g.,
which script (instead of interpreter) accessed a file. As any
executable code, scripts could also use this check [1].
This is different than faccessat(2) which only checks file access
rights, but not the full context e.g. mount point's noexec, stack limit,
and all potential LSM extra checks (e.g. argv, envp, credentials).
Since the use of AT_CHECK follows the exact kernel semantic as for a
real execution, user space gets the same error codes.
With the information that a script interpreter is about to interpret a
script, an LSM security policy can adjust caller's access rights or log
execution request as for native script execution (e.g. role transition).
This is possible thanks to the call to security_bprm_creds_for_exec().
Because LSMs may only change bprm's credentials, use of AT_CHECK with
current kernel code should not be a security issue (e.g. unexpected role
transition). LSMs willing to update the caller's credential could now
do so when bprm->is_check is set. Of course, such policy change should
be in line with the new user space code.
Because AT_CHECK is dedicated to user space interpreters, it doesn't
make sense for the kernel to parse the checked files, look for
interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
if the format is unknown. Because of that, security_bprm_check() is
never called when AT_CHECK is used.
It should be noted that script interpreters cannot directly use
execveat(2) (without this new AT_CHECK flag) because this could lead to
unexpected behaviors e.g., `python script.sh` could lead to Bash being
executed to interpret the script. Unlike the kernel, script
interpreters may just interpret the shebang as a simple comment, which
should not change for backward compatibility reasons.
Because scripts or libraries files might not currently have the
executable permission set, or because we might want specific users to be
allowed to run arbitrary scripts, the following patch provides a dynamic
configuration mechanism with the SECBIT_SHOULD_EXEC_CHECK and
SECBIT_SHOULD_EXEC_RESTRICT securebits.
This is a redesign of the CLIP OS 4's O_MAYEXEC:
https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
This patch has been used for more than a decade with customized script
interpreters. Some examples can be found here:
https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Link: https://docs.python.org/3/library/io.html#io.open_code [1]
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20240704190137.696169-2-mic@digikod.net
---
New design since v18:
https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
---
fs/exec.c | 5 +++--
include/linux/binfmts.h | 7 ++++++-
include/uapi/linux/fcntl.h | 30 ++++++++++++++++++++++++++++++
kernel/audit.h | 1 +
kernel/auditsc.c | 1 +
5 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 40073142288f..ea2a1867afdc 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -931,7 +931,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
.lookup_flags = LOOKUP_FOLLOW,
};
- if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_CHECK)) != 0)
return ERR_PTR(-EINVAL);
if (flags & AT_SYMLINK_NOFOLLOW)
open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
@@ -1595,6 +1595,7 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
bprm->filename = bprm->fdpath;
}
bprm->interp = bprm->filename;
+ bprm->is_check = !!(flags & AT_CHECK);
retval = bprm_mm_init(bprm);
if (!retval)
@@ -1885,7 +1886,7 @@ static int bprm_execve(struct linux_binprm *bprm)
/* Set the unchanging part of bprm->cred */
retval = security_bprm_creds_for_exec(bprm);
- if (retval)
+ if (retval || bprm->is_check)
goto out;
retval = exec_binprm(bprm);
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 70f97f685bff..8ff9c9e33aed 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -42,7 +42,12 @@ struct linux_binprm {
* Set when errors can no longer be returned to the
* original userspace.
*/
- point_of_no_return:1;
+ point_of_no_return:1,
+ /*
+ * Set by user space to check executability according to the
+ * caller's environment.
+ */
+ is_check:1;
struct file *executable; /* Executable to pass to the interpreter */
struct file *interpreter;
struct file *file;
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index c0bcc185fa48..bcd05c59b7df 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -118,6 +118,36 @@
#define AT_HANDLE_FID AT_REMOVEDIR /* file handle is needed to
compare object identity and may not
be usable to open_by_handle_at(2) */
+
+/*
+ * AT_CHECK only performs a check on a regular file and returns 0 if execution
+ * of this file would be allowed, ignoring the file format and then the related
+ * interpreter dependencies (e.g. ELF libraries, script's shebang). AT_CHECK
+ * should only be used if SECBIT_SHOULD_EXEC_CHECK is set for the calling
+ * thread. See securebits.h documentation.
+ *
+ * Programs should use this check to apply kernel-level checks against files
+ * that are not directly executed by the kernel but directly passed to a user
+ * space interpreter instead. All files that contain executable code, from the
+ * point of view of the interpreter, should be checked. The main purpose of
+ * this flag is to improve the security and consistency of an execution
+ * environment to ensure that direct file execution (e.g. ./script.sh) and
+ * indirect file execution (e.g. sh script.sh) lead to the same result. For
+ * instance, this can be used to check if a file is trustworthy according to
+ * the caller's environment.
+ *
+ * In a secure environment, libraries and any executable dependencies should
+ * also be checked. For instance dynamic linking should make sure that all
+ * libraries are allowed for execution to avoid trivial bypass (e.g. using
+ * LD_PRELOAD). For such secure execution environment to make sense, only
+ * trusted code should be executable, which also requires integrity guarantees.
+ *
+ * To avoid race conditions leading to time-of-check to time-of-use issues,
+ * AT_CHECK should be used with AT_EMPTY_PATH to check against a file
+ * descriptor instead of a path.
+ */
+#define AT_CHECK 0x10000
+
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
#endif
diff --git a/kernel/audit.h b/kernel/audit.h
index a60d2840559e..8ebdabd2ab81 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -197,6 +197,7 @@ struct audit_context {
struct open_how openat2;
struct {
int argc;
+ bool is_check;
} execve;
struct {
char *name;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 6f0d6fb6523f..b6316e284342 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2662,6 +2662,7 @@ void __audit_bprm(struct linux_binprm *bprm)
context->type = AUDIT_EXECVE;
context->execve.argc = bprm->argc;
+ context->execve.is_check = bprm->is_check;
}
--
2.45.2
^ permalink raw reply related
* [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT securebits
From: Mickaël Salaün @ 2024-07-04 19:01 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o
Cc: Mickaël Salaün, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Eric Biggers, Eric Chiang,
Fan Wu, Florian Weimer, Geert Uytterhoeven, James Morris,
Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module
In-Reply-To: <20240704190137.696169-1-mic@digikod.net>
These new SECBIT_SHOULD_EXEC_CHECK, SECBIT_SHOULD_EXEC_RESTRICT, and
their *_LOCKED counterparts are designed to be set by processes setting
up an execution environment, such as a user session, a container, or a
security sandbox. Like seccomp filters or Landlock domains, the
securebits are inherited across proceses.
When SECBIT_SHOULD_EXEC_CHECK is set, programs interpreting code should
check executable resources with execveat(2) + AT_CHECK (see previous
patch).
When SECBIT_SHOULD_EXEC_RESTRICT is set, a process should only allow
execution of approved resources, if any (see SECBIT_SHOULD_EXEC_CHECK).
For a secure environment, we might also want
SECBIT_SHOULD_EXEC_CHECK_LOCKED and SECBIT_SHOULD_EXEC_RESTRICT_LOCKED
to be set. For a test environment (e.g. testing on a fleet to identify
potential issues), only the SECBIT_SHOULD_EXEC_CHECK* bits can be set to
still be able to identify potential issues (e.g. with interpreters logs
or LSMs audit entries).
It should be noted that unlike other security bits, the
SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT bits are
dedicated to user space willing to restrict itself. Because of that,
they only make sense in the context of a trusted environment (e.g.
sandbox, container, user session, full system) where the process
changing its behavior (according to these bits) and all its parent
processes are trusted. Otherwise, any parent process could just execute
its own malicious code (interpreting a script or not), or even enforce a
seccomp filter to mask these bits.
Such a secure environment can be achieved with an appropriate access
control policy (e.g. mount's noexec option, file access rights, LSM
configuration) and an enlighten ld.so checking that libraries are
allowed for execution e.g., to protect against illegitimate use of
LD_PRELOAD.
Scripts may need some changes to deal with untrusted data (e.g. stdin,
environment variables), but that is outside the scope of the kernel.
The only restriction enforced by the kernel is the right to ptrace
another process. Processes are denied to ptrace less restricted ones,
unless the tracer has CAP_SYS_PTRACE. This is mainly a safeguard to
avoid trivial privilege escalations e.g., by a debugging process being
abused with a confused deputy attack.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20240704190137.696169-3-mic@digikod.net
---
New design since v18:
https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
---
include/uapi/linux/securebits.h | 56 ++++++++++++++++++++++++++++-
security/commoncap.c | 63 ++++++++++++++++++++++++++++-----
2 files changed, 110 insertions(+), 9 deletions(-)
diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
index d6d98877ff1a..3fdb0382718b 100644
--- a/include/uapi/linux/securebits.h
+++ b/include/uapi/linux/securebits.h
@@ -52,10 +52,64 @@
#define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
(issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
+/*
+ * When SECBIT_SHOULD_EXEC_CHECK is set, a process should check all executable
+ * files with execveat(2) + AT_CHECK. However, such check should only be
+ * performed if all to-be-executed code only comes from regular files. For
+ * instance, if a script interpreter is called with both a script snipped as
+ * argument and a regular file, the interpreter should not check any file.
+ * Doing otherwise would mislead the kernel to think that only the script file
+ * is being executed, which could for instance lead to unexpected permission
+ * change and break current use cases.
+ *
+ * This secure bit may be set by user session managers, service managers,
+ * container runtimes, sandboxer tools... Except for test environments, the
+ * related SECBIT_SHOULD_EXEC_CHECK_LOCKED bit should also be set.
+ *
+ * Ptracing another process is deny if the tracer has SECBIT_SHOULD_EXEC_CHECK
+ * but not the tracee. SECBIT_SHOULD_EXEC_CHECK_LOCKED also checked.
+ */
+#define SECURE_SHOULD_EXEC_CHECK 8
+#define SECURE_SHOULD_EXEC_CHECK_LOCKED 9 /* make bit-8 immutable */
+
+#define SECBIT_SHOULD_EXEC_CHECK (issecure_mask(SECURE_SHOULD_EXEC_CHECK))
+#define SECBIT_SHOULD_EXEC_CHECK_LOCKED \
+ (issecure_mask(SECURE_SHOULD_EXEC_CHECK_LOCKED))
+
+/*
+ * When SECBIT_SHOULD_EXEC_RESTRICT is set, a process should only allow
+ * execution of approved files, if any (see SECBIT_SHOULD_EXEC_CHECK). For
+ * instance, script interpreters called with a script snippet as argument
+ * should always deny such execution if SECBIT_SHOULD_EXEC_RESTRICT is set.
+ * However, if a script interpreter is called with both
+ * SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT, they should
+ * interpret the provided script files if no unchecked code is also provided
+ * (e.g. directly as argument).
+ *
+ * This secure bit may be set by user session managers, service managers,
+ * container runtimes, sandboxer tools... Except for test environments, the
+ * related SECBIT_SHOULD_EXEC_RESTRICT_LOCKED bit should also be set.
+ *
+ * Ptracing another process is deny if the tracer has
+ * SECBIT_SHOULD_EXEC_RESTRICT but not the tracee.
+ * SECBIT_SHOULD_EXEC_RESTRICT_LOCKED is also checked.
+ */
+#define SECURE_SHOULD_EXEC_RESTRICT 10
+#define SECURE_SHOULD_EXEC_RESTRICT_LOCKED 11 /* make bit-8 immutable */
+
+#define SECBIT_SHOULD_EXEC_RESTRICT (issecure_mask(SECURE_SHOULD_EXEC_RESTRICT))
+#define SECBIT_SHOULD_EXEC_RESTRICT_LOCKED \
+ (issecure_mask(SECURE_SHOULD_EXEC_RESTRICT_LOCKED))
+
#define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
issecure_mask(SECURE_NO_SETUID_FIXUP) | \
issecure_mask(SECURE_KEEP_CAPS) | \
- issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
+ issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
+ issecure_mask(SECURE_SHOULD_EXEC_CHECK) | \
+ issecure_mask(SECURE_SHOULD_EXEC_RESTRICT))
#define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
+#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_SHOULD_EXEC_CHECK) | \
+ issecure_mask(SECURE_SHOULD_EXEC_RESTRICT))
+
#endif /* _UAPI_LINUX_SECUREBITS_H */
diff --git a/security/commoncap.c b/security/commoncap.c
index 162d96b3a676..34b4493e2a69 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -117,6 +117,33 @@ int cap_settime(const struct timespec64 *ts, const struct timezone *tz)
return 0;
}
+static bool ptrace_secbits_allowed(const struct cred *tracer,
+ const struct cred *tracee)
+{
+ const unsigned long tracer_secbits = SECURE_ALL_UNPRIVILEGED &
+ tracer->securebits;
+ const unsigned long tracee_secbits = SECURE_ALL_UNPRIVILEGED &
+ tracee->securebits;
+ /* Ignores locking of unset secure bits (cf. SECURE_ALL_LOCKS). */
+ const unsigned long tracer_locked = (tracer_secbits << 1) &
+ tracer->securebits;
+ const unsigned long tracee_locked = (tracee_secbits << 1) &
+ tracee->securebits;
+
+ /* The tracee must not have less constraints than the tracer. */
+ if ((tracer_secbits | tracee_secbits) != tracee_secbits)
+ return false;
+
+ /*
+ * Makes sure that the tracer's locks for restrictions are the same for
+ * the tracee.
+ */
+ if ((tracer_locked | tracee_locked) != tracee_locked)
+ return false;
+
+ return true;
+}
+
/**
* cap_ptrace_access_check - Determine whether the current process may access
* another
@@ -146,7 +173,8 @@ int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
else
caller_caps = &cred->cap_permitted;
if (cred->user_ns == child_cred->user_ns &&
- cap_issubset(child_cred->cap_permitted, *caller_caps))
+ cap_issubset(child_cred->cap_permitted, *caller_caps) &&
+ ptrace_secbits_allowed(cred, child_cred))
goto out;
if (ns_capable(child_cred->user_ns, CAP_SYS_PTRACE))
goto out;
@@ -178,7 +206,8 @@ int cap_ptrace_traceme(struct task_struct *parent)
cred = __task_cred(parent);
child_cred = current_cred();
if (cred->user_ns == child_cred->user_ns &&
- cap_issubset(child_cred->cap_permitted, cred->cap_permitted))
+ cap_issubset(child_cred->cap_permitted, cred->cap_permitted) &&
+ ptrace_secbits_allowed(cred, child_cred))
goto out;
if (has_ns_capability(parent, child_cred->user_ns, CAP_SYS_PTRACE))
goto out;
@@ -1302,21 +1331,39 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
& (old->securebits ^ arg2)) /*[1]*/
|| ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
|| (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
- || (cap_capable(current_cred(),
- current_cred()->user_ns,
- CAP_SETPCAP,
- CAP_OPT_NONE) != 0) /*[4]*/
/*
* [1] no changing of bits that are locked
* [2] no unlocking of locks
* [3] no setting of unsupported bits
- * [4] doing anything requires privilege (go read about
- * the "sendmail capabilities bug")
*/
)
/* cannot change a locked bit */
return -EPERM;
+ /*
+ * Doing anything requires privilege (go read about the
+ * "sendmail capabilities bug"), except for unprivileged bits.
+ * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
+ * restrictions enforced by the kernel but by user space on
+ * itself. The kernel is only in charge of protecting against
+ * privilege escalation with ptrace protections.
+ */
+ if (cap_capable(current_cred(), current_cred()->user_ns,
+ CAP_SETPCAP, CAP_OPT_NONE) != 0) {
+ const unsigned long unpriv_and_locks =
+ SECURE_ALL_UNPRIVILEGED |
+ SECURE_ALL_UNPRIVILEGED << 1;
+ const unsigned long changed = old->securebits ^ arg2;
+
+ /* For legacy reason, denies non-change. */
+ if (!changed)
+ return -EPERM;
+
+ /* Denies privileged changes. */
+ if (changed & ~unpriv_and_locks)
+ return -EPERM;
+ }
+
new = prepare_creds();
if (!new)
return -ENOMEM;
--
2.45.2
^ permalink raw reply related
* [RFC PATCH v19 0/5] Script execution control (was O_MAYEXEC)
From: Mickaël Salaün @ 2024-07-04 19:01 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o
Cc: Mickaël Salaün, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Eric Biggers, Eric Chiang,
Fan Wu, Florian Weimer, Geert Uytterhoeven, James Morris,
Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet, Jordan R Abrahams,
Lakshmi Ramasubramanian, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel, Xiaoming Ni,
Yin Fengwei, kernel-hardening, linux-api, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module
Hi,
The ultimate goal of this patch series is to be able to ensure that
direct file execution (e.g. ./script.sh) and indirect file execution
(e.g. sh script.sh) lead to the same result, especially from a security
point of view.
Overview
--------
This patch series is a new approach of the initial O_MAYEXEC feature,
and a revamp of the previous patch series. Taking into account the last
reviews [1], we now stick to the kernel semantic for file executability.
One major change is the clear split between access check and policy
management.
The first patch brings the AT_CHECK flag to execveat(2). The goal is to
enable user space to check if a file could be executed (by the kernel).
Unlike stat(2) that only checks file permissions, execveat2(2) +
AT_CHECK take into account the full context, including mount points
(noexec), caller's limits, and all potential LSM extra checks (e.g.
argv, envp, credentials).
The second patch brings two new securebits used to set or get a security
policy for a set of processes. For this to be meaningful, all
executable code needs to be trusted. In practice, this means that
(malicious) users can be restricted to only run scripts provided (and
trusted) by the system.
[1] https://lore.kernel.org/r/CAHk-=wjPGNLyzeBMWdQu+kUdQLHQugznwY7CvWjmvNW47D5sog@mail.gmail.com
Script execution
----------------
One important thing to keep in mind is that the goal of this patch
series is to get the same security restrictions with these commands:
* ./script.py
* python script.py
* python < script.py
* python -m script.py
However, on secure systems, we should be able to forbid these commands
because there is no way to reliably identify the origin of the script:
* xargs -a script.py -d '\r' -- python -c
* cat script.py | python
* python
Background
----------
Compared to the previous patch series, there is no more dedicated
syscall nor sysctl configuration. This new patch series only add new
flags: one for execveat(2) and four for prctl(2).
This kind of script interpreter restriction may already be used in
hardened systems, which may need to fork interpreters and install
different versions of the binaries. This mechanism should enable to
avoid the use of duplicate binaries (and potential forked source code)
for secure interpreters (e.g. secure Python [2]) by making it possible
to dynamically enforce restrictions or not.
The ability to control script execution is also required to close a
major IMA measurement/appraisal interpreter integrity [3].
This new execveat + AT_CHECK should not be confused with the O_EXEC flag
(for open) which is intended for execute-only, which obviously doesn't
work for scripts.
I gave a talk about controlling script execution where I explain the
previous approaches [4]. The design of the WIP RFC I talked about
changed quite a bit since then.
[2] https://github.com/zooba/spython
[3] https://lore.kernel.org/lkml/20211014130125.6991-1-zohar@linux.ibm.com/
[4] https://lssna2023.sched.com/event/1K7bO
Execution policy
----------------
The "execution" usage means that the content of the file descriptor is
trusted according to the system policy to be executed by user space,
which means that it interprets the content or (try to) maps it as
executable memory.
It is important to note that this can only enable to extend access
control managed by the kernel. Hence it enables current access control
mechanism to be extended and become a superset of what they can
currently control. Indeed, the security policy could also be delegated
to an LSM, either a MAC system or an integrity system.
Complementary W^X protections can be brought by SELinux or IPE [5].
Being able to restrict execution also enables to protect the kernel by
restricting arbitrary syscalls that an attacker could perform with a
crafted binary or certain script languages. It also improves multilevel
isolation by reducing the ability of an attacker to use side channels
with specific code. These restrictions can natively be enforced for ELF
binaries (with the noexec mount option) but require this kernel
extension to properly handle scripts (e.g. Python, Perl). To get a
consistent execution policy, additional memory restrictions should also
be enforced (e.g. thanks to SELinux).
[5] https://lore.kernel.org/lkml/1716583609-21790-1-git-send-email-wufan@linux.microsoft.com/
Prerequisite for security use
-----------------------------
Because scripts might not currently have the executable permission and
still run well as is, or because we might want specific users to be
allowed to run arbitrary scripts, we also need a configuration
mechanism.
According to the threat model, to get a secure execution environment on
top of these changes, it might be required to configure and enable
existing security mechanisms such as secure boot, restrictive mount
points (e.g. with rw AND noexec), correct file permissions (including
executable libraries), IMA/EVM, SELinux policy...
The first thing to patch is the libc to check loaded libraries (e.g. see
chromeOS changes). The second thing to patch are the script
interpreters by checking direct scripts executability and by checking
their own libraries (e.g. Python's imported files or argument-passed
modules). For instance, the PEP 578 [6] (Runtime Audit Hooks) enables
Python 3.8 to be extended with policy enforcement points related to code
interpretation, which can be used to align with the PowerShell audit
features. Additional Python security improvements (e.g. a limited
interpreter without -c, stdin piping of code) are developed [2] [7].
[6] https://www.python.org/dev/peps/pep-0578/
[7] https://lore.kernel.org/lkml/0c70debd-e79e-d514-06c6-4cd1e021fa8b@python.org/
libc patch
----------
Dynamic linking needs still need to check the libraries the same way
interpreters need to check scripts.
chromeOS patches glibc with a fstatvfs check [8] [9]. This enables to
check against noexec mount points, which is OK but doesn't fit with
execve semantics. Moreover, the kernel is not aware of such check, so
all access control checks are not performed (e.g. file permission, LSMs
security policies, integrity and authenticity checks), it is not handled
with audit, and more importantly this would not work on generic
distributions because of the strict requirement and chromeOS-specific
assumptions.
[8] https://issuetracker.google.com/issues/40054993
[9] https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/6abfc9e327241a5f684b8b941c899b7ca8b6dbc1/sys-libs/glibc/files/local/glibc-2.37/0007-Deny-LD_PRELOAD-of-files-in-NOEXEC-mount.patch
Examples
--------
The initial idea comes from CLIP OS 4 and the original implementation
has been used for more than a decade:
https://github.com/clipos-archive/clipos4_doc
Chrome OS has a similar approach:
https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
User space patches can be found here:
https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
There is more than the O_MAYEXEC changes (which matches this search)
e.g., to prevent Python interactive execution. There are patches for
Bash, Wine, Java (Icedtea), Busybox's ash, Perl and Python. There are
also some related patches which do not directly rely on O_MAYEXEC but
which restrict the use of browser plugins and extensions, which may be
seen as scripts too:
https://github.com/clipos-archive/clipos4_portage-overlay/tree/master/www-client
Past talks and articles
-----------------------
An introduction to O_MAYEXEC was given at the Linux Security Summit
Europe 2018 - Linux Kernel Security Contributions by ANSSI:
https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
The "write xor execute" principle was explained at Kernel Recipes 2018 -
CLIP OS: a defense-in-depth OS:
https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s
See also a first LWN article about O_MAYEXEC and a new one about
trusted_for(2) and its background:
* https://lwn.net/Articles/820000/
* https://lwn.net/Articles/832959/
Previous versions:
v18: https://lore.kernel.org/r/20220104155024.48023-1-mic@digikod.net
v17: https://lore.kernel.org/r/20211115185304.198460-1-mic@digikod.net
v16: https://lore.kernel.org/r/20211110190626.257017-1-mic@digikod.net
v15: https://lore.kernel.org/r/20211012192410.2356090-1-mic@digikod.net
v14: https://lore.kernel.org/r/20211008104840.1733385-1-mic@digikod.net
v13: https://lore.kernel.org/r/20211007182321.872075-1-mic@digikod.net
v12: https://lore.kernel.org/r/20201203173118.379271-1-mic@digikod.net
v11: https://lore.kernel.org/r/20201019164932.1430614-1-mic@digikod.net
v10: https://lore.kernel.org/r/20200924153228.387737-1-mic@digikod.net
v9: https://lore.kernel.org/r/20200910164612.114215-1-mic@digikod.net
v8: https://lore.kernel.org/r/20200908075956.1069018-1-mic@digikod.net
v7: https://lore.kernel.org/r/20200723171227.446711-1-mic@digikod.net
v6: https://lore.kernel.org/r/20200714181638.45751-1-mic@digikod.net
v5: https://lore.kernel.org/r/20200505153156.925111-1-mic@digikod.net
v4: https://lore.kernel.org/r/20200430132320.699508-1-mic@digikod.net
v3: https://lore.kernel.org/r/20200428175129.634352-1-mic@digikod.net
v2: https://lore.kernel.org/r/20190906152455.22757-1-mic@digikod.net
v1: https://lore.kernel.org/r/20181212081712.32347-1-mic@digikod.net
Regards,
Mickaël Salaün (5):
exec: Add a new AT_CHECK flag to execveat(2)
security: Add new SHOULD_EXEC_CHECK and SHOULD_EXEC_RESTRICT
securebits
selftests/exec: Add tests for AT_CHECK and related securebits
selftests/landlock: Add tests for execveat + AT_CHECK
samples/should-exec: Add set-should-exec
fs/exec.c | 5 +-
include/linux/binfmts.h | 7 +-
include/uapi/linux/fcntl.h | 30 ++
include/uapi/linux/securebits.h | 56 ++-
kernel/audit.h | 1 +
kernel/auditsc.c | 1 +
samples/Kconfig | 7 +
samples/Makefile | 1 +
samples/should-exec/.gitignore | 1 +
samples/should-exec/Makefile | 13 +
samples/should-exec/set-should-exec.c | 88 ++++
security/commoncap.c | 63 ++-
tools/testing/selftests/exec/.gitignore | 2 +
tools/testing/selftests/exec/Makefile | 8 +
tools/testing/selftests/exec/config | 2 +
tools/testing/selftests/exec/false.c | 5 +
tools/testing/selftests/exec/should-exec.c | 449 +++++++++++++++++++++
tools/testing/selftests/landlock/fs_test.c | 26 ++
18 files changed, 753 insertions(+), 12 deletions(-)
create mode 100644 samples/should-exec/.gitignore
create mode 100644 samples/should-exec/Makefile
create mode 100644 samples/should-exec/set-should-exec.c
create mode 100644 tools/testing/selftests/exec/config
create mode 100644 tools/testing/selftests/exec/false.c
create mode 100644 tools/testing/selftests/exec/should-exec.c
base-commit: f2661062f16b2de5d7b6a5c42a9a5c96326b8454
--
2.45.2
^ permalink raw reply
* Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer
From: Beau Belgrave @ 2024-07-03 22:26 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, Dmitry V. Levin, Vincent Donnefort, mhiramat,
kernel-team, rdunlap, rppt, david, linux-trace-kernel, linux-mm,
linux-api, linux-kernel, Linus Torvalds
In-Reply-To: <20240702125126.50a6267c@rorschach.local.home>
On Tue, Jul 02, 2024 at 12:51:26PM -0400, Steven Rostedt wrote:
> On Tue, 2 Jul 2024 11:32:53 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
> > If we use '*' for user events already, perhaps we'd want to consider
> > using the same range for the ring buffer ioctls ? Arguably one is
> > about instrumentation and the other is about ring buffer interaction
> > (data transport), but those are both related to tracing.
>
> Yeah, but I still rather keep them separate.
>
> Beau, care to send a patch adding an entry into that ioctl document for
> user events?
>
> -- Steve
Sure thing, sent one out [1].
Thanks,
-Beau
1. https://lore.kernel.org/linux-doc/20240703222501.1547-1-beaub@linux.microsoft.com/
^ permalink raw reply
* Re: [PATCH v20 3/5] arch: allocate vgetrandom_alloc() syscall number
From: Jason A. Donenfeld @ 2024-07-03 19:04 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, patches, Thomas Gleixner, linux-crypto, linux-api,
x86, Greg Kroah-Hartman, Adhemerval Zanella Netto,
Carlos O'Donell, Florian Weimer, Jann Horn, Christian Brauner,
David Hildenbrand, Geert Uytterhoeven, Christian Göttsche,
Jiri Olsa, Stephen Rothwell
In-Reply-To: <fa6eb1f8-b681-4c3f-bf5f-4ca19da78b17@app.fastmail.com>
Hi Arnd,
On Wed, Jul 3, 2024 at 8:59 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Jul 3, 2024, at 20:31, Jason A. Donenfeld wrote:
> > Add vgetrandom_alloc() as syscall 463 (or 573 on alpha) by adding it to
> > all of the various syscall.tbl and unistd.h files.
> >
> > Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>
> I checked that the system calls are all hooked up correctly
> across all architectures:
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
Thanks.
> Unfortunately we now have three syscalls scheduled for
> the number 463, the other ones being uretprobe (only
> for arc, arm64, csky, hexagon, loongarch, nios2, openrisc,
> riscv and x86 for some reason) and setxattrat (on
> all architectures).
>
> It would be nice if you could all coordinate on this to
> pick unique numbers.
Stephen and I were just talking about this when looking at the linux-next merge.
Dibs on 463! Just kidding.
There's going to be merge conflicts anyway, due to the __NR_syscalls
counter changing. So I figured this is something Linus typically just
handles based on what order he merges things in. But I actually don't
know. What's the best way to handle this?
Jason
^ permalink raw reply
* Re: [PATCH v20 3/5] arch: allocate vgetrandom_alloc() syscall number
From: Arnd Bergmann @ 2024-07-03 18:58 UTC (permalink / raw)
To: Jason A . Donenfeld, linux-kernel, patches, Thomas Gleixner
Cc: linux-crypto, linux-api, x86, Greg Kroah-Hartman,
Adhemerval Zanella Netto, Carlos O'Donell, Florian Weimer,
Jann Horn, Christian Brauner, David Hildenbrand,
Geert Uytterhoeven, Christian Göttsche, Jiri Olsa
In-Reply-To: <20240703183115.1075219-4-Jason@zx2c4.com>
On Wed, Jul 3, 2024, at 20:31, Jason A. Donenfeld wrote:
> Add vgetrandom_alloc() as syscall 463 (or 573 on alpha) by adding it to
> all of the various syscall.tbl and unistd.h files.
>
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
I checked that the system calls are all hooked up correctly
across all architectures:
Acked-by: Arnd Bergmann <arnd@arndb.de>
Unfortunately we now have three syscalls scheduled for
the number 463, the other ones being uretprobe (only
for arc, arm64, csky, hexagon, loongarch, nios2, openrisc,
riscv and x86 for some reason) and setxattrat (on
all architectures).
It would be nice if you could all coordinate on this to
pick unique numbers.
Arnd
^ 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