Menu
▾
▴
linux-ima-devel — Discussion list for problems or development.
This list is closed, nobody may subscribe to it.
| 2007 |
Jan
|
Feb
(1) |
Mar
|
Apr
|
May
|
Jun
|
Jul
|
Aug
|
Sep
|
Oct
|
Nov
|
Dec
(1) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 2009 |
Jan
|
Feb
|
Mar
|
Apr
(1) |
May
(1) |
Jun
(2) |
Jul
|
Aug
|
Sep
|
Oct
|
Nov
|
Dec
|
| 2011 |
Jan
|
Feb
|
Mar
(1) |
Apr
|
May
|
Jun
|
Jul
|
Aug
|
Sep
|
Oct
|
Nov
|
Dec
|
| 2013 |
Jan
|
Feb
|
Mar
(7) |
Apr
|
May
(7) |
Jun
(7) |
Jul
(26) |
Aug
|
Sep
(7) |
Oct
(1) |
Nov
(35) |
Dec
(18) |
| 2014 |
Jan
(1) |
Feb
(2) |
Mar
(3) |
Apr
|
May
(16) |
Jun
(35) |
Jul
(103) |
Aug
(45) |
Sep
(226) |
Oct
(200) |
Nov
(66) |
Dec
(42) |
| 2015 |
Jan
(47) |
Feb
(3) |
Mar
(6) |
Apr
(14) |
May
(38) |
Jun
(10) |
Jul
(10) |
Aug
(15) |
Sep
(23) |
Oct
(78) |
Nov
(56) |
Dec
(70) |
| 2016 |
Jan
(9) |
Feb
(8) |
Mar
(15) |
Apr
(18) |
May
(78) |
Jun
(39) |
Jul
(3) |
Aug
(136) |
Sep
(134) |
Oct
(19) |
Nov
(48) |
Dec
(30) |
| 2017 |
Jan
(33) |
Feb
(35) |
Mar
(100) |
Apr
(87) |
May
(169) |
Jun
(119) |
Jul
(165) |
Aug
(241) |
Sep
(128) |
Oct
(42) |
Nov
|
Dec
|
|
From: Magalhaes, G. (B. R&D-CL) <gui...@hp...> - 2017-07-28 14:20:24
|
> > Each measurement entry in the list could have new fields to identify > > the namespace. Since the namespaces can be reused, a timestamp or > > others fields could be added to uniquely identify the namespace id. > > The more fields included in the measurement list, the more > measurements will be added to the measurement list. Wouldn't it be > enough to know that a certain file has been accessed/executed on the > system and base any analytics/forensics on the IMA-audit data. With the recursive application of policy through the namespace hierarchy, a measurement added to the parent namespace could be misleading since the file pathname makes sense in the current namespace but possibly not for the parent namespace. This is the reason why I believe some new field might be needed in the IMA template format to indicate or uniquely identify the namespace. -- Guilherme |
|
From: Mimi Z. <zo...@li...> - 2017-07-28 12:40:05
|
Hi Thiago,
On Thu, 2017-07-06 at 19:17 -0300, Thiago Jung Bauermann wrote:
> Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
> SHA1 digest, most of the code ignores the array and uses the struct to mean
> "type indicator followed by data of unspecified size" and tracks the real
> size of what the struct represents in a separate length variable.
>
> The only exception to that is the EVM code, which correctly uses the
> definition of struct evm_ima_xattr_data.
Right, the current EVM code converts the EVM signature to an HMAC the
first time the file is accessed. So most of the code is based on the
HMAC.
>
> This patch makes this explicit in the code by removing the length
> specification from the array in struct evm_ima_xattr_data. It also changes
> the name of the element from digest to data, since in most places the array
> doesn't hold a digest.
>
> A separate struct evm_hmac_xattr is introduced, with the original
> definition of evm_ima_xattr_data to be used in the places that actually
> expect that definition.
The new structure name implies that the xattr can only be an HMAC. By
moving the new structure to evm.h we also loose the connection that it
has to the evm_ima_xattr_type enumeration.
Instead, how about defining the new struct in terms of the modified
evm_ima_xattr_data struct? Please leave the new structure in
integrity.h.
Mimi
>
> Signed-off-by: Thiago Jung Bauermann <bau...@li...>
> ---
> security/integrity/evm/evm.h | 5 +++++
> security/integrity/evm/evm_crypto.c | 2 +-
> security/integrity/evm/evm_main.c | 8 ++++----
> security/integrity/ima/ima_appraise.c | 7 ++++---
> security/integrity/integrity.h | 2 +-
> 5 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index f5f12727771a..e1081cf2f9c5 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -24,6 +24,11 @@
> #define EVM_INIT_HMAC 0x0001
> #define EVM_INIT_X509 0x0002
>
> +struct evm_hmac_xattr {
> + u8 type; /* Should be EVM_XATTR_HMAC. */
> + u8 digest[SHA1_DIGEST_SIZE];
> +} __packed;
> +
> extern int evm_initialized;
> extern char *evm_hmac;
> extern char *evm_hash;
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index d7f282d75cc1..08dde59f3128 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -252,7 +252,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
> const char *xattr_value, size_t xattr_value_len)
> {
> struct inode *inode = d_backing_inode(dentry);
> - struct evm_ima_xattr_data xattr_data;
> + struct evm_hmac_xattr xattr_data;
> int rc = 0;
>
> rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 063d38aef64e..b7c1e11a915e 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -116,7 +116,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> struct integrity_iint_cache *iint)
> {
> struct evm_ima_xattr_data *xattr_data = NULL;
> - struct evm_ima_xattr_data calc;
> + struct evm_hmac_xattr calc;
> enum integrity_status evm_status = INTEGRITY_PASS;
> int rc, xattr_len;
>
> @@ -147,7 +147,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> /* check value type */
> switch (xattr_data->type) {
> case EVM_XATTR_HMAC:
> - if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
> + if (xattr_len != sizeof(struct evm_hmac_xattr)) {
> evm_status = INTEGRITY_FAIL;
> goto out;
> }
> @@ -155,7 +155,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> xattr_value_len, calc.digest);
> if (rc)
> break;
> - rc = crypto_memneq(xattr_data->digest, calc.digest,
> + rc = crypto_memneq(xattr_data->data, calc.digest,
> sizeof(calc.digest));
> if (rc)
> rc = -EINVAL;
> @@ -467,7 +467,7 @@ int evm_inode_init_security(struct inode *inode,
> const struct xattr *lsm_xattr,
> struct xattr *evm_xattr)
> {
> - struct evm_ima_xattr_data *xattr_data;
> + struct evm_hmac_xattr *xattr_data;
> int rc;
>
> if (!evm_initialized || !evm_protected_xattr(lsm_xattr->name))
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 809ba70fbbbf..87d2b601cf8e 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -156,7 +156,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
> return sig->hash_algo;
> break;
> case IMA_XATTR_DIGEST_NG:
> - ret = xattr_value->digest[0];
> + /* first byte contains algorithm id */
> + ret = xattr_value->data[0];
> if (ret < HASH_ALGO__LAST)
> return ret;
> break;
> @@ -164,7 +165,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
> /* this is for backward compatibility */
> if (xattr_len == 21) {
> unsigned int zero = 0;
> - if (!memcmp(&xattr_value->digest[16], &zero, 4))
> + if (!memcmp(&xattr_value->data[16], &zero, 4))
> return HASH_ALGO_MD5;
> else
> return HASH_ALGO_SHA1;
> @@ -253,7 +254,7 @@ int ima_appraise_measurement(enum ima_hooks func,
> /* xattr length may be longer. md5 hash in previous
> version occupied 20 bytes in xattr, instead of 16
> */
> - rc = memcmp(&xattr_value->digest[hash_start],
> + rc = memcmp(&xattr_value->data[hash_start],
> iint->ima_hash->digest,
> iint->ima_hash->length);
> else
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index a53e7e4ab06c..874211aba6e9 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -63,7 +63,7 @@ enum evm_ima_xattr_type {
>
> struct evm_ima_xattr_data {
> u8 type;
> - u8 digest[SHA1_DIGEST_SIZE];
> + u8 data[];
> } __packed;
>
> #define IMA_MAX_DIGEST_SIZE 64
|
|
From: vip <ae3@a.sauio.com> - 2017-07-28 06:05:40
|
<html>
<head>
<title></title>
<meta content="text/html; charset=gb2312" http-equiv="Content-Type" />
</head>
<body><font size="5" face="Eras Medium ITC">高效做事，高效开发。 优质客户开发方式，只需你的产品关键词或者你目标客户的产品关键词，即可得到客户信息。 通过精准定位邮件投递，高效开发客户，成交订单。 详询请加 <font size="7"><font color="#0000ff">QQ 2755365043</font> </font> 直接观看演示功能。</font> </body>
</html> |
|
From: Stefan B. <st...@li...> - 2017-07-27 20:52:10
|
On 07/27/2017 03:39 PM, Magalhaes, Guilherme (Brazil R&D-CL) wrote:
>
>> There's a vTPM proxy driver in the kernel that enables spawning a
>> frontend /dev/tpm%d and an anonymous backend file descriptor where a
>> vTPM can listen on for TPM commands. I integrated this with 'swtpm' and
>> I have been working on integrating this into runc. Currently each
>> container started with runc can get one (or multiple) vTPMs and
>> /dev/tpm0 [and /dev/tpmrm0 in case of TPM2] then appear inside the
>> container.
>>
> This is an interesting solution especially for nested namespaces with the
> recursive application of measurements and a having list per container.
>
> Following the TCG specs/requirements, what could we say about security
> guarantees of real TPMs Vs this vTPM implementation?
A non-root user may not be able to do access the (permanent) state of
the vTPM state files since the container management stack would restrict
access to the files using DAC. Access to runtime data is also prevented
since the vTPM would not run under the account of the non-root user.
To protect the vTPM's permanent state file from access by a root user it
comes down to preventing the root user from getting a hold of the key
used for encrypting that file. Encrypting the state of the vTPM is
probably the best we can do to approximate a temper-resistant chip, but
preventing the root user from accessing the key may be more challenging.
Preventing root from accessing runtime data could be achieved by using
XGS or a similar technology.
Stefan
|
|
From: Magalhaes, G. (B. R&D-CL) <gui...@hp...> - 2017-07-27 19:40:43
|
> -----Original Message----- > From: Stefan Berger [mailto:st...@li...] > Sent: quinta-feira, 27 de julho de 2017 14:50 > To: Magalhaes, Guilherme (Brazil R&D-CL) > <gui...@hp...>; Mimi Zohar > <zo...@li...>; Serge E. Hallyn <se...@ha...> > Cc: Mehmet Kayaalp <mka...@cs...>; Yuqiong Sun > <sun...@gm...>; containers <con...@li...- > foundation.org>; linux-kernel <lin...@vg...>; David Safford > <dav...@ge...>; James Bottomley > <Jam...@Ha...>; linux-security-module <linux- > sec...@vg...>; ima-devel <linux-ima- > de...@li...>; Yuqiong Sun <su...@us...> > Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA > namespace support > > On 07/27/2017 01:18 PM, Magalhaes, Guilherme (Brazil R&D-CL) wrote: > > > >> -----Original Message----- > >> From: Mimi Zohar [mailto:zo...@li...] > >> Sent: quinta-feira, 27 de julho de 2017 11:39 > >> To: Magalhaes, Guilherme (Brazil R&D-CL) > >> <gui...@hp...>; Serge E. Hallyn <se...@ha...> > >> Cc: Mehmet Kayaalp <mka...@cs...>; Yuqiong Sun > >> <sun...@gm...>; containers <con...@li...- > >> foundation.org>; linux-kernel <lin...@vg...>; David > Safford > >> <dav...@ge...>; James Bottomley > >> <Jam...@Ha...>; linux-security-module > <linux- > >> sec...@vg...>; ima-devel <linux-ima- > >> de...@li...>; Yuqiong Sun <su...@us...> > >> Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with > IMA > >> namespace support > >> > >> On Thu, 2017-07-27 at 12:51 +0000, Magalhaes, Guilherme (Brazil R&D- > >> CL) wrote: > >>> > >>>> On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote: > >>>>> On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote: > >>>>>> On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote: > >>>>>>> On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote: > >>>>>>>> On 07/25/2017 03:48 PM, Mimi Zohar wrote: > >>>>>>>>> On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote: > >>>>>>>>>> On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote: > >>>>>>>>>>> On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley > >> wrote: > >>>>>>>>>>>> On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote: > >>>>>>>>>>>>> On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp > >>>> wrote: > >>>>>>>>>>>>>> From: Yuqiong Sun <su...@us...> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Add new CONFIG_IMA_NS config option. Let clone() create > >> a > >>>>>>>>>>>>>> new IMA namespace upon CLONE_NEWNS flag. Add > >> ima_ns > >>>> data > >>>>>>>>>>>>>> structure in nsproxy. ima_ns is allocated and freed upon > >>>>>>>>>>>>>> IMA namespace creation and exit. Currently, the ima_ns > >>>>>>>>>>>>>> contains no useful IMA data but only a dummy interface. > >>>>>>>>>>>>>> This patch creates the framework for namespacing the > >>>> different aspects of IMA (eg. > >>>>>>>>>>>>>> IMA-audit, IMA-measurement, IMA-appraisal). > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Signed-off-by: Yuqiong Sun <su...@us...> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Changelog: > >>>>>>>>>>>>>> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA > >> flag > >>>>>>>>>>>>> Hi, > >>>>>>>>>>>>> > >>>>>>>>>>>>> So this means that every mount namespace clone will clone > >> a > >>>>>>>>>>>>> new IMA namespace. Is that really ok? > >>>>>>>>>>>> Based on what: space concerns (struct ima_ns is reasonably > >>>> small)? > >>>>>>>>>>>> or whether tying it to the mount namespace is the correct > >>>>>>>>>>>> thing to do. On > >>>>>>>>>>> Mostly the latter. The other would be not so much space > >>>>>>>>>>> concerns as time concerns. Many things use new mounts > >>>>>>>>>>> namespaces, and we wouldn't want multiple IMA calls on all > >>>>>>>>>>> file accesses by all of those. > >>>>>>>>>>> > >>>>>>>>>>>> the latter, it does seem that this should be a property of > >>>>>>>>>>>> either the mount or user ns rather than its own separate ns. > >>>>>>>>>>>> I could see a use where even a container might want multiple > >>>>>>>>>>>> ima keyrings within the container (say containerised apache > >>>>>>>>>>>> service with multiple tenants), so instinct tells me that > >>>>>>>>>>>> mount ns is the correct granularity for this. > >>>>>>>>>>> I wonder whether we could use echo 1 > > >>>>>>>>>>> /sys/kernel/security/ima/newns as the trigger for requesting > >>>>>>>>>>> a new ima ns on the next clone(CLONE_NEWNS). > >>>>>>>>>> I could go with that, but what about the trigger being > >>>>>>>>>> installing or updating the keyring? That's the only operation > >>>>>>>>>> that needs namespace separation, so on mount ns clone, you > >> get > >>>>>>>>>> a pointer to the old ima_ns until you do something that > >>>>>>>>>> requires a new key, which then triggers the copy of the > >> namespace > >>>> and installing it? > >>>>>>>>> It isn't just the keyrings that need to be namespaced, but the > >>>>>>>>> measurement list and policy as well. > >>>>>>>>> > >>>>>>>>> IMA-measurement, IMA-appraisal and IMA-audit are all policy > >>>> based. > >>>>>>>>> As soon as the namespace starts, measurements should be > >> added > >>>>>>>>> to the namespace specific measurement list, not it's parent. > >>>>>>> Shouldn't it be both? > >>>>>> The policy defines which files are measured. The namespace policy > >>>>>> could be different than it's parent's policy, and the parent's > >>>>>> policy could be different than the native policy. Basically, file > >>>>>> measurements need to be added to the namespace measurement > >> list, > >>>>>> recursively, up to the native measurement list. > >>>>> Yes, but if a task t1 is in namespace ns2 which is a child of > >>>>> namespace ns1, and it accesses a file which ns1's policy says must be > >>>>> measured, then will ns1's required measurement happen (and be > >>>> appended > >>>>> to the ns1 measurement list), whether or not ns2's policy requires it? > >>>> Yes, as the file needs to be measured only in the ns1 policy, the > >>>> measurement would exist in the ns1 measurement list, but not in the > >>>> ns2 measurement list. The pseudo code snippet below might help. > >>> This algorithm is potentially extending a PCR in TPM multiple times > >>> for a single file event under a given namespace and duplicating > >>> entries. Any concerns with performance and memory footprint? > >> Going forward we assume associated with each container will be a vTPM. > >> The namespace measurements will extend a vTPM. As the container > comes > >> and goes the associated measurement list, keyring, and vTPM will come > >> and go as well. For this reason, based on policy, the same file > >> measurement might appear in multiple measurement lists. > > My concern is that the base of namespacing the measurement lists is on > the > > integration of containers with vTPM. Associating vTPM with containers as > > they are today is not a simple integration since vTPM requires a VM and > > containers do not. > > There's a vTPM proxy driver in the kernel that enables spawning a > frontend /dev/tpm%d and an anonymous backend file descriptor where a > vTPM can listen on for TPM commands. I integrated this with 'swtpm' and > I have been working on integrating this into runc. Currently each > container started with runc can get one (or multiple) vTPMs and > /dev/tpm0 [and /dev/tpmrm0 in case of TPM2] then appear inside the > container. > This is an interesting solution especially for nested namespaces with the recursive application of measurements and a having list per container. Following the TCG specs/requirements, what could we say about security guarantees of real TPMs Vs this vTPM implementation? -- Guilherme > What is missing for integration with IMA namespacing are patches for: > > 1) IMA to use a tpm_chip to talk to rather than issue TPM commands with > TPM_ANY_NUM as parameter to TPM driver functions > 2) an ioctl for the vtpm proxy driver to attach a vtpm proxy instance's > tpm_chip to an IMA namespace; this ioctl should be called after the > creation of the IMA namespace (by container mgmt. stack [runc]) > 3) - some way for the IMA namespace to release the vTPM proxy's > 'tpm_chip' and other resources once the IMA namespace is deleted > > I have these patches in some form and would post them at a later stage > of namespacing IMA. > > swtpm: https://github.com/stefanberger/swtpm > libtpms: https://github.com/stefanberger/libtpms > runc pull request: https://github.com/opencontainers/runc/pull/1082 > > Stefan |
|
From: Stefan B. <st...@li...> - 2017-07-27 17:52:12
|
On 07/27/2017 01:18 PM, Magalhaes, Guilherme (Brazil R&D-CL) wrote: > >> -----Original Message----- >> From: Mimi Zohar [mailto:zo...@li...] >> Sent: quinta-feira, 27 de julho de 2017 11:39 >> To: Magalhaes, Guilherme (Brazil R&D-CL) >> <gui...@hp...>; Serge E. Hallyn <se...@ha...> >> Cc: Mehmet Kayaalp <mka...@cs...>; Yuqiong Sun >> <sun...@gm...>; containers <con...@li...- >> foundation.org>; linux-kernel <lin...@vg...>; David Safford >> <dav...@ge...>; James Bottomley >> <Jam...@Ha...>; linux-security-module <linux- >> sec...@vg...>; ima-devel <linux-ima- >> de...@li...>; Yuqiong Sun <su...@us...> >> Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA >> namespace support >> >> On Thu, 2017-07-27 at 12:51 +0000, Magalhaes, Guilherme (Brazil R&D- >> CL) wrote: >>> >>>> On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote: >>>>> On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote: >>>>>> On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote: >>>>>>> On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote: >>>>>>>> On 07/25/2017 03:48 PM, Mimi Zohar wrote: >>>>>>>>> On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote: >>>>>>>>>> On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote: >>>>>>>>>>> On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley >> wrote: >>>>>>>>>>>> On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote: >>>>>>>>>>>>> On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp >>>> wrote: >>>>>>>>>>>>>> From: Yuqiong Sun <su...@us...> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Add new CONFIG_IMA_NS config option. Let clone() create >> a >>>>>>>>>>>>>> new IMA namespace upon CLONE_NEWNS flag. Add >> ima_ns >>>> data >>>>>>>>>>>>>> structure in nsproxy. ima_ns is allocated and freed upon >>>>>>>>>>>>>> IMA namespace creation and exit. Currently, the ima_ns >>>>>>>>>>>>>> contains no useful IMA data but only a dummy interface. >>>>>>>>>>>>>> This patch creates the framework for namespacing the >>>> different aspects of IMA (eg. >>>>>>>>>>>>>> IMA-audit, IMA-measurement, IMA-appraisal). >>>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Yuqiong Sun <su...@us...> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Changelog: >>>>>>>>>>>>>> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA >> flag >>>>>>>>>>>>> Hi, >>>>>>>>>>>>> >>>>>>>>>>>>> So this means that every mount namespace clone will clone >> a >>>>>>>>>>>>> new IMA namespace. Is that really ok? >>>>>>>>>>>> Based on what: space concerns (struct ima_ns is reasonably >>>> small)? >>>>>>>>>>>> or whether tying it to the mount namespace is the correct >>>>>>>>>>>> thing to do. On >>>>>>>>>>> Mostly the latter. The other would be not so much space >>>>>>>>>>> concerns as time concerns. Many things use new mounts >>>>>>>>>>> namespaces, and we wouldn't want multiple IMA calls on all >>>>>>>>>>> file accesses by all of those. >>>>>>>>>>> >>>>>>>>>>>> the latter, it does seem that this should be a property of >>>>>>>>>>>> either the mount or user ns rather than its own separate ns. >>>>>>>>>>>> I could see a use where even a container might want multiple >>>>>>>>>>>> ima keyrings within the container (say containerised apache >>>>>>>>>>>> service with multiple tenants), so instinct tells me that >>>>>>>>>>>> mount ns is the correct granularity for this. >>>>>>>>>>> I wonder whether we could use echo 1 > >>>>>>>>>>> /sys/kernel/security/ima/newns as the trigger for requesting >>>>>>>>>>> a new ima ns on the next clone(CLONE_NEWNS). >>>>>>>>>> I could go with that, but what about the trigger being >>>>>>>>>> installing or updating the keyring? That's the only operation >>>>>>>>>> that needs namespace separation, so on mount ns clone, you >> get >>>>>>>>>> a pointer to the old ima_ns until you do something that >>>>>>>>>> requires a new key, which then triggers the copy of the >> namespace >>>> and installing it? >>>>>>>>> It isn't just the keyrings that need to be namespaced, but the >>>>>>>>> measurement list and policy as well. >>>>>>>>> >>>>>>>>> IMA-measurement, IMA-appraisal and IMA-audit are all policy >>>> based. >>>>>>>>> As soon as the namespace starts, measurements should be >> added >>>>>>>>> to the namespace specific measurement list, not it's parent. >>>>>>> Shouldn't it be both? >>>>>> The policy defines which files are measured. The namespace policy >>>>>> could be different than it's parent's policy, and the parent's >>>>>> policy could be different than the native policy. Basically, file >>>>>> measurements need to be added to the namespace measurement >> list, >>>>>> recursively, up to the native measurement list. >>>>> Yes, but if a task t1 is in namespace ns2 which is a child of >>>>> namespace ns1, and it accesses a file which ns1's policy says must be >>>>> measured, then will ns1's required measurement happen (and be >>>> appended >>>>> to the ns1 measurement list), whether or not ns2's policy requires it? >>>> Yes, as the file needs to be measured only in the ns1 policy, the >>>> measurement would exist in the ns1 measurement list, but not in the >>>> ns2 measurement list. The pseudo code snippet below might help. >>> This algorithm is potentially extending a PCR in TPM multiple times >>> for a single file event under a given namespace and duplicating >>> entries. Any concerns with performance and memory footprint? >> Going forward we assume associated with each container will be a vTPM. >> The namespace measurements will extend a vTPM. As the container comes >> and goes the associated measurement list, keyring, and vTPM will come >> and go as well. For this reason, based on policy, the same file >> measurement might appear in multiple measurement lists. > My concern is that the base of namespacing the measurement lists is on the > integration of containers with vTPM. Associating vTPM with containers as > they are today is not a simple integration since vTPM requires a VM and > containers do not. There's a vTPM proxy driver in the kernel that enables spawning a frontend /dev/tpm%d and an anonymous backend file descriptor where a vTPM can listen on for TPM commands. I integrated this with 'swtpm' and I have been working on integrating this into runc. Currently each container started with runc can get one (or multiple) vTPMs and /dev/tpm0 [and /dev/tpmrm0 in case of TPM2] then appear inside the container. What is missing for integration with IMA namespacing are patches for: 1) IMA to use a tpm_chip to talk to rather than issue TPM commands with TPM_ANY_NUM as parameter to TPM driver functions 2) an ioctl for the vtpm proxy driver to attach a vtpm proxy instance's tpm_chip to an IMA namespace; this ioctl should be called after the creation of the IMA namespace (by container mgmt. stack [runc]) 3) - some way for the IMA namespace to release the vTPM proxy's 'tpm_chip' and other resources once the IMA namespace is deleted I have these patches in some form and would post them at a later stage of namespacing IMA. swtpm: https://github.com/stefanberger/swtpm libtpms: https://github.com/stefanberger/libtpms runc pull request: https://github.com/opencontainers/runc/pull/1082 Stefan |
|
From: Magalhaes, G. (B. R&D-CL) <gui...@hp...> - 2017-07-27 17:18:33
|
> -----Original Message----- > From: Mimi Zohar [mailto:zo...@li...] > Sent: quinta-feira, 27 de julho de 2017 11:39 > To: Magalhaes, Guilherme (Brazil R&D-CL) > <gui...@hp...>; Serge E. Hallyn <se...@ha...> > Cc: Mehmet Kayaalp <mka...@cs...>; Yuqiong Sun > <sun...@gm...>; containers <con...@li...- > foundation.org>; linux-kernel <lin...@vg...>; David Safford > <dav...@ge...>; James Bottomley > <Jam...@Ha...>; linux-security-module <linux- > sec...@vg...>; ima-devel <linux-ima- > de...@li...>; Yuqiong Sun <su...@us...> > Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA > namespace support > > On Thu, 2017-07-27 at 12:51 +0000, Magalhaes, Guilherme (Brazil R&D- > CL) wrote: > > > > > > > On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote: > > > > On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote: > > > > > On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote: > > > > > > On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote: > > > > > > > On 07/25/2017 03:48 PM, Mimi Zohar wrote: > > > > > > > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote: > > > > > > > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote: > > > > > > > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley > wrote: > > > > > > > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote: > > > > > > > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp > > > wrote: > > > > > > > >>>>>> > > > > > > > >>>>>>From: Yuqiong Sun <su...@us...> > > > > > > > >>>>>> > > > > > > > >>>>>>Add new CONFIG_IMA_NS config option. Let clone() create > a > > > > > > > >>>>>>new IMA namespace upon CLONE_NEWNS flag. Add > ima_ns > > > data > > > > > > > >>>>>>structure in nsproxy. ima_ns is allocated and freed upon > > > > > > > >>>>>>IMA namespace creation and exit. Currently, the ima_ns > > > > > > > >>>>>>contains no useful IMA data but only a dummy interface. > > > > > > > >>>>>>This patch creates the framework for namespacing the > > > different aspects of IMA (eg. > > > > > > > >>>>>>IMA-audit, IMA-measurement, IMA-appraisal). > > > > > > > >>>>>> > > > > > > > >>>>>>Signed-off-by: Yuqiong Sun <su...@us...> > > > > > > > >>>>>> > > > > > > > >>>>>>Changelog: > > > > > > > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA > flag > > > > > > > >>>>>Hi, > > > > > > > >>>>> > > > > > > > >>>>>So this means that every mount namespace clone will clone > a > > > > > > > >>>>>new IMA namespace. Is that really ok? > > > > > > > >>>>Based on what: space concerns (struct ima_ns is reasonably > > > small)? > > > > > > > >>>>or whether tying it to the mount namespace is the correct > > > > > > > >>>>thing to do. On > > > > > > > >>>Mostly the latter. The other would be not so much space > > > > > > > >>>concerns as time concerns. Many things use new mounts > > > > > > > >>>namespaces, and we wouldn't want multiple IMA calls on all > > > > > > > >>>file accesses by all of those. > > > > > > > >>> > > > > > > > >>>>the latter, it does seem that this should be a property of > > > > > > > >>>>either the mount or user ns rather than its own separate ns. > > > > > > > >>>>I could see a use where even a container might want multiple > > > > > > > >>>>ima keyrings within the container (say containerised apache > > > > > > > >>>>service with multiple tenants), so instinct tells me that > > > > > > > >>>>mount ns is the correct granularity for this. > > > > > > > >>>I wonder whether we could use echo 1 > > > > > > > > >>>/sys/kernel/security/ima/newns as the trigger for requesting > > > > > > > >>>a new ima ns on the next clone(CLONE_NEWNS). > > > > > > > >>I could go with that, but what about the trigger being > > > > > > > >>installing or updating the keyring? That's the only operation > > > > > > > >>that needs namespace separation, so on mount ns clone, you > get > > > > > > > >>a pointer to the old ima_ns until you do something that > > > > > > > >>requires a new key, which then triggers the copy of the > namespace > > > and installing it? > > > > > > > >It isn't just the keyrings that need to be namespaced, but the > > > > > > > >measurement list and policy as well. > > > > > > > > > > > > > > > >IMA-measurement, IMA-appraisal and IMA-audit are all policy > > > based. > > > > > > > > > > > > > > > >As soon as the namespace starts, measurements should be > added > > > > > > > >to the namespace specific measurement list, not it's parent. > > > > > > > > > > > > Shouldn't it be both? > > > > > > > > > > The policy defines which files are measured. The namespace policy > > > > > could be different than it's parent's policy, and the parent's > > > > > policy could be different than the native policy. Basically, file > > > > > measurements need to be added to the namespace measurement > list, > > > > > recursively, up to the native measurement list. > > > > > > > > Yes, but if a task t1 is in namespace ns2 which is a child of > > > > namespace ns1, and it accesses a file which ns1's policy says must be > > > > measured, then will ns1's required measurement happen (and be > > > appended > > > > to the ns1 measurement list), whether or not ns2's policy requires it? > > > > > > Yes, as the file needs to be measured only in the ns1 policy, the > > > measurement would exist in the ns1 measurement list, but not in the > > > ns2 measurement list. The pseudo code snippet below might help. > > > > This algorithm is potentially extending a PCR in TPM multiple times > > for a single file event under a given namespace and duplicating > > entries. Any concerns with performance and memory footprint? > > Going forward we assume associated with each container will be a vTPM. > The namespace measurements will extend a vTPM. As the container comes > and goes the associated measurement list, keyring, and vTPM will come > and go as well. For this reason, based on policy, the same file > measurement might appear in multiple measurement lists. My concern is that the base of namespacing the measurement lists is on the integration of containers with vTPM. Associating vTPM with containers as they are today is not a simple integration since vTPM requires a VM and containers do not. I cannot envision this association right now, but it might be possible after some research to understand the existent possibilities. For example, Intel SGX or clear containers may help with this integration. However, these technologies have trade-offs which could restrict adoption. -- Guilherme > > > What is the reason to adding a measurement to multiple namespace > > measurement lists? How are these lists going to be used? For Remote > > Attestation we need a single list (the native one) which has the > > complete list of measurements and in the same order they were > > extended in the TPM. Additionally, when namespaces are released, > > would the measurement list under that namespace disappear? How to > > store this list considering the namespaces may have a short life and > > be reused thousands of times? > > Different scenarios have different requirements. You're assuming that > only the system owner is interested in the measurement list, not the > container owner. > > The current builtin measurement policies measure everything executed > on the system and anything accessed by real root. The namespace > policy would probably be similar, but instead of measuring files > accessed by real root, it would be files accessed by root in the > namespace. > > > Should the native measurement list have all measurements triggered > > in the whole system, including the ones made under other namespaces? > > Following the algorithm below, if the file is not in the policy of > > the 'native'/initial namespace, the measurement is not added to the > > native measurement list. > > Correct. The policy controls what is included measured, appraised, > and audited. > > > Each measurement entry in the list could have new fields to identify > > the namespace. Since the namespaces can be reused, a timestamp or > > others fields could be added to uniquely identify the namespace id. > > The more fields included in the measurement list, the more > measurements will be added to the measurement list. Wouldn't it be > enough to know that a certain file has been accessed/executed on the > system and base any analytics/forensics on the IMA-audit data. > > > Regarding namespace hierarchy and IMA policy, we could assume that > > if a given namespace has no policy set, the policy of that namespace > > parent is applied and then the native/initial namespace should > > always have a set policy. > > We shouldn't assume measure, appraise, or audit by default. Just like > it is up to the native system to define a policy or if there is a > policy, the "container" owner should define the policy, or if there is > a policy. > > Mimi > > > > > > > do { > > > . > > > . > > > > > > /* calculate file hash based on xattr algorithm */ > > > collect_measurement() > > > > > > /* recursively added to each namespace based on policy */ > > > ima_store_measurement() > > > > > > /* Based on the specific namespace policy and keys. */ > > > if (!once) { > > > once = 1; > > > result = ima_appraise_measurement() > > > } > > > > > > ima_audit_measurement() > > > > > > } while ((ns = ns->parent)); > > > > > > return result; > > > > > > Mimi |
|
From: Mimi Z. <zo...@li...> - 2017-07-27 14:42:22
|
On Thu, 2017-07-27 at 12:51 +0000, Magalhaes, Guilherme (Brazil R&D-
CL) wrote:
>
>
> > On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote:
> > > On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote:
> > > > On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote:
> > > > > On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
> > > > > > On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> > > > > > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> > > > > > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> > > > > > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> > > > > > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> > > > > > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp
> > wrote:
> > > > > > >>>>>>
> > > > > > >>>>>>From: Yuqiong Sun <su...@us...>
> > > > > > >>>>>>
> > > > > > >>>>>>Add new CONFIG_IMA_NS config option. Let clone() create a
> > > > > > >>>>>>new IMA namespace upon CLONE_NEWNS flag. Add ima_ns
> > data
> > > > > > >>>>>>structure in nsproxy. ima_ns is allocated and freed upon
> > > > > > >>>>>>IMA namespace creation and exit. Currently, the ima_ns
> > > > > > >>>>>>contains no useful IMA data but only a dummy interface.
> > > > > > >>>>>>This patch creates the framework for namespacing the
> > different aspects of IMA (eg.
> > > > > > >>>>>>IMA-audit, IMA-measurement, IMA-appraisal).
> > > > > > >>>>>>
> > > > > > >>>>>>Signed-off-by: Yuqiong Sun <su...@us...>
> > > > > > >>>>>>
> > > > > > >>>>>>Changelog:
> > > > > > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> > > > > > >>>>>Hi,
> > > > > > >>>>>
> > > > > > >>>>>So this means that every mount namespace clone will clone a
> > > > > > >>>>>new IMA namespace. Is that really ok?
> > > > > > >>>>Based on what: space concerns (struct ima_ns is reasonably
> > small)?
> > > > > > >>>>or whether tying it to the mount namespace is the correct
> > > > > > >>>>thing to do. On
> > > > > > >>>Mostly the latter. The other would be not so much space
> > > > > > >>>concerns as time concerns. Many things use new mounts
> > > > > > >>>namespaces, and we wouldn't want multiple IMA calls on all
> > > > > > >>>file accesses by all of those.
> > > > > > >>>
> > > > > > >>>>the latter, it does seem that this should be a property of
> > > > > > >>>>either the mount or user ns rather than its own separate ns.
> > > > > > >>>>I could see a use where even a container might want multiple
> > > > > > >>>>ima keyrings within the container (say containerised apache
> > > > > > >>>>service with multiple tenants), so instinct tells me that
> > > > > > >>>>mount ns is the correct granularity for this.
> > > > > > >>>I wonder whether we could use echo 1 >
> > > > > > >>>/sys/kernel/security/ima/newns as the trigger for requesting
> > > > > > >>>a new ima ns on the next clone(CLONE_NEWNS).
> > > > > > >>I could go with that, but what about the trigger being
> > > > > > >>installing or updating the keyring? That's the only operation
> > > > > > >>that needs namespace separation, so on mount ns clone, you get
> > > > > > >>a pointer to the old ima_ns until you do something that
> > > > > > >>requires a new key, which then triggers the copy of the namespace
> > and installing it?
> > > > > > >It isn't just the keyrings that need to be namespaced, but the
> > > > > > >measurement list and policy as well.
> > > > > > >
> > > > > > >IMA-measurement, IMA-appraisal and IMA-audit are all policy
> > based.
> > > > > > >
> > > > > > >As soon as the namespace starts, measurements should be added
> > > > > > >to the namespace specific measurement list, not it's parent.
> > > > >
> > > > > Shouldn't it be both?
> > > >
> > > > The policy defines which files are measured. The namespace policy
> > > > could be different than it's parent's policy, and the parent's
> > > > policy could be different than the native policy. Basically, file
> > > > measurements need to be added to the namespace measurement list,
> > > > recursively, up to the native measurement list.
> > >
> > > Yes, but if a task t1 is in namespace ns2 which is a child of
> > > namespace ns1, and it accesses a file which ns1's policy says must be
> > > measured, then will ns1's required measurement happen (and be
> > appended
> > > to the ns1 measurement list), whether or not ns2's policy requires it?
> >
> > Yes, as the file needs to be measured only in the ns1 policy, the
> > measurement would exist in the ns1 measurement list, but not in the
> > ns2 measurement list. The pseudo code snippet below might help.
>
> This algorithm is potentially extending a PCR in TPM multiple times
> for a single file event under a given namespace and duplicating
> entries. Any concerns with performance and memory footprint?
Going forward we assume associated with each container will be a vTPM.
The namespace measurements will extend a vTPM. As the container comes
and goes the associated measurement list, keyring, and vTPM will come
and go as well. For this reason, based on policy, the same file
measurement might appear in multiple measurement lists.
> What is the reason to adding a measurement to multiple namespace
> measurement lists? How are these lists going to be used? For Remote
> Attestation we need a single list (the native one) which has the
> complete list of measurements and in the same order they were
> extended in the TPM. Additionally, when namespaces are released,
> would the measurement list under that namespace disappear? How to
> store this list considering the namespaces may have a short life and
> be reused thousands of times?
Different scenarios have different requirements. You're assuming that
only the system owner is interested in the measurement list, not the
container owner.
The current builtin measurement policies measure everything executed
on the system and anything accessed by real root. The namespace
policy would probably be similar, but instead of measuring files
accessed by real root, it would be files accessed by root in the
namespace.
> Should the native measurement list have all measurements triggered
> in the whole system, including the ones made under other namespaces?
> Following the algorithm below, if the file is not in the policy of
> the 'native'/initial namespace, the measurement is not added to the
> native measurement list.
Correct. The policy controls what is included measured, appraised,
and audited.
> Each measurement entry in the list could have new fields to identify
> the namespace. Since the namespaces can be reused, a timestamp or
> others fields could be added to uniquely identify the namespace id.
The more fields included in the measurement list, the more
measurements will be added to the measurement list. Wouldn't it be
enough to know that a certain file has been accessed/executed on the
system and base any analytics/forensics on the IMA-audit data.
> Regarding namespace hierarchy and IMA policy, we could assume that
> if a given namespace has no policy set, the policy of that namespace
> parent is applied and then the native/initial namespace should
> always have a set policy.
We shouldn't assume measure, appraise, or audit by default. Just like
it is up to the native system to define a policy or if there is a
policy, the "container" owner should define the policy, or if there is
a policy.
Mimi
> >
> > do {
> > .
> > .
> >
> > /* calculate file hash based on xattr algorithm */
> > collect_measurement()
> >
> > /* recursively added to each namespace based on policy */
> > ima_store_measurement()
> >
> > /* Based on the specific namespace policy and keys. */
> > if (!once) {
> > once = 1;
> > result = ima_appraise_measurement()
> > }
> >
> > ima_audit_measurement()
> >
> > } while ((ns = ns->parent));
> >
> > return result;
> >
> > Mimi
|
|
From: Magalhaes, G. (B. R&D-CL) <gui...@hp...> - 2017-07-27 12:51:45
|
> -----Original Message----- > From: Mimi Zohar [mailto:zo...@li...] > Sent: terça-feira, 25 de julho de 2017 18:29 > To: Serge E. Hallyn <se...@ha...> > Cc: Mehmet Kayaalp <mka...@cs...>; Yuqiong Sun > <sun...@gm...>; containers <con...@li...- > foundation.org>; linux-kernel <lin...@vg...>; David Safford > <dav...@ge...>; James Bottomley > <Jam...@Ha...>; linux-security-module <linux- > sec...@vg...>; ima-devel <linux-ima- > de...@li...>; Yuqiong Sun <su...@us...> > Subject: Re: [Linux-ima-devel] [RFC PATCH 1/5] ima: extend clone() with IMA > namespace support > > On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote: > > On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote: > > > On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote: > > > > On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote: > > > > > On 07/25/2017 03:48 PM, Mimi Zohar wrote: > > > > > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote: > > > > > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote: > > > > > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote: > > > > > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote: > > > > > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp > wrote: > > > > > >>>>>> > > > > > >>>>>>From: Yuqiong Sun <su...@us...> > > > > > >>>>>> > > > > > >>>>>>Add new CONFIG_IMA_NS config option. Let clone() create a > > > > > >>>>>>new IMA namespace upon CLONE_NEWNS flag. Add ima_ns > data > > > > > >>>>>>structure in nsproxy. ima_ns is allocated and freed upon > > > > > >>>>>>IMA namespace creation and exit. Currently, the ima_ns > > > > > >>>>>>contains no useful IMA data but only a dummy interface. > > > > > >>>>>>This patch creates the framework for namespacing the > different aspects of IMA (eg. > > > > > >>>>>>IMA-audit, IMA-measurement, IMA-appraisal). > > > > > >>>>>> > > > > > >>>>>>Signed-off-by: Yuqiong Sun <su...@us...> > > > > > >>>>>> > > > > > >>>>>>Changelog: > > > > > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag > > > > > >>>>>Hi, > > > > > >>>>> > > > > > >>>>>So this means that every mount namespace clone will clone a > > > > > >>>>>new IMA namespace. Is that really ok? > > > > > >>>>Based on what: space concerns (struct ima_ns is reasonably > small)? > > > > > >>>>or whether tying it to the mount namespace is the correct > > > > > >>>>thing to do. On > > > > > >>>Mostly the latter. The other would be not so much space > > > > > >>>concerns as time concerns. Many things use new mounts > > > > > >>>namespaces, and we wouldn't want multiple IMA calls on all > > > > > >>>file accesses by all of those. > > > > > >>> > > > > > >>>>the latter, it does seem that this should be a property of > > > > > >>>>either the mount or user ns rather than its own separate ns. > > > > > >>>>I could see a use where even a container might want multiple > > > > > >>>>ima keyrings within the container (say containerised apache > > > > > >>>>service with multiple tenants), so instinct tells me that > > > > > >>>>mount ns is the correct granularity for this. > > > > > >>>I wonder whether we could use echo 1 > > > > > > >>>/sys/kernel/security/ima/newns as the trigger for requesting > > > > > >>>a new ima ns on the next clone(CLONE_NEWNS). > > > > > >>I could go with that, but what about the trigger being > > > > > >>installing or updating the keyring? That's the only operation > > > > > >>that needs namespace separation, so on mount ns clone, you get > > > > > >>a pointer to the old ima_ns until you do something that > > > > > >>requires a new key, which then triggers the copy of the namespace > and installing it? > > > > > >It isn't just the keyrings that need to be namespaced, but the > > > > > >measurement list and policy as well. > > > > > > > > > > > >IMA-measurement, IMA-appraisal and IMA-audit are all policy > based. > > > > > > > > > > > >As soon as the namespace starts, measurements should be added > > > > > >to the namespace specific measurement list, not it's parent. > > > > > > > > Shouldn't it be both? > > > > > > The policy defines which files are measured. The namespace policy > > > could be different than it's parent's policy, and the parent's > > > policy could be different than the native policy. Basically, file > > > measurements need to be added to the namespace measurement list, > > > recursively, up to the native measurement list. > > > > Yes, but if a task t1 is in namespace ns2 which is a child of > > namespace ns1, and it accesses a file which ns1's policy says must be > > measured, then will ns1's required measurement happen (and be > appended > > to the ns1 measurement list), whether or not ns2's policy requires it? > > Yes, as the file needs to be measured only in the ns1 policy, the > measurement would exist in the ns1 measurement list, but not in the > ns2 measurement list. The pseudo code snippet below might help. This algorithm is potentially extending a PCR in TPM multiple times for a single file event under a given namespace and duplicating entries. Any concerns with performance and memory footprint? What is the reason to adding a measurement to multiple namespace measurement lists? How are these lists going to be used? For Remote Attestation we need a single list (the native one) which has the complete list of measurements and in the same order they were extended in the TPM. Additionally, when namespaces are released, would the measurement list under that namespace disappear? How to store this list considering the namespaces may have a short life and be reused thousands of times? Should the native measurement list have all measurements triggered in the whole system, including the ones made under other namespaces? Following the algorithm below, if the file is not in the policy of the 'native'/initial namespace, the measurement is not added to the native measurement list. Each measurement entry in the list could have new fields to identify the namespace. Since the namespaces can be reused, a timestamp or others fields could be added to uniquely identify the namespace id. Regarding namespace hierarchy and IMA policy, we could assume that if a given namespace has no policy set, the policy of that namespace parent is applied and then the native/initial namespace should always have a set policy. > > do { > . > . > > /* calculate file hash based on xattr algorithm */ > collect_measurement() > > /* recursively added to each namespace based on policy */ > ima_store_measurement() > > /* Based on the specific namespace policy and keys. */ > if (!once) { > once = 1; > result = ima_appraise_measurement() > } > > ima_audit_measurement() > > } while ((ns = ns->parent)); > > return result; > > Mimi > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most engaging > tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Linux-ima-devel mailing list > Lin...@li... > https://lists.sourceforge.net/lists/listinfo/linux-ima-devel |
|
From: kbuild t. r. <lk...@in...> - 2017-07-27 05:38:40
|
Hi Roberto, [auto build test WARNING on integrity/next] [also build test WARNING on v4.13-rc2 next-20170726] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Roberto-Sassu/ima-measure-digest-lists-instead-of-individual-files/20170727-123131 base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next config: x86_64-randconfig-x000-201730 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): In file included from security/integrity/ima/ima_fs.c:27:0: security/integrity/ima/ima.h: In function 'ima_parse_digest_list_metadata': security/integrity/ima/ima.h:165:10: error: 'ENOTSUP' undeclared (first use in this function) return -ENOTSUP; ^~~~~~~ security/integrity/ima/ima.h:165:10: note: each undeclared identifier is reported only once for each function it appears in >> security/integrity/ima/ima.h:166:1: warning: control reaches end of non-void function [-Wreturn-type] } ^ vim +166 security/integrity/ima/ima.h d68a6fe9f Mimi Zohar 2016-12-19 135 3323eec92 Mimi Zohar 2009-02-04 136 /* Internal IMA function definitions */ 3323eec92 Mimi Zohar 2009-02-04 137 int ima_init(void); bab739378 Mimi Zohar 2009-02-04 138 int ima_fs_init(void); 3323eec92 Mimi Zohar 2009-02-04 139 int ima_add_template_entry(struct ima_template_entry *entry, int violation, 9803d413f Roberto Sassu 2013-06-07 140 const char *op, struct inode *inode, 9803d413f Roberto Sassu 2013-06-07 141 const unsigned char *filename); c7c8bb237 Dmitry Kasatkin 2013-04-25 142 int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash); 11d7646df Dmitry Kasatkin 2014-04-17 143 int ima_calc_buffer_hash(const void *buf, loff_t len, 11d7646df Dmitry Kasatkin 2014-04-17 144 struct ima_digest_data *hash); b6f8f16f4 Roberto Sassu 2013-11-08 145 int ima_calc_field_array_hash(struct ima_field_data *field_data, b6f8f16f4 Roberto Sassu 2013-11-08 146 struct ima_template_desc *desc, int num_fields, c7c8bb237 Dmitry Kasatkin 2013-04-25 147 struct ima_digest_data *hash); 09ef54359 Dmitry Kasatkin 2013-06-07 148 int __init ima_calc_boot_aggregate(struct ima_digest_data *hash); 7d802a227 Roberto Sassu 2013-06-07 149 void ima_add_violation(struct file *file, const unsigned char *filename, 8d94eb9b5 Roberto Sassu 2015-04-11 150 struct integrity_iint_cache *iint, 3323eec92 Mimi Zohar 2009-02-04 151 const char *op, const char *cause); 76bb28f61 Dmitry Kasatkin 2012-06-08 152 int ima_init_crypto(void); 3ce1217d6 Roberto Sassu 2013-06-07 153 void ima_putc(struct seq_file *m, void *data, int datalen); 45b26133b Mimi Zohar 2015-06-11 154 void ima_print_digest(struct seq_file *m, u8 *digest, u32 size); a71dc65d3 Roberto Sassu 2013-06-07 155 struct ima_template_desc *ima_template_desc_current(void); 94c3aac56 Mimi Zohar 2016-12-19 156 int ima_restore_measurement_entry(struct ima_template_entry *entry); 94c3aac56 Mimi Zohar 2016-12-19 157 int ima_restore_measurement_list(loff_t bufsize, void *buf); 4b1c19b3d Roberto Sassu 2017-07-25 158 struct ima_digest *ima_lookup_loaded_digest(u8 *digest); 4b1c19b3d Roberto Sassu 2017-07-25 159 int ima_add_digest_data_entry(u8 *digest); 3580b2df6 Roberto Sassu 2017-07-25 160 #ifdef CONFIG_IMA_DIGEST_LIST 3580b2df6 Roberto Sassu 2017-07-25 161 ssize_t ima_parse_digest_list_metadata(loff_t size, void *buf); 3580b2df6 Roberto Sassu 2017-07-25 162 #else 3580b2df6 Roberto Sassu 2017-07-25 163 static inline ssize_t ima_parse_digest_list_metadata(loff_t size, void *buf) 3580b2df6 Roberto Sassu 2017-07-25 164 { 3580b2df6 Roberto Sassu 2017-07-25 @165 return -ENOTSUP; 3580b2df6 Roberto Sassu 2017-07-25 @166 } 3580b2df6 Roberto Sassu 2017-07-25 167 #endif 7b8589cc2 Mimi Zohar 2016-12-19 168 int ima_measurements_show(struct seq_file *m, void *v); d158847ae Mimi Zohar 2016-12-19 169 unsigned long ima_get_binary_runtime_size(void); a71dc65d3 Roberto Sassu 2013-06-07 170 int ima_init_template(void); 3f23d624d Mimi Zohar 2016-12-19 171 void ima_init_template_list(void); 3323eec92 Mimi Zohar 2009-02-04 172 :::::: The code at line 166 was first introduced by commit :::::: 3580b2df63c2ec47030a481fea2d2c865124aff4 ima: added parser of digest lists metadata :::::: TO: Roberto Sassu <rob...@hu...> :::::: CC: 0day robot <fen...@in...> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation |
|
From: kbuild t. r. <lk...@in...> - 2017-07-27 05:21:02
|
Hi Roberto, [auto build test ERROR on integrity/next] [also build test ERROR on v4.13-rc2 next-20170726] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Roberto-Sassu/ima-measure-digest-lists-instead-of-individual-files/20170727-123131 base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next config: x86_64-randconfig-x000-201730 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): In file included from security/integrity/ima/ima_fs.c:27:0: security/integrity/ima/ima.h: In function 'ima_parse_digest_list_metadata': >> security/integrity/ima/ima.h:165:10: error: 'ENOTSUP' undeclared (first use in this function) return -ENOTSUP; ^~~~~~~ security/integrity/ima/ima.h:165:10: note: each undeclared identifier is reported only once for each function it appears in vim +/ENOTSUP +165 security/integrity/ima/ima.h 135 136 /* Internal IMA function definitions */ 137 int ima_init(void); 138 int ima_fs_init(void); 139 int ima_add_template_entry(struct ima_template_entry *entry, int violation, 140 const char *op, struct inode *inode, 141 const unsigned char *filename); 142 int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash); 143 int ima_calc_buffer_hash(const void *buf, loff_t len, 144 struct ima_digest_data *hash); 145 int ima_calc_field_array_hash(struct ima_field_data *field_data, 146 struct ima_template_desc *desc, int num_fields, 147 struct ima_digest_data *hash); 148 int __init ima_calc_boot_aggregate(struct ima_digest_data *hash); 149 void ima_add_violation(struct file *file, const unsigned char *filename, 150 struct integrity_iint_cache *iint, 151 const char *op, const char *cause); 152 int ima_init_crypto(void); 153 void ima_putc(struct seq_file *m, void *data, int datalen); 154 void ima_print_digest(struct seq_file *m, u8 *digest, u32 size); 155 struct ima_template_desc *ima_template_desc_current(void); 156 int ima_restore_measurement_entry(struct ima_template_entry *entry); 157 int ima_restore_measurement_list(loff_t bufsize, void *buf); 158 struct ima_digest *ima_lookup_loaded_digest(u8 *digest); 159 int ima_add_digest_data_entry(u8 *digest); 160 #ifdef CONFIG_IMA_DIGEST_LIST 161 ssize_t ima_parse_digest_list_metadata(loff_t size, void *buf); 162 #else 163 static inline ssize_t ima_parse_digest_list_metadata(loff_t size, void *buf) 164 { > 165 return -ENOTSUP; 166 } 167 #endif 168 int ima_measurements_show(struct seq_file *m, void *v); 169 unsigned long ima_get_binary_runtime_size(void); 170 int ima_init_template(void); 171 void ima_init_template_list(void); 172 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation |
|
From: kbuild t. r. <lk...@in...> - 2017-07-27 05:04:38
|
Hi Roberto, [auto build test WARNING on integrity/next] [also build test WARNING on v4.13-rc2 next-20170726] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Roberto-Sassu/ima-measure-digest-lists-instead-of-individual-files/20170727-123131 base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next config: xtensa-allyesconfig (attached as .config) compiler: xtensa-linux-gcc (GCC) 4.9.0 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=xtensa All warnings (new ones prefixed by >>): security/integrity/ima/ima_digest_list.c: In function 'ima_parse_rpm': >> security/integrity/ima/ima_digest_list.c:147:4: warning: ignoring return value of 'hex2bin', declared with attribute warn_unused_result [-Wunused-result] hex2bin(digest, datap, digest_len); ^ vim +/hex2bin +147 security/integrity/ima/ima_digest_list.c 98 99 static int ima_parse_rpm(loff_t size, void *buf) 100 { 101 void *bufp = buf, *bufendp = buf + size; 102 struct rpm_hdr *hdr = bufp; 103 u32 tags = be32_to_cpu(hdr->tags); 104 struct rpm_entryinfo *entry; 105 void *datap = bufp + sizeof(*hdr) + tags * sizeof(struct rpm_entryinfo); 106 int digest_len = hash_digest_size[ima_hash_algo]; 107 u8 digest[digest_len]; 108 int ret, i, j; 109 110 const unsigned char rpm_header_magic[8] = { 111 0x8e, 0xad, 0xe8, 0x01, 0x00, 0x00, 0x00, 0x00 112 }; 113 114 if (size < sizeof(*hdr)) { 115 pr_err("Missing RPM header\n"); 116 return -EINVAL; 117 } 118 119 if (memcmp(bufp, rpm_header_magic, sizeof(rpm_header_magic))) { 120 pr_err("Invalid RPM header\n"); 121 return -EINVAL; 122 } 123 124 bufp += sizeof(*hdr); 125 126 for (i = 0; i < tags && (bufp + sizeof(*entry)) <= bufendp; 127 i++, bufp += sizeof(*entry)) { 128 entry = bufp; 129 130 if (be32_to_cpu(entry->tag) != RPMTAG_FILEDIGESTS) 131 continue; 132 133 datap += be32_to_cpu(entry->offset); 134 135 for (j = 0; j < be32_to_cpu(entry->count) && 136 datap < bufendp; j++) { 137 if (strlen(datap) == 0) { 138 datap++; 139 continue; 140 } 141 142 if (datap + digest_len * 2 + 1 > bufendp) { 143 pr_err("RPM header read at invalid offset\n"); 144 return -EINVAL; 145 } 146 > 147 hex2bin(digest, datap, digest_len); 148 149 ret = ima_add_digest_data_entry(digest); 150 if (ret < 0 && ret != -EEXIST) 151 return ret; 152 153 datap += digest_len * 2 + 1; 154 } 155 156 break; 157 } 158 159 return 0; 160 } 161 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation |
|
From: Mimi Z. <zo...@li...> - 2017-07-26 21:54:43
|
Hi Roberto, [cc'ing tpmdd-devel] On Tue, 2017-07-25 at 17:44 +0200, Roberto Sassu wrote: > This patch set applies on top of kernel v4.13-rc2. > > IMA, for each file matching policy rules, calculates a digest, creates > a new entry in the measurement list and extends a TPM PCR with the digest > of entry data. The last step causes a noticeable performance reduction. > > Since systems likely access the same files, repeating the above tasks at > every boot can be avoided by replacing individual measurements of likely > accessed files with only one measurement of their digests: the advantage > is that the system performance significantly improves due to less PCR > extend operations; on the other hand, the information about which files > have exactly been accessed and in which sequence is lost. > > If this new measurement reports only good digests (e.g. those of > files included in a Linux distribution), and if verifiers only check > that a system executed good software and didn't access malicious data, > the disadvantages reported earlier would be acceptable. > > The Trusted Computing paradigm measure & load is still respected by IMA > with the proposed optimization. If a file being accessed is not in a > measured digest list, a measurement will be recorded as before. If it is, > the list has already been measured, and the verifier must assume that > files with digest in the list have been accessed. > > Measuring digest lists gives the following benefits: > > - boot time reduction > For a minimal Linux installation with 1400 measurements, the boot time > decreases from 1 minute 30 seconds to 15 seconds, after loading to IMA > the digest of all files packaged by the distribution (32000). The new > list contains 92 entries. Without IMA, the boot time is 8.5 seconds. Before we "fix" a TPM performance problem in IMA, we need to really understand the performance problem first. We've added a "TPM peformance" topic to the Linux Plumber Conference TPM microconference - http://wiki.linuxplumbersconf.org/2017:tpms. We've benchmarked a couple of different TPMs on different systems with TPMs on LPC, I2C, and STI. Originally we were seeing even worse performance than your 1 minute 30 seconds for 1400 measurements. Fortunately, we were able to bring it down to about 17 seconds for a 1000 TPM extends. Refer to commits a233a0289cf9 "tpm: msleep() delays - replace with usleep_range() in i2c nuvoton driver" and 0afb7118ae02 "tpm: add sleep only for retry in i2c_nuvoton_write_status()" for the details. Hamza Attak posted a similar patch to the tpmdd-devel mailing list replacing msleep() with usleep_range() calls. Unfortunately, we're seeing really poor performance with another TPM for other reasons. Mimi > > - lower network and CPU requirements for remote attestation > With the IMA optimization, both the measurement and digest lists > must be verified for a complete evaluation. However, since the lists > are fixed, they could be sent to and checked by the verifier only once. > Then, during a remote attestation, the only remaining task is to verify > the short measurement list. > > - signature-based remote attestation > Digest list signature can be used as a proof of the provenance for the > files whose digest is in the list. Then, if verifiers trust the signer > and only check provenance, remote attestation verification would simply > consist on checking digest lists signatures and that the measurement > list only contain list metadata digests (reference measurement databases > would be no longer required). An example of a signed digest list, > that can be parsed with this patch set, is the RPM package header. > > Digest lists are loaded in two stages by IMA through the new securityfs > interface called 'digest_lists'. Users supply metadata, for the digest > lists they want to load: path, format, digest, signature and algorithm > of the digest. > > Then, after the metadata digest is added to the measurement list, IMA > reads the digest lists at the path specified and loads the digests in > a hash table (digest lists are not measured, since their digest is already > included in the metadata). With metadata measurement instead of digest list > measurement, it is possible to avoid a performance reduction that would > occur by measuring many digest lists (e.g. RPM headers) individually. > If, alternatively, digest lists are loaded together, their signature > cannot be verified. > > Lastly, when a file is accessed, IMA searches the calculated digest in > the hash table. Only if the digest is not found a new entry is added > to the measurement list. > > > Roberto Sassu (12): > ima: generalize ima_read_policy() > ima: generalize ima_write_policy() > ima: generalize policy file operations > ima: use ima_show_htable_value to show hash table data > ima: add functions to manage digest lists > ima: added parser of digest lists metadata > ima: added parser for compact digest list > ima: added parser for RPM data type > ima: introduce securityfs interfaces for digest lists > ima: disable digest lookup if digest lists are not measured > ima: don't report measurements if digests are included in the loaded > lists > ima: added Documentation/security/IMA-digest-lists.txt > > Documentation/security/IMA-digest-lists.txt | 150 +++++++++++++++++ > include/linux/fs.h | 1 + > security/integrity/ima/Kconfig | 11 ++ > security/integrity/ima/Makefile | 1 + > security/integrity/ima/ima.h | 17 ++ > security/integrity/ima/ima_digest_list.c | 247 ++++++++++++++++++++++++++++ > security/integrity/ima/ima_fs.c | 178 ++++++++++++-------- > security/integrity/ima/ima_main.c | 23 ++- > security/integrity/ima/ima_policy.c | 1 + > security/integrity/ima/ima_queue.c | 39 +++++ > 10 files changed, 602 insertions(+), 66 deletions(-) > create mode 100644 Documentation/security/IMA-digest-lists.txt > create mode 100644 security/integrity/ima/ima_digest_list.c > |
|
From: Mimi Z. <zo...@li...> - 2017-07-26 13:23:58
|
With the new ->integrity_read file_operations method support, files
opened with the O_DIRECT flag should work properly. This patch
reverts commit f9b2a735bddd "ima: audit log files opened with O_DIRECT
flag".
Signed-off-by: Mimi Zohar <zo...@li...>
---
Documentation/ABI/testing/ima_policy | 2 +-
security/integrity/ima/ima_api.c | 6 ------
security/integrity/ima/ima_main.c | 5 +----
security/integrity/ima/ima_policy.c | 8 +-------
security/integrity/integrity.h | 1 -
5 files changed, 3 insertions(+), 19 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index f271207743e5..441a78e7b87e 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -24,7 +24,7 @@ Description:
[euid=] [fowner=]]
lsm: [[subj_user=] [subj_role=] [subj_type=]
[obj_user=] [obj_role=] [obj_type=]]
- option: [[appraise_type=]] [permit_directio]
+ option: [[appraise_type=]]
base: func:= [BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK]
[FIRMWARE_CHECK]
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index bbf3ba8bbb09..7bc8e76c06f5 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -210,12 +210,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
if (iint->flags & IMA_COLLECTED)
goto out;
- if (file->f_flags & O_DIRECT) {
- audit_cause = "failed(directio)";
- result = -EACCES;
- goto out;
- }
-
i_version = file_inode(file)->i_version;
hash.hdr.algo = algo;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 664edab0f758..9b8ede84337f 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -240,11 +240,8 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
- if (rc != 0 && rc != -EBADF && rc != -EINVAL) {
- if (file->f_flags & O_DIRECT)
- rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES;
+ if (rc != 0 && rc != -EBADF && rc != -EINVAL)
goto out_digsig;
- }
if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */
pathname = ima_d_path(&file->f_path, &pathbuf, filename);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index cddd9dfb01e1..3b54fb32e837 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -545,7 +545,7 @@ enum {
Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq,
Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
- Opt_appraise_type, Opt_permit_directio,
+ Opt_appraise_type,
Opt_pcr, Opt_dont_failsafe
};
@@ -575,7 +575,6 @@ static match_table_t policy_tokens = {
{Opt_euid_lt, "euid<%s"},
{Opt_fowner_lt, "fowner<%s"},
{Opt_appraise_type, "appraise_type=%s"},
- {Opt_permit_directio, "permit_directio"},
{Opt_pcr, "pcr=%s"},
{Opt_dont_failsafe, "dont_failsafe"},
{Opt_err, NULL}
@@ -892,9 +891,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
else
result = -EINVAL;
break;
- case Opt_permit_directio:
- entry->flags |= IMA_PERMIT_DIRECTIO;
- break;
case Opt_pcr:
if (entry->action != MEASURE) {
result = -EINVAL;
@@ -1179,8 +1175,6 @@ int ima_policy_show(struct seq_file *m, void *v)
}
if (entry->flags & IMA_DIGSIG_REQUIRED)
seq_puts(m, "appraise_type=imasig ");
- if (entry->flags & IMA_PERMIT_DIRECTIO)
- seq_puts(m, "permit_directio ");
rcu_read_unlock();
seq_puts(m, "\n");
return 0;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..790f07e515a7 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -31,7 +31,6 @@
#define IMA_ACTION_RULE_FLAGS 0x06000000
#define IMA_DIGSIG 0x01000000
#define IMA_DIGSIG_REQUIRED 0x02000000
-#define IMA_PERMIT_DIRECTIO 0x04000000
#define IMA_NEW_FILE 0x08000000
#define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
--
2.7.4
|
|
From: Mimi Z. <zo...@li...> - 2017-07-26 13:23:57
|
From: Christoph Hellwig <hc...@ls...>
Add a new ->integrity_read file operation to read data for integrity
hash collection. This is defined to be equivalent to ->read_iter,
except that it will be called with the i_rwsem held exclusively.
Signed-off-by: Christoph Hellwig <hc...@ls...>
Cc: Matthew Garrett <mat...@ne...>
Cc: Jan Kara <ja...@su...>
Cc: "Theodore Ts'o" <ty...@mi...>
Cc: Andreas Dilger <adi...@di...>
Cc: Jaegeuk Kim <ja...@ke...>
Cc: Chao Yu <yu...@hu...>
Cc: Steven Whitehouse <swh...@re...>
Cc: Bob Peterson <rpe...@re...>
Cc: David Woodhouse <dw...@in...>
Cc: Dave Kleikamp <sh...@ke...>
Cc: Ryusuke Konishi <kon...@la...>
Cc: Mark Fasheh <mf...@ve...>
Cc: Joel Becker <jl...@ev...>
Cc: Richard Weinberger <ri...@no...>
Cc: "Darrick J. Wong" <dar...@or...>
Cc: Hugh Dickins <hu...@go...>
Cc: Chris Mason <cl...@fb...>
Signed-off-by: Mimi Zohar <zo...@li...>
Changelog v4:
- define ext2/4 specific ->integrity_read functions.
- properly fail file open with O_DIRECT on filesystem not mounted
with "-o dax".
---
Changelog v3:
- define simple_read_iter_from_buffer
- replace the existing efivarfs ->read method with ->read_iter method.
- squashed other fs definitions of ->integrity_read with this patch.
Changelog v2:
- change iovec to kvec
Changelog v1:
- update the patch description, removing the concept that the presence of
->integrity_read indicates that the file system can support IMA. (Mimi)
fs/btrfs/file.c | 1 +
fs/efivarfs/file.c | 12 +++++++-----
fs/ext2/file.c | 17 +++++++++++++++++
fs/ext4/file.c | 23 +++++++++++++++++++++++
fs/f2fs/file.c | 1 +
fs/gfs2/file.c | 2 ++
fs/jffs2/file.c | 1 +
fs/jfs/file.c | 1 +
fs/libfs.c | 32 ++++++++++++++++++++++++++++++++
fs/nilfs2/file.c | 1 +
fs/ocfs2/file.c | 1 +
fs/ramfs/file-mmu.c | 1 +
fs/ramfs/file-nommu.c | 1 +
fs/ubifs/file.c | 1 +
fs/xfs/xfs_file.c | 21 +++++++++++++++++++++
include/linux/fs.h | 3 +++
mm/shmem.c | 1 +
security/integrity/iint.c | 20 ++++++++++++++------
18 files changed, 129 insertions(+), 11 deletions(-)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 9e75d8a39aac..2542dc66c85c 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3125,6 +3125,7 @@ const struct file_operations btrfs_file_operations = {
#endif
.clone_file_range = btrfs_clone_file_range,
.dedupe_file_range = btrfs_dedupe_file_range,
+ .integrity_read = generic_file_read_iter,
};
void btrfs_auto_defrag_exit(void)
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 5f22e74bbade..17955a92a5b3 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -64,9 +64,10 @@ static ssize_t efivarfs_file_write(struct file *file,
return bytes;
}
-static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
- size_t count, loff_t *ppos)
+static ssize_t efivarfs_file_read_iter(struct kiocb *iocb,
+ struct iov_iter *iter)
{
+ struct file *file = iocb->ki_filp;
struct efivar_entry *var = file->private_data;
unsigned long datasize = 0;
u32 attributes;
@@ -96,8 +97,8 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
goto out_free;
memcpy(data, &attributes, sizeof(attributes));
- size = simple_read_from_buffer(userbuf, count, ppos,
- data, datasize + sizeof(attributes));
+ size = simple_read_iter_from_buffer(iocb, iter, data,
+ datasize + sizeof(attributes));
out_free:
kfree(data);
@@ -174,8 +175,9 @@ efivarfs_file_ioctl(struct file *file, unsigned int cmd, unsigned long p)
const struct file_operations efivarfs_file_operations = {
.open = simple_open,
- .read = efivarfs_file_read,
+ .read_iter = efivarfs_file_read_iter,
.write = efivarfs_file_write,
.llseek = no_llseek,
.unlocked_ioctl = efivarfs_file_ioctl,
+ .integrity_read = efivarfs_file_read_iter,
};
diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index d34d32bdc944..111069de1973 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -192,6 +192,22 @@ static ssize_t ext2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
return generic_file_read_iter(iocb, to);
}
+static ssize_t ext2_file_integrity_read_iter(struct kiocb *iocb,
+ struct iov_iter *to)
+{
+ struct inode *inode = file_inode(iocb->ki_filp);
+
+ lockdep_assert_held(&inode->i_rwsem);
+#ifdef CONFIG_FS_DAX
+ if (!iov_iter_count(to))
+ return 0; /* skip atime */
+
+ if (IS_DAX(iocb->ki_filp->f_mapping->host))
+ return dax_iomap_rw(iocb, to, &ext2_iomap_ops);
+#endif
+ return generic_file_read_iter(iocb, to);
+}
+
static ssize_t ext2_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
{
#ifdef CONFIG_FS_DAX
@@ -216,6 +232,7 @@ const struct file_operations ext2_file_operations = {
.get_unmapped_area = thp_get_unmapped_area,
.splice_read = generic_file_splice_read,
.splice_write = iter_file_splice_write,
+ .integrity_read = ext2_file_integrity_read_iter,
};
const struct inode_operations ext2_file_inode_operations = {
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 58294c9a7e1d..cb423fff935f 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -74,6 +74,28 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
return generic_file_read_iter(iocb, to);
}
+static ssize_t ext4_file_integrity_read_iter(struct kiocb *iocb,
+ struct iov_iter *to)
+{
+ struct inode *inode = file_inode(iocb->ki_filp);
+ int o_direct = iocb->ki_flags & IOCB_DIRECT;
+
+ lockdep_assert_held(&inode->i_rwsem);
+ if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+ return -EIO;
+
+ if (!iov_iter_count(to))
+ return 0; /* skip atime */
+
+#ifdef CONFIG_FS_DAX
+ if (IS_DAX(inode))
+ return dax_iomap_rw(iocb, to, &ext4_iomap_ops);
+#endif
+ if (o_direct)
+ return -EINVAL;
+ return generic_file_read_iter(iocb, to);
+}
+
/*
* Called when an inode is released. Note that this is different
* from ext4_file_open: open gets called at every open, but release
@@ -747,6 +769,7 @@ const struct file_operations ext4_file_operations = {
.splice_read = generic_file_splice_read,
.splice_write = iter_file_splice_write,
.fallocate = ext4_fallocate,
+ .integrity_read = ext4_file_integrity_read_iter,
};
const struct inode_operations ext4_file_inode_operations = {
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 2706130c261b..82ea81da0b2d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2514,4 +2514,5 @@ const struct file_operations f2fs_file_operations = {
#endif
.splice_read = generic_file_splice_read,
.splice_write = iter_file_splice_write,
+ .integrity_read = generic_file_read_iter,
};
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index c2062a108d19..9b49d09ba180 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -1124,6 +1124,7 @@ const struct file_operations gfs2_file_fops = {
.splice_write = gfs2_file_splice_write,
.setlease = simple_nosetlease,
.fallocate = gfs2_fallocate,
+ .integrity_read = generic_file_read_iter,
};
const struct file_operations gfs2_dir_fops = {
@@ -1152,6 +1153,7 @@ const struct file_operations gfs2_file_fops_nolock = {
.splice_write = gfs2_file_splice_write,
.setlease = generic_setlease,
.fallocate = gfs2_fallocate,
+ .integrity_read = generic_file_read_iter,
};
const struct file_operations gfs2_dir_fops_nolock = {
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index c12476e309c6..5a63034cccf5 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -57,6 +57,7 @@ const struct file_operations jffs2_file_operations =
.mmap = generic_file_readonly_mmap,
.fsync = jffs2_fsync,
.splice_read = generic_file_splice_read,
+ .integrity_read = generic_file_read_iter,
};
/* jffs2_file_inode_operations */
diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 739492c7a3fd..423512a810e4 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -162,4 +162,5 @@ const struct file_operations jfs_file_operations = {
#ifdef CONFIG_COMPAT
.compat_ioctl = jfs_compat_ioctl,
#endif
+ .integrity_read = generic_file_read_iter,
};
diff --git a/fs/libfs.c b/fs/libfs.c
index 3aabe553fc45..99333264a0a7 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -16,6 +16,7 @@
#include <linux/exportfs.h>
#include <linux/writeback.h>
#include <linux/buffer_head.h> /* sync_mapping_buffers */
+#include <linux/uio.h>
#include <linux/uaccess.h>
@@ -676,6 +677,37 @@ ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
EXPORT_SYMBOL(simple_write_to_buffer);
/**
+ * simple_read_iter_from_buffer - copy data from the buffer to user space
+ * @iocb: struct containing the file, the current position and other info
+ * @to: the user space buffer to read to
+ * @from: the buffer to read from
+ * @available: the size of the buffer
+ *
+ * The simple_read_iter_from_buffer() function reads up to @available bytes
+ * from the current buffer into the user space buffer.
+ *
+ * On success, the current buffer offset is advanced by the number of bytes
+ * read, or a negative value is returned on error.
+ **/
+ssize_t simple_read_iter_from_buffer(struct kiocb *iocb, struct iov_iter *to,
+ const void *from, size_t available)
+{
+ loff_t pos = iocb->ki_pos;
+ size_t ret;
+
+ if (pos < 0)
+ return -EINVAL;
+ if (pos >= available)
+ return 0;
+ ret = copy_to_iter(from + pos, available - pos, to);
+ if (!ret && iov_iter_count(to))
+ return -EFAULT;
+ iocb->ki_pos = pos + ret;
+ return ret;
+}
+EXPORT_SYMBOL(simple_read_iter_from_buffer);
+
+/**
* memory_read_from_buffer - copy data from the buffer
* @to: the kernel space buffer to read to
* @count: the maximum number of bytes to read
diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
index c5fa3dee72fc..55e058ac487f 100644
--- a/fs/nilfs2/file.c
+++ b/fs/nilfs2/file.c
@@ -150,6 +150,7 @@ const struct file_operations nilfs_file_operations = {
/* .release = nilfs_release_file, */
.fsync = nilfs_sync_file,
.splice_read = generic_file_splice_read,
+ .integrity_read = generic_file_read_iter,
};
const struct inode_operations nilfs_file_inode_operations = {
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index bfeb647459d9..2832a7c92acd 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2536,6 +2536,7 @@ const struct file_operations ocfs2_fops = {
.fallocate = ocfs2_fallocate,
.clone_file_range = ocfs2_file_clone_range,
.dedupe_file_range = ocfs2_file_dedupe_range,
+ .integrity_read = ocfs2_file_read_iter,
};
const struct file_operations ocfs2_dops = {
diff --git a/fs/ramfs/file-mmu.c b/fs/ramfs/file-mmu.c
index 12af0490322f..4f24d1b589b1 100644
--- a/fs/ramfs/file-mmu.c
+++ b/fs/ramfs/file-mmu.c
@@ -47,6 +47,7 @@ const struct file_operations ramfs_file_operations = {
.splice_write = iter_file_splice_write,
.llseek = generic_file_llseek,
.get_unmapped_area = ramfs_mmu_get_unmapped_area,
+ .integrity_read = generic_file_read_iter,
};
const struct inode_operations ramfs_file_inode_operations = {
diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
index 2ef7ce75c062..5ee704fa84e0 100644
--- a/fs/ramfs/file-nommu.c
+++ b/fs/ramfs/file-nommu.c
@@ -50,6 +50,7 @@ const struct file_operations ramfs_file_operations = {
.splice_read = generic_file_splice_read,
.splice_write = iter_file_splice_write,
.llseek = generic_file_llseek,
+ .integrity_read = generic_file_read_iter,
};
const struct inode_operations ramfs_file_inode_operations = {
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 8cad0b19b404..5e52a315e18b 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1747,4 +1747,5 @@ const struct file_operations ubifs_file_operations = {
#ifdef CONFIG_COMPAT
.compat_ioctl = ubifs_compat_ioctl,
#endif
+ .integrity_read = generic_file_read_iter,
};
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index c4893e226fd8..0a6704b563d6 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -292,6 +292,26 @@ xfs_file_read_iter(
return ret;
}
+static ssize_t
+xfs_integrity_read(
+ struct kiocb *iocb,
+ struct iov_iter *to)
+{
+ struct inode *inode = file_inode(iocb->ki_filp);
+ struct xfs_mount *mp = XFS_I(inode)->i_mount;
+
+ lockdep_assert_held(&inode->i_rwsem);
+
+ XFS_STATS_INC(mp, xs_read_calls);
+
+ if (XFS_FORCED_SHUTDOWN(mp))
+ return -EIO;
+
+ if (IS_DAX(inode))
+ return dax_iomap_rw(iocb, to, &xfs_iomap_ops);
+ return generic_file_read_iter(iocb, to);
+}
+
/*
* Zero any on disk space between the current EOF and the new, larger EOF.
*
@@ -1175,6 +1195,7 @@ const struct file_operations xfs_file_operations = {
.fallocate = xfs_file_fallocate,
.clone_file_range = xfs_file_clone_range,
.dedupe_file_range = xfs_file_dedupe_range,
+ .integrity_read = xfs_integrity_read,
};
const struct file_operations xfs_dir_file_operations = {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6e1fd5d21248..8d0d10e1dd93 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1699,6 +1699,7 @@ struct file_operations {
u64);
ssize_t (*dedupe_file_range)(struct file *, u64, u64, struct file *,
u64);
+ ssize_t (*integrity_read)(struct kiocb *, struct iov_iter *);
} __randomize_layout;
struct inode_operations {
@@ -3097,6 +3098,8 @@ extern void simple_release_fs(struct vfsmount **mount, int *count);
extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
loff_t *ppos, const void *from, size_t available);
+extern ssize_t simple_read_iter_from_buffer(struct kiocb *iocb,
+ struct iov_iter *to, const void *from, size_t available);
extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
const void __user *from, size_t count);
diff --git a/mm/shmem.c b/mm/shmem.c
index b0aa6075d164..805d99011ca4 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3849,6 +3849,7 @@ static const struct file_operations shmem_file_operations = {
.splice_read = generic_file_splice_read,
.splice_write = iter_file_splice_write,
.fallocate = shmem_fallocate,
+ .integrity_read = shmem_file_read_iter,
#endif
};
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 6fc888ca468e..df04f35a1d40 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -21,6 +21,7 @@
#include <linux/rbtree.h>
#include <linux/file.h>
#include <linux/uaccess.h>
+#include <linux/uio.h>
#include "integrity.h"
static struct rb_root integrity_iint_tree = RB_ROOT;
@@ -184,18 +185,25 @@ security_initcall(integrity_iintcache_init);
int integrity_kernel_read(struct file *file, loff_t offset,
void *addr, unsigned long count)
{
- mm_segment_t old_fs;
- char __user *buf = (char __user *)addr;
+ struct inode *inode = file_inode(file);
+ struct kvec iov = { .iov_base = addr, .iov_len = count };
+ struct kiocb kiocb;
+ struct iov_iter iter;
ssize_t ret;
+ lockdep_assert_held(&inode->i_rwsem);
+
if (!(file->f_mode & FMODE_READ))
return -EBADF;
+ if (!file->f_op->integrity_read)
+ return -EBADF;
- old_fs = get_fs();
- set_fs(get_ds());
- ret = __vfs_read(file, buf, count, &offset);
- set_fs(old_fs);
+ init_sync_kiocb(&kiocb, file);
+ kiocb.ki_pos = offset;
+ iov_iter_kvec(&iter, READ | ITER_KVEC, &iov, 1, count);
+ ret = file->f_op->integrity_read(&kiocb, &iter);
+ BUG_ON(ret == -EIOCBQUEUED);
return ret;
}
--
2.7.4
|
|
From: Mimi Z. <zo...@li...> - 2017-07-26 13:23:56
|
Permit normally denied access/execute permission for files in policy
on IMA unsupported filesystems. This patch defines the "dont_failsafe"
policy action rule.
Mimi Zohar <zo...@li...>
---
Changelog v3:
- include dont_failsafe rule when displaying policy
- fail attempt to add dont_failsafe rule when appending to the policy
Documentation/ABI/testing/ima_policy | 3 ++-
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_main.c | 11 ++++++++++-
security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++++++-
4 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index e76432b9954d..f271207743e5 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -17,7 +17,8 @@ Description:
rule format: action [condition ...]
- action: measure | dont_measure | appraise | dont_appraise | audit
+ action: measure | dont_meaure | appraise | dont_appraise |
+ audit | dont_failsafe
condition:= base | lsm [option]
base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
[euid=] [fowner=]]
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487ad259..c5f34f7c5b0f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -224,6 +224,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
void ima_policy_stop(struct seq_file *m, void *v);
int ima_policy_show(struct seq_file *m, void *v);
+void set_failsafe(bool flag);
/* Appraise integrity measurements */
#define IMA_APPRAISE_ENFORCE 0x01
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 3941371402ff..664edab0f758 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -38,6 +38,11 @@ int ima_appraise;
int ima_hash_algo = HASH_ALGO_SHA1;
static int hash_setup_done;
+static bool ima_failsafe = 1;
+void set_failsafe(bool flag) {
+ ima_failsafe = flag;
+}
+
static int __init hash_setup(char *str)
{
struct ima_template_desc *template_desc = ima_template_desc_current();
@@ -263,8 +268,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
__putname(pathbuf);
out:
inode_unlock(inode);
- if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
+ if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ if (!ima_failsafe && rc == -EBADF)
+ return 0;
+
return -EACCES;
+ }
return 0;
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 95209a5f8595..43b85a4fb8e8 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -40,12 +40,14 @@
#define APPRAISE 0x0004 /* same as IMA_APPRAISE */
#define DONT_APPRAISE 0x0008
#define AUDIT 0x0040
+#define DONT_FAILSAFE 0x0400
#define INVALID_PCR(a) (((a) < 0) || \
(a) >= (FIELD_SIZEOF(struct integrity_iint_cache, measured_pcrs) * 8))
int ima_policy_flag;
static int temp_ima_appraise;
+static bool temp_failsafe = 1;
#define MAX_LSM_RULES 6
enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
@@ -513,6 +515,9 @@ void ima_update_policy(void)
if (ima_rules != policy) {
ima_policy_flag = 0;
ima_rules = policy;
+
+ /* Only update on initial policy replacement, not append */
+ set_failsafe(temp_failsafe);
}
ima_update_policy_flag();
}
@@ -529,7 +534,7 @@ enum {
Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
Opt_appraise_type, Opt_permit_directio,
- Opt_pcr
+ Opt_pcr, Opt_dont_failsafe
};
static match_table_t policy_tokens = {
@@ -560,6 +565,7 @@ static match_table_t policy_tokens = {
{Opt_appraise_type, "appraise_type=%s"},
{Opt_permit_directio, "permit_directio"},
{Opt_pcr, "pcr=%s"},
+ {Opt_dont_failsafe, "dont_failsafe"},
{Opt_err, NULL}
};
@@ -630,6 +636,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
if ((*p == '\0') || (*p == ' ') || (*p == '\t'))
continue;
token = match_token(p, policy_tokens, args);
+ if (entry->action == DONT_FAILSAFE) {
+ /* no args permitted, force invalid rule */
+ token = Opt_dont_failsafe;
+ }
+
switch (token) {
case Opt_measure:
ima_log_string(ab, "action", "measure");
@@ -671,6 +682,19 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
entry->action = AUDIT;
break;
+ case Opt_dont_failsafe:
+ ima_log_string(ab, "action", "dont_failsafe");
+
+ if (entry->action != UNKNOWN)
+ result = -EINVAL;
+
+ /* Permit on initial policy replacement only */
+ if (ima_rules != &ima_policy_rules)
+ temp_failsafe = 0;
+ else
+ result = -EINVAL;
+ entry->action = DONT_FAILSAFE;
+ break;
case Opt_func:
ima_log_string(ab, "func", args[0].from);
@@ -949,6 +973,7 @@ void ima_delete_rules(void)
int i;
temp_ima_appraise = 0;
+ temp_failsafe = 1;
list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
for (i = 0; i < MAX_LSM_RULES; i++)
kfree(entry->lsm[i].args_p);
@@ -1040,6 +1065,8 @@ int ima_policy_show(struct seq_file *m, void *v)
seq_puts(m, pt(Opt_dont_appraise));
if (entry->action & AUDIT)
seq_puts(m, pt(Opt_audit));
+ if (entry->action & DONT_FAILSAFE)
+ seq_puts(m, pt(Opt_dont_failsafe));
seq_puts(m, " ");
--
2.7.4
|
|
From: Mimi Z. <zo...@li...> - 2017-07-26 13:23:55
|
Permit normally denied access/execute permission for files in policy
on IMA unsupported filesystems. This patch defines "fs_unsafe", a
builtin policy.
Mimi Zohar <zo...@li...>
---
Changelog v3:
- include dont_failsafe rule when displaying policy
Documentation/admin-guide/kernel-parameters.txt | 8 +++++++-
security/integrity/ima/ima_policy.c | 12 ++++++++++++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index d9c171ce4190..4e303be83df6 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1502,7 +1502,7 @@
ima_policy= [IMA]
The builtin policies to load during IMA setup.
- Format: "tcb | appraise_tcb | secure_boot"
+ Format: "tcb | appraise_tcb | secure_boot | fs_unsafe"
The "tcb" policy measures all programs exec'd, files
mmap'd for exec, and all files opened with the read
@@ -1517,6 +1517,12 @@
of files (eg. kexec kernel image, kernel modules,
firmware, policy, etc) based on file signatures.
+ The "fs_unsafe" policy permits normally denied
+ access/execute permission for files in policy on IMA
+ unsupported filesystems. Note this option, as the
+ name implies, is not safe and not recommended for
+ any environments other than testing.
+
ima_tcb [IMA] Deprecated. Use ima_policy= instead.
Load a policy which meets the needs of the Trusted
Computing Base. This means IMA will measure all
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 43b85a4fb8e8..cddd9dfb01e1 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -169,6 +169,10 @@ static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
};
+static struct ima_rule_entry dont_failsafe_rules[] __ro_after_init = {
+ {.action = DONT_FAILSAFE}
+};
+
static LIST_HEAD(ima_default_rules);
static LIST_HEAD(ima_policy_rules);
static LIST_HEAD(ima_temp_rules);
@@ -188,6 +192,7 @@ __setup("ima_tcb", default_measure_policy_setup);
static bool ima_use_appraise_tcb __initdata;
static bool ima_use_secure_boot __initdata;
+static bool ima_use_dont_failsafe __initdata;
static int __init policy_setup(char *str)
{
char *p;
@@ -201,6 +206,10 @@ static int __init policy_setup(char *str)
ima_use_appraise_tcb = 1;
else if (strcmp(p, "secure_boot") == 0)
ima_use_secure_boot = 1;
+ else if (strcmp(p, "fs_unsafe") == 0) {
+ ima_use_dont_failsafe = 1;
+ set_failsafe(0);
+ }
}
return 1;
@@ -470,6 +479,9 @@ void __init ima_init_policy(void)
temp_ima_appraise |= IMA_APPRAISE_POLICY;
}
+ if (ima_use_dont_failsafe)
+ list_add_tail(&dont_failsafe_rules[0].list, &ima_default_rules);
+
ima_rules = &ima_default_rules;
ima_update_policy_flag();
}
--
2.7.4
|
|
From: Mimi Z. <zo...@li...> - 2017-07-26 13:23:41
|
All files matching a "measure" rule must be included in the IMA
measurement list, even when the file hash cannot be calculated.
Similarly, all files matching an "audit" rule must be audited, even when
the file hash can not be calculated.
The file data hash field contained in the IMA measurement list template
data will contain 0's instead of the actual file hash digest.
Mimi Zohar <zo...@li...>
---
Changelog v4:
- Based on both -EBADF and -EINVAL
- clean up ima_collect_measurement()
security/integrity/ima/ima_api.c | 58 +++++++++++++++++++++++----------------
security/integrity/ima/ima_main.c | 4 +--
2 files changed, 37 insertions(+), 25 deletions(-)
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c2edba8de35e..bbf3ba8bbb09 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -199,37 +199,49 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
struct inode *inode = file_inode(file);
const char *filename = file->f_path.dentry->d_name.name;
int result = 0;
+ int length;
+ void *tmpbuf;
+ u64 i_version;
struct {
struct ima_digest_data hdr;
char digest[IMA_MAX_DIGEST_SIZE];
} hash;
- if (!(iint->flags & IMA_COLLECTED)) {
- u64 i_version = file_inode(file)->i_version;
+ if (iint->flags & IMA_COLLECTED)
+ goto out;
- if (file->f_flags & O_DIRECT) {
- audit_cause = "failed(directio)";
- result = -EACCES;
- goto out;
- }
+ if (file->f_flags & O_DIRECT) {
+ audit_cause = "failed(directio)";
+ result = -EACCES;
+ goto out;
+ }
- hash.hdr.algo = algo;
-
- result = (!buf) ? ima_calc_file_hash(file, &hash.hdr) :
- ima_calc_buffer_hash(buf, size, &hash.hdr);
- if (!result) {
- int length = sizeof(hash.hdr) + hash.hdr.length;
- void *tmpbuf = krealloc(iint->ima_hash, length,
- GFP_NOFS);
- if (tmpbuf) {
- iint->ima_hash = tmpbuf;
- memcpy(iint->ima_hash, &hash, length);
- iint->version = i_version;
- iint->flags |= IMA_COLLECTED;
- } else
- result = -ENOMEM;
- }
+ i_version = file_inode(file)->i_version;
+ hash.hdr.algo = algo;
+
+ /* Initialize hash digest to 0's in case of failure */
+ memset(&hash.digest, 0, sizeof(hash.digest));
+
+ result = (!buf) ? ima_calc_file_hash(file, &hash.hdr) :
+ ima_calc_buffer_hash(buf, size, &hash.hdr);
+
+ if (result && result != -EBADF && result != -EINVAL)
+ goto out;
+
+ length = sizeof(hash.hdr) + hash.hdr.length;
+ tmpbuf = krealloc(iint->ima_hash, length, GFP_NOFS);
+ if (!tmpbuf) {
+ result = -ENOMEM;
+ goto out;
}
+
+ iint->ima_hash = tmpbuf;
+ memcpy(iint->ima_hash, &hash, length);
+ iint->version = i_version;
+
+ /* Possibly temporary failure due to type of read (eg. DAX, O_DIRECT) */
+ if (result != -EBADF && result != -EINVAL)
+ iint->flags |= IMA_COLLECTED;
out:
if (result)
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2aebb7984437..3941371402ff 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -235,7 +235,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
- if (rc != 0) {
+ if (rc != 0 && rc != -EBADF && rc != -EINVAL) {
if (file->f_flags & O_DIRECT)
rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES;
goto out_digsig;
@@ -247,7 +247,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
if (action & IMA_MEASURE)
ima_store_measurement(iint, file, pathname,
xattr_value, xattr_len, pcr);
- if (action & IMA_APPRAISE_SUBMASK)
+ if ((rc == 0) && (action & IMA_APPRAISE_SUBMASK))
rc = ima_appraise_measurement(func, iint, file, pathname,
xattr_value, xattr_len, opened);
if (action & IMA_AUDIT)
--
2.7.4
|
|
From: Mimi Z. <zo...@li...> - 2017-07-26 13:23:38
|
With the introduction of IMA-appraisal and the need to write file hashes as security xattrs, IMA needed to take the global i_mutex lock. process_measurement() took the iint->mutex first and then the i_mutex, while setxattr, chmod and chown took the locks in reverse order. To resolve this potential deadlock, the iint->mutex was removed. Some filesystems have recently replaced their filesystem dependent lock with the global i_rwsem (formerly the i_mutex) to read a file. As a result, when IMA attempts to calculate the file hash, reading the file attempts to take the i_rwsem again. To resolve this locking problem, this patch set introduces a new ->integrity_read file operation method. Until all filesystems define the new ->integrity_read method, files that were previously measured might not be currently measured and files that were previously appraised might fail to be appraised properly. Version 2 of this patch set, introduced measurement entries and IMA-audit messages containing file hash values containing 0's, instead of the actual file hash, for files which the file hash could not be calculated. Like for any other file signature verification error, file access/execute permission will be denied, for files in policy that the file hash could not be calculated. To override the IMA policy, allowing unverified code to be accessed/executed on filesystems not supported by IMA, version 2 of this patch set defined a new policy "action" named "dont_failsafe" and a new builtin policy named "fs_unsafe", which can be specified on the boot command line. The new ->integrity_read method supports opening files with O_DIRECT on block devices that support direct IO and are mounted with the "-o dax" option. Version 4 of this patch set removes the "permit_direction" IMA policy option, which is no longer necessary. Change log v4: - define ext2/4 specific ->integrity_read functions based Jan Kara's review. - properly fail file open with O_DIRECT on filesystems not mounted with "-o dax". - remove the "permit_directio" IMA policy option. Change log v3: - define simple_read_iter_from_buffer - replace the existing efivarfs ->read method with ->read_iter method. - squashed other fs definitions of ->integrity_read with this patch. - include dont_failsafe rule when displaying policy. - fail attempt to add dont_failsafe rule when appending to the policy. - moved '---' divider before change log, as requested in review. Mimi Christoph Hellwig (1): ima: use fs method to read integrity data Mimi Zohar (4): ima: always measure and audit files in policy ima: define "dont_failsafe" policy action rule ima: define "fs_unsafe" builtin policy ima: remove permit_directio policy option Documentation/ABI/testing/ima_policy | 5 ++- Documentation/admin-guide/kernel-parameters.txt | 8 +++- fs/btrfs/file.c | 1 + fs/efivarfs/file.c | 12 +++--- fs/ext2/file.c | 17 ++++++++ fs/ext4/file.c | 23 +++++++++++ fs/f2fs/file.c | 1 + fs/gfs2/file.c | 2 + fs/jffs2/file.c | 1 + fs/jfs/file.c | 1 + fs/libfs.c | 32 +++++++++++++++ fs/nilfs2/file.c | 1 + fs/ocfs2/file.c | 1 + fs/ramfs/file-mmu.c | 1 + fs/ramfs/file-nommu.c | 1 + fs/ubifs/file.c | 1 + fs/xfs/xfs_file.c | 21 ++++++++++ include/linux/fs.h | 3 ++ mm/shmem.c | 1 + security/integrity/iint.c | 20 +++++++--- security/integrity/ima/ima.h | 1 + security/integrity/ima/ima_api.c | 52 ++++++++++++++----------- security/integrity/ima/ima_main.c | 18 ++++++--- security/integrity/ima/ima_policy.c | 49 +++++++++++++++++++---- security/integrity/integrity.h | 1 - 25 files changed, 222 insertions(+), 52 deletions(-) -- 2.7.4 |
|
From: Stefan B. <st...@li...> - 2017-07-25 21:35:56
|
On 07/25/2017 04:46 PM, Serge E. Hallyn wrote:
> On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
>> On 07/25/2017 03:48 PM, Mimi Zohar wrote:
>>> On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
>>>> On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
>>>>> On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
>>>>>> On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
>>>>>>> On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
>>>>>>>> From: Yuqiong Sun <su...@us...>
>>>>>>>>
>>>>>>>> Add new CONFIG_IMA_NS config option. Let clone() create a new
>>>>>>>> IMA namespace upon CLONE_NEWNS flag. Add ima_ns data structure
>>>>>>>> in nsproxy. ima_ns is allocated and freed upon IMA namespace
>>>>>>>> creation and exit. Currently, the ima_ns contains no useful IMA
>>>>>>>> data but only a dummy interface. This patch creates the
>>>>>>>> framework for namespacing the different aspects of IMA (eg.
>>>>>>>> IMA-audit, IMA-measurement, IMA-appraisal).
>>>>>>>>
>>>>>>>> Signed-off-by: Yuqiong Sun <su...@us...>
>>>>>>>>
>>>>>>>> Changelog:
>>>>>>>> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
>>>>>>> Hi,
>>>>>>>
>>>>>>> So this means that every mount namespace clone will clone a new
>>>>>>> IMA namespace. Is that really ok?
>>>>>> Based on what: space concerns (struct ima_ns is reasonably small)?
>>>>>> or whether tying it to the mount namespace is the correct thing to
>>>>>> do. On
>>>>> Mostly the latter. The other would be not so much space concerns as
>>>>> time concerns. Many things use new mounts namespaces, and we
>>>>> wouldn't want multiple IMA calls on all file accesses by all of
>>>>> those.
>>>>>
>>>>>> the latter, it does seem that this should be a property of either
>>>>>> the mount or user ns rather than its own separate ns. I could see
>>>>>> a use where even a container might want multiple ima keyrings
>>>>>> within the container (say containerised apache service with
>>>>>> multiple tenants), so instinct tells me that mount ns is the
>>>>>> correct granularity for this.
>>>>> I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
>>>>> as the trigger for requesting a new ima ns on the next
>>>>> clone(CLONE_NEWNS).
>>>> I could go with that, but what about the trigger being installing or
>>>> updating the keyring? That's the only operation that needs namespace
>>>> separation, so on mount ns clone, you get a pointer to the old ima_ns
>>>> until you do something that requires a new key, which then triggers the
>>>> copy of the namespace and installing it?
>>> It isn't just the keyrings that need to be namespaced, but the
>>> measurement list and policy as well.
>>>
>>> IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
>>>
>>> As soon as the namespace starts, measurements should be added to the
>>> namespace specific measurement list, not it's parent.
> Shouldn't it be both?
>
> If not, then it seems to me this must be tied to user namespace.
>
>> IMA is about measuring things, logging what was executed, and
>> finally someone looking at the measurement log and detecting
>> 'things'. So at least one attack that needs to be prevented is a
>> malicious person opening an IMA namespace, executing something
>> malicious, and not leaving any trace on the host because all the
>> logs went into the measurement list of the IMA namespace, which
>> disappeared. That said, I am wondering whether there has to be a
>> minimum set of namespaces (PID, UTS) providing enough 'isolation'
>> that someone may actually open an IMA namespace and run their code.
>> To avoid leaving no traces one could argue to implement recursive
>> logging, so something that is logged inside the namespace will be
>> detected in all parent containers up to the init_ima_ns (host)
>> because it's logged (and TPM extended) there as well. The challenge
>> with that is that logging costs memory and that can be abused as
>> well until the machine needs a reboot... I guess the solution could
>> be requesting an IMA namespace in one way or another but requiring
>> several other namespace flags in the clone() to actually 'get' it.
>> Jumping namespaces with setns() may have to be restricted as well
>> once there is an IMA namespace.
> Wait. So if I create a new IMA namespace, the things I run in
> that namespace are not subject to the parent namespace policy?
We would treat the IMA namespace policy independently of the host
policy. A user can sign his containers' files with his own keys, upload
the container to the cloud and run them with keys that are different
than those of the host. The keys (actually certs) would be found in the
container, same for the policy.
Stefan
>
> -serge
>
|
|
From: Mimi Z. <zo...@li...> - 2017-07-25 21:29:13
|
On Tue, 2017-07-25 at 16:08 -0500, Serge E. Hallyn wrote:
> On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote:
> > On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote:
> > > On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote:
> > > > On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> > > > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
> > > > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
> > > > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
> > > > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
> > > > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
> > > > >>>>>>
> > > > >>>>>>From: Yuqiong Sun <su...@us...>
> > > > >>>>>>
> > > > >>>>>>Add new CONFIG_IMA_NS config option. Let clone() create a new
> > > > >>>>>>IMA namespace upon CLONE_NEWNS flag. Add ima_ns data structure
> > > > >>>>>>in nsproxy. ima_ns is allocated and freed upon IMA namespace
> > > > >>>>>>creation and exit. Currently, the ima_ns contains no useful IMA
> > > > >>>>>>data but only a dummy interface. This patch creates the
> > > > >>>>>>framework for namespacing the different aspects of IMA (eg.
> > > > >>>>>>IMA-audit, IMA-measurement, IMA-appraisal).
> > > > >>>>>>
> > > > >>>>>>Signed-off-by: Yuqiong Sun <su...@us...>
> > > > >>>>>>
> > > > >>>>>>Changelog:
> > > > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
> > > > >>>>>Hi,
> > > > >>>>>
> > > > >>>>>So this means that every mount namespace clone will clone a new
> > > > >>>>>IMA namespace. Is that really ok?
> > > > >>>>Based on what: space concerns (struct ima_ns is reasonably small)?
> > > > >>>>or whether tying it to the mount namespace is the correct thing to
> > > > >>>>do. On
> > > > >>>Mostly the latter. The other would be not so much space concerns as
> > > > >>>time concerns. Many things use new mounts namespaces, and we
> > > > >>>wouldn't want multiple IMA calls on all file accesses by all of
> > > > >>>those.
> > > > >>>
> > > > >>>>the latter, it does seem that this should be a property of either
> > > > >>>>the mount or user ns rather than its own separate ns. I could see
> > > > >>>>a use where even a container might want multiple ima keyrings
> > > > >>>>within the container (say containerised apache service with
> > > > >>>>multiple tenants), so instinct tells me that mount ns is the
> > > > >>>>correct granularity for this.
> > > > >>>I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
> > > > >>>as the trigger for requesting a new ima ns on the next
> > > > >>>clone(CLONE_NEWNS).
> > > > >>I could go with that, but what about the trigger being installing or
> > > > >>updating the keyring? That's the only operation that needs namespace
> > > > >>separation, so on mount ns clone, you get a pointer to the old ima_ns
> > > > >>until you do something that requires a new key, which then triggers the
> > > > >>copy of the namespace and installing it?
> > > > >It isn't just the keyrings that need to be namespaced, but the
> > > > >measurement list and policy as well.
> > > > >
> > > > >IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
> > > > >
> > > > >As soon as the namespace starts, measurements should be added to the
> > > > >namespace specific measurement list, not it's parent.
> > >
> > > Shouldn't it be both?
> >
> > The policy defines which files are measured. The namespace policy
> > could be different than it's parent's policy, and the parent's policy
> > could be different than the native policy. Basically, file
> > measurements need to be added to the namespace measurement list,
> > recursively, up to the native measurement list.
>
> Yes, but if a task t1 is in namespace ns2 which is a child of namespace ns1,
> and it accesses a file which ns1's policy says must be measured, then will
> ns1's required measurement happen (and be appended to the ns1 measurement
> list), whether or not ns2's policy requires it?
Yes, as the file needs to be measured only in the ns1 policy, the
measurement would exist in the ns1 measurement list, but not in the
ns2 measurement list. The pseudo code snippet below might help.
do {
.
.
/* calculate file hash based on xattr algorithm */
collect_measurement()
/* recursively added to each namespace based on policy */
ima_store_measurement()
/* Based on the specific namespace policy and keys. */
if (!once) {
once = 1;
result = ima_appraise_measurement()
}
ima_audit_measurement()
} while ((ns = ns->parent));
return result;
Mimi
|
|
From: Serge E. H. <se...@ha...> - 2017-07-25 21:08:02
|
On Tue, Jul 25, 2017 at 04:57:57PM -0400, Mimi Zohar wrote: > On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote: > > On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote: > > > On 07/25/2017 03:48 PM, Mimi Zohar wrote: > > > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote: > > > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote: > > > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote: > > > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote: > > > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote: > > > >>>>>> > > > >>>>>>From: Yuqiong Sun <su...@us...> > > > >>>>>> > > > >>>>>>Add new CONFIG_IMA_NS config option. Let clone() create a new > > > >>>>>>IMA namespace upon CLONE_NEWNS flag. Add ima_ns data structure > > > >>>>>>in nsproxy. ima_ns is allocated and freed upon IMA namespace > > > >>>>>>creation and exit. Currently, the ima_ns contains no useful IMA > > > >>>>>>data but only a dummy interface. This patch creates the > > > >>>>>>framework for namespacing the different aspects of IMA (eg. > > > >>>>>>IMA-audit, IMA-measurement, IMA-appraisal). > > > >>>>>> > > > >>>>>>Signed-off-by: Yuqiong Sun <su...@us...> > > > >>>>>> > > > >>>>>>Changelog: > > > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag > > > >>>>>Hi, > > > >>>>> > > > >>>>>So this means that every mount namespace clone will clone a new > > > >>>>>IMA namespace. Is that really ok? > > > >>>>Based on what: space concerns (struct ima_ns is reasonably small)? > > > >>>>or whether tying it to the mount namespace is the correct thing to > > > >>>>do. On > > > >>>Mostly the latter. The other would be not so much space concerns as > > > >>>time concerns. Many things use new mounts namespaces, and we > > > >>>wouldn't want multiple IMA calls on all file accesses by all of > > > >>>those. > > > >>> > > > >>>>the latter, it does seem that this should be a property of either > > > >>>>the mount or user ns rather than its own separate ns. I could see > > > >>>>a use where even a container might want multiple ima keyrings > > > >>>>within the container (say containerised apache service with > > > >>>>multiple tenants), so instinct tells me that mount ns is the > > > >>>>correct granularity for this. > > > >>>I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns > > > >>>as the trigger for requesting a new ima ns on the next > > > >>>clone(CLONE_NEWNS). > > > >>I could go with that, but what about the trigger being installing or > > > >>updating the keyring? That's the only operation that needs namespace > > > >>separation, so on mount ns clone, you get a pointer to the old ima_ns > > > >>until you do something that requires a new key, which then triggers the > > > >>copy of the namespace and installing it? > > > >It isn't just the keyrings that need to be namespaced, but the > > > >measurement list and policy as well. > > > > > > > >IMA-measurement, IMA-appraisal and IMA-audit are all policy based. > > > > > > > >As soon as the namespace starts, measurements should be added to the > > > >namespace specific measurement list, not it's parent. > > > > Shouldn't it be both? > > The policy defines which files are measured. The namespace policy > could be different than it's parent's policy, and the parent's policy > could be different than the native policy. Basically, file > measurements need to be added to the namespace measurement list, > recursively, up to the native measurement list. Yes, but if a task t1 is in namespace ns2 which is a child of namespace ns1, and it accesses a file which ns1's policy says must be measured, then will ns1's required measurement happen (and be appended to the ns1 measurement list), whether or not ns2's policy requires it? |
|
From: Mimi Z. <zo...@li...> - 2017-07-25 20:59:33
|
On Tue, 2017-07-25 at 15:46 -0500, Serge E. Hallyn wrote: > On Tue, Jul 25, 2017 at 04:11:29PM -0400, Stefan Berger wrote: > > On 07/25/2017 03:48 PM, Mimi Zohar wrote: > > >On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote: > > >>On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote: > > >>>On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote: > > >>>>On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote: > > >>>>>On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote: > > >>>>>> > > >>>>>>From: Yuqiong Sun <su...@us...> > > >>>>>> > > >>>>>>Add new CONFIG_IMA_NS config option. Let clone() create a new > > >>>>>>IMA namespace upon CLONE_NEWNS flag. Add ima_ns data structure > > >>>>>>in nsproxy. ima_ns is allocated and freed upon IMA namespace > > >>>>>>creation and exit. Currently, the ima_ns contains no useful IMA > > >>>>>>data but only a dummy interface. This patch creates the > > >>>>>>framework for namespacing the different aspects of IMA (eg. > > >>>>>>IMA-audit, IMA-measurement, IMA-appraisal). > > >>>>>> > > >>>>>>Signed-off-by: Yuqiong Sun <su...@us...> > > >>>>>> > > >>>>>>Changelog: > > >>>>>>* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag > > >>>>>Hi, > > >>>>> > > >>>>>So this means that every mount namespace clone will clone a new > > >>>>>IMA namespace. Is that really ok? > > >>>>Based on what: space concerns (struct ima_ns is reasonably small)? > > >>>>or whether tying it to the mount namespace is the correct thing to > > >>>>do. On > > >>>Mostly the latter. The other would be not so much space concerns as > > >>>time concerns. Many things use new mounts namespaces, and we > > >>>wouldn't want multiple IMA calls on all file accesses by all of > > >>>those. > > >>> > > >>>>the latter, it does seem that this should be a property of either > > >>>>the mount or user ns rather than its own separate ns. I could see > > >>>>a use where even a container might want multiple ima keyrings > > >>>>within the container (say containerised apache service with > > >>>>multiple tenants), so instinct tells me that mount ns is the > > >>>>correct granularity for this. > > >>>I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns > > >>>as the trigger for requesting a new ima ns on the next > > >>>clone(CLONE_NEWNS). > > >>I could go with that, but what about the trigger being installing or > > >>updating the keyring? That's the only operation that needs namespace > > >>separation, so on mount ns clone, you get a pointer to the old ima_ns > > >>until you do something that requires a new key, which then triggers the > > >>copy of the namespace and installing it? > > >It isn't just the keyrings that need to be namespaced, but the > > >measurement list and policy as well. > > > > > >IMA-measurement, IMA-appraisal and IMA-audit are all policy based. > > > > > >As soon as the namespace starts, measurements should be added to the > > >namespace specific measurement list, not it's parent. > > Shouldn't it be both? The policy defines which files are measured. The namespace policy could be different than it's parent's policy, and the parent's policy could be different than the native policy. Basically, file measurements need to be added to the namespace measurement list, recursively, up to the native measurement list. Mimi > > If not, then it seems to me this must be tied to user namespace. > > > IMA is about measuring things, logging what was executed, and > > finally someone looking at the measurement log and detecting > > 'things'. So at least one attack that needs to be prevented is a > > malicious person opening an IMA namespace, executing something > > malicious, and not leaving any trace on the host because all the > > logs went into the measurement list of the IMA namespace, which > > disappeared. That said, I am wondering whether there has to be a > > minimum set of namespaces (PID, UTS) providing enough 'isolation' > > that someone may actually open an IMA namespace and run their code. > > To avoid leaving no traces one could argue to implement recursive > > logging, so something that is logged inside the namespace will be > > detected in all parent containers up to the init_ima_ns (host) > > because it's logged (and TPM extended) there as well. The challenge > > with that is that logging costs memory and that can be abused as > > well until the machine needs a reboot... I guess the solution could > > be requesting an IMA namespace in one way or another but requiring > > several other namespace flags in the clone() to actually 'get' it. > > Jumping namespaces with setns() may have to be restricted as well > > once there is an IMA namespace. > > Wait. So if I create a new IMA namespace, the things I run in > that namespace are not subject to the parent namespace policy? |
|
From: Mimi Z. <zo...@li...> - 2017-07-25 20:51:45
|
On Tue, 2017-07-25 at 13:31 -0700, James Bottomley wrote: > On Tue, 2017-07-25 at 15:48 -0400, Mimi Zohar wrote: > > On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote: > > > > > > On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote: > > > > > > > > On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote: > > > > > > > > > > > > > > > On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote: > [...] > > > > > the latter, it does seem that this should be a property of > > > > > either the mount or user ns rather than its own separate ns. I > > > > > could see a use where even a container might want multiple ima > > > > > keyrings within the container (say containerised apache service > > > > > with multiple tenants), so instinct tells me that mount ns is > > > > > the correct granularity for this. > > > > > > > > I wonder whether we could use echo 1 > > > > > /sys/kernel/security/ima/newns > > > > as the trigger for requesting a new ima ns on the next > > > > clone(CLONE_NEWNS). > > > > > > I could go with that, but what about the trigger being installing > > > or updating the keyring? That's the only operation that needs > > > namespace separation, so on mount ns clone, you get a pointer to > > > the old ima_ns until you do something that requires a new key, > > > which then triggers the copy of the namespace and installing it? > > > > It isn't just the keyrings that need to be namespaced, but the > > measurement list and policy as well. > > OK, so trigger to do a just in time copy would be new key or new > policy. The kernel has support for an initial builtin policy, which can be later replaced. The builtin policies, if specified, begin measuring files very early in the boot process. Similarly for namespace, we would want to start measuring files as early as possible. > The measurement list is basically just a has of a file taken > at a policy point. Presumably it doesn't change if we install a new > policy or key, so it sounds like it should be tied to the underlying > mount point? I'm thinking if we set up a hundred mount ns each > pointing to /var/container, we don't want /var/container/bin/something > to have 100 separate measurements each with the same hash. > > > IMA-measurement, IMA-appraisal and IMA-audit are all policy based. > > > > As soon as the namespace starts, measurements should be added to the > > namespace specific measurement list, not it's parent. > > Would the measurement in a child namespace yield a different > measurement in the parent? I'm thinking not, because a measurement is > just a hash. Now if the signature of the hash in the xattr needs a > different key, obviously this differs, but the expensive part > (computing the hash) shouldn't change. Depending on the measurement list template format (eg. ima-ng, ima- sig, custom template format), the template data would contain the file hash, but in addition it might contain the file signature. As keys could be namespace specific, the file signatures could be different. Mimi |
|
From: Serge E. H. <se...@ha...> - 2017-07-25 20:49:41
|
On Tue, Jul 25, 2017 at 04:15:25PM -0400, Mimi Zohar wrote:
> On Tue, 2017-07-25 at 14:43 -0500, Serge E. Hallyn wrote:
> > ...
> > > +static void free_ns_status_cache(struct ima_namespace *ns)
> > > +{
> > > + struct ns_status *status, *next;
> > > +
> > > + write_lock(&ns->ns_status_lock);
> > > + rbtree_postorder_for_each_entry_safe(status, next,
> > > + &ns->ns_status_tree, rb_node)
> > > + kmem_cache_free(ns->ns_status_cache, status);
> > > + ns->ns_status_tree = RB_ROOT;
> > > + write_unlock(&ns->ns_status_lock);
> > > + kmem_cache_destroy(ns->ns_status_cache);
> > > +}
> > > +
> > > static void destroy_ima_ns(struct ima_namespace *ns)
> > > {
> > > put_user_ns(ns->user_ns);
> > > ns_free_inum(&ns->ns);
> > > + free_ns_status_cache(ns);
> > > kfree(ns);
> > > }
> > >
> > > @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
> > > .parent = NULL,
> > > };
> > > EXPORT_SYMBOL(init_ima_ns);
> > > +
> > > +/*
> > > + * __ima_ns_status_find - return the ns_status associated with an inode
> > > + */
> > > +static struct ns_status *__ima_ns_status_find(struct ima_namespace *ns,
> > > + struct inode *inode)
> > > +{
> > > + struct ns_status *status;
> > > + struct rb_node *n = ns->ns_status_tree.rb_node;
> > > +
> > > + while (n) {
> > > + status = rb_entry(n, struct ns_status, rb_node);
> > > +
> > > + if (inode < status->inode)
> > > + n = n->rb_left;
> > > + else if (inode->i_ino > status->i_ino)
> > > + n = n->rb_right;
> > > + else
> > > + break;
> > > + }
> > > + if (!n)
> > > + return NULL;
> > > +
> > > + return status;
> > > +}
> > > +
> > > +/*
> > > + * ima_ns_status_find - return the ns_status associated with an inode
> > > + */
> > > +static struct ns_status *ima_ns_status_find(struct ima_namespace *ns,
> > > + struct inode *inode)
> > > +{
> > > + struct ns_status *status;
> > > +
> > > + read_lock(&ns->ns_status_lock);
> > > + status = __ima_ns_status_find(ns, inode);
> > > + read_unlock(&ns->ns_status_lock);
> > > +
> > > + return status;
> > > +}
> > ...
> > > +
> > > +struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
> > > + struct inode *inode)
> > > +{
> > > + struct ns_status *status;
> > > + int skip_insert = 0;
> > > +
> > > + status = ima_ns_status_find(ns, inode);
> > > + if (status) {
> > > + /*
> > > + * Unlike integrity_iint_cache we are not free'ing the
> > > + * ns_status data when the inode is free'd. So, in addition to
> > > + * checking the inode pointer, we need to make sure the
> > > + * (i_generation, i_ino) pair matches as well. In the future
> > > + * we might want to add support for lazily walking the rbtree
> > > + * to clean it up.
> > > + */
> > > + if (inode->i_ino == status->i_ino &&
> > > + inode->i_generation == status->i_generation)
> > > + return status;
> > > +
> > > + /* Same inode number is reused, overwrite the ns_status */
> > > + skip_insert = 1;
> > > + } else {
> > > + status = kmem_cache_alloc(ns->ns_status_cache, GFP_NOFS);
> > > + if (!status)
> > > + return ERR_PTR(-ENOMEM);
> > > + }
> >
> > What prevents the status from being freed between the read_lock
> > in ima_ns_status_find() and the write_lock in the following line?
> >
> > IIUC it's that ns is always current's ima_ns, which will pin the ns
> > and cause no statuses to be freed. But then the ns should probably
> > not be passed in here? Or a comment should say that ns must be
> > pinned?
> >
> > Just trying to make sure I understand the locking.
>
> iint's are only freed after the last reference to the inode is deleted
> in __fput(). Refer to ima_file_free(). ns_status is a bit different
> in that they are freed on namespace cleanup.
Ok, thanks - that sounds ok then.
|
116 messages has been excluded from this view by a project administrator.
