* Re: [PATCH v2 11/12] ima: Introduce template field evmsig and write to field sig as fallback
From: Mimi Zohar @ 2020-09-17 14:25 UTC (permalink / raw)
To: Roberto Sassu, mjg59
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu
In-Reply-To: <20200904092643.20013-7-roberto.sassu@huawei.com>
Hi Roberto,
On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> With the patch to accept EVM portable signatures when the
> appraise_type=imasig requirement is specified in the policy, appraisal can
> be successfully done even if the file does not have an IMA signature.
>
> However, remote attestation would not see that a different signature type
> was used, as only IMA signatures can be included in the measurement list.
> This patch solves the issue by introducing the new template field 'evmsig'
> to show EVM portable signatures and by including its value in the existing
> field 'sig' if the IMA signature is not found.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Thank you! Just a minor comment below.
<snip>
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index c022ee9e2a4e..2c596c2a89cc 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
>
> @@ -438,7 +439,7 @@ int ima_eventsig_init(struct ima_event_data *event_data,
> struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
>
> if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
> - return 0;
> + return ima_eventevmsig_init(event_data, field_data);
>
> return ima_write_template_field_data(xattr_value, event_data->xattr_len,
> DATA_FMT_HEX, field_data);
> @@ -484,3 +485,39 @@ int ima_eventmodsig_init(struct ima_event_data *event_data,
> return ima_write_template_field_data(data, data_len, DATA_FMT_HEX,
> field_data);
> }
> +
> +/*
> + * ima_eventevmsig_init - include the EVM portable signature as part of the
> + * template data
> + */
> +int ima_eventevmsig_init(struct ima_event_data *event_data,
> + struct ima_field_data *field_data)
> +{
> + struct evm_ima_xattr_data *xattr_data = NULL;
> + int rc = 0;
> +
> + if (!event_data->file)
> + return 0;
> +
> + if (!(file_inode(event_data->file)->i_opflags & IOP_XATTR))
> + return 0;
> +
> + rc = vfs_getxattr_alloc(file_dentry(event_data->file), XATTR_NAME_EVM,
> + (char **)&xattr_data, 0, GFP_NOFS);
> + if (rc <= 0) {
> + if (!rc || rc == -ENODATA)
> + return 0;
> +
> + return rc;
We're including the EVM signature on a best effort basis to help with
attestation. Do we really care why it failed? Are we going to act on
it?
Mimi
> + }
> +
> + if (xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) {
> + kfree(xattr_data);
> + return 0;
> + }
> +
> + rc = ima_write_template_field_data((char *)xattr_data, rc, DATA_FMT_HEX,
> + field_data);
> + kfree(xattr_data);
> + return rc;
> +}
^ permalink raw reply
* RE: [PATCH v2 11/12] ima: Introduce template field evmsig and write to field sig as fallback
From: Roberto Sassu @ 2020-09-17 15:05 UTC (permalink / raw)
To: Mimi Zohar, mjg59@google.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <c8d3c70e74e607a4b73239bef1e9db0d304200fc.camel@linux.ibm.com>
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, September 17, 2020 4:25 PM
> Hi Roberto,
>
> On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> > With the patch to accept EVM portable signatures when the
> > appraise_type=imasig requirement is specified in the policy, appraisal can
> > be successfully done even if the file does not have an IMA signature.
> >
> > However, remote attestation would not see that a different signature
> type
> > was used, as only IMA signatures can be included in the measurement list.
> > This patch solves the issue by introducing the new template field 'evmsig'
> > to show EVM portable signatures and by including its value in the existing
> > field 'sig' if the IMA signature is not found.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
>
> Thank you! Just a minor comment below.
>
> <snip>
>
> > diff --git a/security/integrity/ima/ima_template_lib.c
> b/security/integrity/ima/ima_template_lib.c
> > index c022ee9e2a4e..2c596c2a89cc 100644
> > --- a/security/integrity/ima/ima_template_lib.c
> > +++ b/security/integrity/ima/ima_template_lib.c
> >
> > @@ -438,7 +439,7 @@ int ima_eventsig_init(struct ima_event_data
> *event_data,
> > struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
> >
> > if ((!xattr_value) || (xattr_value->type !=
> EVM_IMA_XATTR_DIGSIG))
> > - return 0;
> > + return ima_eventevmsig_init(event_data, field_data);
> >
> > return ima_write_template_field_data(xattr_value, event_data-
> >xattr_len,
> > DATA_FMT_HEX, field_data);
> > @@ -484,3 +485,39 @@ int ima_eventmodsig_init(struct ima_event_data
> *event_data,
> > return ima_write_template_field_data(data, data_len,
> DATA_FMT_HEX,
> > field_data);
> > }
> > +
> > +/*
> > + * ima_eventevmsig_init - include the EVM portable signature as part of
> the
> > + * template data
> > + */
> > +int ima_eventevmsig_init(struct ima_event_data *event_data,
> > + struct ima_field_data *field_data)
> > +{
> > + struct evm_ima_xattr_data *xattr_data = NULL;
> > + int rc = 0;
> > +
> > + if (!event_data->file)
> > + return 0;
> > +
> > + if (!(file_inode(event_data->file)->i_opflags & IOP_XATTR))
> > + return 0;
> > +
> > + rc = vfs_getxattr_alloc(file_dentry(event_data->file),
> XATTR_NAME_EVM,
> > + (char **)&xattr_data, 0, GFP_NOFS);
> > + if (rc <= 0) {
> > + if (!rc || rc == -ENODATA)
> > + return 0;
> > +
> > + return rc;
>
> We're including the EVM signature on a best effort basis to help with
> attestation. Do we really care why it failed? Are we going to act on
> it?
Hi Mimi
other template field functions have a similar behavior. They return
an error if an operation necessary to retrieve the data cannot be
performed. Should I always return 0?
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> Mimi
>
> > + }
> > +
> > + if (xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) {
> > + kfree(xattr_data);
> > + return 0;
> > + }
> > +
> > + rc = ima_write_template_field_data((char *)xattr_data, rc,
> DATA_FMT_HEX,
> > + field_data);
> > + kfree(xattr_data);
> > + return rc;
> > +}
>
^ permalink raw reply
* Re: [PATCH v2 00/12] IMA/EVM fixes
From: Mimi Zohar @ 2020-09-17 14:33 UTC (permalink / raw)
To: Roberto Sassu, mjg59
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu
In-Reply-To: <681455532936011e3dc5e0050fddc2c3a3a7fc3c.camel@linux.ibm.com>
Hi Roberto,
On Wed, 2020-09-16 at 12:14 -0400, Mimi Zohar wrote:
> On Fri, 2020-09-04 at 11:23 +0200, Roberto Sassu wrote:
> > This patch set includes various fixes for IMA and EVM.
> >
> > Patches 1-3 are trivial fixes.
>
> I've queued these patches in the next-integrity-testing branch for now.
> When reposting this patch set please replace the cover letter subject
> line with a more appropriate one.
>
> > The remaining improve support and usability
> > of EVM portable signatures. In particular patch 4 allows EVM to be used
> > without an HMAC key.
>
> EVM already supports using EVM portable and immutable sigatures without
> an HMAC key.
>
> I was able to apply this patch set, but patch 10/12 does not apply
> cleanly due to the "fallthrough" line. Please hold off on reposting,
> until I've finished reviewing the entire patch set.
Done. Other than the one issue of clearing the EVM_RESET_STATUS in
evm_verifyxattr(), the remaining changes are straight forward.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v2 0/4] [RFC] Implement Trampoline File Descriptor
From: Madhavan T. Venkataraman @ 2020-09-17 15:36 UTC (permalink / raw)
To: Florian Weimer
Cc: kernel-hardening, linux-api, linux-arm-kernel, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module, oleg, x86,
libffi-discuss
In-Reply-To: <87v9gdz01h.fsf@mid.deneb.enyo.de>
On 9/16/20 8:04 PM, Florian Weimer wrote:
> * madvenka:
>
>> Examples of trampolines
>> =======================
>>
>> libffi (A Portable Foreign Function Interface Library):
>>
>> libffi allows a user to define functions with an arbitrary list of
>> arguments and return value through a feature called "Closures".
>> Closures use trampolines to jump to ABI handlers that handle calling
>> conventions and call a target function. libffi is used by a lot
>> of different applications. To name a few:
>>
>> - Python
>> - Java
>> - Javascript
>> - Ruby FFI
>> - Lisp
>> - Objective C
>
> libffi does not actually need this. It currently collocates
> trampolines and the data they need on the same page, but that's
> actually unecessary. It's possible to avoid doing this just by
> changing libffi, without any kernel changes.
>
> I think this has already been done for the iOS port.
>
The trampoline table that has been implemented for the iOS port (MACH)
is based on PC-relative data referencing. That is, the code and data
are placed in adjacent pages so that the code can access the data using
an address relative to the current PC.
This is an ISA feature that is not supported on all architectures.
Now, if it is a performance feature, we can include some architectures
and exclude others. But this is a security feature. IMO, we cannot
exclude any architecture even if it is a legacy one as long as Linux
is running on the architecture. So, we need a solution that does
not assume any specific ISA feature.
>> The code for trampoline X in the trampoline table is:
>>
>> load &code_table[X], code_reg
>> load (code_reg), code_reg
>> load &data_table[X], data_reg
>> load (data_reg), data_reg
>> jump code_reg
>>
>> The addresses &code_table[X] and &data_table[X] are baked into the
>> trampoline code. So, PC-relative data references are not needed. The user
>> can modify code_table[X] and data_table[X] dynamically.
>
> You can put this code into the libffi shared object and map it from
> there, just like the rest of the libffi code. To get more
> trampolines, you can map the page containing the trampolines multiple
> times, each instance preceded by a separate data page with the control
> information.
>
If you put the code in the libffi shared object, how do you pass data to
the code at runtime? If the code we are talking about is a function, then
there is an ABI defined way to pass data to the function. But if the
code we are talking about is some arbitrary code such as a trampoline,
there is no ABI defined way to pass data to it except in a couple of
platforms such as HP PA-RISC that have support for function descriptors
in the ABI itself.
As mentioned before, if the ISA supports PC-relative data references
(e.g., X86 64-bit platforms support RIP-relative data references)
then we can pass data to that code by placing the code and data in
adjacent pages. So, you can implement the trampoline table for X64.
i386 does not support it.
> I think the previous patch submission has also resulted in several
> comments along those lines, so I'm not sure why you are reposting
> this.
IIRC, I have answered all of those comments by mentioning the point
that we need to support all architectures without requiring special
ISA features. Taking the kernel's help in this is one solution.
>
>> libffi
>> ======
>>
>> I have implemented my solution for libffi and provided the changes for
>> X86 and ARM, 32-bit and 64-bit. Here is the reference patch:
>>
>> http://linux.microsoft.com/~madvenka/libffi/libffi.v2.txt
>
> The URL does not appear to work, I get a 403 error.
I apologize for that. That site is supposed to be accessible publicly.
I will contact the administrator and get this resolved.
Sorry for the annoyance.
>
>> If the trampfd patchset gets accepted, I will send the libffi changes
>> to the maintainers for a review. BTW, I have also successfully executed
>> the libffi self tests.
>
> I have not seen your libffi changes, but I expect that the complexity
> is about the same as a userspace-only solution.
>
>
I agree. The complexity is about the same. But the support is for all
architectures. Once the common code is in place, the changes for each
architecture are trivial.
Madhavan
> Cc:ing libffi upstream for awareness. The start of the thread is
> here:
>
> <https://lore.kernel.org/linux-api/20200916150826.5990-1-madvenka@linux.microsoft.com/>
>
^ permalink raw reply
* Re: [PATCH v2 11/12] ima: Introduce template field evmsig and write to field sig as fallback
From: Mimi Zohar @ 2020-09-17 15:55 UTC (permalink / raw)
To: Roberto Sassu, mjg59@google.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <860d8441788b4ff799db738e535e2d7e@huawei.com>
On Thu, 2020-09-17 at 15:05 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Thursday, September 17, 2020 4:25 PM
> > Hi Roberto,
> >
> > On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> > > With the patch to accept EVM portable signatures when the
> > > appraise_type=imasig requirement is specified in the policy, appraisal can
> > > be successfully done even if the file does not have an IMA signature.
> > >
> > > However, remote attestation would not see that a different signature
> > type
> > > was used, as only IMA signatures can be included in the measurement list.
> > > This patch solves the issue by introducing the new template field 'evmsig'
> > > to show EVM portable signatures and by including its value in the existing
> > > field 'sig' if the IMA signature is not found.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> >
> > Thank you! Just a minor comment below.
> >
> > <snip>
> >
> > > diff --git a/security/integrity/ima/ima_template_lib.c
> > b/security/integrity/ima/ima_template_lib.c
> > > index c022ee9e2a4e..2c596c2a89cc 100644
> > > --- a/security/integrity/ima/ima_template_lib.c
> > > +++ b/security/integrity/ima/ima_template_lib.c
> > >
> > > @@ -438,7 +439,7 @@ int ima_eventsig_init(struct ima_event_data
> > *event_data,
> > > struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
> > >
> > > if ((!xattr_value) || (xattr_value->type !=
> > EVM_IMA_XATTR_DIGSIG))
> > > - return 0;
> > > + return ima_eventevmsig_init(event_data, field_data);
> > >
> > > return ima_write_template_field_data(xattr_value, event_data-
> > >xattr_len,
> > > DATA_FMT_HEX, field_data);
> > > @@ -484,3 +485,39 @@ int ima_eventmodsig_init(struct ima_event_data
> > *event_data,
> > > return ima_write_template_field_data(data, data_len,
> > DATA_FMT_HEX,
> > > field_data);
> > > }
> > > +
> > > +/*
> > > + * ima_eventevmsig_init - include the EVM portable signature as part of
> > the
> > > + * template data
> > > + */
> > > +int ima_eventevmsig_init(struct ima_event_data *event_data,
> > > + struct ima_field_data *field_data)
> > > +{
> > > + struct evm_ima_xattr_data *xattr_data = NULL;
> > > + int rc = 0;
> > > +
> > > + if (!event_data->file)
> > > + return 0;
> > > +
> > > + if (!(file_inode(event_data->file)->i_opflags & IOP_XATTR))
> > > + return 0;
> > > +
> > > + rc = vfs_getxattr_alloc(file_dentry(event_data->file),
> > XATTR_NAME_EVM,
> > > + (char **)&xattr_data, 0, GFP_NOFS);
> > > + if (rc <= 0) {
> > > + if (!rc || rc == -ENODATA)
> > > + return 0;
> > > +
> > > + return rc;
> >
> > We're including the EVM signature on a best effort basis to help with
> > attestation. Do we really care why it failed? Are we going to act on
> > it?
>
> Hi Mimi
>
> other template field functions have a similar behavior. They return
> an error if an operation necessary to retrieve the data cannot be
> performed. Should I always return 0?
The EVM signature case is more similar to the IMA signature case, than
to other fields. In the signature cases, if the signature exists, it
is included. My suggestion is based on the difference in how the
vfs_getxattr_alloc() results are handled.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v2 0/4] [RFC] Implement Trampoline File Descriptor
From: Florian Weimer @ 2020-09-17 16:01 UTC (permalink / raw)
To: Madhavan T. Venkataraman
Cc: kernel-hardening, linux-api, linux-arm-kernel, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module, oleg, x86,
libffi-discuss
In-Reply-To: <d96b87ed-9869-c732-9938-a1c717a065f3@linux.microsoft.com>
* Madhavan T. Venkataraman:
> On 9/17/20 10:36 AM, Madhavan T. Venkataraman wrote:
>>>> libffi
>>>> ======
>>>>
>>>> I have implemented my solution for libffi and provided the changes for
>>>> X86 and ARM, 32-bit and 64-bit. Here is the reference patch:
>>>>
>>>> http://linux.microsoft.com/~madvenka/libffi/libffi.v2.txt
>>> The URL does not appear to work, I get a 403 error.
>> I apologize for that. That site is supposed to be accessible publicly.
>> I will contact the administrator and get this resolved.
>>
>> Sorry for the annoyance.
> Could you try the link again and confirm that you can access it?
> Again, sorry for the trouble.
Yes, it works now. Thanks for having it fixed.
^ permalink raw reply
* Re: [PATCH v2 0/4] [RFC] Implement Trampoline File Descriptor
From: Madhavan T. Venkataraman @ 2020-09-17 15:57 UTC (permalink / raw)
To: Florian Weimer
Cc: kernel-hardening, linux-api, linux-arm-kernel, linux-fsdevel,
linux-integrity, linux-kernel, linux-security-module, oleg, x86,
libffi-discuss
In-Reply-To: <96ea02df-4154-5888-1669-f3beeed60b33@linux.microsoft.com>
On 9/17/20 10:36 AM, Madhavan T. Venkataraman wrote:
>>> libffi
>>> ======
>>>
>>> I have implemented my solution for libffi and provided the changes for
>>> X86 and ARM, 32-bit and 64-bit. Here is the reference patch:
>>>
>>> http://linux.microsoft.com/~madvenka/libffi/libffi.v2.txt
>> The URL does not appear to work, I get a 403 error.
> I apologize for that. That site is supposed to be accessible publicly.
> I will contact the administrator and get this resolved.
>
> Sorry for the annoyance.
>
Could you try the link again and confirm that you can access it?
Again, sorry for the trouble.
Madhavan
^ permalink raw reply
* Re: [PATCH v6 1/4] KEYS: trusted: Add generic trusted keys framework
From: Jarkko Sakkinen @ 2020-09-17 16:21 UTC (permalink / raw)
To: Sumit Garg
Cc: zohar, jejb, dhowells, jens.wiklander, corbet, jmorris, serge,
casey, janne.karhunen, daniel.thompson, Markus.Wamser, lhinds,
keyrings, linux-integrity, linux-security-module, linux-doc,
linux-kernel, linux-arm-kernel, op-tee
In-Reply-To: <1600350398-4813-2-git-send-email-sumit.garg@linaro.org>
On Thu, Sep 17, 2020 at 07:16:35PM +0530, Sumit Garg wrote:
> Current trusted keys framework is tightly coupled to use TPM device as
> an underlying implementation which makes it difficult for implementations
> like Trusted Execution Environment (TEE) etc. to provide trusted keys
> support in case platform doesn't posses a TPM device.
>
> So this patch tries to add generic trusted keys framework where underlying
> implementations like TPM, TEE etc. could be easily plugged-in.
I would rephrase this a bit:
"Add a generic trusted keys framework where underlying implementations
can be easily plugged in. Create struct trusted_key_ops to achieve this,
which contains necessary functions of a backend."
I remember asking about this approach that what if there was just a
header for trusted key functions and a compile time decision, which C
file to include instead of ops struct. I don't remember if these was a
conclusion on this or not.
E.g. lets say you have a device with TEE and TPM, should you be able
to be use both at run-time? I might play along how this works now but
somehow, in the commit message preferably, it should be conclude why
one alternative is chosen over another.
/Jarkko
^ permalink raw reply
* Re: [PATCH v6 1/4] KEYS: trusted: Add generic trusted keys framework
From: Jarkko Sakkinen @ 2020-09-17 16:25 UTC (permalink / raw)
To: Sumit Garg
Cc: zohar, jejb, dhowells, jens.wiklander, corbet, jmorris, serge,
casey, janne.karhunen, daniel.thompson, Markus.Wamser, lhinds,
keyrings, linux-integrity, linux-security-module, linux-doc,
linux-kernel, linux-arm-kernel, op-tee
In-Reply-To: <20200917162142.GB9750@linux.intel.com>
On Thu, Sep 17, 2020 at 07:21:49PM +0300, Jarkko Sakkinen wrote:
> On Thu, Sep 17, 2020 at 07:16:35PM +0530, Sumit Garg wrote:
> > Current trusted keys framework is tightly coupled to use TPM device as
> > an underlying implementation which makes it difficult for implementations
> > like Trusted Execution Environment (TEE) etc. to provide trusted keys
> > support in case platform doesn't posses a TPM device.
> >
> > So this patch tries to add generic trusted keys framework where underlying
> > implementations like TPM, TEE etc. could be easily plugged-in.
>
> I would rephrase this a bit:
>
> "Add a generic trusted keys framework where underlying implementations
> can be easily plugged in. Create struct trusted_key_ops to achieve this,
> which contains necessary functions of a backend."
>
> I remember asking about this approach that what if there was just a
> header for trusted key functions and a compile time decision, which C
> file to include instead of ops struct. I don't remember if these was a
> conclusion on this or not.
>
> E.g. lets say you have a device with TEE and TPM, should you be able
> to be use both at run-time? I might play along how this works now but
> somehow, in the commit message preferably, it should be conclude why
> one alternative is chosen over another.
We must somehow seal this discussion because the other changes are
based on this decision.
I don't think tail of this patch set takes a long time spin. This
is the main architectural decision.
/Jarkko
^ permalink raw reply
* [PATCH] ip.7: Document IP_PASSSEC for UDP sockets
From: Stephen Smalley @ 2020-09-17 17:31 UTC (permalink / raw)
To: mtk.manpages
Cc: linux-man, linux-security-module, selinux, smcv, jmorris, serge,
paul, Stephen Smalley
Document the IP_PASSSEC socket option and SCM_SECURITY
ancillary/control message type for UDP sockets.
IP_PASSSEC for UDP sockets was introduced in Linux 2.6.17 [1].
Example NetLabel and IPSEC configurations and usage of this option
can be found in the SELinux Notebook [2] and SELinux testsuite [3].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c7946a7bf45ae86736ab3b43d0085e43947945c
[2] https://github.com/SELinuxProject/selinux-notebook
[3] https://github.com/SELinuxProject/selinux-testsuite
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
man7/ip.7 | 48 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 42 insertions(+), 6 deletions(-)
diff --git a/man7/ip.7 b/man7/ip.7
index 03a9f3f7c..681234c90 100644
--- a/man7/ip.7
+++ b/man7/ip.7
@@ -17,11 +17,6 @@
.\" IP_IPSEC_POLICY (2.5.47)
.\" Needs CAP_NET_ADMIN
.\"
-.\" IP_PASSSEC (2.6.17)
-.\" Boolean
-.\" commit 2c7946a7bf45ae86736ab3b43d0085e43947945c
-.\" Author: Catherine Zhang <cxzhang@watson.ibm.com>
-.\"
.\" IP_MINTTL (2.6.34)
.\" commit d218d11133d888f9745802146a50255a4781d37a
.\" Author: Stephen Hemminger <shemminger@vyatta.com>
@@ -664,6 +659,47 @@ with
.B IP_OPTIONS
puts the current IP options used for sending into the supplied buffer.
.TP
+.BR IP_PASSSEC " (since Linux 2.6.17)"
+.\" commit 2c7946a7bf45ae86736ab3b43d0085e43947945c
+If labeled IPSEC or NetLabel is configured on the sending and receiving
+hosts, this option enables receiving of the security context of the peer
+socket in an ancillary message of type
+.B SCM_SECURITY
+retrieved using
+.BR recvmsg (2).
+This option is only supported for UDP sockets; for TCP or SCTP sockets,
+see the description of the
+.B SO_PEERSEC
+option below.
+.IP
+The value given as an argument to
+.BR setsockopt (2)
+and returned as the result of
+.BR getsockopt (2)
+is an integer boolean flag.
+.IP
+The security context returned in the
+.B SCM_SECURITY
+ancillary message
+is of the same format as the one described under the
+.B SO_PEERSEC
+option below.
+.IP
+NOTE: The reuse of the
+.B SCM_SECURITY
+message type
+for the
+.B IP_PASSSEC
+socket option was likely a mistake since other IP control messages use
+their own numbering scheme in the IP namespace and often use the
+socket option value as the message type. There is no conflict
+currently since the IP option with the same value
+as
+.B SCM_SECURITY
+is
+.B IP_HDRINCL
+and this is never used for a control message type.
+.TP
.BR IP_PKTINFO " (since Linux 2.2)"
.\" Precisely: 2.1.68
Pass an
@@ -1290,13 +1326,13 @@ and
.BR IP_MTU ,
.BR IP_MTU_DISCOVER ,
.BR IP_RECVORIGDSTADDR ,
+.BR IP_PASSSEC ,
.BR IP_PKTINFO ,
.BR IP_RECVERR ,
.BR IP_ROUTER_ALERT ,
and
.BR IP_TRANSPARENT
are Linux-specific.
-.\" IP_PASSSEC is Linux-specific
.\" IP_XFRM_POLICY is Linux-specific
.\" IP_IPSEC_POLICY is a nonstandard extension, also present on some BSDs
.PP
--
2.25.1
^ permalink raw reply related
* RE: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag
From: Roberto Sassu @ 2020-09-17 17:36 UTC (permalink / raw)
To: Mimi Zohar, mjg59@google.com, John Johansen
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <5bbf2169cfa38bb7a3d696e582c1de954a82d5c6.camel@linux.ibm.com>
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, September 17, 2020 2:01 PM
> [Cc'ing John Johansen]
>
> Hi Roberto,
>
> On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> > When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation
> on
> > metadata. Its main purpose is to allow users to freely set metadata when
> > they are protected by a portable signature, until the HMAC key is loaded.
> >
> > However, IMA is not notified about metadata changes and, after the first
> > successful appraisal, always allows access to the files without checking
> > metadata again.
> >
> > This patch introduces the new atomic flag EVM_RESET_STATUS in
> > integrity_iint_cache that is set in the EVM post hooks and cleared in
> > evm_verify_hmac(). IMA checks the new flag in process_measurement()
> and if
> > it is set, it clears the appraisal flags.
> >
> > Although the flag could be cleared also by evm_inode_setxattr() and
> > evm_inode_setattr() before IMA sees it, this does not happen if
> > EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
> > evm_verifyxattr(), this ensures that IMA always sees the flag set before it
> > is cleared.
> >
> > This patch also adds a call to evm_reset_status() in
> > evm_inode_post_setattr() so that EVM won't return the cached status
> the
> > next time appraisal is performed.
> >
> > Cc: stable@vger.kernel.org # 4.16.x
> > Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of
> EVM-protected metadata")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> > security/integrity/evm/evm_main.c | 17 +++++++++++++++--
> > security/integrity/ima/ima_main.c | 8 ++++++--
> > security/integrity/integrity.h | 1 +
> > 3 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > index 4e9f5e8b21d5..05be1ad3e6f3 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -221,8 +221,15 @@ static enum integrity_status
> evm_verify_hmac(struct dentry *dentry,
> > evm_status = (rc == -ENODATA) ?
> > INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> > out:
> > - if (iint)
> > + if (iint) {
> > + /*
> > + * EVM_RESET_STATUS can be cleared only by
> evm_verifyxattr()
> > + * when EVM_ALLOW_METADATA_WRITES is set. This
> guarantees that
> > + * IMA sees the EVM_RESET_STATUS flag set before it is
> cleared.
> > + */
> > + clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> > iint->evm_status = evm_status;
>
> True IMA is currently the only caller of evm_verifyxattr() in the
> upstreamed kernel, but it is an exported function, which may be called
> from elsewhere. The previous version crossed the boundary between EVM
> & IMA with EVM modifying the IMA flag directly. This version assumes
> that IMA will be the only caller. Otherwise, I like this version.
Ok, I think it is better, as you suggested, to export a new EVM function
that tells if evm_reset_status() will be executed in the EVM post hooks, and
to call this function from IMA. IMA would then call ima_reset_appraise_flags()
also depending on the result of the new EVM function.
ima_reset_appraise_flags() should be called in a post hook in IMA.
Should I introduce it?
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> Mimi
>
> > + }
> > kfree(xattr_data);
> > return evm_status;
> > }
> > @@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
> > struct integrity_iint_cache *iint;
> >
> > iint = integrity_iint_find(inode);
> > - if (iint)
> > + if (iint) {
> > + if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> > + set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> > +
> > iint->evm_status = INTEGRITY_UNKNOWN;
> > + }
> > }
> >
> > /**
> > @@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry
> *dentry, int ia_valid)
> > if (!evm_key_loaded())
> > return;
> >
> > + evm_reset_status(dentry->d_inode);
> > +
> > if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> > evm_update_evmxattr(dentry, NULL, NULL, 0);
> > }
^ permalink raw reply
* Re: [RFC PATCH 1/6] security/fbfam: Add a Kconfig to enable the fbfam feature
From: John Wood @ 2020-09-17 17:32 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, John Wood, Kernel Hardening, Matthew Wilcox,
Jonathan Corbet, Alexander Viro, Ingo Molnar, Peter Zijlstra,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt,
Ben Segall, Mel Gorman, Luis Chamberlain, Iurii Zaikin,
James Morris, Serge E. Hallyn, linux-doc, kernel list,
linux-fsdevel, linux-security-module
In-Reply-To: <CAG48ez1V=oVczCCSuRaWX=bbN2cOi0Y9q48=e-Fuhg7mwMOi0A@mail.gmail.com>
On Thu, Sep 10, 2020 at 11:21:58PM +0200, Jann Horn wrote:
> On Thu, Sep 10, 2020 at 10:21 PM Kees Cook <keescook@chromium.org> wrote:
> > From: John Wood <john.wood@gmx.com>
> >
> > Add a menu entry under "Security options" to enable the "Fork brute
> > force attack mitigation" feature.
> [...]
> > +config FBFAM
>
> Please give this a more descriptive name than FBFAM. Some name where,
> if a random kernel developer sees an "#ifdef" with that name in some
> random piece of kernel code, they immediately have a rough idea for
> what kind of feature this is.
>
> Perhaps something like THROTTLE_FORK_CRASHES. Or something else that
> is equally descriptive.
Ok, understood. This will be fixed for the next version. Thanks.
> > + bool "Fork brute force attack mitigation"
> > + default n
>
> "default n" is superfluous and should AFAIK be omitted.
Ok. I will remove it. Thanks.
> > + help
> > + This is a user defense that detects any fork brute force attack
> > + based on the application's crashing rate. When this measure is
> > + triggered the fork system call is blocked.
>
> This help text claims that the mitigation will block fork(), but patch
> 6/6 actually kills the process hierarchy.
Sorry, it's a mistake. It was the first idea but finally the implementation
changed and this description not was modified. Apologies. It will be fixed
for the next version.
Thanks,
John Wood
^ permalink raw reply
* Re: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag
From: Mimi Zohar @ 2020-09-17 17:47 UTC (permalink / raw)
To: Roberto Sassu, mjg59@google.com, John Johansen
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <581966c47e94412ab3fd5b2ca9aacd3d@huawei.com>
On Thu, 2020-09-17 at 17:36 +0000, Roberto Sassu wrote:
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > > index 4e9f5e8b21d5..05be1ad3e6f3 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -221,8 +221,15 @@ static enum integrity_status
> > evm_verify_hmac(struct dentry *dentry,
> > > evm_status = (rc == -ENODATA) ?
> > > INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> > > out:
> > > - if (iint)
> > > + if (iint) {
> > > + /*
> > > + * EVM_RESET_STATUS can be cleared only by
> > evm_verifyxattr()
> > > + * when EVM_ALLOW_METADATA_WRITES is set. This
> > guarantees that
> > > + * IMA sees the EVM_RESET_STATUS flag set before it is
> > cleared.
> > > + */
> > > + clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> > > iint->evm_status = evm_status;
> >
> > True IMA is currently the only caller of evm_verifyxattr() in the
> > upstreamed kernel, but it is an exported function, which may be called
> > from elsewhere. The previous version crossed the boundary between EVM
> > & IMA with EVM modifying the IMA flag directly. This version assumes
> > that IMA will be the only caller. Otherwise, I like this version.
>
> Ok, I think it is better, as you suggested, to export a new EVM function
> that tells if evm_reset_status() will be executed in the EVM post hooks, and
> to call this function from IMA. IMA would then call ima_reset_appraise_flags()
> also depending on the result of the new EVM function.
>
> ima_reset_appraise_flags() should be called in a post hook in IMA.
> Should I introduce it?
Yes, so any callers of evm_verifyxattr() will need to implement the
post hook as well. As much as possible, please limit code duplication.
The last time I looked, there didn't seem to be a locking concern, but
please make sure.
thanks,
Mimi
^ permalink raw reply
* Re: [RFC PATCH 1/6] security/fbfam: Add a Kconfig to enable the fbfam feature
From: John Wood @ 2020-09-17 18:40 UTC (permalink / raw)
To: Kees Cook
Cc: Jann Horn, kernel-hardening, John Wood, Matthew Wilcox,
Jonathan Corbet, Alexander Viro, Ingo Molnar, Peter Zijlstra,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt,
Ben Segall, Mel Gorman, Luis Chamberlain, Iurii Zaikin,
James Morris, Serge E. Hallyn, linux-doc, linux-kernel,
linux-fsdevel, linux-security-module
In-Reply-To: <202009101615.8566BA3967@keescook>
Hi,
On Thu, Sep 10, 2020 at 04:18:08PM -0700, Kees Cook wrote:
> On Thu, Sep 10, 2020 at 01:21:02PM -0700, Kees Cook wrote:
> > From: John Wood <john.wood@gmx.com>
> >
> > Add a menu entry under "Security options" to enable the "Fork brute
> > force attack mitigation" feature.
> >
> > Signed-off-by: John Wood <john.wood@gmx.com>
> > ---
> > security/Kconfig | 1 +
> > security/fbfam/Kconfig | 10 ++++++++++
> > 2 files changed, 11 insertions(+)
> > create mode 100644 security/fbfam/Kconfig
> >
> > diff --git a/security/Kconfig b/security/Kconfig
> > index 7561f6f99f1d..00a90e25b8d5 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -290,6 +290,7 @@ config LSM
> > If unsure, leave this as the default.
> >
> > source "security/Kconfig.hardening"
> > +source "security/fbfam/Kconfig"
>
> Given the layout you've chosen and the interface you've got, I think
> this should just be treated like a regular LSM.
Yes, throughout the review it seems the most appropiate is treat
this feature as a regular LSM. Thanks.
> >
> > endmenu
> >
> > diff --git a/security/fbfam/Kconfig b/security/fbfam/Kconfig
> > new file mode 100644
> > index 000000000000..bbe7f6aad369
> > --- /dev/null
> > +++ b/security/fbfam/Kconfig
> > @@ -0,0 +1,10 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +config FBFAM
>
> To jump on the bikeshed: how about just calling this
> FORK_BRUTE_FORCE_DETECTION or FORK_BRUTE, and the directory could be
> "brute", etc. "fbfam" doesn't tell anyone anything.
Understood. But how about use the fbfam abbreviation in the code? Like as
function name prefix, struct name prefix, ... It would be better to use a
more descriptive name in this scenario? It is not clear to me.
> --
> Kees Cook
Thanks,
John Wood
^ permalink raw reply
* Re: LSM that blocks execution of the code from the anonymous pages
From: Mimi Zohar @ 2020-09-17 20:53 UTC (permalink / raw)
To: Igor Zhbanov, linux-integrity; +Cc: linux-security-module
In-Reply-To: <88b9444e-08bc-4240-7943-298070dfc47c@omprussia.ru>
Hi Igor,
(Reminder the Linux kernel mailing lists convention is to inline/bottom
post.)
On Thu, 2020-09-17 at 23:39 +0300, Igor Zhbanov wrote:
> My question is more about whether this functionality fits into IMA's
> responsibility. I.e. I can propose the changes as the extension of IMA's
> functionality (which I think it would be better), or I could create a separate
> LSM if this functionality doesn't align with IMA's purpose for some reason.
> This is the first question.
>
> And the second question, what kind of operation modes do you think would
> be useful?
>
> 1) no anonymous code for privileged processes (as currently),
> 2) no anonymous code for all processes,
> 3) no anonymous code for all processes with xattr-based exceptions (may be
> with xattr value signing)
These are generic questions not dependent on whether this would be
upstreamed as an independent LSM or as part of IMA. For this reason,
I've Cc'ed the LSM mailing list.
Mimi
>
> For #3 I definitely would prefer to implement the code as a part of IMA
> because of sharing of xattrs cache, etc. to avoid reinventing the wheel.
^ permalink raw reply
* Re: [RFC PATCH 1/6] security/fbfam: Add a Kconfig to enable the fbfam feature
From: Kees Cook @ 2020-09-17 22:05 UTC (permalink / raw)
To: John Wood
Cc: Jann Horn, kernel-hardening, Matthew Wilcox, Jonathan Corbet,
Alexander Viro, Ingo Molnar, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Luis Chamberlain, Iurii Zaikin, James Morris,
Serge E. Hallyn, linux-doc, linux-kernel, linux-fsdevel,
linux-security-module
In-Reply-To: <20200917175146.GB3637@ubuntu>
On Thu, Sep 17, 2020 at 08:40:06PM +0200, John Wood wrote:
> Hi,
>
> On Thu, Sep 10, 2020 at 04:18:08PM -0700, Kees Cook wrote:
> > On Thu, Sep 10, 2020 at 01:21:02PM -0700, Kees Cook wrote:
> > > From: John Wood <john.wood@gmx.com>
> > >
> > > Add a menu entry under "Security options" to enable the "Fork brute
> > > force attack mitigation" feature.
> > >
> > > Signed-off-by: John Wood <john.wood@gmx.com>
> > > ---
> > > security/Kconfig | 1 +
> > > security/fbfam/Kconfig | 10 ++++++++++
> > > 2 files changed, 11 insertions(+)
> > > create mode 100644 security/fbfam/Kconfig
> > >
> > > diff --git a/security/Kconfig b/security/Kconfig
> > > index 7561f6f99f1d..00a90e25b8d5 100644
> > > --- a/security/Kconfig
> > > +++ b/security/Kconfig
> > > @@ -290,6 +290,7 @@ config LSM
> > > If unsure, leave this as the default.
> > >
> > > source "security/Kconfig.hardening"
> > > +source "security/fbfam/Kconfig"
> >
> > Given the layout you've chosen and the interface you've got, I think
> > this should just be treated like a regular LSM.
>
> Yes, throughout the review it seems the most appropiate is treat
> this feature as a regular LSM. Thanks.
>
> > >
> > > endmenu
> > >
> > > diff --git a/security/fbfam/Kconfig b/security/fbfam/Kconfig
> > > new file mode 100644
> > > index 000000000000..bbe7f6aad369
> > > --- /dev/null
> > > +++ b/security/fbfam/Kconfig
> > > @@ -0,0 +1,10 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +config FBFAM
> >
> > To jump on the bikeshed: how about just calling this
> > FORK_BRUTE_FORCE_DETECTION or FORK_BRUTE, and the directory could be
> > "brute", etc. "fbfam" doesn't tell anyone anything.
>
> Understood. But how about use the fbfam abbreviation in the code? Like as
> function name prefix, struct name prefix, ... It would be better to use a
> more descriptive name in this scenario? It is not clear to me.
I don't feel too strongly, but I think having the CONFIG roughly match
the directory name, roughly match the function prefixes should be best.
Maybe call the directory and function prefix "brute"?
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] ip.7: Document IP_PASSSEC for UDP sockets
From: Paul Moore @ 2020-09-17 23:16 UTC (permalink / raw)
To: Stephen Smalley
Cc: mtk.manpages, linux-man, linux-security-module, selinux, smcv,
James Morris, Serge Hallyn
In-Reply-To: <20200917173143.57241-1-stephen.smalley.work@gmail.com>
On Thu, Sep 17, 2020 at 1:31 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> Document the IP_PASSSEC socket option and SCM_SECURITY
> ancillary/control message type for UDP sockets.
>
> IP_PASSSEC for UDP sockets was introduced in Linux 2.6.17 [1].
>
> Example NetLabel and IPSEC configurations and usage of this option
> can be found in the SELinux Notebook [2] and SELinux testsuite [3].
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c7946a7bf45ae86736ab3b43d0085e43947945c
>
> [2] https://github.com/SELinuxProject/selinux-notebook
>
> [3] https://github.com/SELinuxProject/selinux-testsuite
>
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> man7/ip.7 | 48 ++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 42 insertions(+), 6 deletions(-)
Thanks for including the note about the SCM_SECURITY/IP_HDRINCL
conflict. I figure it's probably not the best for another SELinux
person to ACK this, but I will mark it as "reviewed".
Reviewed-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH AUTOSEL 4.14 044/127] selinux: sel_avc_get_stat_idx should increase position index
From: Sasha Levin @ 2020-09-18 2:10 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Vasily Averin, Stephen Smalley, Paul Moore, Sasha Levin, selinux,
linux-security-module
In-Reply-To: <20200918021220.2066485-1-sashal@kernel.org>
From: Vasily Averin <vvs@virtuozzo.com>
[ Upstream commit 8d269a8e2a8f0bca89022f4ec98de460acb90365 ]
If seq_file .next function does not change position index,
read after some lseek can generate unexpected output.
$ dd if=/sys/fs/selinux/avc/cache_stats # usual output
lookups hits misses allocations reclaims frees
817223 810034 7189 7189 6992 7037
1934894 1926896 7998 7998 7632 7683
1322812 1317176 5636 5636 5456 5507
1560571 1551548 9023 9023 9056 9115
0+1 records in
0+1 records out
189 bytes copied, 5,1564e-05 s, 3,7 MB/s
$# read after lseek to midle of last line
$ dd if=/sys/fs/selinux/avc/cache_stats bs=180 skip=1
dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset
056 9115 <<<< end of last line
1560571 1551548 9023 9023 9056 9115 <<< whole last line once again
0+1 records in
0+1 records out
45 bytes copied, 8,7221e-05 s, 516 kB/s
$# read after lseek beyond end of of file
$ dd if=/sys/fs/selinux/avc/cache_stats bs=1000 skip=1
dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset
1560571 1551548 9023 9023 9056 9115 <<<< generates whole last line
0+1 records in
0+1 records out
36 bytes copied, 9,0934e-05 s, 396 kB/s
https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/selinux/selinuxfs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 00eed842c491c..bf50fead9f8c0 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1425,6 +1425,7 @@ static struct avc_cache_stats *sel_avc_get_stat_idx(loff_t *idx)
*idx = cpu + 1;
return &per_cpu(avc_cache_stats, cpu);
}
+ (*idx)++;
return NULL;
}
--
2.25.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.9 35/90] selinux: sel_avc_get_stat_idx should increase position index
From: Sasha Levin @ 2020-09-18 2:14 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Vasily Averin, Stephen Smalley, Paul Moore, Sasha Levin, selinux,
linux-security-module
In-Reply-To: <20200918021455.2067301-1-sashal@kernel.org>
From: Vasily Averin <vvs@virtuozzo.com>
[ Upstream commit 8d269a8e2a8f0bca89022f4ec98de460acb90365 ]
If seq_file .next function does not change position index,
read after some lseek can generate unexpected output.
$ dd if=/sys/fs/selinux/avc/cache_stats # usual output
lookups hits misses allocations reclaims frees
817223 810034 7189 7189 6992 7037
1934894 1926896 7998 7998 7632 7683
1322812 1317176 5636 5636 5456 5507
1560571 1551548 9023 9023 9056 9115
0+1 records in
0+1 records out
189 bytes copied, 5,1564e-05 s, 3,7 MB/s
$# read after lseek to midle of last line
$ dd if=/sys/fs/selinux/avc/cache_stats bs=180 skip=1
dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset
056 9115 <<<< end of last line
1560571 1551548 9023 9023 9056 9115 <<< whole last line once again
0+1 records in
0+1 records out
45 bytes copied, 8,7221e-05 s, 516 kB/s
$# read after lseek beyond end of of file
$ dd if=/sys/fs/selinux/avc/cache_stats bs=1000 skip=1
dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset
1560571 1551548 9023 9023 9056 9115 <<<< generates whole last line
0+1 records in
0+1 records out
36 bytes copied, 9,0934e-05 s, 396 kB/s
https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/selinux/selinuxfs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 72c145dd799f1..ef1226c1c3add 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1416,6 +1416,7 @@ static struct avc_cache_stats *sel_avc_get_stat_idx(loff_t *idx)
*idx = cpu + 1;
return &per_cpu(avc_cache_stats, cpu);
}
+ (*idx)++;
return NULL;
}
--
2.25.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.4 25/64] selinux: sel_avc_get_stat_idx should increase position index
From: Sasha Levin @ 2020-09-18 2:16 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Vasily Averin, Stephen Smalley, Paul Moore, Sasha Levin, selinux,
linux-security-module
In-Reply-To: <20200918021643.2067895-1-sashal@kernel.org>
From: Vasily Averin <vvs@virtuozzo.com>
[ Upstream commit 8d269a8e2a8f0bca89022f4ec98de460acb90365 ]
If seq_file .next function does not change position index,
read after some lseek can generate unexpected output.
$ dd if=/sys/fs/selinux/avc/cache_stats # usual output
lookups hits misses allocations reclaims frees
817223 810034 7189 7189 6992 7037
1934894 1926896 7998 7998 7632 7683
1322812 1317176 5636 5636 5456 5507
1560571 1551548 9023 9023 9056 9115
0+1 records in
0+1 records out
189 bytes copied, 5,1564e-05 s, 3,7 MB/s
$# read after lseek to midle of last line
$ dd if=/sys/fs/selinux/avc/cache_stats bs=180 skip=1
dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset
056 9115 <<<< end of last line
1560571 1551548 9023 9023 9056 9115 <<< whole last line once again
0+1 records in
0+1 records out
45 bytes copied, 8,7221e-05 s, 516 kB/s
$# read after lseek beyond end of of file
$ dd if=/sys/fs/selinux/avc/cache_stats bs=1000 skip=1
dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset
1560571 1551548 9023 9023 9056 9115 <<<< generates whole last line
0+1 records in
0+1 records out
36 bytes copied, 9,0934e-05 s, 396 kB/s
https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/selinux/selinuxfs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index c02da25d7b631..7778e28cce9d7 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1370,6 +1370,7 @@ static struct avc_cache_stats *sel_avc_get_stat_idx(loff_t *idx)
*idx = cpu + 1;
return &per_cpu(avc_cache_stats, cpu);
}
+ (*idx)++;
return NULL;
}
--
2.25.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 073/206] selinux: sel_avc_get_stat_idx should increase position index
From: Sasha Levin @ 2020-09-18 2:05 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Vasily Averin, Stephen Smalley, Paul Moore, Sasha Levin, selinux,
linux-security-module
In-Reply-To: <20200918020802.2065198-1-sashal@kernel.org>
From: Vasily Averin <vvs@virtuozzo.com>
[ Upstream commit 8d269a8e2a8f0bca89022f4ec98de460acb90365 ]
If seq_file .next function does not change position index,
read after some lseek can generate unexpected output.
$ dd if=/sys/fs/selinux/avc/cache_stats # usual output
lookups hits misses allocations reclaims frees
817223 810034 7189 7189 6992 7037
1934894 1926896 7998 7998 7632 7683
1322812 1317176 5636 5636 5456 5507
1560571 1551548 9023 9023 9056 9115
0+1 records in
0+1 records out
189 bytes copied, 5,1564e-05 s, 3,7 MB/s
$# read after lseek to midle of last line
$ dd if=/sys/fs/selinux/avc/cache_stats bs=180 skip=1
dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset
056 9115 <<<< end of last line
1560571 1551548 9023 9023 9056 9115 <<< whole last line once again
0+1 records in
0+1 records out
45 bytes copied, 8,7221e-05 s, 516 kB/s
$# read after lseek beyond end of of file
$ dd if=/sys/fs/selinux/avc/cache_stats bs=1000 skip=1
dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset
1560571 1551548 9023 9023 9056 9115 <<<< generates whole last line
0+1 records in
0+1 records out
36 bytes copied, 9,0934e-05 s, 396 kB/s
https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/selinux/selinuxfs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index f3a5a138a096d..60b3f16bb5c7b 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1509,6 +1509,7 @@ static struct avc_cache_stats *sel_avc_get_stat_idx(loff_t *idx)
*idx = cpu + 1;
return &per_cpu(avc_cache_stats, cpu);
}
+ (*idx)++;
return NULL;
}
--
2.25.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 001/206] selinux: allow labeling before policy is loaded
From: Sasha Levin @ 2020-09-18 2:04 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Jonathan Lebon, Victor Kamensky, Paul Moore, Sasha Levin, selinux,
linux-security-module
From: Jonathan Lebon <jlebon@redhat.com>
[ Upstream commit 3e3e24b42043eceb97ed834102c2d094dfd7aaa6 ]
Currently, the SELinux LSM prevents one from setting the
`security.selinux` xattr on an inode without a policy first being
loaded. However, this restriction is problematic: it makes it impossible
to have newly created files with the correct label before actually
loading the policy.
This is relevant in distributions like Fedora, where the policy is
loaded by systemd shortly after pivoting out of the initrd. In such
instances, all files created prior to pivoting will be unlabeled. One
then has to relabel them after pivoting, an operation which inherently
races with other processes trying to access those same files.
Going further, there are use cases for creating the entire root
filesystem on first boot from the initrd (e.g. Container Linux supports
this today[1], and we'd like to support it in Fedora CoreOS as well[2]).
One can imagine doing this in two ways: at the block device level (e.g.
laying down a disk image), or at the filesystem level. In the former,
labeling can simply be part of the image. But even in the latter
scenario, one still really wants to be able to set the right labels when
populating the new filesystem.
This patch enables this by changing behaviour in the following two ways:
1. allow `setxattr` if we're not initialized
2. don't try to set the in-core inode SID if we're not initialized;
instead leave it as `LABEL_INVALID` so that revalidation may be
attempted at a later time
Note the first hunk of this patch is mostly the same as a previously
discussed one[3], though it was part of a larger series which wasn't
accepted.
[1] https://coreos.com/os/docs/latest/root-filesystem-placement.html
[2] https://github.com/coreos/fedora-coreos-tracker/issues/94
[3] https://www.spinics.net/lists/linux-initramfs/msg04593.html
Co-developed-by: Victor Kamensky <kamensky@cisco.com>
Signed-off-by: Victor Kamensky <kamensky@cisco.com>
Signed-off-by: Jonathan Lebon <jlebon@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/selinux/hooks.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 452254fd89f87..250b725f5754c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3304,6 +3304,9 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
}
+ if (!selinux_state.initialized)
+ return (inode_owner_or_capable(inode) ? 0 : -EPERM);
+
sbsec = inode->i_sb->s_security;
if (!(sbsec->flags & SBLABEL_MNT))
return -EOPNOTSUPP;
@@ -3387,6 +3390,15 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
return;
}
+ if (!selinux_state.initialized) {
+ /* If we haven't even been initialized, then we can't validate
+ * against a policy, so leave the label as invalid. It may
+ * resolve to a valid label on the next revalidation try if
+ * we've since initialized.
+ */
+ return;
+ }
+
rc = security_context_to_sid_force(&selinux_state, value, size,
&newsid);
if (rc) {
--
2.25.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.4 121/330] selinux: sel_avc_get_stat_idx should increase position index
From: Sasha Levin @ 2020-09-18 1:57 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Vasily Averin, Stephen Smalley, Paul Moore, Sasha Levin, selinux,
linux-security-module
In-Reply-To: <20200918020110.2063155-1-sashal@kernel.org>
From: Vasily Averin <vvs@virtuozzo.com>
[ Upstream commit 8d269a8e2a8f0bca89022f4ec98de460acb90365 ]
If seq_file .next function does not change position index,
read after some lseek can generate unexpected output.
$ dd if=/sys/fs/selinux/avc/cache_stats # usual output
lookups hits misses allocations reclaims frees
817223 810034 7189 7189 6992 7037
1934894 1926896 7998 7998 7632 7683
1322812 1317176 5636 5636 5456 5507
1560571 1551548 9023 9023 9056 9115
0+1 records in
0+1 records out
189 bytes copied, 5,1564e-05 s, 3,7 MB/s
$# read after lseek to midle of last line
$ dd if=/sys/fs/selinux/avc/cache_stats bs=180 skip=1
dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset
056 9115 <<<< end of last line
1560571 1551548 9023 9023 9056 9115 <<< whole last line once again
0+1 records in
0+1 records out
45 bytes copied, 8,7221e-05 s, 516 kB/s
$# read after lseek beyond end of of file
$ dd if=/sys/fs/selinux/avc/cache_stats bs=1000 skip=1
dd: /sys/fs/selinux/avc/cache_stats: cannot skip to specified offset
1560571 1551548 9023 9023 9056 9115 <<<< generates whole last line
0+1 records in
0+1 records out
36 bytes copied, 9,0934e-05 s, 396 kB/s
https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/selinux/selinuxfs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index e6c7643c3fc08..e9eaff90cbccd 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1508,6 +1508,7 @@ static struct avc_cache_stats *sel_avc_get_stat_idx(loff_t *idx)
*idx = cpu + 1;
return &per_cpu(avc_cache_stats, cpu);
}
+ (*idx)++;
return NULL;
}
--
2.25.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.4 005/330] selinux: allow labeling before policy is loaded
From: Sasha Levin @ 2020-09-18 1:55 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Jonathan Lebon, Victor Kamensky, Paul Moore, Sasha Levin, selinux,
linux-security-module
In-Reply-To: <20200918020110.2063155-1-sashal@kernel.org>
From: Jonathan Lebon <jlebon@redhat.com>
[ Upstream commit 3e3e24b42043eceb97ed834102c2d094dfd7aaa6 ]
Currently, the SELinux LSM prevents one from setting the
`security.selinux` xattr on an inode without a policy first being
loaded. However, this restriction is problematic: it makes it impossible
to have newly created files with the correct label before actually
loading the policy.
This is relevant in distributions like Fedora, where the policy is
loaded by systemd shortly after pivoting out of the initrd. In such
instances, all files created prior to pivoting will be unlabeled. One
then has to relabel them after pivoting, an operation which inherently
races with other processes trying to access those same files.
Going further, there are use cases for creating the entire root
filesystem on first boot from the initrd (e.g. Container Linux supports
this today[1], and we'd like to support it in Fedora CoreOS as well[2]).
One can imagine doing this in two ways: at the block device level (e.g.
laying down a disk image), or at the filesystem level. In the former,
labeling can simply be part of the image. But even in the latter
scenario, one still really wants to be able to set the right labels when
populating the new filesystem.
This patch enables this by changing behaviour in the following two ways:
1. allow `setxattr` if we're not initialized
2. don't try to set the in-core inode SID if we're not initialized;
instead leave it as `LABEL_INVALID` so that revalidation may be
attempted at a later time
Note the first hunk of this patch is mostly the same as a previously
discussed one[3], though it was part of a larger series which wasn't
accepted.
[1] https://coreos.com/os/docs/latest/root-filesystem-placement.html
[2] https://github.com/coreos/fedora-coreos-tracker/issues/94
[3] https://www.spinics.net/lists/linux-initramfs/msg04593.html
Co-developed-by: Victor Kamensky <kamensky@cisco.com>
Signed-off-by: Victor Kamensky <kamensky@cisco.com>
Signed-off-by: Jonathan Lebon <jlebon@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/selinux/hooks.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 552e73d90fd25..212f48025db81 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3156,6 +3156,9 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
}
+ if (!selinux_state.initialized)
+ return (inode_owner_or_capable(inode) ? 0 : -EPERM);
+
sbsec = inode->i_sb->s_security;
if (!(sbsec->flags & SBLABEL_MNT))
return -EOPNOTSUPP;
@@ -3239,6 +3242,15 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
return;
}
+ if (!selinux_state.initialized) {
+ /* If we haven't even been initialized, then we can't validate
+ * against a policy, so leave the label as invalid. It may
+ * resolve to a valid label on the next revalidation try if
+ * we've since initialized.
+ */
+ return;
+ }
+
rc = security_context_to_sid_force(&selinux_state, value, size,
&newsid);
if (rc) {
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v6 1/4] KEYS: trusted: Add generic trusted keys framework
From: Sumit Garg @ 2020-09-18 7:03 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Mimi Zohar, James Bottomley, David Howells, Jens Wiklander,
Jonathan Corbet, James Morris, Serge E. Hallyn, Casey Schaufler,
Janne Karhunen, Daniel Thompson, Markus Wamser, Luke Hinds,
open list:ASYMMETRIC KEYS, linux-integrity, linux-security-module,
Linux Doc Mailing List, Linux Kernel Mailing List,
linux-arm-kernel, op-tee
In-Reply-To: <20200917162506.GC9750@linux.intel.com>
On Thu, 17 Sep 2020 at 21:55, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, Sep 17, 2020 at 07:21:49PM +0300, Jarkko Sakkinen wrote:
> > On Thu, Sep 17, 2020 at 07:16:35PM +0530, Sumit Garg wrote:
> > > Current trusted keys framework is tightly coupled to use TPM device as
> > > an underlying implementation which makes it difficult for implementations
> > > like Trusted Execution Environment (TEE) etc. to provide trusted keys
> > > support in case platform doesn't posses a TPM device.
> > >
> > > So this patch tries to add generic trusted keys framework where underlying
> > > implementations like TPM, TEE etc. could be easily plugged-in.
> >
> > I would rephrase this a bit:
> >
> > "Add a generic trusted keys framework where underlying implementations
> > can be easily plugged in. Create struct trusted_key_ops to achieve this,
> > which contains necessary functions of a backend."
> >
Okay, will use it instead.
> > I remember asking about this approach that what if there was just a
> > header for trusted key functions and a compile time decision, which C
> > file to include instead of ops struct. I don't remember if these was a
> > conclusion on this or not.
This approach was implemented as part of v5 and we concluded here [1]
to revert back to the dynamic approach as distro vendors won't like to
make opinionated selection at compile time which could rather be
achieved dynamically based on platform capability.
[1] https://www.spinics.net/lists/keyrings/msg08161.html
> >
> > E.g. lets say you have a device with TEE and TPM, should you be able
> > to be use both at run-time? I might play along how this works now but
> > somehow, in the commit message preferably, it should be conclude why
> > one alternative is chosen over another.
>
Okay, so how about adding a kernel module parameter which can enforce
the user's preference about which trust source to use at runtime? And
we should only check availability for that trust source if preference
is provided otherwise by default we can traverse the trust sources
list.
See following change, if this approach looks sane, I can include it in
next version:
diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index edd635a..a566451 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -63,6 +63,11 @@ struct trusted_key_ops {
void (*exit)(void);
};
+struct trusted_key_source {
+ char *name;
+ struct trusted_key_ops *ops;
+};
+
extern struct key_type key_type_trusted;
#define TRUSTED_DEBUG 0
diff --git a/security/keys/trusted-keys/trusted_core.c
b/security/keys/trusted-keys/trusted_core.c
index 83a6a15..74a3d80 100644
--- a/security/keys/trusted-keys/trusted_core.c
+++ b/security/keys/trusted-keys/trusted_core.c
@@ -21,12 +21,16 @@
#include <linux/string.h>
#include <linux/uaccess.h>
-static struct trusted_key_ops *available_trusted_key_ops[] = {
+static char *trusted_key_source;
+module_param_named(source, trusted_key_source, charp, 0);
+MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
+
+static struct trusted_key_source trusted_key_sources[] = {
#if defined(CONFIG_TCG_TPM)
- &tpm_trusted_key_ops,
+ { "tpm", &tpm_trusted_key_ops },
#endif
#if defined(CONFIG_TEE)
- &tee_trusted_key_ops,
+ { "tee", &tee_trusted_key_ops },
#endif
};
static struct trusted_key_ops *trusted_key_ops;
@@ -296,8 +300,13 @@ static int __init init_trusted(void)
{
int i, ret = 0;
- for (i = 0; i < sizeof(available_trusted_key_ops); i++) {
- trusted_key_ops = available_trusted_key_ops[i];
+ for (i = 0; i < ARRAY_SIZE(trusted_key_sources); i++) {
+ if (trusted_key_source &&
+ strncmp(trusted_key_source, trusted_key_sources[i].name,
+ strlen(trusted_key_sources[i].name)))
+ continue;
+
+ trusted_key_ops = trusted_key_sources[i].ops;
ret = trusted_key_ops->init();
if (!ret)
> We must somehow seal this discussion because the other changes are
> based on this decision.
>
> I don't think tail of this patch set takes a long time spin. This
> is the main architectural decision.
Agree.
-Sumit
>
> /Jarkko
^ permalink raw reply related
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