* [PATCH v2 2/2] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Alexander Potapenko @ 2019-03-08 13:27 UTC (permalink / raw)
To: yamada.masahiro, jmorris, serge
Cc: linux-security-module, linux-kbuild, ndesaulniers, kcc, dvyukov,
keescook, sspatil, kernel-hardening
In-Reply-To: <20190308132701.133598-1-glider@google.com>
This config option enables CONFIG_SLUB_DEBUG and CONFIG_PAGE_POISONING
without the need to pass any boot parameters.
No performance optimizations are done at the moment to reduce double
initialization of memory regions.
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Sandeep Patil <sspatil@android.com>
Cc: linux-security-module@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
mm/page_poison.c | 5 +++++
mm/slub.c | 2 ++
security/Kconfig.initmem | 11 +++++++++++
3 files changed, 18 insertions(+)
diff --git a/mm/page_poison.c b/mm/page_poison.c
index 21d4f97cb49b..a1985f33f635 100644
--- a/mm/page_poison.c
+++ b/mm/page_poison.c
@@ -12,9 +12,14 @@ static bool want_page_poisoning __read_mostly;
static int __init early_page_poison_param(char *buf)
{
+#ifdef CONFIG_INIT_ALL_HEAP
+ want_page_poisoning = true;
+ return 0;
+#else
if (!buf)
return -EINVAL;
return strtobool(buf, &want_page_poisoning);
+#endif
}
early_param("page_poison", early_page_poison_param);
diff --git a/mm/slub.c b/mm/slub.c
index 1b08fbcb7e61..00e0197d3f35 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1287,6 +1287,8 @@ static int __init setup_slub_debug(char *str)
if (*str == ',')
slub_debug_slabs = str + 1;
out:
+ if (IS_ENABLED(CONFIG_INIT_ALL_HEAP))
+ slub_debug |= SLAB_POISON;
return 1;
}
diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
index 27aec394365e..5ce49663777a 100644
--- a/security/Kconfig.initmem
+++ b/security/Kconfig.initmem
@@ -13,6 +13,17 @@ config INIT_ALL_MEMORY
if INIT_ALL_MEMORY
+config INIT_ALL_HEAP
+ bool "Initialize all heap"
+ depends on INIT_ALL_MEMORY
+ select CONFIG_PAGE_POISONING
+ select CONFIG_PAGE_POISONING_NO_SANITY
+ select CONFIG_PAGE_POISONING_ZERO
+ select CONFIG_SLUB_DEBUG
+ default y
+ help
+ Enable page poisoning and slub poisoning by default.
+
config INIT_ALL_STACK
bool "Initialize all stack"
depends on INIT_ALL_MEMORY
--
2.21.0.360.g471c308f928-goog
^ permalink raw reply related
* Re: [PATCH 1/1] RFC: initmem: introduce CONFIG_INIT_ALL_MEMORY and CONFIG_INIT_ALL_STACK
From: Alexander Potapenko @ 2019-03-08 13:29 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Masahiro Yamada, James Morris, Serge E. Hallyn,
linux-security-module, Linux Kbuild mailing list,
Kostya Serebryany, Dmitry Vyukov, Kees Cook, Sandeep Patil
In-Reply-To: <CAKwvOdnRNyANL4T3CZxWiE_s+mKTYBBXbczeei8Ne6Sy7jb+og@mail.gmail.com>
On Thu, Mar 7, 2019 at 7:37 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Thu, Mar 7, 2019 at 5:35 AM Alexander Potapenko <glider@google.com> wrote:
> >
> > This patch is a part of a bigger initiative to allow initializing
> > heap/stack memory in the Linux kernels by default.
> > The rationale behind doing so is to reduce the severity of bugs caused
> > by using uninitialized memory.
> >
> > CONFIG_INIT_ALL_MEMORY is going to be an umbrella config for options
> > that force heap and stack initialization.
> >
> > CONFIG_INIT_ALL_STACK turns on stack initialization based on the
> > -ftrivial-auto-var-init Clang flag.
> >
> > -ftrivial-auto-var-init is a Clang flag that provides trivial
> > initializers for uninitialized local variables, variable fields and
> > padding.
> >
> > It has three possible values:
> > pattern - uninitialized locals are filled with a fixed pattern
> > (mostly 0xAA on 64-bit platforms, see https://reviews.llvm.org/D54604
>
> Hopefully the disagreements there about "defining a new dialect of
> C++" don't apply to us in the kernel. We'd like to have this feature;
> our dialect of C is already quite far from what ISO has specified,
> both through the explicit use of `-f` compiler flags and use of most
> GNU C extensions. We already have our own dialect of C for the
> kernel, it's too late for that. But maybe I should go bikeshed in
> cfe-dev...
>
> Thanks for this patch. I'll take a closer look once Kees' feedback
> has been addressed.
I've sent out the updated version.
> > for more details) likely to cause crashes when uninitialized value is
> > used;
> > zero (it's still debated whether this flag makes it to the official
> > Clang release) - uninitialized locals are filled with zeroes;
> > uninitialized (default) - uninitialized locals are left intact.
> >
> > The proposed config builds the kernel with
> > -ftrivial-auto-var-init=pattern.
> >
> > Developers have the possibility to opt-out of this feature on a
> > per-file (by using the INIT_ALL_MEMORY_ Makefile prefix) or per-variable
> > (by using __attribute__((uninitialized))) basis.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Kostya Serebryany <kcc@google.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Sandeep Patil <sspatil@android.com>
> > Cc: linux-security-module@vger.kernel.org
> > Cc: linux-kbuild@vger.kernel.org
> >
> > ---
> > Makefile | 3 ++-
> > scripts/Makefile.initmem | 17 +++++++++++++++++
> > scripts/Makefile.lib | 6 ++++++
> > security/Kconfig | 1 +
> > security/Kconfig.initmem | 22 ++++++++++++++++++++++
> > 5 files changed, 48 insertions(+), 1 deletion(-)
> > create mode 100644 scripts/Makefile.initmem
> > create mode 100644 security/Kconfig.initmem
> >
> > diff --git a/Makefile b/Makefile
> > index f070e0d65186..028ca37878fd 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -448,7 +448,7 @@ export HOSTCXX KBUILD_HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
> >
> > export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS KBUILD_LDFLAGS
> > export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE
> > -export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN
> > +export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN CFLAGS_INITMEM
> > export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
> > export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
> > export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
> > @@ -840,6 +840,7 @@ KBUILD_ARFLAGS := $(call ar-option,D)
> > include scripts/Makefile.kasan
> > include scripts/Makefile.extrawarn
> > include scripts/Makefile.ubsan
> > +include scripts/Makefile.initmem
> >
> > # Add any arch overrides and user supplied CPPFLAGS, AFLAGS and CFLAGS as the
> > # last assignments
> > diff --git a/scripts/Makefile.initmem b/scripts/Makefile.initmem
> > new file mode 100644
> > index 000000000000..f49be398f2c1
> > --- /dev/null
> > +++ b/scripts/Makefile.initmem
> > @@ -0,0 +1,17 @@
> > +ifdef CONFIG_INIT_ALL_MEMORY
> > +
> > +# Clang's -ftrivial-auto-var-init=pattern flag initializes the
> > +# uninitialized parts of local variables (including fields and padding)
> > +# with a fixed pattern (0xAA in most cases).
> > +ifdef CONFIG_INIT_ALL_STACK
> > + CFLAGS_INITMEM := -ftrivial-auto-var-init=pattern
> > +endif
> > +
> > +ifeq ($(call cc-option, $(CFLAGS_INITMEM) -Werror),)
> > + ifneq ($(CONFIG_COMPILE_TEST),y)
> > + $(warning Cannot use CONFIG_INIT_ALL_MEMORY: \
> > + -ftrivial-auto-var-init is not supported by compiler)
> > + endif
> > +endif
> > +
> > +endif
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 12b88d09c3a4..53d18fd15c79 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -131,6 +131,12 @@ _c_flags += $(if $(patsubst n%,, \
> > $(CFLAGS_UBSAN))
> > endif
> >
> > +ifeq ($(CONFIG_INIT_ALL_MEMORY),y)
> > +_c_flags += $(if $(patsubst n%,, \
> > + $(INIT_ALL_MEMORY_$(basetarget).o)$(INIT_ALL_MEMORY)y), \
> > + $(CFLAGS_INITMEM))
> > +endif
> > +
> > ifeq ($(CONFIG_KCOV),y)
> > _c_flags += $(if $(patsubst n%,, \
> > $(KCOV_INSTRUMENT_$(basetarget).o)$(KCOV_INSTRUMENT)$(CONFIG_KCOV_INSTRUMENT_ALL)), \
> > diff --git a/security/Kconfig b/security/Kconfig
> > index e4fe2f3c2c65..cc12a39424dd 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -230,6 +230,7 @@ config STATIC_USERMODEHELPER_PATH
> > If you wish for all usermode helper programs to be disabled,
> > specify an empty string here (i.e. "").
> >
> > +source "security/Kconfig.initmem"
> > source "security/selinux/Kconfig"
> > source "security/smack/Kconfig"
> > source "security/tomoyo/Kconfig"
> > diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> > new file mode 100644
> > index 000000000000..5ac3cf3e7f88
> > --- /dev/null
> > +++ b/security/Kconfig.initmem
> > @@ -0,0 +1,22 @@
> > +menu "Initialize all memory"
> > +
> > +config INIT_ALL_MEMORY
> > + bool "Initialize all memory"
> > + default n
> > + help
> > + Enforce memory initialization to mitigate infoleaks and make
> > + the control-flow bugs depending on uninitialized values more
> > + deterministic.
> > +
> > +if INIT_ALL_MEMORY
> > +
> > +config INIT_ALL_STACK
> > + bool "Initialize all stack"
> > + depends on INIT_ALL_MEMORY
> > + default y
> > + help
> > + Initialize uninitialized stack data with a 0xAA pattern.
> > + This config option only supports Clang builds at the moment.
> > +
> > +endif # INIT_ALL_MEMORY
> > +endmenu
> > --
> > 2.21.0.352.gf09ad66450-goog
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
From: Mimi Zohar @ 2019-03-08 13:39 UTC (permalink / raw)
To: Matthew Garrett
Cc: Justin Forbes, linux-integrity, LSM List, linux-efi,
Linux Kernel Mailing List, David Howells, Seth Forshee, kexec,
Nayna Jain
In-Reply-To: <CACdnJusiLSCyqx7NX5bkpFr2N5YjBt_h3miOgkbey-Zndc3+Kg@mail.gmail.com>
On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote:
> On Thu, Mar 7, 2019 at 2:48 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > I added this last attempt because I'm seeing this on my laptop, with
> > some older, buggy firmware.
>
> Is the issue that it gives incorrect results on the first read, or is
> the issue that it gives incorrect results before ExitBootServices() is
> called? If the former then we should read twice in the boot stub, if
> the latter then we should figure out a way to do this immediately
> after ExitBootServices() instead.
Detecting the secure boot mode isn't the problem. On boot, I am
seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits
"Secure boot could not be determined".
In efi_main() the secure_boot mode is initially unset, so
efi_get_secureboot() is called. efi_get_secureboot() returns the
secure_boot mode correctly as enabled. The problem seems to be in
saving the secure_boot mode for later use.
Mimi
^ permalink raw reply
* Re: [PATCH 2/3] scripts/ima: define a set of common functions
From: Mimi Zohar @ 2019-03-08 13:45 UTC (permalink / raw)
To: Dave Young
Cc: linux-integrity, linux-security-module, linux-kernel, kexec,
David Howells, Eric Biederman
In-Reply-To: <20190308024450.GC7223@dhcp-128-65.nay.redhat.com>
On Fri, 2019-03-08 at 10:44 +0800, Dave Young wrote:
> Hi Mimi,
>
> Still did not get change to have a look at V2, but seems you missed the
> last chunk of comments about the secure boot mode in previous reply?
>
> I just copy it hear:
> '''
> Do you want to get the Secureboot status here?
> I got some advice from Peter Jones previously, thus we have below
> in our kdump scripts:
> https://src.fedoraproject.org/cgit/rpms/kexec-tools.git/tree/kdump-lib.sh
>
> See the function is_secure_boot_enforced(), probably you can refer to
> that function and check setup mode as well.
> '''
Thank you for the pointer to the kdump scripts and reminder.
Mimi
^ permalink raw reply
* Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
From: Matthew Garrett @ 2019-03-08 17:51 UTC (permalink / raw)
To: Mimi Zohar
Cc: Justin Forbes, linux-integrity, LSM List, linux-efi,
Linux Kernel Mailing List, David Howells, Seth Forshee, kexec,
Nayna Jain
In-Reply-To: <1552052377.4134.23.camel@linux.ibm.com>
On Fri, Mar 8, 2019 at 5:40 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote:
> > Is the issue that it gives incorrect results on the first read, or is
> > the issue that it gives incorrect results before ExitBootServices() is
> > called? If the former then we should read twice in the boot stub, if
> > the latter then we should figure out a way to do this immediately
> > after ExitBootServices() instead.
>
> Detecting the secure boot mode isn't the problem. On boot, I am
> seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits
> "Secure boot could not be determined".
>
> In efi_main() the secure_boot mode is initially unset, so
> efi_get_secureboot() is called. efi_get_secureboot() returns the
> secure_boot mode correctly as enabled. The problem seems to be in
> saving the secure_boot mode for later use.
Hm. And this only happens on certain firmware versions? If something's
stepping on boot_params then we have bigger problems.
^ permalink raw reply
* Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
From: Mimi Zohar @ 2019-03-08 18:43 UTC (permalink / raw)
To: Matthew Garrett
Cc: Justin Forbes, linux-integrity, LSM List, linux-efi,
Linux Kernel Mailing List, David Howells, Seth Forshee, kexec,
Nayna Jain
In-Reply-To: <CACdnJut3Y7CogvgtNsEcrEtA1WOxSb9OWP_wZ=3xNgCft2oeDw@mail.gmail.com>
On Fri, 2019-03-08 at 09:51 -0800, Matthew Garrett wrote:
> On Fri, Mar 8, 2019 at 5:40 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote:
> > > Is the issue that it gives incorrect results on the first read, or is
> > > the issue that it gives incorrect results before ExitBootServices() is
> > > called? If the former then we should read twice in the boot stub, if
> > > the latter then we should figure out a way to do this immediately
> > > after ExitBootServices() instead.
> >
> > Detecting the secure boot mode isn't the problem. On boot, I am
> > seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits
> > "Secure boot could not be determined".
> >
> > In efi_main() the secure_boot mode is initially unset, so
> > efi_get_secureboot() is called. efi_get_secureboot() returns the
> > secure_boot mode correctly as enabled. The problem seems to be in
> > saving the secure_boot mode for later use.
>
> Hm. And this only happens on certain firmware versions? If something's
> stepping on boot_params then we have bigger problems.
FYI, efi_printk() works before exit_boot(), but not afterwards. The
system hangs.
Mimi
^ permalink raw reply
* Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
From: Matthew Garrett @ 2019-03-08 20:22 UTC (permalink / raw)
To: Mimi Zohar
Cc: Justin Forbes, linux-integrity, LSM List, linux-efi,
Linux Kernel Mailing List, David Howells, Seth Forshee, kexec,
Nayna Jain
In-Reply-To: <1552070598.4134.51.camel@linux.ibm.com>
On Fri, Mar 8, 2019 at 10:43 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> FYI, efi_printk() works before exit_boot(), but not afterwards. The
> system hangs.
efi_printk() uses boot services to print, so that's not unexpected :)
It would probably be sensible to return an error rather than crash,
though…
^ permalink raw reply
* [GIT PULL] security: integrity subsystem updates for v5.1
From: James Morris @ 2019-03-08 22:44 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-security-module
[-- Attachment #1: Type: text/plain, Size: 4033 bytes --]
Please pull these changes from Mimi Zohar:
"Linux 5.0 introduced the platform keyring to allow verifying the IMA
kexec kernel image signature using the pre-boot keys. This pull
request similarly makes keys on the platform keyring accessible for
verifying the PE kernel image signature.*
Also included in this pull request is a new IMA hook that tags tmp
files, in policy, indicating the file hash needs to be calculated.
The remaining patches are cleanup."
---
The following changes since commit 8bd8ea195f6d135a8d85201116314eb5237ad7e7:
Merge tag 'v4.20-rc7' into next-general (2018-12-17 11:24:28 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-integrity
for you to fetch changes up to c7f7e58fcbf33589f11bfde0506e076a00627e59:
integrity: Remove references to module keyring (2018-12-17 14:09:39 -0800)
----------------------------------------------------------------
Dave Howells (2):
efi: Add EFI signature data types
efi: Add an EFI signature blob parser
Eric Richter (1):
x86/ima: define arch_get_ima_policy() for x86
James Morris (1):
Merge branch 'next-integrity' of git://git.kernel.org/.../zohar/linux-integrity into next-integrity
Josh Boyer (2):
efi: Import certificates from UEFI Secure Boot
efi: Allow the "db" UEFI variable to be suppressed
Mimi Zohar (4):
integrity: support new struct public_key_signature encoding field
x86/ima: retry detecting secure boot mode
ima: don't measure/appraise files on efivarfs
selftests/ima: kexec_load syscall test
Nayna Jain (7):
x86/ima: define arch_ima_get_secureboot
ima: prevent kexec_load syscall based on runtime secureboot flag
ima: refactor ima_init_policy()
ima: add support for arch specific policies
integrity: Define a trusted platform keyring
integrity: Load certs to the platform keyring
ima: Support platform keyring for kernel appraisal
Nikolay Borisov (1):
ima: Use inode_is_open_for_write
Stefan Berger (1):
docs: Extend trusted keys documentation for TPM 2.0
Thiago Jung Bauermann (1):
integrity: Remove references to module keyring
Documentation/security/keys/trusted-encrypted.rst | 31 +++-
arch/x86/kernel/Makefile | 4 +
arch/x86/kernel/ima_arch.c | 75 ++++++++
include/linux/efi.h | 34 ++++
include/linux/ima.h | 15 ++
security/integrity/Kconfig | 11 ++
security/integrity/Makefile | 5 +
security/integrity/digsig.c | 111 ++++++++----
security/integrity/ima/Kconfig | 10 +-
security/integrity/ima/ima_appraise.c | 14 +-
security/integrity/ima/ima_main.c | 21 ++-
security/integrity/ima/ima_policy.c | 171 +++++++++++++-----
security/integrity/integrity.h | 22 ++-
security/integrity/platform_certs/efi_parser.c | 108 ++++++++++++
security/integrity/platform_certs/load_uefi.c | 194 +++++++++++++++++++++
.../integrity/platform_certs/platform_keyring.c | 58 ++++++
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/ima/Makefile | 11 ++
tools/testing/selftests/ima/config | 4 +
tools/testing/selftests/ima/test_kexec_load.sh | 54 ++++++
20 files changed, 861 insertions(+), 93 deletions(-)
create mode 100644 arch/x86/kernel/ima_arch.c
create mode 100644 security/integrity/platform_certs/efi_parser.c
create mode 100644 security/integrity/platform_certs/load_uefi.c
create mode 100644 security/integrity/platform_certs/platform_keyring.c
create mode 100644 tools/testing/selftests/ima/Makefile
create mode 100644 tools/testing/selftests/ima/config
create mode 100755 tools/testing/selftests/ima/test_kexec_load.sh
^ permalink raw reply
* [GIT PULL] security: tpm subsystem updates for v5.1
From: James Morris @ 2019-03-08 22:49 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-security-module
Please pull these updates from Jarkko Sakkinen:
"
Clean up the transmission flow
==============================
Cleaned up the whole transmission flow. Locking of the chip is now done in
the level of tpm_try_get_ops() and tpm_put_ops() instead taking the chip
lock inside tpm_transmit(). The nested calls inside tpm_transmit(), used
with the resource manager, have been refactored out.
Should make easier to perform more complex transactions with the TPM
without making the subsystem a bigger mess (e.g. encrypted channel patches
by James Bottomley).
PPI 1.3 support
===============
TPM PPI 1.3 introduces an additional optional command parameter that may
be
needed for some commands. Display the parameter if the command requires
such a parameter. Only command 23 (SetPCRBanks) needs one.
The PPI request file will show output like this then:
# echo "23 16" > request
# cat request
23 16
# echo "5" > request
# cat request
5
Extend all PCR banks in IMA
===========================
Instead of static PCR banks array, the array of available PCR banks is now
allocated dynamically. The digests sizes are determined dynamically using
a probe PCR read without relying crypto's static list of hash algorithms.
This should finally make sealing of measurements in IMA safe and secure.
TPM 2.0 selftests
=================
Added a test suite to tools/testing/selftests/tpm2 previously outside of
the kernel tree: https://github.com/jsakkine-intel/tpm2-scripts.
"
---
The following changes since commit e7a44cfd639945a0dec749f896adc1d340c2a6aa:
LSM: fix return value check in safesetid_init_securityfs() (2019-02-12 10:59:22 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-tpm
for you to fetch changes up to 5da10728037afea6743b76afddfdc9950cd711b3:
Merge tag 'tpmdd-next-20190213' of git://git.infradead.org/users/jjs/linux-tpmdd into next-tpm (2019-02-13 12:01:00 -0800)
----------------------------------------------------------------
James Morris (1):
Merge tag 'tpmdd-next-20190213' of git://git.infradead.org/users/jjs/linux-tpmdd into next-tpm
Jarkko Sakkinen (21):
tpm/tpm_crb: Avoid unaligned reads in crb_recv()
tpm: Fix some name collisions with drivers/char/tpm.h
selftests: add TPM 2.0 tests
tpm: Unify the send callback behaviour
tpm/tpm_i2c_atmel: Return -E2BIG when the transfer is incomplete
tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
tpm: fix invalid return value in pubek_show()
tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
tpm: print tpm2_commit_space() error inside tpm2_commit_space()
tpm: declare struct tpm_header
tpm: access command header through struct in tpm_try_transmit()
tpm: encapsulate tpm_dev_transmit()
tpm: clean up tpm_try_transmit() error handling flow
tpm: move tpm_validate_commmand() to tpm2-space.c
tpm: move TPM space code out of tpm_transmit()
tpm: remove @space from tpm_transmit()
tpm: use tpm_try_get_ops() in tpm-sysfs.c.
tpm: remove TPM_TRANSMIT_UNLOCKED flag
tpm: introduce tpm_chip_start() and tpm_chip_stop()
tpm: take TPM chip power gating out of tpm_transmit()
tpm: remove @flags from tpm_transmit()
Jerry Snitselaar (2):
tpm: don't print error message in tpm_transmit_cmd when tpm still testing
tpm: don't return bool from update_timeouts
Jia Zhang (2):
tpm: Simplify the measurements loop
tpm: Fix off-by-one when reading binary_bios_measurements
Roberto Sassu (7):
tpm: add _head suffix to tcg_efi_specid_event and tcg_pcr_event2
tpm: dynamically allocate the allocated_banks array
tpm: rename and export tpm2_digest and tpm2_algorithms
tpm: retrieve digest size of unknown algorithms with PCR read
tpm: move tpm_chip definition to include/linux/tpm.h
KEYS: trusted: explicitly use tpm_chip structure from tpm_default_chip()
tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
Stefan Berger (5):
tpm/ppi: pass function revision ID to tpm_eval_dsm()
tpm/ppi: rename TPM_PPI_REVISION_ID to TPM_PPI_REVISION_ID_1
tpm/ppi: Display up to 101 operations as define for version 1.3
tpm/ppi: Possibly show command parameter if TPM PPI 1.3 is used
tpm/ppi: Enable submission of optional command parameter for PPI 1.3
drivers/char/tpm/eventlog/tpm1.c | 41 +-
drivers/char/tpm/eventlog/tpm2.c | 12 +-
drivers/char/tpm/st33zp24/i2c.c | 2 +-
drivers/char/tpm/st33zp24/spi.c | 2 +-
drivers/char/tpm/st33zp24/st33zp24.c | 2 +-
drivers/char/tpm/st33zp24/st33zp24.h | 4 +-
drivers/char/tpm/tpm-chip.c | 124 ++++-
drivers/char/tpm/tpm-dev-common.c | 44 +-
drivers/char/tpm/tpm-interface.c | 327 ++++----------
drivers/char/tpm/tpm-sysfs.c | 138 +++---
drivers/char/tpm/tpm.h | 180 ++------
drivers/char/tpm/tpm1-cmd.c | 43 +-
drivers/char/tpm/tpm2-cmd.c | 208 +++++----
drivers/char/tpm/tpm2-space.c | 90 +++-
drivers/char/tpm/tpm_atmel.c | 2 +-
drivers/char/tpm/tpm_crb.c | 22 +-
drivers/char/tpm/tpm_i2c_atmel.c | 15 +-
drivers/char/tpm/tpm_i2c_infineon.c | 17 +-
drivers/char/tpm/tpm_i2c_nuvoton.c | 18 +-
drivers/char/tpm/tpm_ibmvtpm.c | 8 +-
drivers/char/tpm/tpm_infineon.c | 2 +-
drivers/char/tpm/tpm_nsc.c | 2 +-
drivers/char/tpm/tpm_ppi.c | 78 +++-
drivers/char/tpm/tpm_tis_core.c | 21 +-
drivers/char/tpm/tpm_vtpm_proxy.c | 15 +-
drivers/char/tpm/xen-tpmfront.c | 4 +-
include/linux/tpm.h | 129 +++++-
include/linux/tpm_eventlog.h | 19 +-
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_crypto.c | 10 +-
security/integrity/ima/ima_init.c | 4 +
security/integrity/ima/ima_queue.c | 27 +-
security/keys/trusted.c | 73 ++-
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/tpm2/Makefile | 4 +
tools/testing/selftests/tpm2/test_smoke.sh | 4 +
tools/testing/selftests/tpm2/test_space.sh | 4 +
tools/testing/selftests/tpm2/tpm2.py | 696 +++++++++++++++++++++++++++++
tools/testing/selftests/tpm2/tpm2_tests.py | 227 ++++++++++
39 files changed, 1876 insertions(+), 744 deletions(-)
create mode 100644 tools/testing/selftests/tpm2/Makefile
create mode 100755 tools/testing/selftests/tpm2/test_smoke.sh
create mode 100755 tools/testing/selftests/tpm2/test_space.sh
create mode 100644 tools/testing/selftests/tpm2/tpm2.py
create mode 100644 tools/testing/selftests/tpm2/tpm2_tests.py
^ permalink raw reply
* Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down
From: James Morris @ 2019-03-08 23:00 UTC (permalink / raw)
To: Matthew Garrett; +Cc: linux-security-module, linux-kernel, dhowells
In-Reply-To: <20190306235913.6631-4-matthewgarrett@google.com>
On Wed, 6 Mar 2019, Matthew Garrett wrote:
> From: David Howells <dhowells@redhat.com>
>
> If the kernel is locked down, require that all modules have valid
> signatures that we can verify.
Perhaps note that this won't cover the case where folk are using DM-Verity
with a signed root hash for verifying kernel modules.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down
From: Matthew Garrett @ 2019-03-08 23:30 UTC (permalink / raw)
To: James Morris; +Cc: LSM List, Linux Kernel Mailing List, David Howells
In-Reply-To: <alpine.LRH.2.21.1903090959210.12052@namei.org>
On Fri, Mar 8, 2019 at 3:00 PM James Morris <jmorris@namei.org> wrote:
>
> On Wed, 6 Mar 2019, Matthew Garrett wrote:
>
> > From: David Howells <dhowells@redhat.com>
> >
> > If the kernel is locked down, require that all modules have valid
> > signatures that we can verify.
>
> Perhaps note that this won't cover the case where folk are using DM-Verity
> with a signed root hash for verifying kernel modules.
Mm. I can't see a terribly good way of doing this generically -
loadpin gives no indication to the module loading code that it comes
from a trusted source. Would making the lockdown/module signature
enforcement a separate config option be reasonable?
^ permalink raw reply
* Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down
From: James Morris @ 2019-03-09 4:45 UTC (permalink / raw)
To: Matthew Garrett; +Cc: LSM List, Linux Kernel Mailing List, David Howells
In-Reply-To: <CACdnJuvhPd2dD7cWkxP78YMMFKU3FsigLHgkDntwhnbamNZr-w@mail.gmail.com>
On Fri, 8 Mar 2019, Matthew Garrett wrote:
> On Fri, Mar 8, 2019 at 3:00 PM James Morris <jmorris@namei.org> wrote:
> >
> > On Wed, 6 Mar 2019, Matthew Garrett wrote:
> >
> > > From: David Howells <dhowells@redhat.com>
> > >
> > > If the kernel is locked down, require that all modules have valid
> > > signatures that we can verify.
> >
> > Perhaps note that this won't cover the case where folk are using DM-Verity
> > with a signed root hash for verifying kernel modules.
>
> Mm. I can't see a terribly good way of doing this generically -
> loadpin gives no indication to the module loading code that it comes
> from a trusted source. Would making the lockdown/module signature
> enforcement a separate config option be reasonable?
I was just suggest documenting this.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Inquiry March-2019
From: Daniel Murray @ 2019-03-09 9:50 UTC (permalink / raw)
To: linux-security-module
Hi,friend,
This is Daniel Murray and i am from Sinara Group Co.,LTD in Russia.
We are glad to know about your company from the web and we are interested in your products.
Could you kindly send us your Latest catalog and price list for our trial order.
Thanks and Best Regards,
Daniel Murray
Purchasing Manager
Sinara Group Co.,LTD
^ permalink raw reply
* Re: [PATCH] tomoyo: Add a kernel config option for fuzzing testing.
From: Tetsuo Handa @ 2019-03-11 13:18 UTC (permalink / raw)
To: James Morris; +Cc: Stephen Smalley, linux-security-module
In-Reply-To: <alpine.LRH.2.21.1903051431540.20560@namei.org>
On 2019/03/05 12:32, James Morris wrote:
> On Tue, 5 Mar 2019, Tetsuo Handa wrote:
>
>> I guess that majority of TOMOYO users are now using the upstream version. But
>> pre-LSM version and/or AKARI will remain there until LKM-based LSMs becomes
>> officially supported
>
> You mean dynamically loadable LSMs?
Yes. As long as upstream can't accept all LSM modules, and some people cannot afford
utilizing upstream LSM modules, LKM-based LSMs will be needed by such people.
>
> There are no plans to support this.
Currently you don't have a plan. But I have.
It took 10+ years to be able to allow coexisting inode based access control
and name based access control. And there are people who still cannot afford
keeping upstream LSM modules enabled.
Anyway, your question is irrelevant to whether to allow syzbot to test
TOMOYO module. syzbot already bisected this problem to an innocent
commit 89a9684ea158dd7e ("LSM: Ignore "security=" when "lsm=" is specified")
at https://syzkaller.appspot.com/bug?id=32ab41bbdc0c28643c507dd0cf1eea1a9ce67837 .
Will you send this patch to linux.git so that syzbot can test TOMOYO module?
^ permalink raw reply
* Re: kernel panic: MAC Initialization failed. (3)
From: syzbot @ 2019-03-11 13:26 UTC (permalink / raw)
To: casey, james.morris, jmorris, john.johansen, keescook,
linux-kernel, linux-security-module, penguin-kernel, serge,
syzkaller-bugs, takedakn
In-Reply-To: <000000000000f7d1840582f4df63@google.com>
syzbot has bisected this bug to:
commit 89a9684ea158dd7eef1728be9f0aed9a7d41cf19
Author: Kees Cook <keescook@chromium.org>
Date: Tue Feb 12 18:23:18 2019 +0000
LSM: Ignore "security=" when "lsm=" is specified
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=11572723200000
start commit: 89a9684e LSM: Ignore "security=" when "lsm=" is specified
git tree: linux-next
final crash: https://syzkaller.appspot.com/x/report.txt?x=13572723200000
console output: https://syzkaller.appspot.com/x/log.txt?x=15572723200000
kernel config: https://syzkaller.appspot.com/x/.config?x=c0f38652d28b522f
dashboard link: https://syzkaller.appspot.com/bug?extid=2ee3f8974c2e7dc69feb
userspace arch: amd64
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14c68242c00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17190c8ac00000
Reported-by: syzbot+2ee3f8974c2e7dc69feb@syzkaller.appspotmail.com
Fixes: 89a9684e ("LSM: Ignore "security=" when "lsm=" is specified")
^ permalink raw reply
* [PATCH] secuirty: integrity: ima: pedantic formatting
From: Enrico Weigelt, metux IT consult @ 2019-03-11 13:44 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-integrity, linux-security-module
Formatting of Kconfig files doesn't look so pretty, so let the
Great White Handkerchief come around and clean it up.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
security/integrity/ima/Kconfig | 64 +++++++++++++++++++++---------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index a18f8c6..416b724 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -34,12 +34,12 @@ config IMA_KEXEC
depends on IMA && TCG_TPM && HAVE_IMA_KEXEC
default n
help
- TPM PCRs are only reset on a hard reboot. In order to validate
- a TPM's quote after a soft boot, the IMA measurement list of the
- running kernel must be saved and restored on boot.
+ TPM PCRs are only reset on a hard reboot. In order to validate
+ a TPM's quote after a soft boot, the IMA measurement list of the
+ running kernel must be saved and restored on boot.
- Depending on the IMA policy, the measurement list can grow to
- be very large.
+ Depending on the IMA policy, the measurement list can grow to
+ be very large.
config IMA_MEASURE_PCR_IDX
int
@@ -91,10 +91,10 @@ choice
default IMA_DEFAULT_HASH_SHA1
depends on IMA
help
- Select the default hash algorithm used for the measurement
- list, integrity appraisal and audit log. The compiled default
- hash algorithm can be overwritten using the kernel command
- line 'ima_hash=' option.
+ Select the default hash algorithm used for the measurement
+ list, integrity appraisal and audit log. The compiled default
+ hash algorithm can be overwritten using the kernel command
+ line 'ima_hash=' option.
config IMA_DEFAULT_HASH_SHA1
bool "SHA1 (default)"
@@ -138,9 +138,9 @@ config IMA_READ_POLICY
default y if IMA_WRITE_POLICY
default n if !IMA_WRITE_POLICY
help
- It is often useful to be able to read back the IMA policy. It is
- even more important after introducing CONFIG_IMA_WRITE_POLICY.
- This option allows the root user to see the current policy rules.
+ It is often useful to be able to read back the IMA policy. It is
+ even more important after introducing CONFIG_IMA_WRITE_POLICY.
+ This option allows the root user to see the current policy rules.
config IMA_APPRAISE
bool "Appraise integrity measurements"
@@ -158,12 +158,12 @@ config IMA_APPRAISE
If unsure, say N.
config IMA_ARCH_POLICY
- bool "Enable loading an IMA architecture specific policy"
- depends on KEXEC_VERIFY_SIG || IMA_APPRAISE && INTEGRITY_ASYMMETRIC_KEYS
- default n
- help
- This option enables loading an IMA architecture specific policy
- based on run time secure boot flags.
+ bool "Enable loading an IMA architecture specific policy"
+ depends on KEXEC_VERIFY_SIG || IMA_APPRAISE && INTEGRITY_ASYMMETRIC_KEYS
+ default n
+ help
+ This option enables loading an IMA architecture specific policy
+ based on run time secure boot flags.
config IMA_APPRAISE_BUILD_POLICY
bool "IMA build time configured policy rules"
@@ -238,10 +238,10 @@ config IMA_TRUSTED_KEYRING
select INTEGRITY_TRUSTED_KEYRING
default y
help
- This option requires that all keys added to the .ima
- keyring be signed by a key on the system trusted keyring.
+ This option requires that all keys added to the .ima
+ keyring be signed by a key on the system trusted keyring.
- This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING
+ This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING
config IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
bool "Permit keys validly signed by a built-in or secondary CA cert (EXPERIMENTAL)"
@@ -266,32 +266,32 @@ config IMA_BLACKLIST_KEYRING
depends on IMA_TRUSTED_KEYRING
default n
help
- This option creates an IMA blacklist keyring, which contains all
- revoked IMA keys. It is consulted before any other keyring. If
- the search is successful the requested operation is rejected and
- an error is returned to the caller.
+ This option creates an IMA blacklist keyring, which contains all
+ revoked IMA keys. It is consulted before any other keyring. If
+ the search is successful the requested operation is rejected and
+ an error is returned to the caller.
config IMA_LOAD_X509
bool "Load X509 certificate onto the '.ima' trusted keyring"
depends on IMA_TRUSTED_KEYRING
default n
help
- File signature verification is based on the public keys
- loaded on the .ima trusted keyring. These public keys are
- X509 certificates signed by a trusted key on the
- .system keyring. This option enables X509 certificate
- loading from the kernel onto the '.ima' trusted keyring.
+ File signature verification is based on the public keys
+ loaded on the .ima trusted keyring. These public keys are
+ X509 certificates signed by a trusted key on the
+ .system keyring. This option enables X509 certificate
+ loading from the kernel onto the '.ima' trusted keyring.
config IMA_X509_PATH
string "IMA X509 certificate path"
depends on IMA_LOAD_X509
default "/etc/keys/x509_ima.der"
help
- This option defines IMA X509 certificate path.
+ This option defines IMA X509 certificate path.
config IMA_APPRAISE_SIGNED_INIT
bool "Require signed user-space initialization"
depends on IMA_LOAD_X509
default n
help
- This option requires user-space init to be signed.
+ This option requires user-space init to be signed.
--
1.9.1
^ permalink raw reply related
* Re: kernel panic: MAC Initialization failed. (3)
From: Tetsuo Handa @ 2019-03-11 13:45 UTC (permalink / raw)
To: casey, james.morris, jmorris, john.johansen, keescook, serge,
takedakn
Cc: syzbot, linux-kernel, linux-security-module, syzkaller-bugs
In-Reply-To: <0000000000008407390583d18381@google.com>
F.Y.I. Nothing is wrong with that commit. That commit merely allows enabling TOMOYO and
one of SELinux/Smack/AppArmor at the same time for syzbot's kernel command line options.
This problem will be handled by a patch at
https://lore.kernel.org/linux-security-module/1551362770-8655-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp/
and then updating syzbot to build kernels with this option enabled.
Well, it is wonderful that syzbot started bisecting. ;-)
On 2019/03/11 22:26, syzbot wrote:
> syzbot has bisected this bug to:
>
> commit 89a9684ea158dd7eef1728be9f0aed9a7d41cf19
> Author: Kees Cook <keescook@chromium.org>
> Date: Tue Feb 12 18:23:18 2019 +0000
>
> LSM: Ignore "security=" when "lsm=" is specified
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=11572723200000
> start commit: 89a9684e LSM: Ignore "security=" when "lsm=" is specified
> git tree: linux-next
> final crash: https://syzkaller.appspot.com/x/report.txt?x=13572723200000
> console output: https://syzkaller.appspot.com/x/log.txt?x=15572723200000
> kernel config: https://syzkaller.appspot.com/x/.config?x=c0f38652d28b522f
> dashboard link: https://syzkaller.appspot.com/bug?extid=2ee3f8974c2e7dc69feb
> userspace arch: amd64
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14c68242c00000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17190c8ac00000
>
> Reported-by: syzbot+2ee3f8974c2e7dc69feb@syzkaller.appspotmail.com
> Fixes: 89a9684e ("LSM: Ignore "security=" when "lsm=" is specified")
>
^ permalink raw reply
* Re: [GIT PULL] security: tpm subsystem updates for v5.1
From: pr-tracker-bot @ 2019-03-11 15:55 UTC (permalink / raw)
To: James Morris; +Cc: Linus Torvalds, linux-kernel, linux-security-module
In-Reply-To: <alpine.LRH.2.21.1903090946210.12052@namei.org>
The pull request you sent on Sat, 9 Mar 2019 09:49:56 +1100 (AEDT):
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-tpm
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/5af7f115886f7ec193171e2e49b8000ddd1e7147
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
From: Mimi Zohar @ 2019-03-11 16:54 UTC (permalink / raw)
To: Matthew Garrett
Cc: Justin Forbes, linux-integrity, LSM List, linux-efi,
Linux Kernel Mailing List, David Howells, Seth Forshee, kexec,
Nayna Jain
In-Reply-To: <CACdnJut3Y7CogvgtNsEcrEtA1WOxSb9OWP_wZ=3xNgCft2oeDw@mail.gmail.com>
On Fri, 2019-03-08 at 09:51 -0800, Matthew Garrett wrote:
> On Fri, Mar 8, 2019 at 5:40 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote:
> > > Is the issue that it gives incorrect results on the first read, or is
> > > the issue that it gives incorrect results before ExitBootServices() is
> > > called? If the former then we should read twice in the boot stub, if
> > > the latter then we should figure out a way to do this immediately
> > > after ExitBootServices() instead.
> >
> > Detecting the secure boot mode isn't the problem. On boot, I am
> > seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits
> > "Secure boot could not be determined".
> >
> > In efi_main() the secure_boot mode is initially unset, so
> > efi_get_secureboot() is called. efi_get_secureboot() returns the
> > secure_boot mode correctly as enabled. The problem seems to be in
> > saving the secure_boot mode for later use.
>
> Hm. And this only happens on certain firmware versions? If something's
> stepping on boot_params then we have bigger problems.
I was seeing this problem before and after updating the system
firmware on my laptop last summer. If updating the firmware had
resolved the problem, I wouldn't have included this patch.
Mimi
^ permalink raw reply
* Re: [PATCH v4 2/2] LSM: SafeSetID: gate setgid transitions
From: Micah Morton @ 2019-03-11 17:25 UTC (permalink / raw)
To: James Morris, Serge E. Hallyn, Kees Cook, Casey Schaufler,
Stephen Smalley, linux-security-module
In-Reply-To: <20190305155229.62831-1-mortonm@chromium.org>
Is this ready to be merged if there are no further comments?
On Tue, Mar 5, 2019 at 7:52 AM <mortonm@chromium.org> wrote:
>
> From: Micah Morton <mortonm@chromium.org>
>
> The SafeSetID LSM already gates setuid transitions for UIDs on the
> system whose use of CAP_SETUID has been 'restricted'. This patch
> implements the analogous functionality for setgid transitions, in order
> to restrict the use of CAP_SETGID for certain UIDs on the system. One
> notable consequence of this addition is that a process running under a
> restricted UID (i.e. one that is only allowed to setgid to certain
> approved GIDs) will not be allowed to call the setgroups() syscall to
> set its supplementary group IDs. For now, we leave such support for
> restricted setgroups() to future work, as it would require hooking the
> logic in setgroups() and verifying that the array of GIDs passed in from
> userspace only consists of approved GIDs.
>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
> ---
> Changes since the last patch: Minor updates since patch 1/2 combined the
> 'task_fix_setuid' and 'task_fix_setgid' hooks into one hook called
> 'task_fix_setid'.
>
> Documentation/admin-guide/LSM/SafeSetID.rst | 9 +-
> security/safesetid/lsm.c | 272 +++++++++++++++++---
> security/safesetid/lsm.h | 11 +-
> security/safesetid/securityfs.c | 105 +++++---
> 4 files changed, 320 insertions(+), 77 deletions(-)
>
> diff --git a/Documentation/admin-guide/LSM/SafeSetID.rst b/Documentation/admin-guide/LSM/SafeSetID.rst
> index 670a6544fd39..33307a8e9555 100644
> --- a/Documentation/admin-guide/LSM/SafeSetID.rst
> +++ b/Documentation/admin-guide/LSM/SafeSetID.rst
> @@ -98,10 +98,13 @@ Directions for use
> ==================
> This LSM hooks the setid syscalls to make sure transitions are allowed if an
> applicable restriction policy is in place. Policies are configured through
> -securityfs by writing to the safesetid/add_whitelist_policy and
> +securityfs by writing to the safesetid/add_whitelist_{uid/gid}_policy* and
> safesetid/flush_whitelist_policies files at the location where securityfs is
> -mounted. The format for adding a policy is '<UID>:<UID>', using literal
> +mounted. The format for adding a policy is '<ID>:<ID>', using literal
> numbers, such as '123:456'. To flush the policies, any write to the file is
> sufficient. Again, configuring a policy for a UID will prevent that UID from
> obtaining auxiliary setid privileges, such as allowing a user to set up user
> -namespace UID mappings.
> +namespace ID mappings.
> +
> +*safesetid/add_whitelist_policy can also be used for UID policies, since it is
> +an alias for safesetid/add_whitelist_uid_policy
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index 5deffa92f25f..95c6f35b29f3 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -26,27 +26,30 @@ int safesetid_initialized;
>
> #define NUM_BITS 8 /* 128 buckets in hash table */
>
> -static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
> +static DEFINE_HASHTABLE(safesetid_whitelist_uid_hashtable, NUM_BITS);
> +static DEFINE_HASHTABLE(safesetid_whitelist_gid_hashtable, NUM_BITS);
> +
> +static DEFINE_SPINLOCK(safesetid_whitelist_uid_hashtable_spinlock);
> +static DEFINE_SPINLOCK(safesetid_whitelist_gid_hashtable_spinlock);
>
> /*
> * Hash table entry to store safesetid policy signifying that 'parent' user
> - * can setid to 'child' user.
> + * can setid to 'child' user. This struct is used in both the uid and gid
> + * hashtables.
> */
> -struct entry {
> +struct id_entry {
> struct hlist_node next;
> struct hlist_node dlist; /* for deletion cleanup */
> uint64_t parent_kuid;
> - uint64_t child_kuid;
> + uint64_t child_kid; /* Represents either a UID or a GID */
> };
>
> -static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
> -
> static bool check_setuid_policy_hashtable_key(kuid_t parent)
> {
> - struct entry *entry;
> + struct id_entry *entry;
>
> rcu_read_lock();
> - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> + hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
> entry, next, __kuid_val(parent)) {
> if (entry->parent_kuid == __kuid_val(parent)) {
> rcu_read_unlock();
> @@ -61,13 +64,49 @@ static bool check_setuid_policy_hashtable_key(kuid_t parent)
> static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> kuid_t child)
> {
> - struct entry *entry;
> + struct id_entry *entry;
> +
> + rcu_read_lock();
> + hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
> + entry, next, __kuid_val(parent)) {
> + if (entry->parent_kuid == __kuid_val(parent) &&
> + entry->child_kid == __kuid_val(child)) {
> + rcu_read_unlock();
> + return true;
> + }
> + }
> + rcu_read_unlock();
> +
> + return false;
> +}
> +
> +static bool check_setgid_policy_hashtable_key(kuid_t parent)
> +{
> + struct id_entry *entry;
> +
> + rcu_read_lock();
> + hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
> + entry, next, __kuid_val(parent)) {
> + if (entry->parent_kuid == __kuid_val(parent)) {
> + rcu_read_unlock();
> + return true;
> + }
> + }
> + rcu_read_unlock();
> +
> + return false;
> +}
> +
> +static bool check_setgid_policy_hashtable_key_value(kuid_t parent,
> + kgid_t child)
> +{
> + struct id_entry *entry;
>
> rcu_read_lock();
> - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> + hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
> entry, next, __kuid_val(parent)) {
> if (entry->parent_kuid == __kuid_val(parent) &&
> - entry->child_kuid == __kuid_val(child)) {
> + entry->child_kid == __kgid_val(child)) {
> rcu_read_unlock();
> return true;
> }
> @@ -77,6 +116,12 @@ static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> return false;
> }
>
> +/*
> + * This hook causes the security_capable check to fail when there are
> + * restriction policies for a UID and the process is trying to do something
> + * (other than a setid transition) that is gated by CAP_SETUID/CAP_SETGID
> + * (e.g. allowing user to set up userns UID/GID mappings).
> + */
> static int safesetid_security_capable(const struct cred *cred,
> struct user_namespace *ns,
> int cap,
> @@ -85,17 +130,19 @@ static int safesetid_security_capable(const struct cred *cred,
> if (cap == CAP_SETUID &&
> check_setuid_policy_hashtable_key(cred->uid)) {
> if (!(opts & CAP_OPT_INSETID)) {
> - /*
> - * Deny if we're not in a set*uid() syscall to avoid
> - * giving powers gated by CAP_SETUID that are related
> - * to functionality other than calling set*uid() (e.g.
> - * allowing user to set up userns uid mappings).
> - */
> pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions",
> __kuid_val(cred->uid));
> return -1;
> }
> }
> + if (cap == CAP_SETGID &&
> + check_setgid_policy_hashtable_key(cred->uid)) {
> + if (!(opts & CAP_OPT_INSETID)) {
> + pr_warn("Operation requires CAP_SETGID, which is not available to UID %u for operations besides approved set*gid transitions",
> + __kuid_val(cred->uid));
> + return -1;
> + }
> + }
> return 0;
> }
>
> @@ -115,19 +162,48 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
> return -EACCES;
> }
>
> +static int check_gid_transition(kuid_t parent, kgid_t child)
> +{
> + if (check_setgid_policy_hashtable_key_value(parent, child))
> + return 0;
> + pr_warn("Denied UID %d setting GID to %d",
> + __kuid_val(parent),
> + __kgid_val(child));
> + /*
> + * Kill this process to avoid potential security vulnerabilities
> + * that could arise from a missing whitelist entry preventing a
> + * privileged process from dropping to a lesser-privileged one.
> + */
> + force_sig(SIGKILL, current);
> + return -EACCES;
> +}
> +
> /*
> * Check whether there is either an exception for user under old cred struct to
> - * set*uid to user under new cred struct, or the UID transition is allowed (by
> - * Linux set*uid rules) even without CAP_SETUID.
> + * set*id to user under new cred struct, or the ID transition is allowed (by
> + * Linux set*id rules) even without CAP_SET*ID.
> */
> static int safesetid_task_fix_setid(struct cred *new,
> const struct cred *old,
> int flags)
> {
> + if (flags == LSM_SETUID_RE ||
> + flags == LSM_SETUID_ID ||
> + flags == LSM_SETUID_RES ||
> + flags == LSM_SETUID_FS) {
> + /* Do nothing if there are no setuid restrictions for this UID. */
> + if (!check_setuid_policy_hashtable_key(old->uid))
> + return 0;
> + }
>
> - /* Do nothing if there are no setuid restrictions for this UID. */
> - if (!check_setuid_policy_hashtable_key(old->uid))
> + if (flags == LSM_SETGID_RE ||
> + flags == LSM_SETGID_ID ||
> + flags == LSM_SETGID_RES ||
> + flags == LSM_SETGID_FS) {
> + /* Do nothing if there are no setgid restrictions for this UID. */
> + if (!check_setgid_policy_hashtable_key(old->uid))
> return 0;
> + }
>
> switch (flags) {
> case LSM_SETUID_RE:
> @@ -201,6 +277,77 @@ static int safesetid_task_fix_setid(struct cred *new,
> return check_uid_transition(old->fsuid, new->fsuid);
> }
> break;
> + case LSM_SETGID_RE:
> + /*
> + * Users for which setgid restrictions exist can only set the
> + * real GID to the real GID or the effective GID, unless an
> + * explicit whitelist policy allows the transition.
> + */
> + if (!gid_eq(old->gid, new->gid) &&
> + !gid_eq(old->egid, new->gid)) {
> + return check_gid_transition(old->uid, new->gid);
> + }
> + /*
> + * Users for which setgid restrictions exist can only set the
> + * effective GID to the real GID, the effective GID, or the
> + * saved set-GID, unless an explicit whitelist policy allows
> + * the transition.
> + */
> + if (!gid_eq(old->gid, new->egid) &&
> + !gid_eq(old->egid, new->egid) &&
> + !gid_eq(old->sgid, new->egid)) {
> + return check_gid_transition(old->euid, new->egid);
> + }
> + break;
> + case LSM_SETGID_ID:
> + /*
> + * Users for which setgid restrictions exist cannot change the
> + * real GID or saved set-GID unless an explicit whitelist
> + * policy allows the transition.
> + */
> + if (!gid_eq(old->gid, new->gid))
> + return check_gid_transition(old->uid, new->gid);
> + if (!gid_eq(old->sgid, new->sgid))
> + return check_gid_transition(old->suid, new->sgid);
> + break;
> + case LSM_SETGID_RES:
> + /*
> + * Users for which setgid restrictions exist cannot change the
> + * real GID, effective GID, or saved set-GID to anything but
> + * one of: the current real GID, the current effective GID or
> + * the current saved set-user-ID unless an explicit whitelist
> + * policy allows the transition.
> + */
> + if (!gid_eq(new->gid, old->gid) &&
> + !gid_eq(new->gid, old->egid) &&
> + !gid_eq(new->gid, old->sgid)) {
> + return check_gid_transition(old->uid, new->gid);
> + }
> + if (!gid_eq(new->egid, old->gid) &&
> + !gid_eq(new->egid, old->egid) &&
> + !gid_eq(new->egid, old->sgid)) {
> + return check_gid_transition(old->euid, new->egid);
> + }
> + if (!gid_eq(new->sgid, old->gid) &&
> + !gid_eq(new->sgid, old->egid) &&
> + !gid_eq(new->sgid, old->sgid)) {
> + return check_gid_transition(old->suid, new->sgid);
> + }
> + break;
> + case LSM_SETGID_FS:
> + /*
> + * Users for which setgid restrictions exist cannot change the
> + * filesystem GID to anything but one of: the current real GID,
> + * the current effective GID or the current saved set-GID
> + * unless an explicit whitelist policy allows the transition.
> + */
> + if (!gid_eq(new->fsgid, old->gid) &&
> + !gid_eq(new->fsgid, old->egid) &&
> + !gid_eq(new->fsgid, old->sgid) &&
> + !gid_eq(new->fsgid, old->fsgid)) {
> + return check_gid_transition(old->fsuid, new->fsgid);
> + }
> + break;
> default:
> pr_warn("Unknown setid state %d\n", flags);
> force_sig(SIGKILL, current);
> @@ -209,49 +356,96 @@ static int safesetid_task_fix_setid(struct cred *new,
> return 0;
> }
>
> -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
> +int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child)
> {
> - struct entry *new;
> + struct id_entry *new;
>
> /* Return if entry already exists */
> if (check_setuid_policy_hashtable_key_value(parent, child))
> return 0;
>
> - new = kzalloc(sizeof(struct entry), GFP_KERNEL);
> + new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
> + if (!new)
> + return -ENOMEM;
> + new->parent_kuid = __kuid_val(parent);
> + new->child_kid = __kuid_val(child);
> + spin_lock(&safesetid_whitelist_uid_hashtable_spinlock);
> + /* Return if the entry got added since we checked above */
> + if (check_setuid_policy_hashtable_key_value(parent, child)) {
> + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> + kfree(new);
> + return 0;
> + }
> + hash_add_rcu(safesetid_whitelist_uid_hashtable,
> + &new->next,
> + __kuid_val(parent));
> + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> + return 0;
> +}
> +
> +int add_safesetid_whitelist_gid_entry(kuid_t parent, kgid_t child)
> +{
> + struct id_entry *new;
> +
> + /* Return if entry already exists */
> + if (check_setgid_policy_hashtable_key_value(parent, child))
> + return 0;
> +
> + new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
> if (!new)
> return -ENOMEM;
> new->parent_kuid = __kuid_val(parent);
> - new->child_kuid = __kuid_val(child);
> - spin_lock(&safesetid_whitelist_hashtable_spinlock);
> - hash_add_rcu(safesetid_whitelist_hashtable,
> + new->child_kid = __kgid_val(child);
> + spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
> + /* Return if the entry got added since we checked above */
> + if (check_setgid_policy_hashtable_key_value(parent, child)) {
> + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> + kfree(new);
> + return 0;
> + }
> + hash_add_rcu(safesetid_whitelist_gid_hashtable,
> &new->next,
> __kuid_val(parent));
> - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> return 0;
> }
>
> void flush_safesetid_whitelist_entries(void)
> {
> - struct entry *entry;
> + struct id_entry *id_entry;
> struct hlist_node *hlist_node;
> unsigned int bkt_loop_cursor;
> - HLIST_HEAD(free_list);
> + HLIST_HEAD(uid_free_list);
> + HLIST_HEAD(gid_free_list);
>
> /*
> * Could probably use hash_for_each_rcu here instead, but this should
> * be fine as well.
> */
> - spin_lock(&safesetid_whitelist_hashtable_spinlock);
> - hash_for_each_safe(safesetid_whitelist_hashtable, bkt_loop_cursor,
> - hlist_node, entry, next) {
> - hash_del_rcu(&entry->next);
> - hlist_add_head(&entry->dlist, &free_list);
> + spin_lock(&safesetid_whitelist_uid_hashtable_spinlock);
> + hash_for_each_safe(safesetid_whitelist_uid_hashtable, bkt_loop_cursor,
> + hlist_node, id_entry, next) {
> + hash_del_rcu(&id_entry->next);
> + hlist_add_head(&id_entry->dlist, &uid_free_list);
> + }
> + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> + synchronize_rcu();
> + hlist_for_each_entry_safe(id_entry, hlist_node, &uid_free_list, dlist) {
> + hlist_del(&id_entry->dlist);
> + kfree(id_entry);
> + }
> +
> + spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
> + hash_for_each_safe(safesetid_whitelist_gid_hashtable, bkt_loop_cursor,
> + hlist_node, id_entry, next) {
> + hash_del_rcu(&id_entry->next);
> + hlist_add_head(&id_entry->dlist, &gid_free_list);
> }
> - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> synchronize_rcu();
> - hlist_for_each_entry_safe(entry, hlist_node, &free_list, dlist) {
> - hlist_del(&entry->dlist);
> - kfree(entry);
> + hlist_for_each_entry_safe(id_entry, hlist_node, &gid_free_list, dlist) {
> + hlist_del(&id_entry->dlist);
> + kfree(id_entry);
> }
> }
>
> diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> index c1ea3c265fcf..e9ae192caff2 100644
> --- a/security/safesetid/lsm.h
> +++ b/security/safesetid/lsm.h
> @@ -21,13 +21,16 @@ extern int safesetid_initialized;
>
> /* Function type. */
> enum safesetid_whitelist_file_write_type {
> - SAFESETID_WHITELIST_ADD, /* Add whitelist policy. */
> + SAFESETID_WHITELIST_ADD_UID, /* Add UID whitelist policy. */
> + SAFESETID_WHITELIST_ADD_GID, /* Add GID whitelist policy. */
> SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
> };
>
> -/* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
> -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
> -
> +/* Add entry to safesetid whitelist to allow 'parent' to setuid to 'child'. */
> +int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child);
> +/* Add entry to safesetid whitelist to allow 'parent' to setgid to 'child'. */
> +int add_safesetid_whitelist_gid_entry(kgid_t parent, kgid_t child);
> +/* Flush all UID/GID whitelist policies. */
> void flush_safesetid_whitelist_entries(void);
>
> #endif /* _SAFESETID_H */
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 2c6c829be044..c4c25ba7275f 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -26,20 +26,19 @@ struct safesetid_file_entry {
>
> static struct safesetid_file_entry safesetid_files[] = {
> {.name = "add_whitelist_policy",
> - .type = SAFESETID_WHITELIST_ADD},
> + .type = SAFESETID_WHITELIST_ADD_UID},
> + {.name = "add_whitelist_uid_policy",
> + .type = SAFESETID_WHITELIST_ADD_UID},
> + {.name = "add_whitelist_gid_policy",
> + .type = SAFESETID_WHITELIST_ADD_GID},
> {.name = "flush_whitelist_policies",
> .type = SAFESETID_WHITELIST_FLUSH},
> };
>
> -/*
> - * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> - * variables pointed to by 'parent' and 'child' will get updated but this
> - * function will return an error.
> - */
> -static int parse_safesetid_whitelist_policy(const char __user *buf,
> +static int parse_userbuf_to_longs(const char __user *buf,
> size_t len,
> - kuid_t *parent,
> - kuid_t *child)
> + long *parent,
> + long *child)
> {
> char *kern_buf;
> char *parent_buf;
> @@ -47,8 +46,6 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> const char separator[] = ":";
> int ret;
> size_t first_substring_length;
> - long parsed_parent;
> - long parsed_child;
>
> /* Duplicate string from user memory and NULL-terminate */
> kern_buf = memdup_user_nul(buf, len);
> @@ -71,27 +68,15 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> goto free_kern;
> }
>
> - ret = kstrtol(parent_buf, 0, &parsed_parent);
> + ret = kstrtol(parent_buf, 0, parent);
> if (ret)
> goto free_both;
>
> child_buf = kern_buf + first_substring_length + 1;
> - ret = kstrtol(child_buf, 0, &parsed_child);
> + ret = kstrtol(child_buf, 0, child);
> if (ret)
> goto free_both;
>
> - *parent = make_kuid(current_user_ns(), parsed_parent);
> - if (!uid_valid(*parent)) {
> - ret = -EINVAL;
> - goto free_both;
> - }
> -
> - *child = make_kuid(current_user_ns(), parsed_child);
> - if (!uid_valid(*child)) {
> - ret = -EINVAL;
> - goto free_both;
> - }
> -
> free_both:
> kfree(parent_buf);
> free_kern:
> @@ -99,6 +84,52 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> return ret;
> }
>
> +static int parse_safesetid_whitelist_uid_policy(const char __user *buf,
> + size_t len,
> + kuid_t *parent_uid,
> + kuid_t *child_uid)
> +{
> + int ret;
> + long parent, child;
> +
> + ret = parse_userbuf_to_longs(buf, len, &parent, &child);
> + if (ret)
> + return ret;
> +
> + *parent_uid = make_kuid(current_user_ns(), parent);
> + if (!uid_valid(*parent_uid))
> + return -EINVAL;
> +
> + *child_uid = make_kuid(current_user_ns(), child);
> + if (!uid_valid(*child_uid))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int parse_safesetid_whitelist_gid_policy(const char __user *buf,
> + size_t len,
> + kgid_t *parent_gid,
> + kgid_t *child_gid)
> +{
> + int ret;
> + long parent, child;
> +
> + ret = parse_userbuf_to_longs(buf, len, &parent, &child);
> + if (ret)
> + return ret;
> +
> + *parent_gid = make_kgid(current_user_ns(), parent);
> + if (!gid_valid(*parent_gid))
> + return -EINVAL;
> +
> + *child_gid = make_kgid(current_user_ns(), child);
> + if (!gid_valid(*child_gid))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static ssize_t safesetid_file_write(struct file *file,
> const char __user *buf,
> size_t len,
> @@ -106,8 +137,10 @@ static ssize_t safesetid_file_write(struct file *file,
> {
> struct safesetid_file_entry *file_entry =
> file->f_inode->i_private;
> - kuid_t parent;
> - kuid_t child;
> + kuid_t uid_parent;
> + kuid_t uid_child;
> + kgid_t gid_parent;
> + kgid_t gid_child;
> int ret;
>
> if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
> @@ -120,13 +153,23 @@ static ssize_t safesetid_file_write(struct file *file,
> case SAFESETID_WHITELIST_FLUSH:
> flush_safesetid_whitelist_entries();
> break;
> - case SAFESETID_WHITELIST_ADD:
> - ret = parse_safesetid_whitelist_policy(buf, len, &parent,
> - &child);
> + case SAFESETID_WHITELIST_ADD_UID:
> + ret = parse_safesetid_whitelist_uid_policy(buf, len, &uid_parent,
> + &uid_child);
> + if (ret)
> + return ret;
> +
> + ret = add_safesetid_whitelist_uid_entry(uid_parent, uid_child);
> + if (ret)
> + return ret;
> + break;
> + case SAFESETID_WHITELIST_ADD_GID:
> + ret = parse_safesetid_whitelist_gid_policy(buf, len, &gid_parent,
> + &gid_child);
> if (ret)
> return ret;
>
> - ret = add_safesetid_whitelist_entry(parent, child);
> + ret = add_safesetid_whitelist_gid_entry(gid_parent, gid_child);
> if (ret)
> return ret;
> break;
> --
> 2.21.0.352.gf09ad66450-goog
>
^ permalink raw reply
* Re: [PATCH v4 2/2] LSM: SafeSetID: gate setgid transitions
From: James Morris @ 2019-03-11 18:38 UTC (permalink / raw)
To: Micah Morton
Cc: Serge E. Hallyn, Kees Cook, Casey Schaufler, Stephen Smalley,
linux-security-module
In-Reply-To: <CAJ-EccP0ZvQFnNQ0UdCrsWdJYSfPgwWW=3bV11zSgK4QXtv3_w@mail.gmail.com>
On Mon, 11 Mar 2019, Micah Morton wrote:
> Is this ready to be merged if there are no further comments?
Nothing gets merged to the subsystem trees until the main merge window is
closed (i.e. v5.1-rc1).
I'd expect / hope for more review of this to happen, too.
>
> On Tue, Mar 5, 2019 at 7:52 AM <mortonm@chromium.org> wrote:
> >
> > From: Micah Morton <mortonm@chromium.org>
> >
> > The SafeSetID LSM already gates setuid transitions for UIDs on the
> > system whose use of CAP_SETUID has been 'restricted'. This patch
> > implements the analogous functionality for setgid transitions, in order
> > to restrict the use of CAP_SETGID for certain UIDs on the system. One
> > notable consequence of this addition is that a process running under a
> > restricted UID (i.e. one that is only allowed to setgid to certain
> > approved GIDs) will not be allowed to call the setgroups() syscall to
> > set its supplementary group IDs. For now, we leave such support for
> > restricted setgroups() to future work, as it would require hooking the
> > logic in setgroups() and verifying that the array of GIDs passed in from
> > userspace only consists of approved GIDs.
> >
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
> > ---
> > Changes since the last patch: Minor updates since patch 1/2 combined the
> > 'task_fix_setuid' and 'task_fix_setgid' hooks into one hook called
> > 'task_fix_setid'.
> >
> > Documentation/admin-guide/LSM/SafeSetID.rst | 9 +-
> > security/safesetid/lsm.c | 272 +++++++++++++++++---
> > security/safesetid/lsm.h | 11 +-
> > security/safesetid/securityfs.c | 105 +++++---
> > 4 files changed, 320 insertions(+), 77 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/LSM/SafeSetID.rst b/Documentation/admin-guide/LSM/SafeSetID.rst
> > index 670a6544fd39..33307a8e9555 100644
> > --- a/Documentation/admin-guide/LSM/SafeSetID.rst
> > +++ b/Documentation/admin-guide/LSM/SafeSetID.rst
> > @@ -98,10 +98,13 @@ Directions for use
> > ==================
> > This LSM hooks the setid syscalls to make sure transitions are allowed if an
> > applicable restriction policy is in place. Policies are configured through
> > -securityfs by writing to the safesetid/add_whitelist_policy and
> > +securityfs by writing to the safesetid/add_whitelist_{uid/gid}_policy* and
> > safesetid/flush_whitelist_policies files at the location where securityfs is
> > -mounted. The format for adding a policy is '<UID>:<UID>', using literal
> > +mounted. The format for adding a policy is '<ID>:<ID>', using literal
> > numbers, such as '123:456'. To flush the policies, any write to the file is
> > sufficient. Again, configuring a policy for a UID will prevent that UID from
> > obtaining auxiliary setid privileges, such as allowing a user to set up user
> > -namespace UID mappings.
> > +namespace ID mappings.
> > +
> > +*safesetid/add_whitelist_policy can also be used for UID policies, since it is
> > +an alias for safesetid/add_whitelist_uid_policy
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index 5deffa92f25f..95c6f35b29f3 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -26,27 +26,30 @@ int safesetid_initialized;
> >
> > #define NUM_BITS 8 /* 128 buckets in hash table */
> >
> > -static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
> > +static DEFINE_HASHTABLE(safesetid_whitelist_uid_hashtable, NUM_BITS);
> > +static DEFINE_HASHTABLE(safesetid_whitelist_gid_hashtable, NUM_BITS);
> > +
> > +static DEFINE_SPINLOCK(safesetid_whitelist_uid_hashtable_spinlock);
> > +static DEFINE_SPINLOCK(safesetid_whitelist_gid_hashtable_spinlock);
> >
> > /*
> > * Hash table entry to store safesetid policy signifying that 'parent' user
> > - * can setid to 'child' user.
> > + * can setid to 'child' user. This struct is used in both the uid and gid
> > + * hashtables.
> > */
> > -struct entry {
> > +struct id_entry {
> > struct hlist_node next;
> > struct hlist_node dlist; /* for deletion cleanup */
> > uint64_t parent_kuid;
> > - uint64_t child_kuid;
> > + uint64_t child_kid; /* Represents either a UID or a GID */
> > };
> >
> > -static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
> > -
> > static bool check_setuid_policy_hashtable_key(kuid_t parent)
> > {
> > - struct entry *entry;
> > + struct id_entry *entry;
> >
> > rcu_read_lock();
> > - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> > + hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
> > entry, next, __kuid_val(parent)) {
> > if (entry->parent_kuid == __kuid_val(parent)) {
> > rcu_read_unlock();
> > @@ -61,13 +64,49 @@ static bool check_setuid_policy_hashtable_key(kuid_t parent)
> > static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> > kuid_t child)
> > {
> > - struct entry *entry;
> > + struct id_entry *entry;
> > +
> > + rcu_read_lock();
> > + hash_for_each_possible_rcu(safesetid_whitelist_uid_hashtable,
> > + entry, next, __kuid_val(parent)) {
> > + if (entry->parent_kuid == __kuid_val(parent) &&
> > + entry->child_kid == __kuid_val(child)) {
> > + rcu_read_unlock();
> > + return true;
> > + }
> > + }
> > + rcu_read_unlock();
> > +
> > + return false;
> > +}
> > +
> > +static bool check_setgid_policy_hashtable_key(kuid_t parent)
> > +{
> > + struct id_entry *entry;
> > +
> > + rcu_read_lock();
> > + hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
> > + entry, next, __kuid_val(parent)) {
> > + if (entry->parent_kuid == __kuid_val(parent)) {
> > + rcu_read_unlock();
> > + return true;
> > + }
> > + }
> > + rcu_read_unlock();
> > +
> > + return false;
> > +}
> > +
> > +static bool check_setgid_policy_hashtable_key_value(kuid_t parent,
> > + kgid_t child)
> > +{
> > + struct id_entry *entry;
> >
> > rcu_read_lock();
> > - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> > + hash_for_each_possible_rcu(safesetid_whitelist_gid_hashtable,
> > entry, next, __kuid_val(parent)) {
> > if (entry->parent_kuid == __kuid_val(parent) &&
> > - entry->child_kuid == __kuid_val(child)) {
> > + entry->child_kid == __kgid_val(child)) {
> > rcu_read_unlock();
> > return true;
> > }
> > @@ -77,6 +116,12 @@ static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> > return false;
> > }
> >
> > +/*
> > + * This hook causes the security_capable check to fail when there are
> > + * restriction policies for a UID and the process is trying to do something
> > + * (other than a setid transition) that is gated by CAP_SETUID/CAP_SETGID
> > + * (e.g. allowing user to set up userns UID/GID mappings).
> > + */
> > static int safesetid_security_capable(const struct cred *cred,
> > struct user_namespace *ns,
> > int cap,
> > @@ -85,17 +130,19 @@ static int safesetid_security_capable(const struct cred *cred,
> > if (cap == CAP_SETUID &&
> > check_setuid_policy_hashtable_key(cred->uid)) {
> > if (!(opts & CAP_OPT_INSETID)) {
> > - /*
> > - * Deny if we're not in a set*uid() syscall to avoid
> > - * giving powers gated by CAP_SETUID that are related
> > - * to functionality other than calling set*uid() (e.g.
> > - * allowing user to set up userns uid mappings).
> > - */
> > pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions",
> > __kuid_val(cred->uid));
> > return -1;
> > }
> > }
> > + if (cap == CAP_SETGID &&
> > + check_setgid_policy_hashtable_key(cred->uid)) {
> > + if (!(opts & CAP_OPT_INSETID)) {
> > + pr_warn("Operation requires CAP_SETGID, which is not available to UID %u for operations besides approved set*gid transitions",
> > + __kuid_val(cred->uid));
> > + return -1;
> > + }
> > + }
> > return 0;
> > }
> >
> > @@ -115,19 +162,48 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
> > return -EACCES;
> > }
> >
> > +static int check_gid_transition(kuid_t parent, kgid_t child)
> > +{
> > + if (check_setgid_policy_hashtable_key_value(parent, child))
> > + return 0;
> > + pr_warn("Denied UID %d setting GID to %d",
> > + __kuid_val(parent),
> > + __kgid_val(child));
> > + /*
> > + * Kill this process to avoid potential security vulnerabilities
> > + * that could arise from a missing whitelist entry preventing a
> > + * privileged process from dropping to a lesser-privileged one.
> > + */
> > + force_sig(SIGKILL, current);
> > + return -EACCES;
> > +}
> > +
> > /*
> > * Check whether there is either an exception for user under old cred struct to
> > - * set*uid to user under new cred struct, or the UID transition is allowed (by
> > - * Linux set*uid rules) even without CAP_SETUID.
> > + * set*id to user under new cred struct, or the ID transition is allowed (by
> > + * Linux set*id rules) even without CAP_SET*ID.
> > */
> > static int safesetid_task_fix_setid(struct cred *new,
> > const struct cred *old,
> > int flags)
> > {
> > + if (flags == LSM_SETUID_RE ||
> > + flags == LSM_SETUID_ID ||
> > + flags == LSM_SETUID_RES ||
> > + flags == LSM_SETUID_FS) {
> > + /* Do nothing if there are no setuid restrictions for this UID. */
> > + if (!check_setuid_policy_hashtable_key(old->uid))
> > + return 0;
> > + }
> >
> > - /* Do nothing if there are no setuid restrictions for this UID. */
> > - if (!check_setuid_policy_hashtable_key(old->uid))
> > + if (flags == LSM_SETGID_RE ||
> > + flags == LSM_SETGID_ID ||
> > + flags == LSM_SETGID_RES ||
> > + flags == LSM_SETGID_FS) {
> > + /* Do nothing if there are no setgid restrictions for this UID. */
> > + if (!check_setgid_policy_hashtable_key(old->uid))
> > return 0;
> > + }
> >
> > switch (flags) {
> > case LSM_SETUID_RE:
> > @@ -201,6 +277,77 @@ static int safesetid_task_fix_setid(struct cred *new,
> > return check_uid_transition(old->fsuid, new->fsuid);
> > }
> > break;
> > + case LSM_SETGID_RE:
> > + /*
> > + * Users for which setgid restrictions exist can only set the
> > + * real GID to the real GID or the effective GID, unless an
> > + * explicit whitelist policy allows the transition.
> > + */
> > + if (!gid_eq(old->gid, new->gid) &&
> > + !gid_eq(old->egid, new->gid)) {
> > + return check_gid_transition(old->uid, new->gid);
> > + }
> > + /*
> > + * Users for which setgid restrictions exist can only set the
> > + * effective GID to the real GID, the effective GID, or the
> > + * saved set-GID, unless an explicit whitelist policy allows
> > + * the transition.
> > + */
> > + if (!gid_eq(old->gid, new->egid) &&
> > + !gid_eq(old->egid, new->egid) &&
> > + !gid_eq(old->sgid, new->egid)) {
> > + return check_gid_transition(old->euid, new->egid);
> > + }
> > + break;
> > + case LSM_SETGID_ID:
> > + /*
> > + * Users for which setgid restrictions exist cannot change the
> > + * real GID or saved set-GID unless an explicit whitelist
> > + * policy allows the transition.
> > + */
> > + if (!gid_eq(old->gid, new->gid))
> > + return check_gid_transition(old->uid, new->gid);
> > + if (!gid_eq(old->sgid, new->sgid))
> > + return check_gid_transition(old->suid, new->sgid);
> > + break;
> > + case LSM_SETGID_RES:
> > + /*
> > + * Users for which setgid restrictions exist cannot change the
> > + * real GID, effective GID, or saved set-GID to anything but
> > + * one of: the current real GID, the current effective GID or
> > + * the current saved set-user-ID unless an explicit whitelist
> > + * policy allows the transition.
> > + */
> > + if (!gid_eq(new->gid, old->gid) &&
> > + !gid_eq(new->gid, old->egid) &&
> > + !gid_eq(new->gid, old->sgid)) {
> > + return check_gid_transition(old->uid, new->gid);
> > + }
> > + if (!gid_eq(new->egid, old->gid) &&
> > + !gid_eq(new->egid, old->egid) &&
> > + !gid_eq(new->egid, old->sgid)) {
> > + return check_gid_transition(old->euid, new->egid);
> > + }
> > + if (!gid_eq(new->sgid, old->gid) &&
> > + !gid_eq(new->sgid, old->egid) &&
> > + !gid_eq(new->sgid, old->sgid)) {
> > + return check_gid_transition(old->suid, new->sgid);
> > + }
> > + break;
> > + case LSM_SETGID_FS:
> > + /*
> > + * Users for which setgid restrictions exist cannot change the
> > + * filesystem GID to anything but one of: the current real GID,
> > + * the current effective GID or the current saved set-GID
> > + * unless an explicit whitelist policy allows the transition.
> > + */
> > + if (!gid_eq(new->fsgid, old->gid) &&
> > + !gid_eq(new->fsgid, old->egid) &&
> > + !gid_eq(new->fsgid, old->sgid) &&
> > + !gid_eq(new->fsgid, old->fsgid)) {
> > + return check_gid_transition(old->fsuid, new->fsgid);
> > + }
> > + break;
> > default:
> > pr_warn("Unknown setid state %d\n", flags);
> > force_sig(SIGKILL, current);
> > @@ -209,49 +356,96 @@ static int safesetid_task_fix_setid(struct cred *new,
> > return 0;
> > }
> >
> > -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
> > +int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child)
> > {
> > - struct entry *new;
> > + struct id_entry *new;
> >
> > /* Return if entry already exists */
> > if (check_setuid_policy_hashtable_key_value(parent, child))
> > return 0;
> >
> > - new = kzalloc(sizeof(struct entry), GFP_KERNEL);
> > + new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
> > + if (!new)
> > + return -ENOMEM;
> > + new->parent_kuid = __kuid_val(parent);
> > + new->child_kid = __kuid_val(child);
> > + spin_lock(&safesetid_whitelist_uid_hashtable_spinlock);
> > + /* Return if the entry got added since we checked above */
> > + if (check_setuid_policy_hashtable_key_value(parent, child)) {
> > + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> > + kfree(new);
> > + return 0;
> > + }
> > + hash_add_rcu(safesetid_whitelist_uid_hashtable,
> > + &new->next,
> > + __kuid_val(parent));
> > + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> > + return 0;
> > +}
> > +
> > +int add_safesetid_whitelist_gid_entry(kuid_t parent, kgid_t child)
> > +{
> > + struct id_entry *new;
> > +
> > + /* Return if entry already exists */
> > + if (check_setgid_policy_hashtable_key_value(parent, child))
> > + return 0;
> > +
> > + new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
> > if (!new)
> > return -ENOMEM;
> > new->parent_kuid = __kuid_val(parent);
> > - new->child_kuid = __kuid_val(child);
> > - spin_lock(&safesetid_whitelist_hashtable_spinlock);
> > - hash_add_rcu(safesetid_whitelist_hashtable,
> > + new->child_kid = __kgid_val(child);
> > + spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
> > + /* Return if the entry got added since we checked above */
> > + if (check_setgid_policy_hashtable_key_value(parent, child)) {
> > + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> > + kfree(new);
> > + return 0;
> > + }
> > + hash_add_rcu(safesetid_whitelist_gid_hashtable,
> > &new->next,
> > __kuid_val(parent));
> > - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> > + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> > return 0;
> > }
> >
> > void flush_safesetid_whitelist_entries(void)
> > {
> > - struct entry *entry;
> > + struct id_entry *id_entry;
> > struct hlist_node *hlist_node;
> > unsigned int bkt_loop_cursor;
> > - HLIST_HEAD(free_list);
> > + HLIST_HEAD(uid_free_list);
> > + HLIST_HEAD(gid_free_list);
> >
> > /*
> > * Could probably use hash_for_each_rcu here instead, but this should
> > * be fine as well.
> > */
> > - spin_lock(&safesetid_whitelist_hashtable_spinlock);
> > - hash_for_each_safe(safesetid_whitelist_hashtable, bkt_loop_cursor,
> > - hlist_node, entry, next) {
> > - hash_del_rcu(&entry->next);
> > - hlist_add_head(&entry->dlist, &free_list);
> > + spin_lock(&safesetid_whitelist_uid_hashtable_spinlock);
> > + hash_for_each_safe(safesetid_whitelist_uid_hashtable, bkt_loop_cursor,
> > + hlist_node, id_entry, next) {
> > + hash_del_rcu(&id_entry->next);
> > + hlist_add_head(&id_entry->dlist, &uid_free_list);
> > + }
> > + spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> > + synchronize_rcu();
> > + hlist_for_each_entry_safe(id_entry, hlist_node, &uid_free_list, dlist) {
> > + hlist_del(&id_entry->dlist);
> > + kfree(id_entry);
> > + }
> > +
> > + spin_lock(&safesetid_whitelist_gid_hashtable_spinlock);
> > + hash_for_each_safe(safesetid_whitelist_gid_hashtable, bkt_loop_cursor,
> > + hlist_node, id_entry, next) {
> > + hash_del_rcu(&id_entry->next);
> > + hlist_add_head(&id_entry->dlist, &gid_free_list);
> > }
> > - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> > + spin_unlock(&safesetid_whitelist_gid_hashtable_spinlock);
> > synchronize_rcu();
> > - hlist_for_each_entry_safe(entry, hlist_node, &free_list, dlist) {
> > - hlist_del(&entry->dlist);
> > - kfree(entry);
> > + hlist_for_each_entry_safe(id_entry, hlist_node, &gid_free_list, dlist) {
> > + hlist_del(&id_entry->dlist);
> > + kfree(id_entry);
> > }
> > }
> >
> > diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> > index c1ea3c265fcf..e9ae192caff2 100644
> > --- a/security/safesetid/lsm.h
> > +++ b/security/safesetid/lsm.h
> > @@ -21,13 +21,16 @@ extern int safesetid_initialized;
> >
> > /* Function type. */
> > enum safesetid_whitelist_file_write_type {
> > - SAFESETID_WHITELIST_ADD, /* Add whitelist policy. */
> > + SAFESETID_WHITELIST_ADD_UID, /* Add UID whitelist policy. */
> > + SAFESETID_WHITELIST_ADD_GID, /* Add GID whitelist policy. */
> > SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
> > };
> >
> > -/* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
> > -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
> > -
> > +/* Add entry to safesetid whitelist to allow 'parent' to setuid to 'child'. */
> > +int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child);
> > +/* Add entry to safesetid whitelist to allow 'parent' to setgid to 'child'. */
> > +int add_safesetid_whitelist_gid_entry(kgid_t parent, kgid_t child);
> > +/* Flush all UID/GID whitelist policies. */
> > void flush_safesetid_whitelist_entries(void);
> >
> > #endif /* _SAFESETID_H */
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 2c6c829be044..c4c25ba7275f 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -26,20 +26,19 @@ struct safesetid_file_entry {
> >
> > static struct safesetid_file_entry safesetid_files[] = {
> > {.name = "add_whitelist_policy",
> > - .type = SAFESETID_WHITELIST_ADD},
> > + .type = SAFESETID_WHITELIST_ADD_UID},
> > + {.name = "add_whitelist_uid_policy",
> > + .type = SAFESETID_WHITELIST_ADD_UID},
> > + {.name = "add_whitelist_gid_policy",
> > + .type = SAFESETID_WHITELIST_ADD_GID},
> > {.name = "flush_whitelist_policies",
> > .type = SAFESETID_WHITELIST_FLUSH},
> > };
> >
> > -/*
> > - * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> > - * variables pointed to by 'parent' and 'child' will get updated but this
> > - * function will return an error.
> > - */
> > -static int parse_safesetid_whitelist_policy(const char __user *buf,
> > +static int parse_userbuf_to_longs(const char __user *buf,
> > size_t len,
> > - kuid_t *parent,
> > - kuid_t *child)
> > + long *parent,
> > + long *child)
> > {
> > char *kern_buf;
> > char *parent_buf;
> > @@ -47,8 +46,6 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> > const char separator[] = ":";
> > int ret;
> > size_t first_substring_length;
> > - long parsed_parent;
> > - long parsed_child;
> >
> > /* Duplicate string from user memory and NULL-terminate */
> > kern_buf = memdup_user_nul(buf, len);
> > @@ -71,27 +68,15 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> > goto free_kern;
> > }
> >
> > - ret = kstrtol(parent_buf, 0, &parsed_parent);
> > + ret = kstrtol(parent_buf, 0, parent);
> > if (ret)
> > goto free_both;
> >
> > child_buf = kern_buf + first_substring_length + 1;
> > - ret = kstrtol(child_buf, 0, &parsed_child);
> > + ret = kstrtol(child_buf, 0, child);
> > if (ret)
> > goto free_both;
> >
> > - *parent = make_kuid(current_user_ns(), parsed_parent);
> > - if (!uid_valid(*parent)) {
> > - ret = -EINVAL;
> > - goto free_both;
> > - }
> > -
> > - *child = make_kuid(current_user_ns(), parsed_child);
> > - if (!uid_valid(*child)) {
> > - ret = -EINVAL;
> > - goto free_both;
> > - }
> > -
> > free_both:
> > kfree(parent_buf);
> > free_kern:
> > @@ -99,6 +84,52 @@ static int parse_safesetid_whitelist_policy(const char __user *buf,
> > return ret;
> > }
> >
> > +static int parse_safesetid_whitelist_uid_policy(const char __user *buf,
> > + size_t len,
> > + kuid_t *parent_uid,
> > + kuid_t *child_uid)
> > +{
> > + int ret;
> > + long parent, child;
> > +
> > + ret = parse_userbuf_to_longs(buf, len, &parent, &child);
> > + if (ret)
> > + return ret;
> > +
> > + *parent_uid = make_kuid(current_user_ns(), parent);
> > + if (!uid_valid(*parent_uid))
> > + return -EINVAL;
> > +
> > + *child_uid = make_kuid(current_user_ns(), child);
> > + if (!uid_valid(*child_uid))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int parse_safesetid_whitelist_gid_policy(const char __user *buf,
> > + size_t len,
> > + kgid_t *parent_gid,
> > + kgid_t *child_gid)
> > +{
> > + int ret;
> > + long parent, child;
> > +
> > + ret = parse_userbuf_to_longs(buf, len, &parent, &child);
> > + if (ret)
> > + return ret;
> > +
> > + *parent_gid = make_kgid(current_user_ns(), parent);
> > + if (!gid_valid(*parent_gid))
> > + return -EINVAL;
> > +
> > + *child_gid = make_kgid(current_user_ns(), child);
> > + if (!gid_valid(*child_gid))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > static ssize_t safesetid_file_write(struct file *file,
> > const char __user *buf,
> > size_t len,
> > @@ -106,8 +137,10 @@ static ssize_t safesetid_file_write(struct file *file,
> > {
> > struct safesetid_file_entry *file_entry =
> > file->f_inode->i_private;
> > - kuid_t parent;
> > - kuid_t child;
> > + kuid_t uid_parent;
> > + kuid_t uid_child;
> > + kgid_t gid_parent;
> > + kgid_t gid_child;
> > int ret;
> >
> > if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
> > @@ -120,13 +153,23 @@ static ssize_t safesetid_file_write(struct file *file,
> > case SAFESETID_WHITELIST_FLUSH:
> > flush_safesetid_whitelist_entries();
> > break;
> > - case SAFESETID_WHITELIST_ADD:
> > - ret = parse_safesetid_whitelist_policy(buf, len, &parent,
> > - &child);
> > + case SAFESETID_WHITELIST_ADD_UID:
> > + ret = parse_safesetid_whitelist_uid_policy(buf, len, &uid_parent,
> > + &uid_child);
> > + if (ret)
> > + return ret;
> > +
> > + ret = add_safesetid_whitelist_uid_entry(uid_parent, uid_child);
> > + if (ret)
> > + return ret;
> > + break;
> > + case SAFESETID_WHITELIST_ADD_GID:
> > + ret = parse_safesetid_whitelist_gid_policy(buf, len, &gid_parent,
> > + &gid_child);
> > if (ret)
> > return ret;
> >
> > - ret = add_safesetid_whitelist_entry(parent, child);
> > + ret = add_safesetid_whitelist_gid_entry(gid_parent, gid_child);
> > if (ret)
> > return ret;
> > break;
> > --
> > 2.21.0.352.gf09ad66450-goog
> >
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
From: Matthew Garrett @ 2019-03-11 19:20 UTC (permalink / raw)
To: Mimi Zohar
Cc: Justin Forbes, linux-integrity, LSM List, linux-efi,
Linux Kernel Mailing List, David Howells, Seth Forshee, kexec,
Nayna Jain
In-Reply-To: <1552323292.6100.45.camel@linux.ibm.com>
On Mon, Mar 11, 2019 at 9:55 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Fri, 2019-03-08 at 09:51 -0800, Matthew Garrett wrote:
> > Hm. And this only happens on certain firmware versions? If something's
> > stepping on boot_params then we have bigger problems.
>
> I was seeing this problem before and after updating the system
> firmware on my laptop last summer. If updating the firmware had
> resolved the problem, I wouldn't have included this patch.
Ah, sorry, when you said that you saw this with older firmware I
thought that meant that newer firmware fixed it. What machine are you
testing on?
^ permalink raw reply
* Re: [PATCH 1/1] smack: removal of global rule list
From: Casey Schaufler @ 2019-03-11 21:05 UTC (permalink / raw)
To: Vishal Goel, linux-security-module, linux-kernel; +Cc: pankaj.m, a.sahrawat
In-Reply-To: <1551957924-25113-1-git-send-email-vishal.goel@samsung.com>
On 3/7/2019 3:25 AM, Vishal Goel wrote:
> In this patch, global rule list has been removed. Now all
> smack rules will be read using "smack_known_list". This list contains
> all the smack labels and internally each smack label structure
> maintains the list of smack rules corresponding to that smack label.
> So there is no need to maintain extra list.
>
> 1) Small Memory Optimization
> For eg. if there are 20000 rules, then it will save 625KB(20000*32),
> which is critical for small embedded systems.
> 2) Reducing the time taken in writing rules on load/load2 interface
> 3) Since global rule list is just used to read the rules, so there
> will be no performance impact on system
>
> Signed-off-by: Vishal Goel <vishal.goel@samsung.com>
> Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
Looks fine. I will take it for 5.2.
> ---
> security/smack/smackfs.c | 53 ++++++++++++++----------------------------------
> 1 file changed, 15 insertions(+), 38 deletions(-)
>
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index f6482e5..2a8a1f5 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -67,7 +67,6 @@ enum smk_inos {
> /*
> * List locks
> */
> -static DEFINE_MUTEX(smack_master_list_lock);
> static DEFINE_MUTEX(smack_cipso_lock);
> static DEFINE_MUTEX(smack_ambient_lock);
> static DEFINE_MUTEX(smk_net4addr_lock);
> @@ -134,15 +133,7 @@ enum smk_inos {
>
> /*
> * Rule lists are maintained for each label.
> - * This master list is just for reading /smack/load and /smack/load2.
> */
> -struct smack_master_list {
> - struct list_head list;
> - struct smack_rule *smk_rule;
> -};
> -
> -static LIST_HEAD(smack_rule_list);
> -
> struct smack_parsed_rule {
> struct smack_known *smk_subject;
> struct smack_known *smk_object;
> @@ -211,7 +202,6 @@ static void smk_netlabel_audit_set(struct netlbl_audit *nap)
> * @srp: the rule to add or replace
> * @rule_list: the list of rules
> * @rule_lock: the rule list lock
> - * @global: if non-zero, indicates a global rule
> *
> * Looks through the current subject/object/access list for
> * the subject/object pair and replaces the access that was
> @@ -223,10 +213,9 @@ static void smk_netlabel_audit_set(struct netlbl_audit *nap)
> */
> static int smk_set_access(struct smack_parsed_rule *srp,
> struct list_head *rule_list,
> - struct mutex *rule_lock, int global)
> + struct mutex *rule_lock)
> {
> struct smack_rule *sp;
> - struct smack_master_list *smlp;
> int found = 0;
> int rc = 0;
>
> @@ -258,22 +247,6 @@ static int smk_set_access(struct smack_parsed_rule *srp,
> sp->smk_access = srp->smk_access1 & ~srp->smk_access2;
>
> list_add_rcu(&sp->list, rule_list);
> - /*
> - * If this is a global as opposed to self and a new rule
> - * it needs to get added for reporting.
> - */
> - if (global) {
> - mutex_unlock(rule_lock);
> - smlp = kzalloc(sizeof(*smlp), GFP_KERNEL);
> - if (smlp != NULL) {
> - smlp->smk_rule = sp;
> - mutex_lock(&smack_master_list_lock);
> - list_add_rcu(&smlp->list, &smack_rule_list);
> - mutex_unlock(&smack_master_list_lock);
> - } else
> - rc = -ENOMEM;
> - return rc;
> - }
> }
>
> out:
> @@ -540,9 +513,9 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>
> if (rule_list == NULL)
> rc = smk_set_access(&rule, &rule.smk_subject->smk_rules,
> - &rule.smk_subject->smk_rules_lock, 1);
> + &rule.smk_subject->smk_rules_lock);
> else
> - rc = smk_set_access(&rule, rule_list, rule_lock, 0);
> + rc = smk_set_access(&rule, rule_list, rule_lock);
>
> if (rc)
> goto out;
> @@ -636,21 +609,23 @@ static void smk_rule_show(struct seq_file *s, struct smack_rule *srp, int max)
>
> static void *load2_seq_start(struct seq_file *s, loff_t *pos)
> {
> - return smk_seq_start(s, pos, &smack_rule_list);
> + return smk_seq_start(s, pos, &smack_known_list);
> }
>
> static void *load2_seq_next(struct seq_file *s, void *v, loff_t *pos)
> {
> - return smk_seq_next(s, v, pos, &smack_rule_list);
> + return smk_seq_next(s, v, pos, &smack_known_list);
> }
>
> static int load_seq_show(struct seq_file *s, void *v)
> {
> struct list_head *list = v;
> - struct smack_master_list *smlp =
> - list_entry_rcu(list, struct smack_master_list, list);
> + struct smack_rule *srp;
> + struct smack_known *skp =
> + list_entry_rcu(list, struct smack_known, list);
>
> - smk_rule_show(s, smlp->smk_rule, SMK_LABELLEN);
> + list_for_each_entry_rcu(srp, &skp->smk_rules, list)
> + smk_rule_show(s, srp, SMK_LABELLEN);
>
> return 0;
> }
> @@ -2352,10 +2327,12 @@ static ssize_t smk_write_access(struct file *file, const char __user *buf,
> static int load2_seq_show(struct seq_file *s, void *v)
> {
> struct list_head *list = v;
> - struct smack_master_list *smlp =
> - list_entry_rcu(list, struct smack_master_list, list);
> + struct smack_rule *srp;
> + struct smack_known *skp =
> + list_entry_rcu(list, struct smack_known, list);
>
> - smk_rule_show(s, smlp->smk_rule, SMK_LONGLABEL);
> + list_for_each_entry_rcu(srp, &skp->smk_rules, list)
> + smk_rule_show(s, srp, SMK_LONGLABEL);
>
> return 0;
> }
^ permalink raw reply
* Re: [PULL REQUEST] Kernel lockdown patches for 5.2
From: Matthew Garrett @ 2019-03-12 0:42 UTC (permalink / raw)
To: Mimi Zohar
Cc: James Morris, LSM List, Linux Kernel Mailing List, David Howells
In-Reply-To: <CACdnJuuib7kYEw1o0Bh1p=X+MmJH+pqupECWnWTJ2e2jE2XRkg@mail.gmail.com>
On Wed, Mar 6, 2019 at 8:24 PM Matthew Garrett <mjg59@google.com> wrote:
>
> On Wed, Mar 6, 2019 at 7:56 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > The kexec and kernel modules patches in this patch set continues to
> > ignore IMA. This patch set should up front either provide an
> > alternative solution to coordinate the different signature
> > verification methods or rely on the architecture specific policy for
> > that coordination.
>
> Hi Mimi,
>
> I'm working on a patch for this at the moment which can then be added
> to either patchset. Is there a tree that contains the proposed Power
> architecture policy? I want to make sure I don't accidentally end up
> depending on anything x86.
I've been digging into this some more, and want to ensure that I get
the appropriate semantics. Are we happy with the x86 solution for
module signing (ie, if the arch policy is enabled and the kernel
supports module signatures, use module signatures rather than IMA
signatures)? If so, that just leaves kexec. For platforms that support
PE signing for kernels (x86 and arm), are we ok punting to that? If so
then to maintain the semantics we have for lockdown in general (ie, no
way for a user to modify ring 0 code) then I think that would mean
allowing kexec_file() only when the following criteria are met:
1) IMA is appraising kexec with digital signatures, either ima digital
signatures or ima hashes with associated EVM digital signatures
2) CONFIG_INTEGRITY_TRUSTED_KEYRING is set in order to prevent an
attacker being able to add a key to the keyring
Does this sound reasonable? Are there any further criteria that are
required for this?
^ permalink raw reply
* Re: [PULL REQUEST] Kernel lockdown patches for 5.2
From: Mimi Zohar @ 2019-03-12 1:52 UTC (permalink / raw)
To: Matthew Garrett
Cc: James Morris, LSM List, Linux Kernel Mailing List, David Howells
In-Reply-To: <CACdnJuszfo4gTUD88p+8jeHU5e3hSdp+GEr8_=F1AMThPb7yWg@mail.gmail.com>
On Mon, 2019-03-11 at 17:42 -0700, Matthew Garrett wrote:
> On Wed, Mar 6, 2019 at 8:24 PM Matthew Garrett <mjg59@google.com> wrote:
> >
> > On Wed, Mar 6, 2019 at 7:56 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > The kexec and kernel modules patches in this patch set continues to
> > > ignore IMA. This patch set should up front either provide an
> > > alternative solution to coordinate the different signature
> > > verification methods or rely on the architecture specific policy for
> > > that coordination.
> >
> > Hi Mimi,
> >
> > I'm working on a patch for this at the moment which can then be added
> > to either patchset. Is there a tree that contains the proposed Power
> > architecture policy? I want to make sure I don't accidentally end up
> > depending on anything x86.
>
> I've been digging into this some more, and want to ensure that I get
> the appropriate semantics. Are we happy with the x86 solution for
> module signing (ie, if the arch policy is enabled and the kernel
> supports module signatures, use module signatures rather than IMA
> signatures)?
There's a slight nuance you're missing. If the arch policy is enabled
and the kernel supports module signatures, do not add an IMA appraise
rule. A custom policy could require an IMA signature, as well as the
module appended signature.
Saying only use the module signatures, even if the IMA custom policy
contains a kernel module rule, doesn't make sense.
> If so, that just leaves kexec. For platforms that support
> PE signing for kernels (x86 and arm), are we ok punting to that?
Similarly, if the custom policy has a kexec kernel image policy rule,
it shouldn't be ignored.
> If so
> then to maintain the semantics we have for lockdown in general (ie, no
> way for a user to modify ring 0 code) then I think that would mean
> allowing kexec_file() only when the following criteria are met:
>
> 1) IMA is appraising kexec with digital signatures, either ima digital
> signatures or ima hashes with associated EVM digital signatures
The kernel image could be signed with an appended signature as well.
> 2) CONFIG_INTEGRITY_TRUSTED_KEYRING is set in order to prevent an
> attacker being able to add a key to the keyring
Agreed
> Does this sound reasonable? Are there any further criteria that are
> required for this?
With the caveats described above.
Mimi
^ 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