Linux Security Modules development
 help / color / mirror / Atom feed

* Re: Preferred subj= with multiple LSMs
From: Paul Moore @ 2019-07-16 23:13 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Steve Grubb, Richard Guy Briggs, linux-audit@redhat.com,
	Linux Security Module list
In-Reply-To: <27e2c710-efe6-d9cd-d4f9-bc217df5ede3@schaufler-ca.com>

On Tue, Jul 16, 2019 at 6:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> It sounds as if some variant of the Hideous format:
>
>         subj=selinux='a:b:c:d',apparmor='z'
>         subj=selinux/a:b:c:d/apparmor/z
>         subj=(selinux)a:b:c:d/(apparmor)z
>
> would meet Steve's searchability requirements, but with significant
> parsing performance penalties.

I think "hideous format" sums it up nicely.  Whatever we choose here
we are likely going to be stuck with for some time and I'm near to
100% that multiplexing the labels onto a single field is going to be a
disaster.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply

* Re: Preferred subj= with multiple LSMs
From: Paul Moore @ 2019-07-16 23:09 UTC (permalink / raw)
  To: Steve Grubb
  Cc: Casey Schaufler, Richard Guy Briggs, linux-audit@redhat.com,
	Linux Security Module list
In-Reply-To: <2517266.eHZzEmjMsX@x2>

On Tue, Jul 16, 2019 at 5:46 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Tuesday, July 16, 2019 5:25:21 PM EDT Paul Moore wrote:

...

> > Agreed.  While I'm not going to be on a specific Linux release, I do
> > believe that at some point in the future the LSM stacking work is
> > going to land in Linus' tree.  Perhaps you'll never see it Steve, but
> > we need to prepare the code to handle it when it happens.
>
> And I agree with that. I'm saying that if we push it all in subj= then it is
> not a big penalty.

I'm going to disagree on that quite severely.  As I mentioned
previously there isn't an easy or sane way to delimit between the
different LSM labels which means sorting out the multiplexed "subj"
field is going to be a post processing nightmare.

> It saves major breakage. Every single event is required to
> have a subj= field if its a MAC system.

All of the options we've discussed still record the LSM credentials in
the audit record; no one is talking about *not* recording the LSM
credentials.  What we are discussing is *how* they are recorded.

> By changing it to lsm_subj= it changes
> the layout of every single event. And it make more to parse. And searching
> the labels is worse because it has to iterate over a list of *_subj to match
> it. This will hurt performance because it is for every single event.

I can almost guarantee that looking for subj/subj_X is going to be
much easier than safely parsing a multiplexed subj field.  Not to
mention the multiplexed approach is just awful to read compared to
some of the other suggestions.

> > For my own sanity, here is a quick summary of the constraints as I
> > currently see them, please feel free to add/disagree:
> >
> > * We can't have multiple "subj" fields in a single audit record.
> > * The different LSMs all have different label formats and allowed
> > characters.  Further, a given label format may not be unique for a
> > given LSM; for example, Smack could be configured with a subset of
> > SELinux labels.
> > * Steve's audit tools appear to require a "subj" and "obj" fields for
> > LSM information or else they break into tiny little pieces.
>
> It changes all knowledge of where to look for things. And considering
> considering that events could be aggregated from systems of different ages/
> distributions, audit userspace will always have to be backwards compatible.

The subj_X approach is still backwards compatible, the difference is
that old versions of the tools get a "?" for the LSM creds which is a
rather sane way of indicating something is different.  The multiplexed
approach would result in effectively garbage for the LSM creds unless
the higher layers of audit tooling are updated to understand the new
multiplexed format *and* *continuously* *updated* as new LSMs are
stacked because you need to understand each LSMs label format if you
are going to safely parse the multiplexed format.  With the subj_X
approach the higher layer tooling simply needs to look for subj_X
fields when it sees "subj=?", and then it can safely extract/parse
each LSM's label without needing to understand or inspect the labels
themselves.

> > What if we preserved the existing subj/obj fields in the case where
> > there is only one "major" LSM (SELinux, Smack, AppArmor, etc.):
> >
> >   subj=<lsm_label>
> >
> > ... and in the case of multiple major LSMs we set the subj value to
> > "?" and introduce new subj_X fields (as necessary) as discussed above:
> >
> >   subj=? subj_smack=<smack_label> subj_selinux=<selinux_label> ...
> >
> > ... I believe that Steve's old/existing userspace tools would simply
> > report "?"/unknown LSM credentials where new multi-LSM tools could
> > report the multiple different labels.
>
> Common Criteria as well as other standards require subject labels to be
> searchable. So, changing behavior based on how many modules will still cause
> problems with performance because I'll always have to assume it could be
> either way and try both.

Once again, I believe that the subj_X approach is going to be faster
than safely parsing the multiplexed format.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply

* Re: Preferred subj= with multiple LSMs
From: Casey Schaufler @ 2019-07-16 22:18 UTC (permalink / raw)
  To: Steve Grubb, Paul Moore
  Cc: Richard Guy Briggs, linux-audit@redhat.com,
	Linux Security Module list, casey
In-Reply-To: <2517266.eHZzEmjMsX@x2>

