[PATCH v2 1/3] evm: check hash algorithm passed to init_desc()

[PATCH v2 1/3] evm: check hash algorithm passed to init_desc()

[Roberto Sassu]

This patch prevents memory access beyond the evm_tfm array by checking the
validity of the index (hash algorithm) passed to init_desc(). The hash
algorithm can be arbitrarily set if the security.ima xattr type is not
EVM_XATTR_HMAC.

Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
 security/integrity/evm/evm_crypto.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index e11564eb645b..82a38e801ee4 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -89,6 +89,9 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 		tfm = &hmac_tfm;
 		algo = evm_hmac;
 	} else {
+		if (hash_algo >= HASH_ALGO__LAST)
+			return ERR_PTR(-EINVAL);
+
 		tfm = &evm_tfm[hash_algo];
 		algo = hash_algo_name[hash_algo];
 	}
-- 
2.17.1

Re: [PATCH v2 1/3] evm: check hash algorithm passed to init_desc()

[Mimi Zohar]

On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
> This patch prevents memory access beyond the evm_tfm array by checking the
> validity of the index (hash algorithm) passed to init_desc(). The hash
> algorithm can be arbitrarily set if the security.ima xattr type is not
> EVM_XATTR_HMAC.
> 
> Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: stable@vger.kernel.org

Thanks, queued.

> ---
>  security/integrity/evm/evm_crypto.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index e11564eb645b..82a38e801ee4 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -89,6 +89,9 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
>  		tfm = &hmac_tfm;
>  		algo = evm_hmac;
>  	} else {
> +		if (hash_algo >= HASH_ALGO__LAST)
> +			return ERR_PTR(-EINVAL);
> +
>  		tfm = &evm_tfm[hash_algo];
>  		algo = hash_algo_name[hash_algo];
>  	}

[PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status

[Roberto Sassu]

Currently, ima_appraise_measurement() ignores the EVM status when
evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a valid
security.ima xattr with type IMA_XATTR_DIGEST or IMA_XATTR_DIGEST_NG,
ima_appraise_measurement() returns INTEGRITY_PASS regardless of the EVM
status. The problem is that the EVM status is overwritten with the
appraisal status.

This patch mitigates the issue by selecting signature verification as the
only method allowed for appraisal when EVM is not initialized. Since the
new behavior might break user space, it must be turned on by adding the
'-evm' suffix to the value of the ima_appraise= kernel option.

Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
 Documentation/admin-guide/kernel-parameters.txt | 3 ++-
 security/integrity/ima/ima_appraise.c           | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 138f6664b2e2..d84a2e612b93 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1585,7 +1585,8 @@
 			Set number of hash buckets for inode cache.
 
 	ima_appraise=	[IMA] appraise integrity measurements
-			Format: { "off" | "enforce" | "fix" | "log" }
+			Format: { "off" | "enforce" | "fix" | "log" |
+				  "enforce-evm" | "log-evm" }
 			default: "enforce"
 
 	ima_appraise_tcb [IMA] Deprecated.  Use ima_policy= instead.
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 5fb7127bbe68..afef06e10fb9 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -18,6 +18,7 @@
 
 #include "ima.h"
 
+static bool ima_appraise_req_evm __ro_after_init;
 static int __init default_appraise_setup(char *str)
 {
 #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
@@ -28,6 +29,9 @@ static int __init default_appraise_setup(char *str)
 	else if (strncmp(str, "fix", 3) == 0)
 		ima_appraise = IMA_APPRAISE_FIX;
 #endif
+	if (strcmp(str, "enforce-evm") == 0 ||
+	    strcmp(str, "log-evm") == 0)
+		ima_appraise_req_evm = true;
 	return 1;
 }
 
@@ -245,7 +249,11 @@ int ima_appraise_measurement(enum ima_hooks func,
 	switch (status) {
 	case INTEGRITY_PASS:
 	case INTEGRITY_PASS_IMMUTABLE:
+		break;
 	case INTEGRITY_UNKNOWN:
+		if (ima_appraise_req_evm &&
+		    xattr_value->type != EVM_IMA_XATTR_DIGSIG)
+			goto out;
 		break;
 	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
 	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
-- 
2.17.1

Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status

[Mimi Zohar]

On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
> Currently, ima_appraise_measurement() ignores the EVM status when
> evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a valid
> security.ima xattr with type IMA_XATTR_DIGEST or IMA_XATTR_DIGEST_NG,
> ima_appraise_measurement() returns INTEGRITY_PASS regardless of the EVM
> status. The problem is that the EVM status is overwritten with the
> > appraisal statu

Roberto, your framing of this problem is harsh and misleading.  IMA
and EVM are intentionally independent of each other and can be
configured independently of each other.  The intersection of the two
is the call to evm_verifyxattr().  INTEGRITY_UNKNOWN is returned for a
number of reasons - when EVM is not configured, the EVM hmac key has
not yet been loaded, the protected security attribute is unknown, or
the file is not in policy.

This patch does not differentiate between any of the above cases,
requiring mutable files to always be protected by EVM, when specified
as an "ima_appraise=" option on the boot command line.

IMA could be extended to require EVM on a per IMA policy rule basis.  
Instead of framing allowing IMA file hashes without EVM as a bug that
has existed from the very beginning, now that IMA/EVM have matured and
is being used, you could frame it as extending IMA or hardening.

> 
> This patch mitigates the issue by selecting signature verification as the
> only method allowed for appraisal when EVM is not initialized. Since the
> new behavior might break user space, it must be turned on by adding the
> '-evm' suffix to the value of the ima_appraise= kernel option.
> 
> Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: stable@vger.kernel.org
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 3 ++-
>  security/integrity/ima/ima_appraise.c           | 8 ++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 138f6664b2e2..d84a2e612b93 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1585,7 +1585,8 @@
>  			Set number of hash buckets for inode cache.
>  
>  	ima_appraise=	[IMA] appraise integrity measurements
> -			Format: { "off" | "enforce" | "fix" | "log" }
> +			Format: { "off" | "enforce" | "fix" | "log" |
> +				  "enforce-evm" | "log-evm" } 

Is it necessary to define both "enforce-evm" and "log-evm"?  Perhaps
defining "require-evm" is sufficient.

Mimi

>  			default: "enforce"
>  
>  	ima_appraise_tcb [IMA] Deprecated.  Use ima_policy= instead.
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 5fb7127bbe68..afef06e10fb9 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -18,6 +18,7 @@
>  
>  #include "ima.h"
>  
> +static bool ima_appraise_req_evm __ro_after_init;
>  static int __init default_appraise_setup(char *str)
>  {
>  #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> @@ -28,6 +29,9 @@ static int __init default_appraise_setup(char *str)
>  	else if (strncmp(str, "fix", 3) == 0)
>  		ima_appraise = IMA_APPRAISE_FIX;
>  #endif
> +	if (strcmp(str, "enforce-evm") == 0 ||
> +	    strcmp(str, "log-evm") == 0)
> +		ima_appraise_req_evm = true;
>  	return 1;
>  }
>  
> @@ -245,7 +249,11 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	switch (status) {
>  	case INTEGRITY_PASS:
>  	case INTEGRITY_PASS_IMMUTABLE:
> +		break;
>  	case INTEGRITY_UNKNOWN:
> +		if (ima_appraise_req_evm &&
> +		    xattr_value->type != EVM_IMA_XATTR_DIGSIG)
> +			goto out;
>  		break;
>  	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
>  	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */

Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status

[Roberto Sassu]

On 5/30/2019 2:00 PM, Mimi Zohar wrote:
> On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
>> Currently, ima_appraise_measurement() ignores the EVM status when
>> evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a valid
>> security.ima xattr with type IMA_XATTR_DIGEST or IMA_XATTR_DIGEST_NG,
>> ima_appraise_measurement() returns INTEGRITY_PASS regardless of the EVM
>> status. The problem is that the EVM status is overwritten with the
>>> appraisal statu
> 
> Roberto, your framing of this problem is harsh and misleading.  IMA
> and EVM are intentionally independent of each other and can be
> configured independently of each other.  The intersection of the two
> is the call to evm_verifyxattr().  INTEGRITY_UNKNOWN is returned for a
> number of reasons - when EVM is not configured, the EVM hmac key has
> not yet been loaded, the protected security attribute is unknown, or
> the file is not in policy.
> 
> This patch does not differentiate between any of the above cases,
> requiring mutable files to always be protected by EVM, when specified
> as an "ima_appraise=" option on the boot command line.
> 
> IMA could be extended to require EVM on a per IMA policy rule basis.
> Instead of framing allowing IMA file hashes without EVM as a bug that
> has existed from the very beginning, now that IMA/EVM have matured and
> is being used, you could frame it as extending IMA or hardening.

I'm seeing it from the perspective of an administrator that manages an
already hardened system, and expects that the system only grants access
to files with a valid signature/HMAC. That system would not enforce this
behavior if EVM keys are removed and the digest in security.ima is set
to the actual file digest.

Framing it as a bug rather than an extension would in my opinion help to
convince people about the necessity to switch to the safe mode, if their
system is already hardened.


>> This patch mitigates the issue by selecting signature verification as the
>> only method allowed for appraisal when EVM is not initialized. Since the
>> new behavior might break user space, it must be turned on by adding the
>> '-evm' suffix to the value of the ima_appraise= kernel option.
>>
>> Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> Cc: stable@vger.kernel.org
>> ---
>>   Documentation/admin-guide/kernel-parameters.txt | 3 ++-
>>   security/integrity/ima/ima_appraise.c           | 8 ++++++++
>>   2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 138f6664b2e2..d84a2e612b93 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1585,7 +1585,8 @@
>>   			Set number of hash buckets for inode cache.
>>   
>>   	ima_appraise=	[IMA] appraise integrity measurements
>> -			Format: { "off" | "enforce" | "fix" | "log" }
>> +			Format: { "off" | "enforce" | "fix" | "log" |
>> +				  "enforce-evm" | "log-evm" }
> 
> Is it necessary to define both "enforce-evm" and "log-evm"?  Perhaps
> defining "require-evm" is sufficient.

ima_appraise= accepts as values modes of operation. I consider the -evm
suffix as a modifier of already defined modes.

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status

[Mimi Zohar]

On Mon, 2019-06-03 at 11:25 +0200, Roberto Sassu wrote:
> On 5/30/2019 2:00 PM, Mimi Zohar wrote:
> > On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
> >> Currently, ima_appraise_measurement() ignores the EVM status when
> >> evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a valid
> >> security.ima xattr with type IMA_XATTR_DIGEST or IMA_XATTR_DIGEST_NG,
> >> ima_appraise_measurement() returns INTEGRITY_PASS regardless of the EVM
> >> status. The problem is that the EVM status is overwritten with the
> >>> appraisal status
> > 
> > Roberto, your framing of this problem is harsh and misleading.  IMA
> > and EVM are intentionally independent of each other and can be
> > configured independently of each other.  The intersection of the two
> > is the call to evm_verifyxattr().  INTEGRITY_UNKNOWN is returned for a
> > number of reasons - when EVM is not configured, the EVM hmac key has
> > not yet been loaded, the protected security attribute is unknown, or
> > the file is not in policy.
> > 
> > This patch does not differentiate between any of the above cases,
> > requiring mutable files to always be protected by EVM, when specified
> > as an "ima_appraise=" option on the boot command line.
> > 
> > IMA could be extended to require EVM on a per IMA policy rule basis.
> > Instead of framing allowing IMA file hashes without EVM as a bug that
> > has existed from the very beginning, now that IMA/EVM have matured and
> > is being used, you could frame it as extending IMA or hardening.
> 
> I'm seeing it from the perspective of an administrator that manages an
> already hardened system, and expects that the system only grants access
> to files with a valid signature/HMAC. That system would not enforce this
> behavior if EVM keys are removed and the digest in security.ima is set
> to the actual file digest.
> 
> Framing it as a bug rather than an extension would in my opinion help to
> convince people about the necessity to switch to the safe mode, if their
> system is already hardened.

I don't disagree with you that people should be using EVM to protect
IMA hashes.  If you claim this is a bug in the design from the very
beginning, then there needs some explanation as to why it was
upstreamed as it was.  My review of this patch provided that
context/background.

Mimi

> 
> 
> >> This patch mitigates the issue by selecting signature verification as the
> >> only method allowed for appraisal when EVM is not initialized. Since the
> >> new behavior might break user space, it must be turned on by adding the
> >> '-evm' suffix to the value of the ima_appraise= kernel option.
> >>
> >> Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
> >> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> >> Cc: stable@vger.kernel.org
> 

Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status

[James Bottomley]

On Mon, 2019-06-03 at 11:25 +0200, Roberto Sassu wrote:
> On 5/30/2019 2:00 PM, Mimi Zohar wrote:
> > On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
> > > Currently, ima_appraise_measurement() ignores the EVM status when
> > > evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a
> > > valid security.ima xattr with type IMA_XATTR_DIGEST or
> > > IMA_XATTR_DIGEST_NG, ima_appraise_measurement() returns
> > > INTEGRITY_PASS regardless of the EVM status. The problem is that
> > > the EVM status is overwritten with the appraisal statu
> > 
> > Roberto, your framing of this problem is harsh and misleading.  IMA
> > and EVM are intentionally independent of each other and can be
> > configured independently of each other.  The intersection of the
> > two is the call to evm_verifyxattr().  INTEGRITY_UNKNOWN is
> > returned for a number of reasons - when EVM is not configured, the
> > EVM hmac key has not yet been loaded, the protected security
> > attribute is unknown, or the file is not in policy.
> > 
> > This patch does not differentiate between any of the above cases,
> > requiring mutable files to always be protected by EVM, when
> > specified as an "ima_appraise=" option on the boot command line.
> > 
> > IMA could be extended to require EVM on a per IMA policy rule
> > basis. Instead of framing allowing IMA file hashes without EVM as a
> > bug that has existed from the very beginning, now that IMA/EVM have
> > matured and is being used, you could frame it as extending IMA
> > or hardening.
> 
> I'm seeing it from the perspective of an administrator that manages
> an already hardened system, and expects that the system only grants
> access to files with a valid signature/HMAC. That system would not
> enforce this behavior if EVM keys are removed and the digest in
> security.ima is set to the actual file digest.
> 
> Framing it as a bug rather than an extension would in my opinion help
> to convince people about the necessity to switch to the safe mode, if
> their system is already hardened.

I have a use case for IMA where I use it to enforce immutability of
containers.  In this use case, the cluster admin places hashes on
executables as the image is unpacked so that if an executable file is
changed, IMA will cause an execution failure.  For this use case, I
don't care about the EVM, in fact we don't use it, because the only
object is to fail execution if a binary is mutated.

So I can see your use case requires IMA+EVM, but requiring it would
cause more complexity for my use case.

James

Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status

[Chuck Lever]

> On Jun 3, 2019, at 9:43 AM, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> On Mon, 2019-06-03 at 11:25 +0200, Roberto Sassu wrote:
>> On 5/30/2019 2:00 PM, Mimi Zohar wrote:
>>> On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
>>>> Currently, ima_appraise_measurement() ignores the EVM status when
>>>> evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a
>>>> valid security.ima xattr with type IMA_XATTR_DIGEST or
>>>> IMA_XATTR_DIGEST_NG, ima_appraise_measurement() returns
>>>> INTEGRITY_PASS regardless of the EVM status. The problem is that
>>>> the EVM status is overwritten with the appraisal statu
>>> 
>>> Roberto, your framing of this problem is harsh and misleading.  IMA
>>> and EVM are intentionally independent of each other and can be
>>> configured independently of each other.  The intersection of the
>>> two is the call to evm_verifyxattr().  INTEGRITY_UNKNOWN is
>>> returned for a number of reasons - when EVM is not configured, the
>>> EVM hmac key has not yet been loaded, the protected security
>>> attribute is unknown, or the file is not in policy.
>>> 
>>> This patch does not differentiate between any of the above cases,
>>> requiring mutable files to always be protected by EVM, when
>>> specified as an "ima_appraise=" option on the boot command line.
>>> 
>>> IMA could be extended to require EVM on a per IMA policy rule
>>> basis. Instead of framing allowing IMA file hashes without EVM as a
>>> bug that has existed from the very beginning, now that IMA/EVM have
>>> matured and is being used, you could frame it as extending IMA
>>> or hardening.
>> 
>> I'm seeing it from the perspective of an administrator that manages
>> an already hardened system, and expects that the system only grants
>> access to files with a valid signature/HMAC. That system would not
>> enforce this behavior if EVM keys are removed and the digest in
>> security.ima is set to the actual file digest.
>> 
>> Framing it as a bug rather than an extension would in my opinion help
>> to convince people about the necessity to switch to the safe mode, if
>> their system is already hardened.
> 
> I have a use case for IMA where I use it to enforce immutability of
> containers.  In this use case, the cluster admin places hashes on
> executables as the image is unpacked so that if an executable file is
> changed, IMA will cause an execution failure.  For this use case, I
> don't care about the EVM, in fact we don't use it, because the only
> object is to fail execution if a binary is mutated.
> 
> So I can see your use case requires IMA+EVM, but requiring it would
> cause more complexity for my use case.

... and would not work at all for the current NFS IMA implementation.


--
Chuck Lever

Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status

[Roberto Sassu]

On 6/3/2019 3:43 PM, James Bottomley wrote:
> On Mon, 2019-06-03 at 11:25 +0200, Roberto Sassu wrote:
>> On 5/30/2019 2:00 PM, Mimi Zohar wrote:
>>> On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
>>>> Currently, ima_appraise_measurement() ignores the EVM status when
>>>> evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a
>>>> valid security.ima xattr with type IMA_XATTR_DIGEST or
>>>> IMA_XATTR_DIGEST_NG, ima_appraise_measurement() returns
>>>> INTEGRITY_PASS regardless of the EVM status. The problem is that
>>>> the EVM status is overwritten with the appraisal statu
>>>
>>> Roberto, your framing of this problem is harsh and misleading.  IMA
>>> and EVM are intentionally independent of each other and can be
>>> configured independently of each other.  The intersection of the
>>> two is the call to evm_verifyxattr().  INTEGRITY_UNKNOWN is
>>> returned for a number of reasons - when EVM is not configured, the
>>> EVM hmac key has not yet been loaded, the protected security
>>> attribute is unknown, or the file is not in policy.
>>>
>>> This patch does not differentiate between any of the above cases,
>>> requiring mutable files to always be protected by EVM, when
>>> specified as an "ima_appraise=" option on the boot command line.
>>>
>>> IMA could be extended to require EVM on a per IMA policy rule
>>> basis. Instead of framing allowing IMA file hashes without EVM as a
>>> bug that has existed from the very beginning, now that IMA/EVM have
>>> matured and is being used, you could frame it as extending IMA
>>> or hardening.
>>
>> I'm seeing it from the perspective of an administrator that manages
>> an already hardened system, and expects that the system only grants
>> access to files with a valid signature/HMAC. That system would not
>> enforce this behavior if EVM keys are removed and the digest in
>> security.ima is set to the actual file digest.
>>
>> Framing it as a bug rather than an extension would in my opinion help
>> to convince people about the necessity to switch to the safe mode, if
>> their system is already hardened.
> 
> I have a use case for IMA where I use it to enforce immutability of
> containers.  In this use case, the cluster admin places hashes on
> executables as the image is unpacked so that if an executable file is
> changed, IMA will cause an execution failure.  For this use case, I
> don't care about the EVM, in fact we don't use it, because the only
> object is to fail execution if a binary is mutated.

How would you prevent root in the container from updating security.ima?

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status

[James Bottomley]

On Mon, 2019-06-03 at 16:29 +0200, Roberto Sassu wrote:
> On 6/3/2019 3:43 PM, James Bottomley wrote:
> > On Mon, 2019-06-03 at 11:25 +0200, Roberto Sassu wrote:
> > > On 5/30/2019 2:00 PM, Mimi Zohar wrote:
> > > > On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
> > > > > Currently, ima_appraise_measurement() ignores the EVM status
> > > > > when evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file
> > > > > has a valid security.ima xattr with type IMA_XATTR_DIGEST or
> > > > > IMA_XATTR_DIGEST_NG, ima_appraise_measurement() returns
> > > > > INTEGRITY_PASS regardless of the EVM status. The problem is
> > > > > that the EVM status is overwritten with the appraisal statu
> > > > 
> > > > Roberto, your framing of this problem is harsh and
> > > > misleading.  IMA and EVM are intentionally independent of each
> > > > other and can be configured independently of each other.  The
> > > > intersection of the two is the call to
> > > > evm_verifyxattr().  INTEGRITY_UNKNOWN is
> > > > returned for a number of reasons - when EVM is not configured,
> > > > the EVM hmac key has not yet been loaded, the protected
> > > > security attribute is unknown, or the file is not in policy.
> > > > 
> > > > This patch does not differentiate between any of the above
> > > > cases, requiring mutable files to always be protected by EVM,
> > > > when specified as an "ima_appraise=" option on the boot command
> > > > line.
> > > > 
> > > > IMA could be extended to require EVM on a per IMA policy rule
> > > > basis. Instead of framing allowing IMA file hashes without EVM
> > > > as a bug that has existed from the very beginning, now that
> > > > IMA/EVM have matured and is being used, you could frame it as
> > > > extending IMA or hardening.
> > > 
> > > I'm seeing it from the perspective of an administrator that
> > > manages an already hardened system, and expects that the system
> > > only grants access to files with a valid signature/HMAC. That
> > > system would not enforce this behavior if EVM keys are removed
> > > and the digest in security.ima is set to the actual file digest.
> > > 
> > > Framing it as a bug rather than an extension would in my opinion
> > > help to convince people about the necessity to switch to the safe
> > > mode, if their system is already hardened.
> > 
> > I have a use case for IMA where I use it to enforce immutability of
> > containers.  In this use case, the cluster admin places hashes on
> > executables as the image is unpacked so that if an executable file
> > is changed, IMA will cause an execution failure.  For this use
> > case, I don't care about the EVM, in fact we don't use it, because
> > the only object is to fail execution if a binary is mutated.
> 
> How would you prevent root in the container from updating
> security.ima?

We don't.  We only guarantee immutability for unprivileged containers,
so root can't be inside.

James

Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status

[Roberto Sassu]

On 6/3/2019 4:31 PM, James Bottomley wrote:
> On Mon, 2019-06-03 at 16:29 +0200, Roberto Sassu wrote:
>> On 6/3/2019 3:43 PM, James Bottomley wrote:
>>> On Mon, 2019-06-03 at 11:25 +0200, Roberto Sassu wrote:
>>>> On 5/30/2019 2:00 PM, Mimi Zohar wrote:
>>>>> On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
>>>>>> Currently, ima_appraise_measurement() ignores the EVM status
>>>>>> when evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file
>>>>>> has a valid security.ima xattr with type IMA_XATTR_DIGEST or
>>>>>> IMA_XATTR_DIGEST_NG, ima_appraise_measurement() returns
>>>>>> INTEGRITY_PASS regardless of the EVM status. The problem is
>>>>>> that the EVM status is overwritten with the appraisal statu
>>>>>
>>>>> Roberto, your framing of this problem is harsh and
>>>>> misleading.  IMA and EVM are intentionally independent of each
>>>>> other and can be configured independently of each other.  The
>>>>> intersection of the two is the call to
>>>>> evm_verifyxattr().  INTEGRITY_UNKNOWN is
>>>>> returned for a number of reasons - when EVM is not configured,
>>>>> the EVM hmac key has not yet been loaded, the protected
>>>>> security attribute is unknown, or the file is not in policy.
>>>>>
>>>>> This patch does not differentiate between any of the above
>>>>> cases, requiring mutable files to always be protected by EVM,
>>>>> when specified as an "ima_appraise=" option on the boot command
>>>>> line.
>>>>>
>>>>> IMA could be extended to require EVM on a per IMA policy rule
>>>>> basis. Instead of framing allowing IMA file hashes without EVM
>>>>> as a bug that has existed from the very beginning, now that
>>>>> IMA/EVM have matured and is being used, you could frame it as
>>>>> extending IMA or hardening.
>>>>
>>>> I'm seeing it from the perspective of an administrator that
>>>> manages an already hardened system, and expects that the system
>>>> only grants access to files with a valid signature/HMAC. That
>>>> system would not enforce this behavior if EVM keys are removed
>>>> and the digest in security.ima is set to the actual file digest.
>>>>
>>>> Framing it as a bug rather than an extension would in my opinion
>>>> help to convince people about the necessity to switch to the safe
>>>> mode, if their system is already hardened.
>>>
>>> I have a use case for IMA where I use it to enforce immutability of
>>> containers.  In this use case, the cluster admin places hashes on
>>> executables as the image is unpacked so that if an executable file
>>> is changed, IMA will cause an execution failure.  For this use
>>> case, I don't care about the EVM, in fact we don't use it, because
>>> the only object is to fail execution if a binary is mutated.
>>
>> How would you prevent root in the container from updating
>> security.ima?
> 
> We don't.  We only guarantee immutability for unprivileged containers,
> so root can't be inside.

Ok.

Regarding the new behavior, this must be explicitly enabled by adding
ima_appraise=enforce-evm or log-evm to the kernel command line.
Otherwise, the current behavior is preserved with this patch. Would this
be ok?

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status

[James Bottomley]

On Mon, 2019-06-03 at 16:44 +0200, Roberto Sassu wrote:
> On 6/3/2019 4:31 PM, James Bottomley wrote:
> > On Mon, 2019-06-03 at 16:29 +0200, Roberto Sassu wrote:
[...]
> > > How would you prevent root in the container from updating
> > > security.ima?
> > 
> > We don't.  We only guarantee immutability for unprivileged
> > containers, so root can't be inside.
> 
> Ok.
> 
> Regarding the new behavior, this must be explicitly enabled by adding
> ima_appraise=enforce-evm or log-evm to the kernel command line.
> Otherwise, the current behavior is preserved with this patch. Would
> this be ok?

Sure, as long as it's an opt-in flag, meaning the behaviour of my
kernels on physical cloud systems doesn't change as I upgrade them, I'm
fine with that.

James

[PATCH v2 3/3] ima: show rules with IMA_INMASK correctly

[Roberto Sassu]

Show the '^' character when a policy rule has flag IMA_INMASK.

Fixes: 80eae209d63ac ("IMA: allow reading back the current IMA policy")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
 security/integrity/ima/ima_policy.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e0cc323f948f..ae4034f041c4 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1146,10 +1146,10 @@ enum {
 };
 
 static const char *const mask_tokens[] = {
-	"MAY_EXEC",
-	"MAY_WRITE",
-	"MAY_READ",
-	"MAY_APPEND"
+	"^MAY_EXEC",
+	"^MAY_WRITE",
+	"^MAY_READ",
+	"^MAY_APPEND"
 };
 
 #define __ima_hook_stringify(str)	(#str),
@@ -1209,6 +1209,7 @@ int ima_policy_show(struct seq_file *m, void *v)
 	struct ima_rule_entry *entry = v;
 	int i;
 	char tbuf[64] = {0,};
+	int offset = 0;
 
 	rcu_read_lock();
 
@@ -1232,15 +1233,17 @@ int ima_policy_show(struct seq_file *m, void *v)
 	if (entry->flags & IMA_FUNC)
 		policy_func_show(m, entry->func);
 
-	if (entry->flags & IMA_MASK) {
+	if ((entry->flags & IMA_MASK) || (entry->flags & IMA_INMASK)) {
+		if (entry->flags & IMA_MASK)
+			offset = 1;
 		if (entry->mask & MAY_EXEC)
-			seq_printf(m, pt(Opt_mask), mt(mask_exec));
+			seq_printf(m, pt(Opt_mask), mt(mask_exec) + offset);
 		if (entry->mask & MAY_WRITE)
-			seq_printf(m, pt(Opt_mask), mt(mask_write));
+			seq_printf(m, pt(Opt_mask), mt(mask_write) + offset);
 		if (entry->mask & MAY_READ)
-			seq_printf(m, pt(Opt_mask), mt(mask_read));
+			seq_printf(m, pt(Opt_mask), mt(mask_read) + offset);
 		if (entry->mask & MAY_APPEND)
-			seq_printf(m, pt(Opt_mask), mt(mask_append));
+			seq_printf(m, pt(Opt_mask), mt(mask_append) + offset);
 		seq_puts(m, " ");
 	}
 
-- 
2.17.1

Re: [PATCH v2 3/3] ima: show rules with IMA_INMASK correctly

[Mimi Zohar]

On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
> Show the '^' character when a policy rule has flag IMA_INMASK.
> 
> Fixes: 80eae209d63ac ("IMA: allow reading back the current IMA policy")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: stable@vger.kernel.org

Thanks, queued.

> ---
>  security/integrity/ima/ima_policy.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index e0cc323f948f..ae4034f041c4 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1146,10 +1146,10 @@ enum {
>  };
>  
>  static const char *const mask_tokens[] = {
> -	"MAY_EXEC",
> -	"MAY_WRITE",
> -	"MAY_READ",
> -	"MAY_APPEND"
> +	"^MAY_EXEC",
> +	"^MAY_WRITE",
> +	"^MAY_READ",
> +	"^MAY_APPEND"
>  };
>  
>  #define __ima_hook_stringify(str)	(#str),
> @@ -1209,6 +1209,7 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	struct ima_rule_entry *entry = v;
>  	int i;
>  	char tbuf[64] = {0,};
> +	int offset = 0;
>  
>  	rcu_read_lock();
>  
> @@ -1232,15 +1233,17 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	if (entry->flags & IMA_FUNC)
>  		policy_func_show(m, entry->func);
>  
> -	if (entry->flags & IMA_MASK) {
> +	if ((entry->flags & IMA_MASK) || (entry->flags & IMA_INMASK)) {
> +		if (entry->flags & IMA_MASK)
> +			offset = 1;
>  		if (entry->mask & MAY_EXEC)
> -			seq_printf(m, pt(Opt_mask), mt(mask_exec));
> +			seq_printf(m, pt(Opt_mask), mt(mask_exec) + offset);
>  		if (entry->mask & MAY_WRITE)
> -			seq_printf(m, pt(Opt_mask), mt(mask_write));
> +			seq_printf(m, pt(Opt_mask), mt(mask_write) + offset);
>  		if (entry->mask & MAY_READ)
> -			seq_printf(m, pt(Opt_mask), mt(mask_read));
> +			seq_printf(m, pt(Opt_mask), mt(mask_read) + offset);
>  		if (entry->mask & MAY_APPEND)
> -			seq_printf(m, pt(Opt_mask), mt(mask_append));
> +			seq_printf(m, pt(Opt_mask), mt(mask_append) + offset);
>  		seq_puts(m, " ");
>  	}
>