On 7/16/2019 2:46 PM, Steve Grubb wrote:
> On Tuesday, July 16, 2019 5:25:21 PM EDT Paul Moore wrote:
>> On Tue, Jul 16, 2019 at 2:41 PM Casey Schaufler <casey@schaufler-ca.com> 
> wrote:
>>> On 7/16/2019 11:06 AM, Steve Grubb wrote:
>>>> On Tuesday, July 16, 2019 1:43:18 PM EDT Paul Moore wrote:
>>>>> On Tue, Jul 16, 2019 at 1:30 PM Casey Schaufler
>>>>> <casey@schaufler-ca.com>
>>>> wrote:
>>>>>> On 7/16/2019 10:12 AM, Paul Moore wrote:
>>>>>>> On Mon, Jul 15, 2019 at 6:56 PM Steve Grubb <sgrubb@redhat.com> 
> wrote:
>>>>>>>> On Monday, July 15, 2019 5:28:56 PM EDT Paul Moore wrote:
>>>>>>>>> On Mon, Jul 15, 2019 at 3:37 PM Casey Schaufler
>>>>>>>>> <casey@schaufler-ca.com>
>>>>>>>> wrote:
>>>>>>>>>> On 7/15/2019 12:04 PM, Richard Guy Briggs wrote:
>>>>>>>>>>> On 2019-07-13 11:08, Steve Grubb wrote:
>>>>>>> ...
>>>>>>>
>>>>>>>>>>> Steve's answer is the obvious one, ideally allocating a seperate
>>>>>>>>>>> range
>>>>>>>>>>> to each LSM with each message type having its own well defined
>>>>>>>>>>> format.
>>>>>>>>>> It doesn't address the issue of success records, or records
>>>>>>>>>> generated outside the security modules.
>>>>>>>>> Yes, exactly.  The individual LSM will presumably will continue to
>>>>>>>>> generate their own audit records as they do today and I would
>>>>>>>>> imagine
>>>>>>>>> that the subject and object fields could remain as they do today
>>>>>>>>> for
>>>>>>>>> the LSM specific records.
>>>>>>>>>
>>>>>>>>> The trick is the other records which are not LSM specific but
>>>>>>>>> still
>>>>>>>>> want to include subject and/or object information.  Unfortunately
>>>>>>>>> we
>>>>>>>>> are stuck with some tough limitations given the current audit
>>>>>>>>> record
>>>>>>>>> format and Steve's audit userspace tools;
>>>>>>>> Not really. We just need to approach the problem thinking about how
>>>>>>>> to
>>>>>>>> make it work based on how things currently work.
>>>>>>> I suppose it is all somewhat "subjective" - bad joke fully intended
>>>>>>> :)
>>>>>>> - with respect to what one considers good/bad/limiting.  My personal
>>>>>>> view is that an ideal solution would allow for multiple independent
>>>>>>> subj/obj labels without having to multiplex on a single subj/obj
>>>>>>> field.  My gut feeling is that this would confuse your tools, yes?
>>>>>>>
>>>>>>>> For example Casey had a list of possible formats. Like this one:
>>>>>>>>
>>>>>>>> Option 3:
>>>>>>>>         lsms=selinux,apparmor subj=x:y:z:s:c subj=a
>>>>>>>>
>>>>>>>> I'd suggest something almost like that. The first field could be a
>>>>>>>> map
>>>>>>>> to
>>>>>>>> decipher the labels. Then we could have a comma separated list of
>>>>>>>> labels.
>>>>>>>>
>>>>>>>> lsms=selinux,apparmor subj=x:y:z:s:c,a
>>>>>>> Some quick comments:
>>>>>>>
>>>>>>> * My usual reminder that new fields for existing audit records must
>>>>>>> be
>>>>>>> added to the end of the record.
>>>>>>>
>>>>>>> * If we are going to multiplex the labels on a single field (more on
>>>>>>> that below) I might suggest using "subj_lsms" instead of "lsms" so
>>>>>>> we
>>>>>>> leave ourself some wiggle room in the future.
>>>>>>>
>>>>>>> * Multiplexing on a single "subj" field is going to be difficult
>>>>>>> because picking the label delimiter is going to be a pain.  For
>>>>>>> example, in the example above a comma is used, which at the very
>>>>>>> least
>>>>>>> is a valid part of a SELinux label and I suspect for Smack as well
>>>>>>> (I'm not sure about the other LSMs).  I suspect the only way to
>>>>>>> parse
>>>>>>> out the component labels would be to have knowledge of the LSMs in
>>>>>>> use, as well as the policies loaded at the time the audit record was
>>>>>>> generated.
>>>>>>>
>>>>>>> This may be a faulty assumption, but assuming your tools will fall
>>>>>>> over if they see multiple "subj" fields, could we do something like
>>>>>>>
>>>>>>> the following (something between option #2 and #3):
>>>>>>>   subj1_lsm=smack subj1=<smack_label> subj2_lsm=selinux
>>>>>>>
>>>>>>> subj2=<selinux_label> ...
>>>>>> If it's not a subj= field why use the indirection?
>>>>>>
>>>>>>         subj_smack=<smack_label> subj_selinux=<selinux_label>
>>>>>>
>>>>>> would be easier.
>>>>> Good point, that looks reasonable to me.
>>>> But doing something like this will totally break all parsers. To be
>>>> honest, I don't know if I'll ever see more than one labeled security
>>>> system running at the same time. And this would be a big penalty to
>>>> pay for the flexibility that someone, somewhere just might possibly do
>>>> this.
>>> While I have never seen multiple-LSM plans from RedHat/IBM I
>>> have seen them from Ubuntu. This isn't hypothetical. I know that
>>> it's a hard problem, which is why we need to get it as right as
>>> possible.
>> Agreed.  While I'm not going to be on a specific Linux release, I do
>> believe that at some point in the future the LSM stacking work is
>> going to land in Linus' tree.  Perhaps you'll never see it Steve, but
>> we need to prepare the code to handle it when it happens.
> And I agree with that. I'm saying that if we push it all in subj= then it is 
> not a big penalty. It saves major breakage. Every single event is required to 
> have a subj= field if its a MAC system. By changing it to lsm_subj= it changes 
> the layout of every single event. And it make more to parse. And searching 
> the labels is worse because it has to iterate over a list of *_subj to match 
> it. This will hurt performance because it is for every single event.
>
>> For my own sanity, here is a quick summary of the constraints as I
>> currently see them, please feel free to add/disagree:
>>
>> * We can't have multiple "subj" fields in a single audit record.
>> * The different LSMs all have different label formats and allowed
>> characters.  Further, a given label format may not be unique for a
>> given LSM; for example, Smack could be configured with a subset of
>> SELinux labels.
>> * Steve's audit tools appear to require a "subj" and "obj" fields for
>> LSM information or else they break into tiny little pieces.
> It changes all knowledge of where to look for things. And considering 
> considering that events could be aggregated from systems of different ages/
> distributions, audit userspace will always have to be backwards compatible.
>  
>> What if we preserved the existing subj/obj fields in the case where
>> there is only one "major" LSM (SELinux, Smack, AppArmor, etc.):
>>
>>   subj=<lsm_label>
>>
>> ... and in the case of multiple major LSMs we set the subj value to
>> "?" and introduce new subj_X fields (as necessary) as discussed above:
>>
>>   subj=? subj_smack=<smack_label> subj_selinux=<selinux_label> ...
>>
>> ... I believe that Steve's old/existing userspace tools would simply
>> report "?"/unknown LSM credentials where new multi-LSM tools could
>> report the multiple different labels. 
> Common Criteria as well as other standards require subject labels to be 
> searchable. So, changing behavior based on how many modules will still cause 
> problems with performance because I'll always have to assume it could be 
> either way and try both.
>
>> While this may not be perfect,
>> it avoids having to multiplex the different labels into a single field
>> (which is a big win IMHO) with the only issue being that multi-LSM
>> solutions will need an updated audit toolset to see the new labels
>> (which seems like a reasonable requirement).
> Why would not multiplexing different labels in the same field be a big win? Its 
> a big loss in my mind. Using the same field preserves backward compatibility, 
> is more compact in bytes, creates performance problems, changes all mapping 
> of what things means, etc. IOW, this makes things much worse.

It sounds as if some variant of the Hideous format:

	subj=selinux='a:b:c:d',apparmor='z'
	subj=selinux/a:b:c:d/apparmor/z
	subj=(selinux)a:b:c:d/(apparmor)z

would meet Steve's searchability requirements, but with significant
parsing performance penalties. 

>
> -Steve
>
>


^ permalink raw reply

* [RFC] acpi: Ignore acpi_rsdp kernel param when the kernel has been locked down
From: Matthew Garrett @ 2019-07-16 22:15 UTC (permalink / raw)
  To: linux-security-module
  Cc: Matthew Garrett, Josh Boyer, David Howells, Kees Cook, Dave Young,
	linux-acpi
In-Reply-To: <937ce9400ed86ad089d743dcca7b5926a7172566>

From: Matthew Garrett <mjg59@google.com> 

How about this? It still results in the early boot environment trusting
the RSDP parameter, but only for SRAT parsing - it'll be ignored
everywhere in the kernel proper if lockdown is enforcing.

This option allows userspace to pass the RSDP address to the kernel, which
makes it possible for a user to modify the workings of hardware. Reject
the option when the kernel is locked down. This requires some reworking
of the existing RSDP command line logic, since the early boot code also
makes use of a command-line passed RSDP when locating the SRAT table
before the lockdown code has been initialised. This is achieved by
separating the command line RSDP path in the early boot code from the
generic RSDP path, and then copying the command line RSDP into boot
params in the kernel proper if lockdown is not enabled. If lockdown is
enabled and an RSDP is provided on the command line, this will only be
used when parsing SRAT (which shouldn't permit kernel code execution)
and will be ignored in the rest of the kernel.

(Modified by Matthew Garrett in order to handle the early boot RSDP
environment)

Signed-off-by: Josh Boyer <jwboyer@redhat.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Matthew Garrett <mjg59@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
cc: Dave Young <dyoung@redhat.com>
cc: linux-acpi@vger.kernel.org
---
 arch/x86/boot/compressed/acpi.c | 19 +++++++++++++------
 arch/x86/include/asm/acpi.h     |  9 +++++++++
 arch/x86/include/asm/x86_init.h |  2 ++
 arch/x86/kernel/acpi/boot.c     |  5 +++++
 arch/x86/kernel/x86_init.c      |  1 +
 drivers/acpi/osl.c              | 14 +++++++++++++-
 include/linux/acpi.h            |  6 ++++++
 7 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 15255f388a85..149795c369f2 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -26,7 +26,7 @@ struct mem_vector immovable_mem[MAX_NUMNODES*2];
  */
 #define MAX_ADDR_LEN 19
 
-static acpi_physical_address get_acpi_rsdp(void)
+static acpi_physical_address get_cmdline_acpi_rsdp(void)
 {
 	acpi_physical_address addr = 0;
 
@@ -278,10 +278,7 @@ acpi_physical_address get_rsdp_addr(void)
 {
 	acpi_physical_address pa;
 
-	pa = get_acpi_rsdp();
-
-	if (!pa)
-		pa = boot_params->acpi_rsdp_addr;
+	pa = boot_params->acpi_rsdp_addr;
 
 	/*
 	 * Try to get EFI data from setup_data. This can happen when we're a
@@ -311,7 +308,17 @@ static unsigned long get_acpi_srat_table(void)
 	char arg[10];
 	u8 *entry;
 
-	rsdp = (struct acpi_table_rsdp *)(long)boot_params->acpi_rsdp_addr;
+	/*
+	 * Check whether we were given an RSDP on the command line. We don't
+	 * stash this in boot params because the kernel itself may have
+	 * different ideas about whether to trust a command-line parameter.
+	 */
+	rsdp = (struct acpi_table_rsdp *)get_cmdline_acpi_rsdp();
+
+	if (!rsdp)
+		rsdp = (struct acpi_table_rsdp *)(long)
+			boot_params->acpi_rsdp_addr;
+
 	if (!rsdp)
 		return 0;
 
diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index aac686e1e005..bc9693c9107e 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -117,6 +117,12 @@ static inline bool acpi_has_cpu_in_madt(void)
 	return !!acpi_lapic;
 }
 
+#define ACPI_HAVE_ARCH_SET_ROOT_POINTER
+static inline void acpi_arch_set_root_pointer(u64 addr)
+{
+	x86_init.acpi.set_root_pointer(addr);
+}
+
 #define ACPI_HAVE_ARCH_GET_ROOT_POINTER
 static inline u64 acpi_arch_get_root_pointer(void)
 {
@@ -125,6 +131,7 @@ static inline u64 acpi_arch_get_root_pointer(void)
 
 void acpi_generic_reduced_hw_init(void);
 
+void x86_default_set_root_pointer(u64 addr);
 u64 x86_default_get_root_pointer(void);
 
 #else /* !CONFIG_ACPI */
@@ -138,6 +145,8 @@ static inline void disable_acpi(void) { }
 
 static inline void acpi_generic_reduced_hw_init(void) { }
 
+static inline void x86_default_set_root_pointer(u64 addr) { }
+
 static inline u64 x86_default_get_root_pointer(void)
 {
 	return 0;
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index b85a7c54c6a1..d584128435cb 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -134,10 +134,12 @@ struct x86_hyper_init {
 
 /**
  * struct x86_init_acpi - x86 ACPI init functions
+ * @set_root_poitner:		set RSDP address
  * @get_root_pointer:		get RSDP address
  * @reduced_hw_early_init:	hardware reduced platform early init
  */
 struct x86_init_acpi {
+	void (*set_root_pointer)(u64 addr);
 	u64 (*get_root_pointer)(void);
 	void (*reduced_hw_early_init)(void);
 };
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 17b33ef604f3..04205ce127a1 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1760,6 +1760,11 @@ void __init arch_reserve_mem_area(acpi_physical_address addr, size_t size)
 	e820__update_table_print();
 }
 
+void x86_default_set_root_pointer(u64 addr)
+{
+	boot_params.acpi_rsdp_addr = addr;
+}
+
 u64 x86_default_get_root_pointer(void)
 {
 	return boot_params.acpi_rsdp_addr;
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 50a2b492fdd6..d0b8f5585a73 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -95,6 +95,7 @@ struct x86_init_ops x86_init __initdata = {
 	},
 
 	.acpi = {
+		.set_root_pointer	= x86_default_set_root_pointer,
 		.get_root_pointer	= x86_default_get_root_pointer,
 		.reduced_hw_early_init	= acpi_generic_reduced_hw_init,
 	},
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 9c0edf2fc0dd..d43df3a3fa8d 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -26,6 +26,7 @@
 #include <linux/list.h>
 #include <linux/jiffies.h>
 #include <linux/semaphore.h>
+#include <linux/security.h>
 
 #include <asm/io.h>
 #include <linux/uaccess.h>
@@ -180,8 +181,19 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
 	acpi_physical_address pa;
 
 #ifdef CONFIG_KEXEC
-	if (acpi_rsdp)
+	/*
+	 * We may have been provided with an RSDP on the command line,
+	 * but if a malicious user has done so they may be pointing us
+	 * at modified ACPI tables that could alter kernel behaviour -
+	 * so, we check the lockdown status before making use of
+	 * it. If we trust it then also stash it in an architecture
+	 * specific location (if appropriate) so it can be carried
+	 * over further kexec()s.
+	 */
+	if (acpi_rsdp && !security_locked_down(LOCKDOWN_ACPI_TABLES)) {
+		acpi_arch_set_root_pointer(acpi_rsdp);
 		return acpi_rsdp;
+	}
 #endif
 	pa = acpi_arch_get_root_pointer();
 	if (pa)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 451e7b544342..e826f7311b2b 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -639,6 +639,12 @@ bool acpi_gtdt_c3stop(int type);
 int acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem, int *timer_count);
 #endif
 
+#ifndef ACPI_HAVE_ARCH_SET_ROOT_POINTER
+static inline void acpi_arch_set_root_pointer(u64 addr)
+{
+}
+#endif
+
 #ifndef ACPI_HAVE_ARCH_GET_ROOT_POINTER
 static inline u64 acpi_arch_get_root_pointer(void)
 {
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply related

* Re: Preferred subj= with multiple LSMs
From: Steve Grubb @ 2019-07-16 21:46 UTC (permalink / raw)
  To: Paul Moore
  Cc: Casey Schaufler, Richard Guy Briggs, linux-audit@redhat.com,
	Linux Security Module list
In-Reply-To: <CAHC9VhTQLihNQ1iGjJB=LAn=C6BQokFsjsRcj8O_O9AjqQ7HBg@mail.gmail.com>

On Tuesday, July 16, 2019 5:25:21 PM EDT Paul Moore wrote:
> On Tue, Jul 16, 2019 at 2:41 PM Casey Schaufler <casey@schaufler-ca.com> 
wrote:
> > On 7/16/2019 11:06 AM, Steve Grubb wrote:
> > > On Tuesday, July 16, 2019 1:43:18 PM EDT Paul Moore wrote:
> > >> On Tue, Jul 16, 2019 at 1:30 PM Casey Schaufler
> > >> <casey@schaufler-ca.com>
> > > 
> > > wrote:
> > >>> On 7/16/2019 10:12 AM, Paul Moore wrote:
> > >>>> On Mon, Jul 15, 2019 at 6:56 PM Steve Grubb <sgrubb@redhat.com> 
wrote:
> > >>>>> On Monday, July 15, 2019 5:28:56 PM EDT Paul Moore wrote:
> > >>>>>> On Mon, Jul 15, 2019 at 3:37 PM Casey Schaufler
> > >>>>>> <casey@schaufler-ca.com>
> > >>>>> 
> > >>>>> wrote:
> > >>>>>>> On 7/15/2019 12:04 PM, Richard Guy Briggs wrote:
> > >>>>>>>> On 2019-07-13 11:08, Steve Grubb wrote:
> > >>>> ...
> > >>>> 
> > >>>>>>>> Steve's answer is the obvious one, ideally allocating a seperate
> > >>>>>>>> range
> > >>>>>>>> to each LSM with each message type having its own well defined
> > >>>>>>>> format.
> > >>>>>>> 
> > >>>>>>> It doesn't address the issue of success records, or records
> > >>>>>>> generated outside the security modules.
> > >>>>>> 
> > >>>>>> Yes, exactly.  The individual LSM will presumably will continue to
> > >>>>>> generate their own audit records as they do today and I would
> > >>>>>> imagine
> > >>>>>> that the subject and object fields could remain as they do today
> > >>>>>> for
> > >>>>>> the LSM specific records.
> > >>>>>> 
> > >>>>>> The trick is the other records which are not LSM specific but
> > >>>>>> still
> > >>>>>> want to include subject and/or object information.  Unfortunately
> > >>>>>> we
> > >>>>>> are stuck with some tough limitations given the current audit
> > >>>>>> record
> > >>>>>> format and Steve's audit userspace tools;
> > >>>>> 
> > >>>>> Not really. We just need to approach the problem thinking about how
> > >>>>> to
> > >>>>> make it work based on how things currently work.
> > >>>> 
> > >>>> I suppose it is all somewhat "subjective" - bad joke fully intended
> > >>>> :)
> > >>>> - with respect to what one considers good/bad/limiting.  My personal
> > >>>> view is that an ideal solution would allow for multiple independent
> > >>>> subj/obj labels without having to multiplex on a single subj/obj
> > >>>> field.  My gut feeling is that this would confuse your tools, yes?
> > >>>> 
> > >>>>> For example Casey had a list of possible formats. Like this one:
> > >>>>> 
> > >>>>> Option 3:
> > >>>>>         lsms=selinux,apparmor subj=x:y:z:s:c subj=a
> > >>>>> 
> > >>>>> I'd suggest something almost like that. The first field could be a
> > >>>>> map
> > >>>>> to
> > >>>>> decipher the labels. Then we could have a comma separated list of
> > >>>>> labels.
> > >>>>> 
> > >>>>> lsms=selinux,apparmor subj=x:y:z:s:c,a
> > >>>> 
> > >>>> Some quick comments:
> > >>>> 
> > >>>> * My usual reminder that new fields for existing audit records must
> > >>>> be
> > >>>> added to the end of the record.
> > >>>> 
> > >>>> * If we are going to multiplex the labels on a single field (more on
> > >>>> that below) I might suggest using "subj_lsms" instead of "lsms" so
> > >>>> we
> > >>>> leave ourself some wiggle room in the future.
> > >>>> 
> > >>>> * Multiplexing on a single "subj" field is going to be difficult
> > >>>> because picking the label delimiter is going to be a pain.  For
> > >>>> example, in the example above a comma is used, which at the very
> > >>>> least
> > >>>> is a valid part of a SELinux label and I suspect for Smack as well
> > >>>> (I'm not sure about the other LSMs).  I suspect the only way to
> > >>>> parse
> > >>>> out the component labels would be to have knowledge of the LSMs in
> > >>>> use, as well as the policies loaded at the time the audit record was
> > >>>> generated.
> > >>>> 
> > >>>> This may be a faulty assumption, but assuming your tools will fall
> > >>>> over if they see multiple "subj" fields, could we do something like
> > >>>> 
> > >>>> the following (something between option #2 and #3):
> > >>>>   subj1_lsm=smack subj1=<smack_label> subj2_lsm=selinux
> > >>>> 
> > >>>> subj2=<selinux_label> ...
> > >>> 
> > >>> If it's not a subj= field why use the indirection?
> > >>> 
> > >>>         subj_smack=<smack_label> subj_selinux=<selinux_label>
> > >>> 
> > >>> would be easier.
> > >> 
> > >> Good point, that looks reasonable to me.
> > > 
> > > But doing something like this will totally break all parsers. To be
> > > honest, I don't know if I'll ever see more than one labeled security
> > > system running at the same time. And this would be a big penalty to
> > > pay for the flexibility that someone, somewhere just might possibly do
> > > this.
> > 
> > While I have never seen multiple-LSM plans from RedHat/IBM I
> > have seen them from Ubuntu. This isn't hypothetical. I know that
> > it's a hard problem, which is why we need to get it as right as
> > possible.
> 
> Agreed.  While I'm not going to be on a specific Linux release, I do
> believe that at some point in the future the LSM stacking work is
> going to land in Linus' tree.  Perhaps you'll never see it Steve, but
> we need to prepare the code to handle it when it happens.

And I agree with that. I'm saying that if we push it all in subj= then it is 
not a big penalty. It saves major breakage. Every single event is required to 
have a subj= field if its a MAC system. By changing it to lsm_subj= it changes 
the layout of every single event. And it make more to parse. And searching 
the labels is worse because it has to iterate over a list of *_subj to match 
it. This will hurt performance because it is for every single event.

> For my own sanity, here is a quick summary of the constraints as I
> currently see them, please feel free to add/disagree:
> 
> * We can't have multiple "subj" fields in a single audit record.
> * The different LSMs all have different label formats and allowed
> characters.  Further, a given label format may not be unique for a
> given LSM; for example, Smack could be configured with a subset of
> SELinux labels.
> * Steve's audit tools appear to require a "subj" and "obj" fields for
> LSM information or else they break into tiny little pieces.

It changes all knowledge of where to look for things. And considering 
considering that events could be aggregated from systems of different ages/
distributions, audit userspace will always have to be backwards compatible.
 
> What if we preserved the existing subj/obj fields in the case where
> there is only one "major" LSM (SELinux, Smack, AppArmor, etc.):
> 
>   subj=<lsm_label>
> 
> ... and in the case of multiple major LSMs we set the subj value to
> "?" and introduce new subj_X fields (as necessary) as discussed above:
> 
>   subj=? subj_smack=<smack_label> subj_selinux=<selinux_label> ...
> 
> ... I believe that Steve's old/existing userspace tools would simply
> report "?"/unknown LSM credentials where new multi-LSM tools could
> report the multiple different labels. 

Common Criteria as well as other standards require subject labels to be 
searchable. So, changing behavior based on how many modules will still cause 
problems with performance because I'll always have to assume it could be 
either way and try both.

> While this may not be perfect,
> it avoids having to multiplex the different labels into a single field
> (which is a big win IMHO) with the only issue being that multi-LSM
> solutions will need an updated audit toolset to see the new labels
> (which seems like a reasonable requirement).

Why would not multiplexing different labels in the same field be a big win? Its 
a big loss in my mind. Using the same field preserves backward compatibility, 
is more compact in bytes, creates performance problems, changes all mapping 
of what things means, etc. IOW, this makes things much worse.

-Steve



^ permalink raw reply

* Re: Preferred subj= with multiple LSMs
From: Paul Moore @ 2019-07-16 21:25 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Steve Grubb, Richard Guy Briggs, linux-audit@redhat.com,
	Linux Security Module list
In-Reply-To: <d1a237d3-4b72-48b0-27d6-fb168354ad31@schaufler-ca.com>

On Tue, Jul 16, 2019 at 2:41 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/16/2019 11:06 AM, Steve Grubb wrote:
> > On Tuesday, July 16, 2019 1:43:18 PM EDT Paul Moore wrote:
> >> On Tue, Jul 16, 2019 at 1:30 PM Casey Schaufler <casey@schaufler-ca.com>
> > wrote:
> >>> On 7/16/2019 10:12 AM, Paul Moore wrote:
> >>>> On Mon, Jul 15, 2019 at 6:56 PM Steve Grubb <sgrubb@redhat.com> wrote:
> >>>>> On Monday, July 15, 2019 5:28:56 PM EDT Paul Moore wrote:
> >>>>>> On Mon, Jul 15, 2019 at 3:37 PM Casey Schaufler
> >>>>>> <casey@schaufler-ca.com>
> >>>>> wrote:
> >>>>>>> On 7/15/2019 12:04 PM, Richard Guy Briggs wrote:
> >>>>>>>> On 2019-07-13 11:08, Steve Grubb wrote:
> >>>> ...
> >>>>
> >>>>>>>> Steve's answer is the obvious one, ideally allocating a seperate
> >>>>>>>> range
> >>>>>>>> to each LSM with each message type having its own well defined
> >>>>>>>> format.
> >>>>>>> It doesn't address the issue of success records, or records
> >>>>>>> generated outside the security modules.
> >>>>>> Yes, exactly.  The individual LSM will presumably will continue to
> >>>>>> generate their own audit records as they do today and I would imagine
> >>>>>> that the subject and object fields could remain as they do today for
> >>>>>> the LSM specific records.
> >>>>>>
> >>>>>> The trick is the other records which are not LSM specific but still
> >>>>>> want to include subject and/or object information.  Unfortunately we
> >>>>>> are stuck with some tough limitations given the current audit record
> >>>>>> format and Steve's audit userspace tools;
> >>>>> Not really. We just need to approach the problem thinking about how to
> >>>>> make it work based on how things currently work.
> >>>> I suppose it is all somewhat "subjective" - bad joke fully intended :)
> >>>> - with respect to what one considers good/bad/limiting.  My personal
> >>>> view is that an ideal solution would allow for multiple independent
> >>>> subj/obj labels without having to multiplex on a single subj/obj
> >>>> field.  My gut feeling is that this would confuse your tools, yes?
> >>>>
> >>>>> For example Casey had a list of possible formats. Like this one:
> >>>>>
> >>>>> Option 3:
> >>>>>         lsms=selinux,apparmor subj=x:y:z:s:c subj=a
> >>>>>
> >>>>> I'd suggest something almost like that. The first field could be a map
> >>>>> to
> >>>>> decipher the labels. Then we could have a comma separated list of
> >>>>> labels.
> >>>>>
> >>>>> lsms=selinux,apparmor subj=x:y:z:s:c,a
> >>>> Some quick comments:
> >>>>
> >>>> * My usual reminder that new fields for existing audit records must be
> >>>> added to the end of the record.
> >>>>
> >>>> * If we are going to multiplex the labels on a single field (more on
> >>>> that below) I might suggest using "subj_lsms" instead of "lsms" so we
> >>>> leave ourself some wiggle room in the future.
> >>>>
> >>>> * Multiplexing on a single "subj" field is going to be difficult
> >>>> because picking the label delimiter is going to be a pain.  For
> >>>> example, in the example above a comma is used, which at the very least
> >>>> is a valid part of a SELinux label and I suspect for Smack as well
> >>>> (I'm not sure about the other LSMs).  I suspect the only way to parse
> >>>> out the component labels would be to have knowledge of the LSMs in
> >>>> use, as well as the policies loaded at the time the audit record was
> >>>> generated.
> >>>>
> >>>> This may be a faulty assumption, but assuming your tools will fall
> >>>> over if they see multiple "subj" fields, could we do something like
> >>>>
> >>>> the following (something between option #2 and #3):
> >>>>   subj1_lsm=smack subj1=<smack_label> subj2_lsm=selinux
> >>>>
> >>>> subj2=<selinux_label> ...
> >>> If it's not a subj= field why use the indirection?
> >>>
> >>>         subj_smack=<smack_label> subj_selinux=<selinux_label>
> >>>
> >>> would be easier.
> >>
> >> Good point, that looks reasonable to me.
> >
> > But doing something like this will totally break all parsers. To be honest, I
> > don't know if I'll ever see more than one labeled security system running at
> > the same time. And this would be a big penalty to pay for the flexibility that
> > someone, somewhere just might possibly do this.
>
> While I have never seen multiple-LSM plans from RedHat/IBM I
> have seen them from Ubuntu. This isn't hypothetical. I know that
> it's a hard problem, which is why we need to get it as right as
> possible.

Agreed.  While I'm not going to be on a specific Linux release, I do
believe that at some point in the future the LSM stacking work is
going to land in Linus' tree.  Perhaps you'll never see it Steve, but
we need to prepare the code to handle it when it happens.

For my own sanity, here is a quick summary of the constraints as I
currently see them, please feel free to add/disagree:

* We can't have multiple "subj" fields in a single audit record.
* The different LSMs all have different label formats and allowed
characters.  Further, a given label format may not be unique for a
given LSM; for example, Smack could be configured with a subset of
SELinux labels.
* Steve's audit tools appear to require a "subj" and "obj" fields for
LSM information or else they break into tiny little pieces.

What if we preserved the existing subj/obj fields in the case where
there is only one "major" LSM (SELinux, Smack, AppArmor, etc.):

  subj=<lsm_label>

... and in the case of multiple major LSMs we set the subj value to
"?" and introduce new subj_X fields (as necessary) as discussed above:

  subj=? subj_smack=<smack_label> subj_selinux=<selinux_label> ...

... I believe that Steve's old/existing userspace tools would simply
report "?"/unknown LSM credentials where new multi-LSM tools could
report the multiple different labels.  While this may not be perfect,
it avoids having to multiplex the different labels into a single field
(which is a big win IMHO) with the only issue being that multi-LSM
solutions will need an updated audit toolset to see the new labels
(which seems like a reasonable requirement).

-- 
paul moore
www.paul-moore.com

^ permalink raw reply

* Re: [PATCH V35 15/29] acpi: Ignore acpi_rsdp kernel param when the kernel has been locked down
From: Matthew Garrett @ 2019-07-16 20:34 UTC (permalink / raw)
  To: Dave Young
  Cc: James Morris, LSM List, Linux Kernel Mailing List, Linux API,
	Josh Boyer, David Howells, Borislav Petkov, Kees Cook, linux-acpi
In-Reply-To: <20190716025923.GA5793@dhcp-128-65.nay.redhat.com>

On Mon, Jul 15, 2019 at 7:59 PM Dave Young <dyoung@redhat.com> wrote:
> I'm very sorry I noticed this late, but have to say this will not work for
> X86 with latest kernel code.

No problem, thank you for catching this! I'll update the patch and
send a new version.

^ permalink raw reply

* Re: [PATCH V35 23/29] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Matthew Garrett @ 2019-07-16 20:32 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: James Morris, LSM List, Linux Kernel Mailing List, Linux API,
	David Howells, Alexei Starovoitov, Network Development,
	Chun-Yi Lee
In-Reply-To: <5d363f09-d649-5693-45c0-bb99d69f0f38@iogearbox.net>

On Mon, Jul 15, 2019 at 3:54 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> Hmm, does security_locked_down() ever return a code > 0 or why do you
> have the double check on return code? If not, then for clarity the
> ret code from security_locked_down() should be checked as 'ret < 0'
> as well and out label should be at the memset directly instead.

It doesn't, so I'll update. Thanks!

^ permalink raw reply

* Re: [GIT PULL] SafeSetID LSM changes for 5.3
From: pr-tracker-bot @ 2019-07-16 19:40 UTC (permalink / raw)
  To: Micah Morton; +Cc: torvalds, Linux Kernel Mailing List, linux-security-module
In-Reply-To: <CAJ-EccPGqp4PmRkFk505QhDKHWn-ajxS0__Nk9VS32jV_+3Y2A@mail.gmail.com>

The pull request you sent on Mon, 15 Jul 2019 09:04:48 -0700:

> https://github.com/micah-morton/linux.git tags/safesetid-5.3

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/1ec4013bab89058dcc594dfe7b5a20f5d46bbc5f

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

^ permalink raw reply

* Re: [GIT PULL] SafeSetID LSM changes for 5.3
From: Linus Torvalds @ 2019-07-16 19:13 UTC (permalink / raw)
  To: Micah Morton; +Cc: Linux Kernel Mailing List, linux-security-module
In-Reply-To: <CAHk-=whY1J-LFvTa8ihiPNRSv1dwxPk9ycPCEhdcjsk7c=tGAw@mail.gmail.com>

On Tue, Jul 16, 2019 at 12:06 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>  - Please use the "git pull-request" format and then add any extra
> notes you feel are necessary
>
>    Yes, your pull request is *almost* git pull-request, but you seem
> to have actively removed whitespace making it almost illegible. It's
> really hard to pick out the line that has the actual git repository
> address, because it's basically hidden inside one big blob of text.

Extra note on this: you seem to have done "git pull-request" in a wide
window, and then copied-and-pasted it into your MUA.

So the diffstat lines are also very long, and then they line-wrap and
it all looks nasty.

Avoid this by either using a file for the output (that you then edit
for your own added messages), or using a normal 80x25 terminal or
something.

I guess I should ask Junio to add a "--stat=80" to the upstream git
request-pull script.

                   Linus

^ permalink raw reply

* Re: [GIT PULL] SafeSetID LSM changes for 5.3
From: Linus Torvalds @ 2019-07-16 19:06 UTC (permalink / raw)
  To: Micah Morton; +Cc: Linux Kernel Mailing List, linux-security-module
In-Reply-To: <CAJ-EccPGqp4PmRkFk505QhDKHWn-ajxS0__Nk9VS32jV_+3Y2A@mail.gmail.com>

On Mon, Jul 15, 2019 at 9:05 AM Micah Morton <mortonm@chromium.org> wrote:
>
> I'm maintaining the new SafeSetID LSM and was told to set up my own
> tree for sending pull requests rather than sending my changes through
> James Morris and the security subsystem tree.

Yes. It would be good if you also added yourself to the MAINTAINERS
file. Right now there's no entry for security/safesetid at all.

> This is my first time doing one of these pull requests so hopefully I
> didn't screw something up.

So a couple of notes:

 - *please* don't rebase your work in the day before

   Was this in linux-next? was this tested at all? Hard to tell, since
it was rebased recently, so for all I know it's all completely new

 - don't use a random kernel-of-the-day as the base for development

   This is related to the rebasing issue, but is true even if you
don't rebase. There is no way that it was a good idea to pick my
random - possibly completely broken - kernel from Sunday afternoon in
the middle of a merge window as a base for development.

   If you start development, or if you have to rebase (for some *good*
reason) you need to do so on a good stable base, not on the quick-sand
that is "random kernel of the day during the busiest merge activity".

 - Please use the "git pull-request" format and then add any extra
notes you feel are necessary

   Yes, your pull request is *almost* git pull-request, but you seem
to have actively removed whitespace making it almost illegible. It's
really hard to pick out the line that has the actual git repository
address, because it's basically hidden inside one big blob of text.

I've pulled this as-is since it's the first time, but I expect better next time.

There are various resources on some cleanliness issues, and people
fairly recently tried to combine it under

   Documentation/maintainer/rebasing-and-merging.rst

which covers at least the basics on why not to rebase etc.

And if you *do* end up rebasing, consider the end result "untested",
so then it should have been done before the merge window even started,
and the rebased branch should have been in linux-next. And not sent to
me the very next day.

                   Linus

^ permalink raw reply

* Re: Preferred subj= with multiple LSMs
From: Casey Schaufler @ 2019-07-16 18:41 UTC (permalink / raw)
  To: Steve Grubb, Paul Moore
  Cc: Richard Guy Briggs, linux-audit@redhat.com,
	Linux Security Module list, casey
In-Reply-To: <2477603.130G60v5SE@x2>

On 7/16/2019 11:06 AM, Steve Grubb wrote:
> On Tuesday, July 16, 2019 1:43:18 PM EDT Paul Moore wrote:
>> On Tue, Jul 16, 2019 at 1:30 PM Casey Schaufler <casey@schaufler-ca.com> 
> wrote:
>>> On 7/16/2019 10:12 AM, Paul Moore wrote:
>>>> On Mon, Jul 15, 2019 at 6:56 PM Steve Grubb <sgrubb@redhat.com> wrote:
>>>>> On Monday, July 15, 2019 5:28:56 PM EDT Paul Moore wrote:
>>>>>> On Mon, Jul 15, 2019 at 3:37 PM Casey Schaufler
>>>>>> <casey@schaufler-ca.com>
>>>>> wrote:
>>>>>>> On 7/15/2019 12:04 PM, Richard Guy Briggs wrote:
>>>>>>>> On 2019-07-13 11:08, Steve Grubb wrote:
>>>> ...
>>>>
>>>>>>>> Steve's answer is the obvious one, ideally allocating a seperate
>>>>>>>> range
>>>>>>>> to each LSM with each message type having its own well defined
>>>>>>>> format.
>>>>>>> It doesn't address the issue of success records, or records
>>>>>>> generated outside the security modules.
>>>>>> Yes, exactly.  The individual LSM will presumably will continue to
>>>>>> generate their own audit records as they do today and I would imagine
>>>>>> that the subject and object fields could remain as they do today for
>>>>>> the LSM specific records.
>>>>>>
>>>>>> The trick is the other records which are not LSM specific but still
>>>>>> want to include subject and/or object information.  Unfortunately we
>>>>>> are stuck with some tough limitations given the current audit record
>>>>>> format and Steve's audit userspace tools;
>>>>> Not really. We just need to approach the problem thinking about how to
>>>>> make it work based on how things currently work.
>>>> I suppose it is all somewhat "subjective" - bad joke fully intended :)
>>>> - with respect to what one considers good/bad/limiting.  My personal
>>>> view is that an ideal solution would allow for multiple independent
>>>> subj/obj labels without having to multiplex on a single subj/obj
>>>> field.  My gut feeling is that this would confuse your tools, yes?
>>>>
>>>>> For example Casey had a list of possible formats. Like this one:
>>>>>
>>>>> Option 3:
>>>>>         lsms=selinux,apparmor subj=x:y:z:s:c subj=a
>>>>>
>>>>> I'd suggest something almost like that. The first field could be a map
>>>>> to
>>>>> decipher the labels. Then we could have a comma separated list of
>>>>> labels.
>>>>>
>>>>> lsms=selinux,apparmor subj=x:y:z:s:c,a
>>>> Some quick comments:
>>>>
>>>> * My usual reminder that new fields for existing audit records must be
>>>> added to the end of the record.
>>>>
>>>> * If we are going to multiplex the labels on a single field (more on
>>>> that below) I might suggest using "subj_lsms" instead of "lsms" so we
>>>> leave ourself some wiggle room in the future.
>>>>
>>>> * Multiplexing on a single "subj" field is going to be difficult
>>>> because picking the label delimiter is going to be a pain.  For
>>>> example, in the example above a comma is used, which at the very least
>>>> is a valid part of a SELinux label and I suspect for Smack as well
>>>> (I'm not sure about the other LSMs).  I suspect the only way to parse
>>>> out the component labels would be to have knowledge of the LSMs in
>>>> use, as well as the policies loaded at the time the audit record was
>>>> generated.
>>>>
>>>> This may be a faulty assumption, but assuming your tools will fall
>>>> over if they see multiple "subj" fields, could we do something like
>>>>
>>>> the following (something between option #2 and #3):
>>>>   subj1_lsm=smack subj1=<smack_label> subj2_lsm=selinux
>>>>
>>>> subj2=<selinux_label> ...
>>> If it's not a subj= field why use the indirection?
>>>
>>>         subj_smack=<smack_label> subj_selinux=<selinux_label>
>>>
>>> would be easier.
>> Good point, that looks reasonable to me.
> But doing something like this will totally break all parsers. To be honest, I 
> don't know if I'll ever see more than one labeled security system running at 
> the same time. And this would be a big penalty to pay for the flexibility that 
> someone, somewhere just might possibly do this.

While I have never seen multiple-LSM plans from RedHat/IBM I
have seen them from Ubuntu. This isn't hypothetical. I know that
it's a hard problem, which is why we need to get it as right as
possible.



^ permalink raw reply

* Re: [RFC PATCH v6 0/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Singh Khurana @ 2019-07-16 18:08 UTC (permalink / raw)
  To: Milan Broz
  Cc: ebiggers, linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, agk, snitzer, dm-devel, jmorris, Scott Shell,
	Nazmus Sakib, mpatocka
In-Reply-To: <395efa90-65d8-d832-3e2b-2b8ee3794688@gmail.com>


Hello Milan,
On Tue, 16 Jul 2019, Milan Broz wrote:

> On 12/07/2019 19:33, Jaskaran Singh Khurana wrote:
>>
>> Hello Milan,
>>
>>> Changes in v6:
>>>
>>> Address comments from Milan Broz and Eric Biggers on v5.
>>>
>>> -Keep the verification code under config DM_VERITY_VERIFY_ROOTHASH_SIG.
>>>
>>> -Change the command line parameter to requires_signatures(bool) which will
>>> force root hash to be signed and trusted if specified.
>>>
>>> -Fix the signature not being present in verity_status. Merged the
>>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmbroz%2Flinux.git%2Fcommit%2F%3Fh%3Ddm-cryptsetup%26id%3Da26c10806f5257e255b6a436713127e762935ad3&amp;data=02%7C01%7CJaskaran.Khurana%40microsoft.com%7C18f92445e46940aeebb008d6fe50c610%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636976020210890638&amp;sdata=aY0V9%2FBz2RHryIvoftGKUGnyPp9Fsc1JY4FZbHfW4hg%3D&amp;reserved=0
>>> made by Milan Broz and tested it.
>>>
>>>
>>
>> Could you please provide feedback on this v6 version.
>
> Hi,
>
> I am ok with the v6 patch; I think Mike will return to it in 5.4 reviews.
>

Thanks for the help and also for reviewing this patch. Could you please 
add Reviewed-by/Tested-by tag to the patch.

> But the documentation is very brief. I spent quite a long time to configure the system properly.
> I think you should add more description (at least to patch header) how to use this feature in combination with system keyring.
>

I will add more documentation to the patch header describing the steps 
required for setup.

> Do I understand correctly that these steps need to be done?
>
> - user configures a certificate and adds it in kernel builtin keyring (I used CONFIG_SYSTEM_TRUSTED_KEYS option).
> - the dm-verity device root hash is signed directly by a key of this cert
> - the signature is uploaded to the user keyring
> - reference to signature in keyring is added as an optional dm-verity table parameter root_hash_sig_key_desc
> - optionally, require_signatures dm-verity module is set to enforce signatures.
>
> For reference, below is the bash script I used (with unpatched veritysetup to generate working DM table), is the expected workflow here?

The steps and workflow is correct. I will send the cryptsetup changes for 
review.

>
> #!/bin/bash
>
> NAME=test
> DEV=/dev/sdc
> DEV_HASH=/dev/sdd
> ROOT_HASH=778fccab393842688c9af89cfd0c5cde69377cbe21ed439109ec856f2aa8a423
> SIGN=sign.txt
> SIGN_NAME=verity:$NAME
>
> # get unsigned device-mapper table using unpatched veritysetup
> veritysetup open $DEV $NAME $DEV_HASH $ROOT_HASH
> TABLE=$(dmsetup table $NAME)
> veritysetup close $NAME
>
> # Generate self-signed CA key, must be in .config as CONFIG_SYSTEM_TRUSTED_KEYS="path/ca.pem"
> #openssl req -x509 -newkey rsa:1024 -keyout ca_key.pem -out ca.pem -nodes -days 365 -set_serial 01 -subj /CN=example.com
>
> # sign root hash directly by CA cert
> echo -n $ROOT_HASH | openssl smime -sign -nocerts -noattr -binary -inkey ca_key.pem -signer ca.pem -outform der -out $SIGN
>
> # load signature to keyring
> keyctl padd user $SIGN_NAME @u <$SIGN
>
> # add device-mapper table, now with sighed root hash optional argument
> dmsetup create -r $NAME --table "$TABLE 2 root_hash_sig_key_desc $SIGN_NAME"
> dmsetup table $NAME
>
> # cleanup
> dmsetup remove $NAME
> keyctl clear @u
>
>

Thanks for testing the changes and all the guidance here.

> Milan
>
Regards,
Jaskaran.

^ permalink raw reply

* Re: Preferred subj= with multiple LSMs
From: Steve Grubb @ 2019-07-16 18:06 UTC (permalink / raw)
  To: Paul Moore
  Cc: Casey Schaufler, Richard Guy Briggs, linux-audit@redhat.com,
	Linux Security Module list
In-Reply-To: <CAHC9VhSTwvueKcK2yhckwayh9YGou7gt2Gny36DOTaNkrck+Mg@mail.gmail.com>

On Tuesday, July 16, 2019 1:43:18 PM EDT Paul Moore wrote:
> On Tue, Jul 16, 2019 at 1:30 PM Casey Schaufler <casey@schaufler-ca.com> 
wrote:
> > On 7/16/2019 10:12 AM, Paul Moore wrote:
> > > On Mon, Jul 15, 2019 at 6:56 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > >> On Monday, July 15, 2019 5:28:56 PM EDT Paul Moore wrote:
> > >>> On Mon, Jul 15, 2019 at 3:37 PM Casey Schaufler
> > >>> <casey@schaufler-ca.com>
> > >> 
> > >> wrote:
> > >>>> On 7/15/2019 12:04 PM, Richard Guy Briggs wrote:
> > >>>>> On 2019-07-13 11:08, Steve Grubb wrote:
> > > ...
> > > 
> > >>>>> Steve's answer is the obvious one, ideally allocating a seperate
> > >>>>> range
> > >>>>> to each LSM with each message type having its own well defined
> > >>>>> format.
> > >>>> 
> > >>>> It doesn't address the issue of success records, or records
> > >>>> generated outside the security modules.
> > >>> 
> > >>> Yes, exactly.  The individual LSM will presumably will continue to
> > >>> generate their own audit records as they do today and I would imagine
> > >>> that the subject and object fields could remain as they do today for
> > >>> the LSM specific records.
> > >>> 
> > >>> The trick is the other records which are not LSM specific but still
> > >>> want to include subject and/or object information.  Unfortunately we
> > >>> are stuck with some tough limitations given the current audit record
> > >>> format and Steve's audit userspace tools;
> > >> 
> > >> Not really. We just need to approach the problem thinking about how to
> > >> make it work based on how things currently work.
> > > 
> > > I suppose it is all somewhat "subjective" - bad joke fully intended :)
> > > - with respect to what one considers good/bad/limiting.  My personal
> > > view is that an ideal solution would allow for multiple independent
> > > subj/obj labels without having to multiplex on a single subj/obj
> > > field.  My gut feeling is that this would confuse your tools, yes?
> > > 
> > >> For example Casey had a list of possible formats. Like this one:
> > >> 
> > >> Option 3:
> > >>         lsms=selinux,apparmor subj=x:y:z:s:c subj=a
> > >> 
> > >> I'd suggest something almost like that. The first field could be a map
> > >> to
> > >> decipher the labels. Then we could have a comma separated list of
> > >> labels.
> > >> 
> > >> lsms=selinux,apparmor subj=x:y:z:s:c,a
> > > 
> > > Some quick comments:
> > > 
> > > * My usual reminder that new fields for existing audit records must be
> > > added to the end of the record.
> > > 
> > > * If we are going to multiplex the labels on a single field (more on
> > > that below) I might suggest using "subj_lsms" instead of "lsms" so we
> > > leave ourself some wiggle room in the future.
> > > 
> > > * Multiplexing on a single "subj" field is going to be difficult
> > > because picking the label delimiter is going to be a pain.  For
> > > example, in the example above a comma is used, which at the very least
> > > is a valid part of a SELinux label and I suspect for Smack as well
> > > (I'm not sure about the other LSMs).  I suspect the only way to parse
> > > out the component labels would be to have knowledge of the LSMs in
> > > use, as well as the policies loaded at the time the audit record was
> > > generated.
> > > 
> > > This may be a faulty assumption, but assuming your tools will fall
> > > over if they see multiple "subj" fields, could we do something like
> > > 
> > > the following (something between option #2 and #3):
> > >   subj1_lsm=smack subj1=<smack_label> subj2_lsm=selinux
> > > 
> > > subj2=<selinux_label> ...
> > 
> > If it's not a subj= field why use the indirection?
> > 
> >         subj_smack=<smack_label> subj_selinux=<selinux_label>
> > 
> > would be easier.
> 
> Good point, that looks reasonable to me.

But doing something like this will totally break all parsers. To be honest, I 
don't know if I'll ever see more than one labeled security system running at 
the same time. And this would be a big penalty to pay for the flexibility that 
someone, somewhere just might possibly do this.

-Steve




^ permalink raw reply

* Re: Preferred subj= with multiple LSMs
From: Casey Schaufler @ 2019-07-16 17:58 UTC (permalink / raw)
  To: Paul Moore
  Cc: Steve Grubb, Richard Guy Briggs, linux-audit@redhat.com,
	Linux Security Module list, casey
In-Reply-To: <CAHC9VhSTwvueKcK2yhckwayh9YGou7gt2Gny36DOTaNkrck+Mg@mail.gmail.com>

On 7/16/2019 10:43 AM, Paul Moore wrote:
> On Tue, Jul 16, 2019 at 1:30 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 7/16/2019 10:12 AM, Paul Moore wrote:
>>> On Mon, Jul 15, 2019 at 6:56 PM Steve Grubb <sgrubb@redhat.com> wrote:
>>>> On Monday, July 15, 2019 5:28:56 PM EDT Paul Moore wrote:
>>>>> On Mon, Jul 15, 2019 at 3:37 PM Casey Schaufler <casey@schaufler-ca.com>
>>>> wrote:
>>>>>> On 7/15/2019 12:04 PM, Richard Guy Briggs wrote:
>>>>>>> On 2019-07-13 11:08, Steve Grubb wrote:
>>> ...
>>>
>>>>>>> Steve's answer is the obvious one, ideally allocating a seperate range
>>>>>>> to each LSM with each message type having its own well defined format.
>>>>>> It doesn't address the issue of success records, or records
>>>>>> generated outside the security modules.
>>>>> Yes, exactly.  The individual LSM will presumably will continue to
>>>>> generate their own audit records as they do today and I would imagine
>>>>> that the subject and object fields could remain as they do today for
>>>>> the LSM specific records.
>>>>>
>>>>> The trick is the other records which are not LSM specific but still
>>>>> want to include subject and/or object information.  Unfortunately we
>>>>> are stuck with some tough limitations given the current audit record
>>>>> format and Steve's audit userspace tools;
>>>> Not really. We just need to approach the problem thinking about how to make
>>>> it work based on how things currently work.
>>> I suppose it is all somewhat "subjective" - bad joke fully intended :)
>>> - with respect to what one considers good/bad/limiting.  My personal
>>> view is that an ideal solution would allow for multiple independent
>>> subj/obj labels without having to multiplex on a single subj/obj
>>> field.  My gut feeling is that this would confuse your tools, yes?
>>>
>>>> For example Casey had a list of possible formats. Like this one:
>>>>
>>>> Option 3:
>>>>         lsms=selinux,apparmor subj=x:y:z:s:c subj=a
>>>>
>>>> I'd suggest something almost like that. The first field could be a map to
>>>> decipher the labels. Then we could have a comma separated list of labels.
>>>>
>>>> lsms=selinux,apparmor subj=x:y:z:s:c,a
>>> Some quick comments:
>>>
>>> * My usual reminder that new fields for existing audit records must be
>>> added to the end of the record.
>>>
>>> * If we are going to multiplex the labels on a single field (more on
>>> that below) I might suggest using "subj_lsms" instead of "lsms" so we
>>> leave ourself some wiggle room in the future.
>>>
>>> * Multiplexing on a single "subj" field is going to be difficult
>>> because picking the label delimiter is going to be a pain.  For
>>> example, in the example above a comma is used, which at the very least
>>> is a valid part of a SELinux label and I suspect for Smack as well
>>> (I'm not sure about the other LSMs).  I suspect the only way to parse
>>> out the component labels would be to have knowledge of the LSMs in
>>> use, as well as the policies loaded at the time the audit record was
>>> generated.
>>>
>>> This may be a faulty assumption, but assuming your tools will fall
>>> over if they see multiple "subj" fields, could we do something like
>>> the following (something between option #2 and #3):
>>>
>>>   subj1_lsm=smack subj1=<smack_label> subj2_lsm=selinux
>>> subj2=<selinux_label> ...
>> If it's not a subj= field why use the indirection?
>>
>>         subj_smack=<smack_label> subj_selinux=<selinux_label>
>>
>> would be easier.
> Good point, that looks reasonable to me.

Which raises the question of what to do with the subj= :

	- omit it
	- subj=?
	- subj=some-special-message
	- subj=label-of-first-lsm



^ permalink raw reply

* Re: Preferred subj= with multiple LSMs
From: Paul Moore @ 2019-07-16 17:43 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Steve Grubb, Richard Guy Briggs, linux-audit@redhat.com,
	Linux Security Module list
In-Reply-To: <c993f63a-7c2d-c6c8-cfa6-3cfba410770d@schaufler-ca.com>

On Tue, Jul 16, 2019 at 1:30 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/16/2019 10:12 AM, Paul Moore wrote:
> > On Mon, Jul 15, 2019 at 6:56 PM Steve Grubb <sgrubb@redhat.com> wrote:
> >> On Monday, July 15, 2019 5:28:56 PM EDT Paul Moore wrote:
> >>> On Mon, Jul 15, 2019 at 3:37 PM Casey Schaufler <casey@schaufler-ca.com>
> >> wrote:
> >>>> On 7/15/2019 12:04 PM, Richard Guy Briggs wrote:
> >>>>> On 2019-07-13 11:08, Steve Grubb wrote:
> > ...
> >
> >>>>> Steve's answer is the obvious one, ideally allocating a seperate range
> >>>>> to each LSM with each message type having its own well defined format.
> >>>> It doesn't address the issue of success records, or records
> >>>> generated outside the security modules.
> >>> Yes, exactly.  The individual LSM will presumably will continue to
> >>> generate their own audit records as they do today and I would imagine
> >>> that the subject and object fields could remain as they do today for
> >>> the LSM specific records.
> >>>
> >>> The trick is the other records which are not LSM specific but still
> >>> want to include subject and/or object information.  Unfortunately we
> >>> are stuck with some tough limitations given the current audit record
> >>> format and Steve's audit userspace tools;
> >> Not really. We just need to approach the problem thinking about how to make
> >> it work based on how things currently work.
> > I suppose it is all somewhat "subjective" - bad joke fully intended :)
> > - with respect to what one considers good/bad/limiting.  My personal
> > view is that an ideal solution would allow for multiple independent
> > subj/obj labels without having to multiplex on a single subj/obj
> > field.  My gut feeling is that this would confuse your tools, yes?
> >
> >> For example Casey had a list of possible formats. Like this one:
> >>
> >> Option 3:
> >>         lsms=selinux,apparmor subj=x:y:z:s:c subj=a
> >>
> >> I'd suggest something almost like that. The first field could be a map to
> >> decipher the labels. Then we could have a comma separated list of labels.
> >>
> >> lsms=selinux,apparmor subj=x:y:z:s:c,a
> > Some quick comments:
> >
> > * My usual reminder that new fields for existing audit records must be
> > added to the end of the record.
> >
> > * If we are going to multiplex the labels on a single field (more on
> > that below) I might suggest using "subj_lsms" instead of "lsms" so we
> > leave ourself some wiggle room in the future.
> >
> > * Multiplexing on a single "subj" field is going to be difficult
> > because picking the label delimiter is going to be a pain.  For
> > example, in the example above a comma is used, which at the very least
> > is a valid part of a SELinux label and I suspect for Smack as well
> > (I'm not sure about the other LSMs).  I suspect the only way to parse
> > out the component labels would be to have knowledge of the LSMs in
> > use, as well as the policies loaded at the time the audit record was
> > generated.
> >
> > This may be a faulty assumption, but assuming your tools will fall
> > over if they see multiple "subj" fields, could we do something like
> > the following (something between option #2 and #3):
> >
> >   subj1_lsm=smack subj1=<smack_label> subj2_lsm=selinux
> > subj2=<selinux_label> ...
>
> If it's not a subj= field why use the indirection?
>
>         subj_smack=<smack_label> subj_selinux=<selinux_label>
>
> would be easier.

Good point, that looks reasonable to me.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply

* Re: Preferred subj= with multiple LSMs
From: Casey Schaufler @ 2019-07-16 17:29 UTC (permalink / raw)
  To: Paul Moore, Steve Grubb
  Cc: Richard Guy Briggs, linux-audit@redhat.com,
	Linux Security Module list, casey
In-Reply-To: <CAHC9VhSELVZN8feH56zsANqoHu16mPMD04Ww60W=r6tWs+8WnQ@mail.gmail.com>

On 7/16/2019 10:12 AM, Paul Moore wrote:
> On Mon, Jul 15, 2019 at 6:56 PM Steve Grubb <sgrubb@redhat.com> wrote:
>> On Monday, July 15, 2019 5:28:56 PM EDT Paul Moore wrote:
>>> On Mon, Jul 15, 2019 at 3:37 PM Casey Schaufler <casey@schaufler-ca.com>
>> wrote:
>>>> On 7/15/2019 12:04 PM, Richard Guy Briggs wrote:
>>>>> On 2019-07-13 11:08, Steve Grubb wrote:
> ...
>
>>>>> Steve's answer is the obvious one, ideally allocating a seperate range
>>>>> to each LSM with each message type having its own well defined format.
>>>> It doesn't address the issue of success records, or records
>>>> generated outside the security modules.
>>> Yes, exactly.  The individual LSM will presumably will continue to
>>> generate their own audit records as they do today and I would imagine
>>> that the subject and object fields could remain as they do today for
>>> the LSM specific records.
>>>
>>> The trick is the other records which are not LSM specific but still
>>> want to include subject and/or object information.  Unfortunately we
>>> are stuck with some tough limitations given the current audit record
>>> format and Steve's audit userspace tools;
>> Not really. We just need to approach the problem thinking about how to make
>> it work based on how things currently work.
> I suppose it is all somewhat "subjective" - bad joke fully intended :)
> - with respect to what one considers good/bad/limiting.  My personal
> view is that an ideal solution would allow for multiple independent
> subj/obj labels without having to multiplex on a single subj/obj
> field.  My gut feeling is that this would confuse your tools, yes?
>
>> For example Casey had a list of possible formats. Like this one:
>>
>> Option 3:
>>         lsms=selinux,apparmor subj=x:y:z:s:c subj=a
>>
>> I'd suggest something almost like that. The first field could be a map to
>> decipher the labels. Then we could have a comma separated list of labels.
>>
>> lsms=selinux,apparmor subj=x:y:z:s:c,a
> Some quick comments:
>
> * My usual reminder that new fields for existing audit records must be
> added to the end of the record.
>
> * If we are going to multiplex the labels on a single field (more on
> that below) I might suggest using "subj_lsms" instead of "lsms" so we
> leave ourself some wiggle room in the future.
>
> * Multiplexing on a single "subj" field is going to be difficult
> because picking the label delimiter is going to be a pain.  For
> example, in the example above a comma is used, which at the very least
> is a valid part of a SELinux label and I suspect for Smack as well
> (I'm not sure about the other LSMs).  I suspect the only way to parse
> out the component labels would be to have knowledge of the LSMs in
> use, as well as the policies loaded at the time the audit record was
> generated.
>
> This may be a faulty assumption, but assuming your tools will fall
> over if they see multiple "subj" fields, could we do something like
> the following (something between option #2 and #3):
>
>   subj1_lsm=smack subj1=<smack_label> subj2_lsm=selinux
> subj2=<selinux_label> ...

If it's not a subj= field why use the indirection?

	subj_smack=<smack_label> subj_selinux=<selinux_label>

would be easier. 


^ permalink raw reply

* Re: Preferred subj= with multiple LSMs
From: Casey Schaufler @ 2019-07-16 17:16 UTC (permalink / raw)
  To: Steve Grubb
  Cc: Paul Moore, Richard Guy Briggs, linux-audit@redhat.com, casey,
	Linux Security Module list
In-Reply-To: <3577098.oGDFHdoSSQ@x2>

On 7/16/2019 9:14 AM, Steve Grubb wrote:
> On Tuesday, July 16, 2019 12:00:05 PM EDT Casey Schaufler wrote:
>>
>> Unless there's an objection I will use this format with
>> a slight modification. Smack allows commas in labels, so
>> using a bare comma can lead to ambiguity.
>>
>> lsms=smack,apparmor subj="TS/Alpha,Beta","a"

Oops! '/' isn't allowed in a Smack label. How embarrassing is that?

>>
>> It's more code change than some of the other options,
>> but if it has the best chance of working with user space
>> I'm game.
> Quoting has a specific meaning in audit fields. So, we really shouldn't do 
> that. We can simply pick another field delimiter. I really don't care which it 
> is as long as its illegal for use in a label. For example, we use 
>
> #define AUDIT_KEY_SEPARATOR 0x01
>
> to separate key fields. We can pick almost anything. (exclamation mark, semi-
> colon, hash, plus symbol, tilde, 0x02, whatever) But it will need to be 
> documented and put into the API so that everyone is aware of the convention.

Unless there's objection I'll document and use '/',

lsms=selinux,apparmor subj=a:b:c:d/a

If there is objection without alternative presented I'll use 0x02,
because no one (I hope) is going to allow that in their label, and
keys have set precedence for unprintable characters.

>
> -Steve
>
>


^ permalink raw reply

* Re: [RFC PATCH] security, capability: pass object information to security_capable
From: Stephen Smalley @ 2019-07-16 15:08 UTC (permalink / raw)
  To: Andy Lutomirski, Serge E. Hallyn
  Cc: Casey Schaufler, Nicholas Franck, Paul Moore, selinux, LSM List,
	James Morris, Kees Cook, John Johansen, mortonm
In-Reply-To: <CALCETrXR3RoRF0kZk_G-gAg=D6LNAcBJYiiHFHSe3S=bRZcwNA@mail.gmail.com>

On 7/16/19 10:21 AM, Andy Lutomirski wrote:
> On Tue, Jul 16, 2019 at 7:03 AM Serge E. Hallyn <serge@hallyn.com> wrote:
>>
>> On Fri, Jul 12, 2019 at 12:54:02PM -0700, Casey Schaufler wrote:
>>> On 7/12/2019 11:25 AM, Stephen Smalley wrote:
>>>> On 7/12/19 1:58 PM, Casey Schaufler wrote:
>>>>> On 7/12/2019 10:34 AM, Nicholas Franck wrote:
>>>>>> At present security_capable does not pass any object information
>>>>>> and therefore can neither audit the particular object nor take it
>>>>>> into account. Augment the security_capable interface to support
>>>>>> passing supplementary data. Use this facility initially to convey
>>>>>> the inode for capability checks relevant to inodes. This only
>>>>>> addresses capable_wrt_inode_uidgid calls; other capability checks
>>>>>> relevant to inodes will be addressed in subsequent changes. In the
>>>>>> future, this will be further extended to pass object information for
>>>>>> other capability checks such as the target task for CAP_KILL.
>>>>>
>>>>> This seems wrong to me. The capability system has nothing to do
>>>>> with objects. Passing object information through security_capable()
>>>>> may be convenient, but isn't relevant to the purpose of the interface.
>>>>> It appears that there are very few places where the object information
>>>>> is actually useful.
>>>>
>>>> A fair number of capabilities are checked upon some attempted object access (often right after comparing UIDs or other per-object state), and the particular object can be very helpful in both audit and in access control.  More below.
>>>
>>> I'm not disagreeing with that. What I'm saying is that the capability
>>> check interface is not the right place to pass that information. The
>>> capability check has no use for the object information. I would much
>>
>> I've had to argue this before while doing the namespaced file
>> capabilities implementation.  Perhaps this would be worth writing something
>> more formal about.  My main argument is, even if we want to claim that the
>> capabilities model is and should be object agnostic, the implementation
>> of user namespaces (currently) is such that the whole view of the user's
>> privilege must include information which is stored with the object.
>>
>> There are various user namespaces.
>>
>> The Linux capabilities ( :-) ) model is user namespaced.  It must be, in
>> order to be useful.  If we're going to use file capabilities in distros,
>> and distros are going to run in containers, then the capabilities must
>> be namespaced.  Otherwise, capabilities will not be used, and heck, should
>> just be dropped.
>>
>> The only way to find out which user namespace has privilege over an inode
>> is to look at the inode.
>>
>> Therefore, object information is needed.
> 
> Agreed.  The concept in the kernel is "capability over a namespace."
> 
> That being said, sticking a flexible object type into ns_capable()
> seems prematurely general to me.  How about adding
> security_capable_wrt_inode_uidgid() and allowing LSMs to hook that?
> The current implementation would go into commoncap.  The obvious
> extensions I can think of are security_dac_read_search(..., inode,
> ...) and security_dac_override(..., inode, ...).  (Or dentry or
> whatever is appropriate.)

1) Not even all the inode-related capability checks go through 
capable_wrt_inode_uidgid(), so a hook there won't suffice.

2) Other capabilities have other kinds of objects, e.g. tasks, sysvipc, 
etc, and we'll want those to be handled too.

> 
> If this patch were restructured like that, the semantics would be
> obvious, and it would arguably be a genuine cleanup instead of a whole
> new mechanism of unknown scope.
> 


^ permalink raw reply

* Re: [RFC PATCH] security, capability: pass object information to security_capable
From: Casey Schaufler @ 2019-07-16 15:03 UTC (permalink / raw)
  To: Andy Lutomirski, Serge E. Hallyn
  Cc: Stephen Smalley, Nicholas Franck, Paul Moore, selinux, LSM List,
	James Morris, Kees Cook, John Johansen, mortonm, casey
In-Reply-To: <CALCETrXR3RoRF0kZk_G-gAg=D6LNAcBJYiiHFHSe3S=bRZcwNA@mail.gmail.com>

On 7/16/2019 7:21 AM, Andy Lutomirski wrote:
> On Tue, Jul 16, 2019 at 7:03 AM Serge E. Hallyn <serge@hallyn.com> wrote:
>> On Fri, Jul 12, 2019 at 12:54:02PM -0700, Casey Schaufler wrote:
>>> On 7/12/2019 11:25 AM, Stephen Smalley wrote:
>>>> On 7/12/19 1:58 PM, Casey Schaufler wrote:
>>>>> On 7/12/2019 10:34 AM, Nicholas Franck wrote:
>>>>>> At present security_capable does not pass any object information
>>>>>> and therefore can neither audit the particular object nor take it
>>>>>> into account. Augment the security_capable interface to support
>>>>>> passing supplementary data. Use this facility initially to convey
>>>>>> the inode for capability checks relevant to inodes. This only
>>>>>> addresses capable_wrt_inode_uidgid calls; other capability checks
>>>>>> relevant to inodes will be addressed in subsequent changes. In the
>>>>>> future, this will be further extended to pass object information for
>>>>>> other capability checks such as the target task for CAP_KILL.
>>>>> This seems wrong to me. The capability system has nothing to do
>>>>> with objects. Passing object information through security_capable()
>>>>> may be convenient, but isn't relevant to the purpose of the interface.
>>>>> It appears that there are very few places where the object information
>>>>> is actually useful.
>>>> A fair number of capabilities are checked upon some attempted object access (often right after comparing UIDs or other per-object state), and the particular object can be very helpful in both audit and in access control.  More below.
>>> I'm not disagreeing with that. What I'm saying is that the capability
>>> check interface is not the right place to pass that information. The
>>> capability check has no use for the object information. I would much
>> I've had to argue this before while doing the namespaced file
>> capabilities implementation.  Perhaps this would be worth writing something
>> more formal about.  My main argument is, even if we want to claim that the
>> capabilities model is and should be object agnostic, the implementation
>> of user namespaces (currently) is such that the whole view of the user's
>> privilege must include information which is stored with the object.
>>
>> There are various user namespaces.
>>
>> The Linux capabilities ( :-) ) model is user namespaced.  It must be, in
>> order to be useful.  If we're going to use file capabilities in distros,
>> and distros are going to run in containers, then the capabilities must
>> be namespaced.  Otherwise, capabilities will not be used, and heck, should
>> just be dropped.
>>
>> The only way to find out which user namespace has privilege over an inode
>> is to look at the inode.
>>
>> Therefore, object information is needed.
> Agreed.  The concept in the kernel is "capability over a namespace."
>
> That being said, sticking a flexible object type into ns_capable()
> seems prematurely general to me.  How about adding
> security_capable_wrt_inode_uidgid() and allowing LSMs to hook that?
> The current implementation would go into commoncap.  The obvious
> extensions I can think of are security_dac_read_search(..., inode,
> ...) and security_dac_override(..., inode, ...).  (Or dentry or
> whatever is appropriate.)

Would you have an LSM interface for each capability then?
security_sysadmin()? security_chown()? Or do you want to add
security_hey_look_here_is_yet_another_special_case() for
each if () in the kernel?

Sorry, I got carried away. I've been wallowing in the LSM
for too long not to be sensitive to just how fragile the
whole thing is. Adding a bunch more single use interfaces
isn't going to help it be useful in the long run. Please,
let's not go hog wild adding LSM functions. Please.

>
> If this patch were restructured like that, the semantics would be
> obvious, and it would arguably be a genuine cleanup instead of a whole
> new mechanism of unknown scope.


^ permalink raw reply

* Re: [RFC PATCH] security, capability: pass object information to security_capable
From: Casey Schaufler @ 2019-07-16 14:43 UTC (permalink / raw)
  To: Serge E. Hallyn
  Cc: Stephen Smalley, Nicholas Franck, paul, selinux,
	linux-security-module, luto, jmorris, keescook, john.johansen,
	mortonm
In-Reply-To: <20190716140349.GA4991@mail.hallyn.com>

On 7/16/2019 7:03 AM, Serge E. Hallyn wrote:
> On Fri, Jul 12, 2019 at 12:54:02PM -0700, Casey Schaufler wrote:
>> On 7/12/2019 11:25 AM, Stephen Smalley wrote:
>>> On 7/12/19 1:58 PM, Casey Schaufler wrote:
>>>> On 7/12/2019 10:34 AM, Nicholas Franck wrote:
>>>>> At present security_capable does not pass any object information
>>>>> and therefore can neither audit the particular object nor take it
>>>>> into account. Augment the security_capable interface to support
>>>>> passing supplementary data. Use this facility initially to convey
>>>>> the inode for capability checks relevant to inodes. This only
>>>>> addresses capable_wrt_inode_uidgid calls; other capability checks
>>>>> relevant to inodes will be addressed in subsequent changes. In the
>>>>> future, this will be further extended to pass object information for
>>>>> other capability checks such as the target task for CAP_KILL.
>>>> This seems wrong to me. The capability system has nothing to do
>>>> with objects. Passing object information through security_capable()
>>>> may be convenient, but isn't relevant to the purpose of the interface.
>>>> It appears that there are very few places where the object information
>>>> is actually useful.
>>> A fair number of capabilities are checked upon some attempted object access (often right after comparing UIDs or other per-object state), and the particular object can be very helpful in both audit and in access control.  More below.
>> I'm not disagreeing with that. What I'm saying is that the capability
>> check interface is not the right place to pass that information. The
>> capability check has no use for the object information. I would much
> I've had to argue this before while doing the namespaced file
> capabilities implementation.  Perhaps this would be worth writing something
> more formal about.  My main argument is, even if we want to claim that the
> capabilities model is and should be object agnostic, the implementation
> of user namespaces (currently) is such that the whole view of the user's
> privilege must include information which is stored with the object.
>
> There are various user namespaces.
>
> The Linux capabilities ( :-) ) model is user namespaced.  It must be, in
> order to be useful.  If we're going to use file capabilities in distros,
> and distros are going to run in containers, then the capabilities must
> be namespaced.  Otherwise, capabilities will not be used, and heck, should
> just be dropped.
>
> The only way to find out which user namespace has privilege over an inode
> is to look at the inode.
>
> Therefore, object information is needed.
>
> Until now we've sneaked around that by doing things like capable_wrt_inode_uidgid()
> and rootid_from_xattr().
>
> Again, this crucial: IMO, you have to be able to use a distro the same way in a
> container and not.  Either we support using capabilities in a user namespaced
> container, or we drop capabilities support will not be used, and we may as
> well drop the module.
>
> Now, yes, if someone tries to extend this stuff to do pathname parsing, then we
> might have to put our footsies down.  But we've been dancing around this for a
> long time anyway, so passing the inode in so we can do better logging gets a +1
> from me.

I shake my head and sigh, but as I don't have a better
solution, nor the time to go looking for one, I'm not
going to place obstacles. That, and it's entirely possible
that my view is wrong.

>
> -serge


^ permalink raw reply

* Re: [RFC PATCH] security, capability: pass object information to security_capable
From: Andy Lutomirski @ 2019-07-16 14:21 UTC (permalink / raw)
  To: Serge E. Hallyn
  Cc: Casey Schaufler, Stephen Smalley, Nicholas Franck, Paul Moore,
	selinux, LSM List, James Morris, Kees Cook, John Johansen,
	mortonm
In-Reply-To: <20190716140349.GA4991@mail.hallyn.com>

On Tue, Jul 16, 2019 at 7:03 AM Serge E. Hallyn <serge@hallyn.com> wrote:
>
> On Fri, Jul 12, 2019 at 12:54:02PM -0700, Casey Schaufler wrote:
> > On 7/12/2019 11:25 AM, Stephen Smalley wrote:
> > > On 7/12/19 1:58 PM, Casey Schaufler wrote:
> > >> On 7/12/2019 10:34 AM, Nicholas Franck wrote:
> > >>> At present security_capable does not pass any object information
> > >>> and therefore can neither audit the particular object nor take it
> > >>> into account. Augment the security_capable interface to support
> > >>> passing supplementary data. Use this facility initially to convey
> > >>> the inode for capability checks relevant to inodes. This only
> > >>> addresses capable_wrt_inode_uidgid calls; other capability checks
> > >>> relevant to inodes will be addressed in subsequent changes. In the
> > >>> future, this will be further extended to pass object information for
> > >>> other capability checks such as the target task for CAP_KILL.
> > >>
> > >> This seems wrong to me. The capability system has nothing to do
> > >> with objects. Passing object information through security_capable()
> > >> may be convenient, but isn't relevant to the purpose of the interface.
> > >> It appears that there are very few places where the object information
> > >> is actually useful.
> > >
> > > A fair number of capabilities are checked upon some attempted object access (often right after comparing UIDs or other per-object state), and the particular object can be very helpful in both audit and in access control.  More below.
> >
> > I'm not disagreeing with that. What I'm saying is that the capability
> > check interface is not the right place to pass that information. The
> > capability check has no use for the object information. I would much
>
> I've had to argue this before while doing the namespaced file
> capabilities implementation.  Perhaps this would be worth writing something
> more formal about.  My main argument is, even if we want to claim that the
> capabilities model is and should be object agnostic, the implementation
> of user namespaces (currently) is such that the whole view of the user's
> privilege must include information which is stored with the object.
>
> There are various user namespaces.
>
> The Linux capabilities ( :-) ) model is user namespaced.  It must be, in
> order to be useful.  If we're going to use file capabilities in distros,
> and distros are going to run in containers, then the capabilities must
> be namespaced.  Otherwise, capabilities will not be used, and heck, should
> just be dropped.
>
> The only way to find out which user namespace has privilege over an inode
> is to look at the inode.
>
> Therefore, object information is needed.

Agreed.  The concept in the kernel is "capability over a namespace."

That being said, sticking a flexible object type into ns_capable()
seems prematurely general to me.  How about adding
security_capable_wrt_inode_uidgid() and allowing LSMs to hook that?
The current implementation would go into commoncap.  The obvious
extensions I can think of are security_dac_read_search(..., inode,
...) and security_dac_override(..., inode, ...).  (Or dentry or
whatever is appropriate.)

If this patch were restructured like that, the semantics would be
obvious, and it would arguably be a genuine cleanup instead of a whole
new mechanism of unknown scope.

^ permalink raw reply

* Re: [RFC PATCH] security,capability: pass object information to security_capable
From: Serge E. Hallyn @ 2019-07-16 14:16 UTC (permalink / raw)
  To: Nicholas Franck
  Cc: paul, selinux, linux-security-module, luto, jmorris, sds,
	keescook, serge, john.johansen, casey, mortonm
In-Reply-To: <20190712173404.14417-1-nhfran2@tycho.nsa.gov>

On Fri, Jul 12, 2019 at 01:34:04PM -0400, Nicholas Franck wrote:
> At present security_capable does not pass any object information
> and therefore can neither audit the particular object nor take it
> into account. Augment the security_capable interface to support
> passing supplementary data. Use this facility initially to convey
> the inode for capability checks relevant to inodes. This only
> addresses capable_wrt_inode_uidgid calls; other capability checks
> relevant to inodes will be addressed in subsequent changes. In the
> future, this will be further extended to pass object information for
> other capability checks such as the target task for CAP_KILL.
> 
> In SELinux this new information is leveraged here to include the inode
> in the audit message. In the future, it could also be used to perform
> a per inode capability checks.
> 
> It would be possible to fold the existing opts argument into this new
> supplementary data structure. This was omitted from this change to
> minimize changes.
> 
> Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>

One comment below, but overall

Acked-by: Serge Hallyn <serge@hallyn.com>

> ---
>  include/linux/capability.h             |  7 ++++++
>  include/linux/lsm_audit.h              |  5 +++-
>  include/linux/lsm_hooks.h              |  3 ++-
>  include/linux/security.h               | 23 +++++++++++++-----
>  kernel/capability.c                    | 33 ++++++++++++++++++--------
>  kernel/seccomp.c                       |  2 +-
>  security/apparmor/capability.c         |  8 ++++---
>  security/apparmor/include/capability.h |  4 +++-
>  security/apparmor/ipc.c                |  2 +-
>  security/apparmor/lsm.c                |  5 ++--
>  security/apparmor/resource.c           |  2 +-
>  security/commoncap.c                   | 11 +++++----
>  security/lsm_audit.c                   | 21 ++++++++++++++--
>  security/safesetid/lsm.c               |  3 ++-
>  security/security.c                    |  5 ++--
>  security/selinux/hooks.c               | 20 +++++++++-------
>  security/smack/smack_access.c          |  2 +-
>  17 files changed, 110 insertions(+), 46 deletions(-)
> 
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index ecce0f43c73a..f72de64c179d 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -211,6 +211,8 @@ extern bool capable(int cap);
>  extern bool ns_capable(struct user_namespace *ns, int cap);
>  extern bool ns_capable_noaudit(struct user_namespace *ns, int cap);
>  extern bool ns_capable_setid(struct user_namespace *ns, int cap);
> +extern bool ns_capable_inode(struct user_namespace *ns, int cap,
> +			     const struct inode *inode);
>  #else
>  static inline bool has_capability(struct task_struct *t, int cap)
>  {
> @@ -246,6 +248,11 @@ static inline bool ns_capable_setid(struct user_namespace *ns, int cap)
>  {
>  	return true;
>  }
> +static bool ns_capable_inode(struct user_namespace *ns, int cap,
> +			     const struct inode *inode)
> +{
> +	return true;
> +}
>  #endif /* CONFIG_MULTIUSER */
>  extern bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const struct inode *inode);
>  extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap);
> diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
> index 915330abf6e5..15d2a62639f0 100644
> --- a/include/linux/lsm_audit.h
> +++ b/include/linux/lsm_audit.h
> @@ -79,7 +79,10 @@ struct common_audit_data {
>  		struct dentry *dentry;
>  		struct inode *inode;
>  		struct lsm_network_audit *net;
> -		int cap;
> +		struct  {
> +			int cap;
> +			struct cap_aux_data *cad;
> +		} cap_struct;
>  		int ipc_id;
>  		struct task_struct *tsk;
>  #ifdef CONFIG_KEYS
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..b2a37d613030 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1469,7 +1469,8 @@ union security_list_options {
>  	int (*capable)(const struct cred *cred,
>  			struct user_namespace *ns,
>  			int cap,
> -			unsigned int opts);
> +			unsigned int opts,
> +			struct cap_aux_data *cad);
>  	int (*quotactl)(int cmds, int type, int id, struct super_block *sb);
>  	int (*quota_on)(struct dentry *dentry);
>  	int (*syslog)(int type);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..8fce5e69dc52 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -77,9 +77,18 @@ enum lsm_event {
>  	LSM_POLICY_CHANGE,
>  };
>  
> +
> +struct cap_aux_data {
> +	char type;
> +#define CAP_AUX_DATA_INODE	1
> +	union	{
> +		const struct inode *inode;
> +	} u;
> +};
> +
>  /* These functions are in security/commoncap.c */
>  extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> -		       int cap, unsigned int opts);
> +		       int cap, unsigned int opts, struct cap_aux_data *cad);
>  extern int cap_settime(const struct timespec64 *ts, const struct timezone *tz);
>  extern int cap_ptrace_access_check(struct task_struct *child, unsigned int mode);
>  extern int cap_ptrace_traceme(struct task_struct *parent);
> @@ -215,9 +224,10 @@ int security_capset(struct cred *new, const struct cred *old,
>  		    const kernel_cap_t *inheritable,
>  		    const kernel_cap_t *permitted);
>  int security_capable(const struct cred *cred,
> -		       struct user_namespace *ns,
> -		       int cap,
> -		       unsigned int opts);
> +		     struct user_namespace *ns,
> +		     int cap,
> +		     unsigned int opts,
> +		     struct cap_aux_data *cad);
>  int security_quotactl(int cmds, int type, int id, struct super_block *sb);
>  int security_quota_on(struct dentry *dentry);
>  int security_syslog(int type);
> @@ -478,9 +488,10 @@ static inline int security_capset(struct cred *new,
>  static inline int security_capable(const struct cred *cred,
>  				   struct user_namespace *ns,
>  				   int cap,
> -				   unsigned int opts)
> +				   unsigned int opts,
> +				   struct cap_aux_data *cad)
>  {
> -	return cap_capable(cred, ns, cap, opts);
> +	return cap_capable(cred, ns, cap, opts, NULL);
>  }
>  
>  static inline int security_quotactl(int cmds, int type, int id,
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 1444f3954d75..c3723443904a 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -297,7 +297,7 @@ bool has_ns_capability(struct task_struct *t,
>  	int ret;
>  
>  	rcu_read_lock();
> -	ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NONE);
> +	ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NONE, NULL);
>  	rcu_read_unlock();
>  
>  	return (ret == 0);
> @@ -338,7 +338,7 @@ bool has_ns_capability_noaudit(struct task_struct *t,
>  	int ret;
>  
>  	rcu_read_lock();
> -	ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NOAUDIT);
> +	ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NOAUDIT, NULL);
>  	rcu_read_unlock();
>  
>  	return (ret == 0);
> @@ -363,7 +363,8 @@ bool has_capability_noaudit(struct task_struct *t, int cap)
>  
>  static bool ns_capable_common(struct user_namespace *ns,
>  			      int cap,
> -			      unsigned int opts)
> +			      unsigned int opts,
> +			      struct cap_aux_data *cad)
>  {
>  	int capable;
>  
> @@ -372,7 +373,7 @@ static bool ns_capable_common(struct user_namespace *ns,
>  		BUG();
>  	}
>  
> -	capable = security_capable(current_cred(), ns, cap, opts);
> +	capable = security_capable(current_cred(), ns, cap, opts, cad);
>  	if (capable == 0) {
>  		current->flags |= PF_SUPERPRIV;
>  		return true;
> @@ -393,7 +394,7 @@ static bool ns_capable_common(struct user_namespace *ns,
>   */
>  bool ns_capable(struct user_namespace *ns, int cap)
>  {
> -	return ns_capable_common(ns, cap, CAP_OPT_NONE);
> +	return ns_capable_common(ns, cap, CAP_OPT_NONE, NULL);
>  }
>  EXPORT_SYMBOL(ns_capable);
>  
> @@ -411,7 +412,7 @@ EXPORT_SYMBOL(ns_capable);
>   */
>  bool ns_capable_noaudit(struct user_namespace *ns, int cap)
>  {
> -	return ns_capable_common(ns, cap, CAP_OPT_NOAUDIT);
> +	return ns_capable_common(ns, cap, CAP_OPT_NOAUDIT, NULL);
>  }
>  EXPORT_SYMBOL(ns_capable_noaudit);
>  
> @@ -430,7 +431,7 @@ EXPORT_SYMBOL(ns_capable_noaudit);
>   */
>  bool ns_capable_setid(struct user_namespace *ns, int cap)
>  {
> -	return ns_capable_common(ns, cap, CAP_OPT_INSETID);
> +	return ns_capable_common(ns, cap, CAP_OPT_INSETID, NULL);
>  }
>  EXPORT_SYMBOL(ns_capable_setid);
>  
> @@ -470,7 +471,7 @@ bool file_ns_capable(const struct file *file, struct user_namespace *ns,
>  	if (WARN_ON_ONCE(!cap_valid(cap)))
>  		return false;
>  
> -	if (security_capable(file->f_cred, ns, cap, CAP_OPT_NONE) == 0)
> +	if (security_capable(file->f_cred, ns, cap, CAP_OPT_NONE, NULL) == 0)
>  		return true;
>  
>  	return false;
> @@ -503,7 +504,8 @@ bool capable_wrt_inode_uidgid(const struct inode *inode, int cap)
>  {
>  	struct user_namespace *ns = current_user_ns();
>  
> -	return ns_capable(ns, cap) && privileged_wrt_inode_uidgid(ns, inode);
> +	return ns_capable_inode(ns, cap, inode) &&
> +		privileged_wrt_inode_uidgid(ns, inode);
>  }
>  EXPORT_SYMBOL(capable_wrt_inode_uidgid);
>  
> @@ -524,7 +526,18 @@ bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns)
>  	cred = rcu_dereference(tsk->ptracer_cred);
>  	if (cred)
>  		ret = security_capable(cred, ns, CAP_SYS_PTRACE,
> -				       CAP_OPT_NOAUDIT);
> +				       CAP_OPT_NOAUDIT, NULL);
>  	rcu_read_unlock();
>  	return (ret == 0);
>  }
> +
> +bool ns_capable_inode(struct user_namespace *ns, int cap,
> +			const struct inode *inode)
> +{
> +	struct cap_aux_data cad;
> +
> +	cad.type = CAP_AUX_DATA_INODE;
> +	cad.u.inode = inode;
> +	return ns_capable_common(ns, cap, CAP_OPT_NONE, &cad);
> +}
> +EXPORT_SYMBOL(ns_capable_inode);
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 811b4a86cdf6..d59dd7079ece 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -446,7 +446,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
>  	 */
>  	if (!task_no_new_privs(current) &&
>  	    security_capable(current_cred(), current_user_ns(),
> -				     CAP_SYS_ADMIN, CAP_OPT_NOAUDIT) != 0)
> +			     CAP_SYS_ADMIN, CAP_OPT_NOAUDIT, NULL) != 0)
>  		return ERR_PTR(-EACCES);
>  
>  	/* Allocate a new seccomp_filter */
> diff --git a/security/apparmor/capability.c b/security/apparmor/capability.c
> index 752f73980e30..c45192a16733 100644
> --- a/security/apparmor/capability.c
> +++ b/security/apparmor/capability.c
> @@ -50,7 +50,7 @@ static void audit_cb(struct audit_buffer *ab, void *va)
>  	struct common_audit_data *sa = va;
>  
>  	audit_log_format(ab, " capname=");
> -	audit_log_untrustedstring(ab, capability_names[sa->u.cap]);
> +	audit_log_untrustedstring(ab, capability_names[sa->u.cap_struct.cap]);
>  }
>  
>  /**
> @@ -148,13 +148,15 @@ static int profile_capable(struct aa_profile *profile, int cap,
>   *
>   * Returns: 0 on success, or else an error code.
>   */
> -int aa_capable(struct aa_label *label, int cap, unsigned int opts)
> +int aa_capable(struct aa_label *label, int cap, unsigned int opts,
> +	       struct cap_aux_data *cad)
>  {
>  	struct aa_profile *profile;
>  	int error = 0;
>  	DEFINE_AUDIT_DATA(sa, LSM_AUDIT_DATA_CAP, OP_CAPABLE);
>  
> -	sa.u.cap = cap;
> +	sa.u.cap_struct.cap = cap;
> +	sa.u.cap_struct.cad = cad;
>  	error = fn_for_each_confined(label, profile,
>  			profile_capable(profile, cap, opts, &sa));
>  
> diff --git a/security/apparmor/include/capability.h b/security/apparmor/include/capability.h
> index 1b3663b6ab12..d888f09d76d1 100644
> --- a/security/apparmor/include/capability.h
> +++ b/security/apparmor/include/capability.h
> @@ -20,6 +20,7 @@
>  #include "apparmorfs.h"
>  
>  struct aa_label;
> +struct cap_aux_data;
>  
>  /* aa_caps - confinement data for capabilities
>   * @allowed: capabilities mask
> @@ -40,7 +41,8 @@ struct aa_caps {
>  
>  extern struct aa_sfs_entry aa_sfs_entry_caps[];
>  
> -int aa_capable(struct aa_label *label, int cap, unsigned int opts);
> +int aa_capable(struct aa_label *label, int cap, unsigned int opts,
> +	       struct cap_aux_data *cad);
>  
>  static inline void aa_free_cap_rules(struct aa_caps *caps)
>  {
> diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
> index aacd1e95cb59..deb5267ca695 100644
> --- a/security/apparmor/ipc.c
> +++ b/security/apparmor/ipc.c
> @@ -108,7 +108,7 @@ static int profile_tracer_perm(struct aa_profile *tracer,
>  	aad(sa)->peer = tracee;
>  	aad(sa)->request = 0;
>  	aad(sa)->error = aa_capable(&tracer->label, CAP_SYS_PTRACE,
> -				    CAP_OPT_NONE);
> +				    CAP_OPT_NONE, NULL);
>  
>  	return aa_audit(AUDIT_APPARMOR_AUTO, tracer, sa, audit_ptrace_cb);
>  }
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 87500bde5a92..82790accb679 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -172,14 +172,15 @@ static int apparmor_capget(struct task_struct *target, kernel_cap_t *effective,
>  }
>  
>  static int apparmor_capable(const struct cred *cred, struct user_namespace *ns,
> -			    int cap, unsigned int opts)
> +			    int cap, unsigned int opts,
> +			    struct cap_aux_data *cad)
>  {
>  	struct aa_label *label;
>  	int error = 0;
>  
>  	label = aa_get_newest_cred_label(cred);
>  	if (!unconfined(label))
> -		error = aa_capable(label, cap, opts);
> +		error = aa_capable(label, cap, opts, cad);
>  	aa_put_label(label);
>  
>  	return error;
> diff --git a/security/apparmor/resource.c b/security/apparmor/resource.c
> index 552ed09cb47e..9b3d4b4437f2 100644
> --- a/security/apparmor/resource.c
> +++ b/security/apparmor/resource.c
> @@ -124,7 +124,7 @@ int aa_task_setrlimit(struct aa_label *label, struct task_struct *task,
>  	 */
>  
>  	if (label != peer &&
> -	    aa_capable(label, CAP_SYS_RESOURCE, CAP_OPT_NOAUDIT) != 0)
> +	    aa_capable(label, CAP_SYS_RESOURCE, CAP_OPT_NOAUDIT, NULL) != 0)
>  		error = fn_for_each(label, profile,
>  				audit_resource(profile, resource,
>  					       new_rlim->rlim_max, peer,
> diff --git a/security/commoncap.c b/security/commoncap.c
> index c477fb673701..1860ea50f473 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -68,7 +68,7 @@ static void warn_setuid_and_fcaps_mixed(const char *fname)
>   * kernel's capable() and has_capability() returns 1 for this case.
>   */
>  int cap_capable(const struct cred *cred, struct user_namespace *targ_ns,
> -		int cap, unsigned int opts)
> +		int cap, unsigned int opts, struct cap_aux_data *cad)
>  {
>  	struct user_namespace *ns = targ_ns;
>  
> @@ -226,7 +226,7 @@ static inline int cap_inh_is_capped(void)
>  	 * capability
>  	 */
>  	if (cap_capable(current_cred(), current_cred()->user_ns,
> -			CAP_SETPCAP, CAP_OPT_NONE) == 0)
> +			CAP_SETPCAP, CAP_OPT_NONE, NULL) == 0)
>  		return 0;
>  	return 1;
>  }
> @@ -1211,7 +1211,8 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
>  		    || (cap_capable(current_cred(),
>  				    current_cred()->user_ns,
>  				    CAP_SETPCAP,
> -				    CAP_OPT_NONE) != 0)			/*[4]*/
> +				    CAP_OPT_NONE,
> +				    NULL) != 0)				/*[4]*/
>  			/*
>  			 * [1] no changing of bits that are locked
>  			 * [2] no unlocking of locks
> @@ -1307,7 +1308,7 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages)
>  	int cap_sys_admin = 0;
>  
>  	if (cap_capable(current_cred(), &init_user_ns,
> -				CAP_SYS_ADMIN, CAP_OPT_NOAUDIT) == 0)
> +				CAP_SYS_ADMIN, CAP_OPT_NOAUDIT, NULL) == 0)
>  		cap_sys_admin = 1;
>  
>  	return cap_sys_admin;
> @@ -1328,7 +1329,7 @@ int cap_mmap_addr(unsigned long addr)
>  
>  	if (addr < dac_mmap_min_addr) {
>  		ret = cap_capable(current_cred(), &init_user_ns, CAP_SYS_RAWIO,
> -				  CAP_OPT_NONE);
> +				  CAP_OPT_NONE, NULL);
>  		/* set PF_SUPERPRIV if it turns out we allow the low mmap */
>  		if (ret == 0)
>  			current->flags |= PF_SUPERPRIV;
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 33028c098ef3..4871b2508a4a 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -229,9 +229,26 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>  	case LSM_AUDIT_DATA_IPC:
>  		audit_log_format(ab, " key=%d ", a->u.ipc_id);
>  		break;
> -	case LSM_AUDIT_DATA_CAP:
> -		audit_log_format(ab, " capability=%d ", a->u.cap);
> +	case LSM_AUDIT_DATA_CAP: {
> +		const struct inode *inode;
> +
> +		if (a->u.cap_struct.cad) {
> +			switch (a->u.cap_struct.cad->type) {
> +			case CAP_AUX_DATA_INODE: {

Putting a { after the 'case:' seems weird to me, and leads to the suspicion
arousing two brackets in a row on same indent level down below.

> +				inode = a->u.cap_struct.cad->u.inode;
> +
> +				audit_log_format(ab, " dev=");
> +				audit_log_untrustedstring(ab,
> +					inode->i_sb->s_id);
> +				audit_log_format(ab, " ino=%lu",
> +					inode->i_ino);
> +				break;
> +			}
> +			}
> +		}
> +		audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
>  		break;
> +	}
>  	case LSM_AUDIT_DATA_PATH: {
>  		struct inode *inode;
>  

^ permalink raw reply

* Re: [RFC PATCH] security, capability: pass object information to security_capable
From: Serge E. Hallyn @ 2019-07-16 14:03 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Stephen Smalley, Nicholas Franck, paul, selinux,
	linux-security-module, luto, jmorris, keescook, serge,
	john.johansen, mortonm
In-Reply-To: <16cade37-9467-ca7f-ddea-b8254c501f48@schaufler-ca.com>

On Fri, Jul 12, 2019 at 12:54:02PM -0700, Casey Schaufler wrote:
> On 7/12/2019 11:25 AM, Stephen Smalley wrote:
> > On 7/12/19 1:58 PM, Casey Schaufler wrote:
> >> On 7/12/2019 10:34 AM, Nicholas Franck wrote:
> >>> At present security_capable does not pass any object information
> >>> and therefore can neither audit the particular object nor take it
> >>> into account. Augment the security_capable interface to support
> >>> passing supplementary data. Use this facility initially to convey
> >>> the inode for capability checks relevant to inodes. This only
> >>> addresses capable_wrt_inode_uidgid calls; other capability checks
> >>> relevant to inodes will be addressed in subsequent changes. In the
> >>> future, this will be further extended to pass object information for
> >>> other capability checks such as the target task for CAP_KILL.
> >>
> >> This seems wrong to me. The capability system has nothing to do
> >> with objects. Passing object information through security_capable()
> >> may be convenient, but isn't relevant to the purpose of the interface.
> >> It appears that there are very few places where the object information
> >> is actually useful.
> >
> > A fair number of capabilities are checked upon some attempted object access (often right after comparing UIDs or other per-object state), and the particular object can be very helpful in both audit and in access control.  More below.
> 
> I'm not disagreeing with that. What I'm saying is that the capability
> check interface is not the right place to pass that information. The
> capability check has no use for the object information. I would much

I've had to argue this before while doing the namespaced file
capabilities implementation.  Perhaps this would be worth writing something
more formal about.  My main argument is, even if we want to claim that the
capabilities model is and should be object agnostic, the implementation
of user namespaces (currently) is such that the whole view of the user's
privilege must include information which is stored with the object.

There are various user namespaces.

The Linux capabilities ( :-) ) model is user namespaced.  It must be, in
order to be useful.  If we're going to use file capabilities in distros,
and distros are going to run in containers, then the capabilities must
be namespaced.  Otherwise, capabilities will not be used, and heck, should
just be dropped.

The only way to find out which user namespace has privilege over an inode
is to look at the inode.

Therefore, object information is needed.

Until now we've sneaked around that by doing things like capable_wrt_inode_uidgid()
and rootid_from_xattr().

Again, this crucial: IMO, you have to be able to use a distro the same way in a
container and not.  Either we support using capabilities in a user namespaced
container, or we drop capabilities support will not be used, and we may as
well drop the module.

Now, yes, if someone tries to extend this stuff to do pathname parsing, then we
might have to put our footsies down.  But we've been dancing around this for a
long time anyway, so passing the inode in so we can do better logging gets a +1
from me.

-serge

^ permalink raw reply

* Re: [RFC PATCH v6 0/1] Add dm verity root hash pkcs7 sig validation.
From: Milan Broz @ 2019-07-16 12:59 UTC (permalink / raw)
  To: Jaskaran Singh Khurana
  Cc: ebiggers, linux-security-module, linux-kernel, linux-integrity,
	linux-fsdevel, agk, snitzer, dm-devel, jmorris, Scott Shell,
	Nazmus Sakib, mpatocka
In-Reply-To: <alpine.LRH.2.21.1907121025510.66082@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.inter>

On 12/07/2019 19:33, Jaskaran Singh Khurana wrote:
> 
> Hello Milan,
> 
>> Changes in v6:
>>
>> Address comments from Milan Broz and Eric Biggers on v5.
>>
>> -Keep the verification code under config DM_VERITY_VERIFY_ROOTHASH_SIG.
>>
>> -Change the command line parameter to requires_signatures(bool) which will
>> force root hash to be signed and trusted if specified.
>>
>> -Fix the signature not being present in verity_status. Merged the
>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fmbroz%2Flinux.git%2Fcommit%2F%3Fh%3Ddm-cryptsetup%26id%3Da26c10806f5257e255b6a436713127e762935ad3&amp;data=02%7C01%7CJaskaran.Khurana%40microsoft.com%7C18f92445e46940aeebb008d6fe50c610%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636976020210890638&amp;sdata=aY0V9%2FBz2RHryIvoftGKUGnyPp9Fsc1JY4FZbHfW4hg%3D&amp;reserved=0
>> made by Milan Broz and tested it.
>>
>>
> 
> Could you please provide feedback on this v6 version.

Hi,

I am ok with the v6 patch; I think Mike will return to it in 5.4 reviews.

But the documentation is very brief. I spent quite a long time to configure the system properly.
I think you should add more description (at least to patch header) how to use this feature in combination with system keyring.

Do I understand correctly that these steps need to be done?

 - user configures a certificate and adds it in kernel builtin keyring (I used CONFIG_SYSTEM_TRUSTED_KEYS option).
 - the dm-verity device root hash is signed directly by a key of this cert
 - the signature is uploaded to the user keyring
 - reference to signature in keyring is added as an optional dm-verity table parameter root_hash_sig_key_desc
 - optionally, require_signatures dm-verity module is set to enforce signatures.

For reference, below is the bash script I used (with unpatched veritysetup to generate working DM table), is the expected workflow here?

#!/bin/bash

NAME=test
DEV=/dev/sdc
DEV_HASH=/dev/sdd
ROOT_HASH=778fccab393842688c9af89cfd0c5cde69377cbe21ed439109ec856f2aa8a423
SIGN=sign.txt
SIGN_NAME=verity:$NAME

# get unsigned device-mapper table using unpatched veritysetup
veritysetup open $DEV $NAME $DEV_HASH $ROOT_HASH
TABLE=$(dmsetup table $NAME)
veritysetup close $NAME

# Generate self-signed CA key, must be in .config as CONFIG_SYSTEM_TRUSTED_KEYS="path/ca.pem"
#openssl req -x509 -newkey rsa:1024 -keyout ca_key.pem -out ca.pem -nodes -days 365 -set_serial 01 -subj /CN=example.com

# sign root hash directly by CA cert
echo -n $ROOT_HASH | openssl smime -sign -nocerts -noattr -binary -inkey ca_key.pem -signer ca.pem -outform der -out $SIGN

# load signature to keyring
keyctl padd user $SIGN_NAME @u <$SIGN

# add device-mapper table, now with sighed root hash optional argument
dmsetup create -r $NAME --table "$TABLE 2 root_hash_sig_key_desc $SIGN_NAME"
dmsetup table $NAME

# cleanup
dmsetup remove $NAME
keyctl clear @u


Milan

^ 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