[RFC][PATCH 0/2] ima: preserve integrity of dynamic data

[RFC][PATCH 0/2] ima: preserve integrity of dynamic data

[Roberto Sassu]

One of the most challenging tasks for remote attestation is how to
handle the measurement of dynamic data (mutable files). When the default
policy is selected, IMA measures files accessed by root processes (TCB).

However, if a file was previously modified by another process, the digest
included in the new measurement list is unknown, and verifiers must assume,
during a remote attestation, that the system was compromised, because they
don't know if that file was written by a process in the TCB or not.

The goal of this patch set is to enforce an integrity policy on appraised
files, to avoid reporting measurements of dynamic data after they have been
modified. Only the initial state should be reported (e.g. the file
signature, or a digest list).

In order to properly enforce an integrity policy, it is necessary to
specify in the IMA policy process credentials rather than file attributes.

For example, the rule:

appraise fowner=0

could be replaced with:

appraise uid=0
appraise euid=0

This patch set has been developed on top of linux-integrity/next
(commit 9785a867) and https://patchwork.kernel.org/patch/10013185/
(ima: Store measurement after appraisal).

Roberto Sassu (2):
  ima: preserve the integrity of appraised files
  ima: don't measure files in the TCB if Biba strict policy is enforced

 Documentation/admin-guide/kernel-parameters.txt |  4 ++
 security/integrity/ima/ima.h                    | 23 ++++++++++
 security/integrity/ima/ima_appraise.c           | 61 +++++++++++++++++++++++++
 security/integrity/ima/ima_main.c               | 40 ++++++++++++----
 4 files changed, 118 insertions(+), 10 deletions(-)

-- 
2.11.0

[RFC][PATCH 1/2] ima: preserve the integrity of appraised files

[Roberto Sassu]

The existing 'ima_appraise_tcb' policy aims at protecting the integrity
of files owned by root against offline attacks, while LSMs should decide
if those files can be modified, and new files can be created. However,
LSMs cannot take this decision independently, if IMA appraises only
a subset of files that a process is allowed to access. A process can
become compromised due to reading files not appraised by IMA.

To avoid this issue, the IMA policy should contain as criteria process
credentials rather than files attributes. Then, when a process matches
those criteria, files will be always appraised by IMA, regardless of
file attributes.

The IMA policy with process credentials will define which process belongs
to the TCB and which not. With this information, IMA would be be able
to preserve the integrity of appraised files, without an LSM, for example
by denying writes by processes outside the TCB (Biba strict policy).

This patch adds support for enforcing the Biba strict policy. More
policies will be introduced later. Enforcement will be enabled by
adding 'ima_integrity_policy=biba-strict' to the kernel command line.

To enforce this policy, it is necessary to upload to IMA a new policy
which defines the subjects part of the TCB. For example, the rule
'appraise fowner=0' could be replaced with two rules: 'appraise uid=0'
and 'appraise euid=0'.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  4 ++
 security/integrity/ima/ima.h                    | 23 ++++++++++
 security/integrity/ima/ima_appraise.c           | 61 +++++++++++++++++++++++++
 security/integrity/ima/ima_main.c               | 25 ++++++----
 4 files changed, 104 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 05496622b4ef..37810c6a3b28 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1532,6 +1532,10 @@
 	                [IMA] Define a custom template format.
 			Format: { "field1|...|fieldN" }
 
+	ima_integrity_policy=
+			[IMA] Select an integrity policy to enforce.
+			Policies: { "biba-strict" }
+
 	ima.ahash_minsize= [IMA] Minimum file size for asynchronous hash usage
 			Format: <min_file_size>
 			Set the minimal file size for using asynchronous hash.
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487ad259..377e1d8c2afb 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -36,6 +36,8 @@ enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN,
 		     IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII };
 enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
 
+enum integrity_policies { BIBA_STRICT = 1, INTEGRITY_POLICY__LAST };
+
 /* digest size for IMA, fits SHA1 or MD5 */
 #define IMA_DIGEST_SIZE		SHA1_DIGEST_SIZE
 #define IMA_EVENT_NAME_LEN_MAX	255
@@ -57,6 +59,7 @@ extern int ima_initialized;
 extern int ima_used_chip;
 extern int ima_hash_algo;
 extern int ima_appraise;
+extern int ima_integrity_policy;
 
 /* IMA event related data */
 struct ima_event_data {
@@ -247,6 +250,11 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 				 int xattr_len);
 int ima_read_xattr(struct dentry *dentry,
 		   struct evm_ima_xattr_data **xattr_value);
+bool ima_inode_is_appraised(struct dentry *dentry, struct inode *inode);
+bool ima_appraise_biba_check(struct file *file,
+			     struct integrity_iint_cache *iint,
+			     int must_appraise, char **pathbuf,
+			     const char **pathname, char *namebuf);
 
 #else
 static inline int ima_appraise_measurement(enum ima_hooks func,
@@ -289,6 +297,21 @@ static inline int ima_read_xattr(struct dentry *dentry,
 	return 0;
 }
 
+static inline bool ima_inode_is_appraised(struct dentry *dentry,
+					  struct inode *inode);
+{
+	return false;
+}
+
+static inline bool ima_appraise_biba_check(struct file *file,
+					   struct integrity_iint_cache *iint,
+					   int must_appraise, char **pathbuf,
+					   const char **pathname,
+					   char *namebuf)
+{
+	return false;
+}
+
 #endif /* CONFIG_IMA_APPRAISE */
 
 /* LSM based policy rules require audit */
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 809ba70fbbbf..c24824ef64c4 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -18,6 +18,10 @@
 
 #include "ima.h"
 
+static char *ima_integrity_policies_str[INTEGRITY_POLICY__LAST] = {
+	[BIBA_STRICT] = "biba-strict",
+};
+
 static int __init default_appraise_setup(char *str)
 {
 #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
@@ -33,6 +37,15 @@ static int __init default_appraise_setup(char *str)
 
 __setup("ima_appraise=", default_appraise_setup);
 
+static int __init integrity_policy_setup(char *str)
+{
+	if (strcmp(str, ima_integrity_policies_str[BIBA_STRICT]) == 0)
+		ima_integrity_policy = BIBA_STRICT;
+
+	return 1;
+}
+__setup("ima_integrity_policy=", integrity_policy_setup);
+
 /*
  * is_ima_appraise_enabled - return appraise status
  *
@@ -189,6 +202,54 @@ int ima_read_xattr(struct dentry *dentry,
 	return ret;
 }
 
+bool ima_inode_is_appraised(struct dentry *dentry, struct inode *inode)
+{
+	ssize_t len;
+
+	len =  __vfs_getxattr(dentry, inode, XATTR_NAME_IMA, NULL, 0);
+
+	return (len > 0 && len >= sizeof(struct evm_ima_xattr_data));
+}
+
+/*
+ * ima_appraise_biba_check - detect violations of a Biba policy
+ *
+ * The appraisal policy identifies which subjects belong to the TCB. Files
+ * with a valid digital signature or HMAC are also part of the TCB. This
+ * function detects attempts to write appraised files by subjects outside
+ * the TCB. The Biba strict policy denies this operation.
+ *
+ * Return: true if current operation violates a Biba policy, false otherwise
+ */
+bool ima_appraise_biba_check(struct file *file,
+			     struct integrity_iint_cache *iint,
+			     int must_appraise, char **pathbuf,
+			     const char **pathname, char *namebuf)
+{
+	struct inode *inode = file_inode(file);
+	fmode_t mode = file->f_mode;
+	char *cause = "write_down";
+
+	/* check write up, ima_appraise_measurement() checks read down */
+	if (!must_appraise && (mode & FMODE_WRITE)) {
+		if (IS_IMA(inode)) {
+			if (!iint)
+				iint = integrity_iint_find(inode);
+			if (iint->flags & IMA_APPRAISE)
+				goto out_violation;
+		} else if (ima_inode_is_appraised(file_dentry(file), inode)) {
+			goto out_violation;
+		}
+	}
+	return false;
+out_violation:
+	*pathname = ima_d_path(&file->f_path, pathbuf, namebuf);
+	integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, *pathname,
+			    ima_integrity_policies_str[ima_integrity_policy],
+			    cause, 0, 0);
+	return true;
+}
+
 /*
  * ima_appraise_measurement - appraise file measurement
  *
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index bb7e36e90c79..6e85ea8e2373 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -31,6 +31,7 @@ int ima_initialized;
 
 #ifdef CONFIG_IMA_APPRAISE
 int ima_appraise = IMA_APPRAISE_ENFORCE;
+int ima_integrity_policy;
 #else
 int ima_appraise;
 #endif
@@ -103,7 +104,8 @@ static void ima_rdwr_violation_check(struct file *file,
 	if (!send_tomtou && !send_writers)
 		return;
 
-	*pathname = ima_d_path(&file->f_path, pathbuf, filename);
+	if (!*pathname)
+		*pathname = ima_d_path(&file->f_path, pathbuf, filename);
 
 	if (send_tomtou)
 		ima_add_violation(file, *pathname, iint,
@@ -168,7 +170,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
 	struct evm_ima_xattr_data *xattr_value = NULL;
 	int xattr_len = 0;
-	bool violation_check;
+	bool violation_check, policy_violation = false;
 	enum hash_algo hash_algo;
 
 	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
@@ -181,7 +183,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	action = ima_get_action(inode, mask, func, &pcr);
 	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
 			   (ima_policy_flag & IMA_MEASURE));
-	if (!action && !violation_check)
+	if (!action && !violation_check && !ima_integrity_policy)
 		return 0;
 
 	must_appraise = action & IMA_APPRAISE;
@@ -198,13 +200,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 			goto out;
 	}
 
-	if (violation_check) {
+	if (ima_integrity_policy)
+		policy_violation = ima_appraise_biba_check(file, iint,
+						must_appraise, &pathbuf,
+						&pathname, filename);
+	if (violation_check)
 		ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
 					 &pathbuf, &pathname);
-		if (!action) {
-			rc = 0;
-			goto out_free;
-		}
+	if (!action) {
+		rc = 0;
+		goto out_free;
 	}
 
 	/* Determine if already appraised/measured based on bitmask
@@ -260,7 +265,9 @@ 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_integrity_policy && policy_violation)) &&
+	    (ima_appraise & IMA_APPRAISE_ENFORCE))
 		return -EACCES;
 	return 0;
 }
-- 
2.11.0

Re: [RFC][PATCH 1/2] ima: preserve the integrity of appraised files

[Mimi Zohar]

On Fri, 2017-10-20 at 17:41 +0200, Roberto Sassu wrote:
> The existing 'ima_appraise_tcb' policy aims at protecting the integrity
> of files owned by root against offline attacks, while LSMs should decide
> if those files can be modified, and new files can be created. However,
> LSMs cannot take this decision independently, if IMA appraises only
> a subset of files that a process is allowed to access. A process can
> become compromised due to reading files not appraised by IMA.
> 
> To avoid this issue, the IMA policy should contain as criteria process
> credentials rather than files attributes. Then, when a process matches
> those criteria, files will be always appraised by IMA, regardless of
> file attributes.
> 
> The IMA policy with process credentials will define which process belongs
> to the TCB and which not. With this information, IMA would be be able
> to preserve the integrity of appraised files, without an LSM, for example
> by denying writes by processes outside the TCB (Biba strict policy).
> 
> This patch adds support for enforcing the Biba strict policy. More
> policies will be introduced later. Enforcement will be enabled by
> adding 'ima_integrity_policy=biba-strict' to the kernel command line.

Way, way back, before the any of the integrity code was upstreamed,
the original integrity design had LSMs calling exported integrity
functions to verify file integrity (eg. evm_verifyxattr).  A decision
was made, at the time, to have a clear delineation between Mandatory
Access Control (MAC) and integrity.  There have been recent
discussions about LSMs blurring this line and calling
evm_verifyxattr() directly.

Never was there any thought or discussion of adding MAC to the
integrity subsystem.  A Biba implementation doesn't belong in IMA, but
should be defined as a separate LSM.  (Years ago we implemented a low-
water mark LSM named SLIM, based on LOMAC.)

Mimi

> To enforce this policy, it is necessary to upload to IMA a new policy
> which defines the subjects part of the TCB. For example, the rule
> 'appraise fowner=0' could be replaced with two rules: 'appraise uid=0'
> and 'appraise euid=0'.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  4 ++
>  security/integrity/ima/ima.h                    | 23 ++++++++++
>  security/integrity/ima/ima_appraise.c           | 61 +++++++++++++++++++++++++
>  security/integrity/ima/ima_main.c               | 25 ++++++----
>  4 files changed, 104 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 05496622b4ef..37810c6a3b28 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1532,6 +1532,10 @@
>  	                [IMA] Define a custom template format.
>  			Format: { "field1|...|fieldN" }
> 
> +	ima_integrity_policy=
> +			[IMA] Select an integrity policy to enforce.
> +			Policies: { "biba-strict" }
> +
>  	ima.ahash_minsize= [IMA] Minimum file size for asynchronous hash usage
>  			Format: <min_file_size>
>  			Set the minimal file size for using asynchronous hash.
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d52b487ad259..377e1d8c2afb 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -36,6 +36,8 @@ enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN,
>  		     IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII };
>  enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
> 
> +enum integrity_policies { BIBA_STRICT = 1, INTEGRITY_POLICY__LAST };
> +
>  /* digest size for IMA, fits SHA1 or MD5 */
>  #define IMA_DIGEST_SIZE		SHA1_DIGEST_SIZE
>  #define IMA_EVENT_NAME_LEN_MAX	255
> @@ -57,6 +59,7 @@ extern int ima_initialized;
>  extern int ima_used_chip;
>  extern int ima_hash_algo;
>  extern int ima_appraise;
> +extern int ima_integrity_policy;
> 
>  /* IMA event related data */
>  struct ima_event_data {
> @@ -247,6 +250,11 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
>  				 int xattr_len);
>  int ima_read_xattr(struct dentry *dentry,
>  		   struct evm_ima_xattr_data **xattr_value);
> +bool ima_inode_is_appraised(struct dentry *dentry, struct inode *inode);
> +bool ima_appraise_biba_check(struct file *file,
> +			     struct integrity_iint_cache *iint,
> +			     int must_appraise, char **pathbuf,
> +			     const char **pathname, char *namebuf);
> 
>  #else
>  static inline int ima_appraise_measurement(enum ima_hooks func,
> @@ -289,6 +297,21 @@ static inline int ima_read_xattr(struct dentry *dentry,
>  	return 0;
>  }
> 
> +static inline bool ima_inode_is_appraised(struct dentry *dentry,
> +					  struct inode *inode);
> +{
> +	return false;
> +}
> +
> +static inline bool ima_appraise_biba_check(struct file *file,
> +					   struct integrity_iint_cache *iint,
> +					   int must_appraise, char **pathbuf,
> +					   const char **pathname,
> +					   char *namebuf)
> +{
> +	return false;
> +}
> +
>  #endif /* CONFIG_IMA_APPRAISE */
> 
>  /* LSM based policy rules require audit */
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 809ba70fbbbf..c24824ef64c4 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -18,6 +18,10 @@
> 
>  #include "ima.h"
> 
> +static char *ima_integrity_policies_str[INTEGRITY_POLICY__LAST] = {
> +	[BIBA_STRICT] = "biba-strict",
> +};
> +
>  static int __init default_appraise_setup(char *str)
>  {
>  #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> @@ -33,6 +37,15 @@ static int __init default_appraise_setup(char *str)
> 
>  __setup("ima_appraise=", default_appraise_setup);
> 
> +static int __init integrity_policy_setup(char *str)
> +{
> +	if (strcmp(str, ima_integrity_policies_str[BIBA_STRICT]) == 0)
> +		ima_integrity_policy = BIBA_STRICT;
> +
> +	return 1;
> +}
> +__setup("ima_integrity_policy=", integrity_policy_setup);
> +
>  /*
>   * is_ima_appraise_enabled - return appraise status
>   *
> @@ -189,6 +202,54 @@ int ima_read_xattr(struct dentry *dentry,
>  	return ret;
>  }
> 
> +bool ima_inode_is_appraised(struct dentry *dentry, struct inode *inode)
> +{
> +	ssize_t len;
> +
> +	len =  __vfs_getxattr(dentry, inode, XATTR_NAME_IMA, NULL, 0);
> +
> +	return (len > 0 && len >= sizeof(struct evm_ima_xattr_data));
> +}
> +
> +/*
> + * ima_appraise_biba_check - detect violations of a Biba policy
> + *
> + * The appraisal policy identifies which subjects belong to the TCB. Files
> + * with a valid digital signature or HMAC are also part of the TCB. This
> + * function detects attempts to write appraised files by subjects outside
> + * the TCB. The Biba strict policy denies this operation.
> + *
> + * Return: true if current operation violates a Biba policy, false otherwise
> + */
> +bool ima_appraise_biba_check(struct file *file,
> +			     struct integrity_iint_cache *iint,
> +			     int must_appraise, char **pathbuf,
> +			     const char **pathname, char *namebuf)
> +{
> +	struct inode *inode = file_inode(file);
> +	fmode_t mode = file->f_mode;
> +	char *cause = "write_down";
> +
> +	/* check write up, ima_appraise_measurement() checks read down */
> +	if (!must_appraise && (mode & FMODE_WRITE)) {
> +		if (IS_IMA(inode)) {
> +			if (!iint)
> +				iint = integrity_iint_find(inode);
> +			if (iint->flags & IMA_APPRAISE)
> +				goto out_violation;
> +		} else if (ima_inode_is_appraised(file_dentry(file), inode)) {
> +			goto out_violation;
> +		}
> +	}
> +	return false;
> +out_violation:
> +	*pathname = ima_d_path(&file->f_path, pathbuf, namebuf);
> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, *pathname,
> +			    ima_integrity_policies_str[ima_integrity_policy],
> +			    cause, 0, 0);
> +	return true;
> +}
> +
>  /*
>   * ima_appraise_measurement - appraise file measurement
>   *
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index bb7e36e90c79..6e85ea8e2373 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -31,6 +31,7 @@ int ima_initialized;
> 
>  #ifdef CONFIG_IMA_APPRAISE
>  int ima_appraise = IMA_APPRAISE_ENFORCE;
> +int ima_integrity_policy;
>  #else
>  int ima_appraise;
>  #endif
> @@ -103,7 +104,8 @@ static void ima_rdwr_violation_check(struct file *file,
>  	if (!send_tomtou && !send_writers)
>  		return;
> 
> -	*pathname = ima_d_path(&file->f_path, pathbuf, filename);
> +	if (!*pathname)
> +		*pathname = ima_d_path(&file->f_path, pathbuf, filename);
> 
>  	if (send_tomtou)
>  		ima_add_violation(file, *pathname, iint,
> @@ -168,7 +170,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>  	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
>  	struct evm_ima_xattr_data *xattr_value = NULL;
>  	int xattr_len = 0;
> -	bool violation_check;
> +	bool violation_check, policy_violation = false;
>  	enum hash_algo hash_algo;
> 
>  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> @@ -181,7 +183,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>  	action = ima_get_action(inode, mask, func, &pcr);
>  	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
>  			   (ima_policy_flag & IMA_MEASURE));
> -	if (!action && !violation_check)
> +	if (!action && !violation_check && !ima_integrity_policy)
>  		return 0;
> 
>  	must_appraise = action & IMA_APPRAISE;
> @@ -198,13 +200,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>  			goto out;
>  	}
> 
> -	if (violation_check) {
> +	if (ima_integrity_policy)
> +		policy_violation = ima_appraise_biba_check(file, iint,
> +						must_appraise, &pathbuf,
> +						&pathname, filename);
> +	if (violation_check)
>  		ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
>  					 &pathbuf, &pathname);
> -		if (!action) {
> -			rc = 0;
> -			goto out_free;
> -		}
> +	if (!action) {
> +		rc = 0;
> +		goto out_free;
>  	}
> 
>  	/* Determine if already appraised/measured based on bitmask
> @@ -260,7 +265,9 @@ 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_integrity_policy && policy_violation)) &&
> +	    (ima_appraise & IMA_APPRAISE_ENFORCE))
>  		return -EACCES;
>  	return 0;
>  }

Re: [RFC][PATCH 1/2] ima: preserve the integrity of appraised files

[Roberto Sassu]

On 10/23/2017 1:46 PM, Mimi Zohar wrote:
> On Fri, 2017-10-20 at 17:41 +0200, Roberto Sassu wrote:
>> The existing 'ima_appraise_tcb' policy aims at protecting the integrity
>> of files owned by root against offline attacks, while LSMs should decide
>> if those files can be modified, and new files can be created. However,
>> LSMs cannot take this decision independently, if IMA appraises only
>> a subset of files that a process is allowed to access. A process can
>> become compromised due to reading files not appraised by IMA.
>>
>> To avoid this issue, the IMA policy should contain as criteria process
>> credentials rather than files attributes. Then, when a process matches
>> those criteria, files will be always appraised by IMA, regardless of
>> file attributes.
>>
>> The IMA policy with process credentials will define which process belongs
>> to the TCB and which not. With this information, IMA would be be able
>> to preserve the integrity of appraised files, without an LSM, for example
>> by denying writes by processes outside the TCB (Biba strict policy).
>>
>> This patch adds support for enforcing the Biba strict policy. More
>> policies will be introduced later. Enforcement will be enabled by
>> adding 'ima_integrity_policy=biba-strict' to the kernel command line.
> 
> Way, way back, before the any of the integrity code was upstreamed,
> the original integrity design had LSMs calling exported integrity
> functions to verify file integrity (eg. evm_verifyxattr).  A decision
> was made, at the time, to have a clear delineation between Mandatory
> Access Control (MAC) and integrity.  There have been recent
> discussions about LSMs blurring this line and calling
> evm_verifyxattr() directly.

If IMA/EVM were used to check the integrity of every file (content +
labels) before any other LSMs makes security decisions, I would agree
with you that there are two distinct layers: IMA/EVM at the bottom,
that protect the system against offline attacks (the association
between file content and LSM labels); LSMs at the top, protect it
against runtime attacks (i.e. preserve the integrity of the TCB).

If instead IMA appraises a subset of the system, e.g. when the default
appraisal policy (called appraise_tcb) is selected, then LSMs alone
cannot guarantee anymore the runtime integrity of the system if subjects
in the LSMs TCB are allowed to read files not verified by IMA (read up
violation in the Biba strict model, because the integrity of files not
verified by IMA is low).

Then, in order to preserve the runtime integrity of the system, IMA must
complement LSMs and prevent the non-appraised portion of the system
from interacting with the appraised portion.

For example, the recent work by Matthew Garrett (IMA: Support using new
creds in appraisal policy) goes in this direction. With the policy:

appraise euid=0 func=CREDS_CHECK

IMA enforces the Biba strict policy (read up) because it prevents bad
code, loaded through execve(), from being executed by the TCB (root
processes).

As I mentioned in the patch set description, it could be possible to
enforce a more generic policy, which includes also FILE_CHECK and
MMAP_CHECK.

With digital signatures, the enforcement of the write down rule is
guaranteed because signed files are immutable. This patch set adds
support for enforcing the write down rule on mutable files.

Roberto


> Never was there any thought or discussion of adding MAC to the
> integrity subsystem.  A Biba implementation doesn't belong in IMA, but
> should be defined as a separate LSM.  (Years ago we implemented a low-
> water mark LSM named SLIM, based on LOMAC.)
> 
> Mimi
> 
>> To enforce this policy, it is necessary to upload to IMA a new policy
>> which defines the subjects part of the TCB. For example, the rule
>> 'appraise fowner=0' could be replaced with two rules: 'appraise uid=0'
>> and 'appraise euid=0'.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   Documentation/admin-guide/kernel-parameters.txt |  4 ++
>>   security/integrity/ima/ima.h                    | 23 ++++++++++
>>   security/integrity/ima/ima_appraise.c           | 61 +++++++++++++++++++++++++
>>   security/integrity/ima/ima_main.c               | 25 ++++++----
>>   4 files changed, 104 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 05496622b4ef..37810c6a3b28 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1532,6 +1532,10 @@
>>   	                [IMA] Define a custom template format.
>>   			Format: { "field1|...|fieldN" }
>>
>> +	ima_integrity_policy=
>> +			[IMA] Select an integrity policy to enforce.
>> +			Policies: { "biba-strict" }
>> +
>>   	ima.ahash_minsize= [IMA] Minimum file size for asynchronous hash usage
>>   			Format: <min_file_size>
>>   			Set the minimal file size for using asynchronous hash.
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index d52b487ad259..377e1d8c2afb 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -36,6 +36,8 @@ enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN,
>>   		     IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII };
>>   enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
>>
>> +enum integrity_policies { BIBA_STRICT = 1, INTEGRITY_POLICY__LAST };
>> +
>>   /* digest size for IMA, fits SHA1 or MD5 */
>>   #define IMA_DIGEST_SIZE		SHA1_DIGEST_SIZE
>>   #define IMA_EVENT_NAME_LEN_MAX	255
>> @@ -57,6 +59,7 @@ extern int ima_initialized;
>>   extern int ima_used_chip;
>>   extern int ima_hash_algo;
>>   extern int ima_appraise;
>> +extern int ima_integrity_policy;
>>
>>   /* IMA event related data */
>>   struct ima_event_data {
>> @@ -247,6 +250,11 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
>>   				 int xattr_len);
>>   int ima_read_xattr(struct dentry *dentry,
>>   		   struct evm_ima_xattr_data **xattr_value);
>> +bool ima_inode_is_appraised(struct dentry *dentry, struct inode *inode);
>> +bool ima_appraise_biba_check(struct file *file,
>> +			     struct integrity_iint_cache *iint,
>> +			     int must_appraise, char **pathbuf,
>> +			     const char **pathname, char *namebuf);
>>
>>   #else
>>   static inline int ima_appraise_measurement(enum ima_hooks func,
>> @@ -289,6 +297,21 @@ static inline int ima_read_xattr(struct dentry *dentry,
>>   	return 0;
>>   }
>>
>> +static inline bool ima_inode_is_appraised(struct dentry *dentry,
>> +					  struct inode *inode);
>> +{
>> +	return false;
>> +}
>> +
>> +static inline bool ima_appraise_biba_check(struct file *file,
>> +					   struct integrity_iint_cache *iint,
>> +					   int must_appraise, char **pathbuf,
>> +					   const char **pathname,
>> +					   char *namebuf)
>> +{
>> +	return false;
>> +}
>> +
>>   #endif /* CONFIG_IMA_APPRAISE */
>>
>>   /* LSM based policy rules require audit */
>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>> index 809ba70fbbbf..c24824ef64c4 100644
>> --- a/security/integrity/ima/ima_appraise.c
>> +++ b/security/integrity/ima/ima_appraise.c
>> @@ -18,6 +18,10 @@
>>
>>   #include "ima.h"
>>
>> +static char *ima_integrity_policies_str[INTEGRITY_POLICY__LAST] = {
>> +	[BIBA_STRICT] = "biba-strict",
>> +};
>> +
>>   static int __init default_appraise_setup(char *str)
>>   {
>>   #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
>> @@ -33,6 +37,15 @@ static int __init default_appraise_setup(char *str)
>>
>>   __setup("ima_appraise=", default_appraise_setup);
>>
>> +static int __init integrity_policy_setup(char *str)
>> +{
>> +	if (strcmp(str, ima_integrity_policies_str[BIBA_STRICT]) == 0)
>> +		ima_integrity_policy = BIBA_STRICT;
>> +
>> +	return 1;
>> +}
>> +__setup("ima_integrity_policy=", integrity_policy_setup);
>> +
>>   /*
>>    * is_ima_appraise_enabled - return appraise status
>>    *
>> @@ -189,6 +202,54 @@ int ima_read_xattr(struct dentry *dentry,
>>   	return ret;
>>   }
>>
>> +bool ima_inode_is_appraised(struct dentry *dentry, struct inode *inode)
>> +{
>> +	ssize_t len;
>> +
>> +	len =  __vfs_getxattr(dentry, inode, XATTR_NAME_IMA, NULL, 0);
>> +
>> +	return (len > 0 && len >= sizeof(struct evm_ima_xattr_data));
>> +}
>> +
>> +/*
>> + * ima_appraise_biba_check - detect violations of a Biba policy
>> + *
>> + * The appraisal policy identifies which subjects belong to the TCB. Files
>> + * with a valid digital signature or HMAC are also part of the TCB. This
>> + * function detects attempts to write appraised files by subjects outside
>> + * the TCB. The Biba strict policy denies this operation.
>> + *
>> + * Return: true if current operation violates a Biba policy, false otherwise
>> + */
>> +bool ima_appraise_biba_check(struct file *file,
>> +			     struct integrity_iint_cache *iint,
>> +			     int must_appraise, char **pathbuf,
>> +			     const char **pathname, char *namebuf)
>> +{
>> +	struct inode *inode = file_inode(file);
>> +	fmode_t mode = file->f_mode;
>> +	char *cause = "write_down";
>> +
>> +	/* check write up, ima_appraise_measurement() checks read down */
>> +	if (!must_appraise && (mode & FMODE_WRITE)) {
>> +		if (IS_IMA(inode)) {
>> +			if (!iint)
>> +				iint = integrity_iint_find(inode);
>> +			if (iint->flags & IMA_APPRAISE)
>> +				goto out_violation;
>> +		} else if (ima_inode_is_appraised(file_dentry(file), inode)) {
>> +			goto out_violation;
>> +		}
>> +	}
>> +	return false;
>> +out_violation:
>> +	*pathname = ima_d_path(&file->f_path, pathbuf, namebuf);
>> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, *pathname,
>> +			    ima_integrity_policies_str[ima_integrity_policy],
>> +			    cause, 0, 0);
>> +	return true;
>> +}
>> +
>>   /*
>>    * ima_appraise_measurement - appraise file measurement
>>    *
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index bb7e36e90c79..6e85ea8e2373 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -31,6 +31,7 @@ int ima_initialized;
>>
>>   #ifdef CONFIG_IMA_APPRAISE
>>   int ima_appraise = IMA_APPRAISE_ENFORCE;
>> +int ima_integrity_policy;
>>   #else
>>   int ima_appraise;
>>   #endif
>> @@ -103,7 +104,8 @@ static void ima_rdwr_violation_check(struct file *file,
>>   	if (!send_tomtou && !send_writers)
>>   		return;
>>
>> -	*pathname = ima_d_path(&file->f_path, pathbuf, filename);
>> +	if (!*pathname)
>> +		*pathname = ima_d_path(&file->f_path, pathbuf, filename);
>>
>>   	if (send_tomtou)
>>   		ima_add_violation(file, *pathname, iint,
>> @@ -168,7 +170,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>>   	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
>>   	struct evm_ima_xattr_data *xattr_value = NULL;
>>   	int xattr_len = 0;
>> -	bool violation_check;
>> +	bool violation_check, policy_violation = false;
>>   	enum hash_algo hash_algo;
>>
>>   	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
>> @@ -181,7 +183,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>>   	action = ima_get_action(inode, mask, func, &pcr);
>>   	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
>>   			   (ima_policy_flag & IMA_MEASURE));
>> -	if (!action && !violation_check)
>> +	if (!action && !violation_check && !ima_integrity_policy)
>>   		return 0;
>>
>>   	must_appraise = action & IMA_APPRAISE;
>> @@ -198,13 +200,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>>   			goto out;
>>   	}
>>
>> -	if (violation_check) {
>> +	if (ima_integrity_policy)
>> +		policy_violation = ima_appraise_biba_check(file, iint,
>> +						must_appraise, &pathbuf,
>> +						&pathname, filename);
>> +	if (violation_check)
>>   		ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
>>   					 &pathbuf, &pathname);
>> -		if (!action) {
>> -			rc = 0;
>> -			goto out_free;
>> -		}
>> +	if (!action) {
>> +		rc = 0;
>> +		goto out_free;
>>   	}
>>
>>   	/* Determine if already appraised/measured based on bitmask
>> @@ -260,7 +265,9 @@ 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_integrity_policy && policy_violation)) &&
>> +	    (ima_appraise & IMA_APPRAISE_ENFORCE))
>>   		return -EACCES;
>>   	return 0;
>>   }
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

Re: [RFC][PATCH 1/2] ima: preserve the integrity of appraised files

[Mimi Zohar]

On Mon, 2017-10-23 at 15:41 +0200, Roberto Sassu wrote:
> On 10/23/2017 1:46 PM, Mimi Zohar wrote:
> > On Fri, 2017-10-20 at 17:41 +0200, Roberto Sassu wrote:
> >> The existing 'ima_appraise_tcb' policy aims at protecting the integrity
> >> of files owned by root against offline attacks, while LSMs should decide
> >> if those files can be modified, and new files can be created. However,
> >> LSMs cannot take this decision independently, if IMA appraises only
> >> a subset of files that a process is allowed to access. A process can
> >> become compromised due to reading files not appraised by IMA.
> >>
> >> To avoid this issue, the IMA policy should contain as criteria process
> >> credentials rather than files attributes. Then, when a process matches
> >> those criteria, files will be always appraised by IMA, regardless of
> >> file attributes.
> >>
> >> The IMA policy with process credentials will define which process belongs
> >> to the TCB and which not. With this information, IMA would be be able
> >> to preserve the integrity of appraised files, without an LSM, for example
> >> by denying writes by processes outside the TCB (Biba strict policy).
> >>
> >> This patch adds support for enforcing the Biba strict policy. More
> >> policies will be introduced later. Enforcement will be enabled by
> >> adding 'ima_integrity_policy=biba-strict' to the kernel command line.
> > 
> > Way, way back, before the any of the integrity code was upstreamed,
> > the original integrity design had LSMs calling exported integrity
> > functions to verify file integrity (eg. evm_verifyxattr).  A decision
> > was made, at the time, to have a clear delineation between Mandatory
> > Access Control (MAC) and integrity.  There have been recent
> > discussions about LSMs blurring this line and calling
> > evm_verifyxattr() directly.
> 
> If IMA/EVM were used to check the integrity of every file (content +
> labels) before any other LSMs makes security decisions, I would agree
> with you that there are two distinct layers: IMA/EVM at the bottom,
> that protect the system against offline attacks (the association
> between file content and LSM labels); LSMs at the top, protect it
> against runtime attacks (i.e. preserve the integrity of the TCB).
> 
> If instead IMA appraises a subset of the system, e.g. when the default
> appraisal policy (called appraise_tcb) is selected, then LSMs alone
> cannot guarantee anymore the runtime integrity of the system if subjects
> in the LSMs TCB are allowed to read files not verified by IMA (read up
> violation in the Biba strict model, because the integrity of files not
> verified by IMA is low).
> 
> Then, in order to preserve the runtime integrity of the system, IMA must
> complement LSMs and prevent the non-appraised portion of the system
> from interacting with the appraised portion.

Instead of adding MAC to IMA, reverse it.  Have the LSMs call the
integrity subsystem to verify a file's integrity before granting
permissions.

> For example, the recent work by Matthew Garrett (IMA: Support using new
> creds in appraisal policy) goes in this direction. With the policy:
> 
> appraise euid=0 func=CREDS_CHECK
> 
> IMA enforces the Biba strict policy (read up) because it prevents bad
> code, loaded through execve(), from being executed by the TCB (root
> processes).
> 
> As I mentioned in the patch set description, it could be possible to
> enforce a more generic policy, which includes also FILE_CHECK and
> MMAP_CHECK.
> 
> With digital signatures, the enforcement of the write down rule is
> guaranteed because signed files are immutable. This patch set adds
> support for enforcing the write down rule on mutable files.
> 
> Roberto
> 
> 
> > Never was there any thought or discussion of adding MAC to the
> > integrity subsystem.  A Biba implementation doesn't belong in IMA, but
> > should be defined as a separate LSM.  (Years ago we implemented a low-
> > water mark LSM named SLIM, based on LOMAC.)
> > 
> > Mimi
> > 
> >> To enforce this policy, it is necessary to upload to IMA a new policy
> >> which defines the subjects part of the TCB. For example, the rule
> >> 'appraise fowner=0' could be replaced with two rules: 'appraise uid=0'
> >> and 'appraise euid=0'.
> >>
> >> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> >> ---
> >>   Documentation/admin-guide/kernel-parameters.txt |  4 ++
> >>   security/integrity/ima/ima.h                    | 23 ++++++++++
> >>   security/integrity/ima/ima_appraise.c           | 61 +++++++++++++++++++++++++
> >>   security/integrity/ima/ima_main.c               | 25 ++++++----
> >>   4 files changed, 104 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >> index 05496622b4ef..37810c6a3b28 100644
> >> --- a/Documentation/admin-guide/kernel-parameters.txt
> >> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -1532,6 +1532,10 @@
> >>   	                [IMA] Define a custom template format.
> >>   			Format: { "field1|...|fieldN" }
> >>
> >> +	ima_integrity_policy=
> >> +			[IMA] Select an integrity policy to enforce.

The boot command line "ima_policy=" already adds support for loading
different builtin policies.  The different policies can be
concatenated together (eg. ima_policy="tcb | appraise_tcb |
secure_boot").  There's no need for a new mechanism for loading
builtin policies.   

> >> +			Policies: { "biba-strict" }
> >> +
> >>   	ima.ahash_minsize= [IMA] Minimum file size for asynchronous hash usage
> >>   			Format: <min_file_size>
> >>   			Set the minimal file size for using asynchronous hash.
> >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> >> index d52b487ad259..377e1d8c2afb 100644
> >> --- a/security/integrity/ima/ima.h
> >> +++ b/security/integrity/ima/ima.h
> >> @@ -36,6 +36,8 @@ enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN,
> >>   		     IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII };
> >>   enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
> >>
> >> +enum integrity_policies { BIBA_STRICT = 1, INTEGRITY_POLICY__LAST };
> >> +
> >>   /* digest size for IMA, fits SHA1 or MD5 */
> >>   #define IMA_DIGEST_SIZE		SHA1_DIGEST_SIZE
> >>   #define IMA_EVENT_NAME_LEN_MAX	255
> >> @@ -57,6 +59,7 @@ extern int ima_initialized;
> >>   extern int ima_used_chip;
> >>   extern int ima_hash_algo;
> >>   extern int ima_appraise;
> >> +extern int ima_integrity_policy;
> >>
> >>   /* IMA event related data */
> >>   struct ima_event_data {
> >> @@ -247,6 +250,11 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
> >>   				 int xattr_len);
> >>   int ima_read_xattr(struct dentry *dentry,
> >>   		   struct evm_ima_xattr_data **xattr_value);
> >> +bool ima_inode_is_appraised(struct dentry *dentry, struct inode *inode);
> >> +bool ima_appraise_biba_check(struct file *file,
> >> +			     struct integrity_iint_cache *iint,
> >> +			     int must_appraise, char **pathbuf,
> >> +			     const char **pathname, char *namebuf);
> >>
> >>   #else
> >>   static inline int ima_appraise_measurement(enum ima_hooks func,
> >> @@ -289,6 +297,21 @@ static inline int ima_read_xattr(struct dentry *dentry,
> >>   	return 0;
> >>   }
> >>
> >> +static inline bool ima_inode_is_appraised(struct dentry *dentry,
> >> +					  struct inode *inode);
> >> +{
> >> +	return false;
> >> +}
> >> +
> >> +static inline bool ima_appraise_biba_check(struct file *file,
> >> +					   struct integrity_iint_cache *iint,
> >> +					   int must_appraise, char **pathbuf,
> >> +					   const char **pathname,
> >> +					   char *namebuf)
> >> +{
> >> +	return false;
> >> +}
> >> +
> >>   #endif /* CONFIG_IMA_APPRAISE */
> >>
> >>   /* LSM based policy rules require audit */
> >> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> >> index 809ba70fbbbf..c24824ef64c4 100644
> >> --- a/security/integrity/ima/ima_appraise.c
> >> +++ b/security/integrity/ima/ima_appraise.c
> >> @@ -18,6 +18,10 @@
> >>
> >>   #include "ima.h"
> >>
> >> +static char *ima_integrity_policies_str[INTEGRITY_POLICY__LAST] = {
> >> +	[BIBA_STRICT] = "biba-strict",
> >> +};
> >> +
> >>   static int __init default_appraise_setup(char *str)
> >>   {
> >>   #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> >> @@ -33,6 +37,15 @@ static int __init default_appraise_setup(char *str)
> >>
> >>   __setup("ima_appraise=", default_appraise_setup);
> >>
> >> +static int __init integrity_policy_setup(char *str)
> >> +{
> >> +	if (strcmp(str, ima_integrity_policies_str[BIBA_STRICT]) == 0)
> >> +		ima_integrity_policy = BIBA_STRICT;
> >> +
> >> +	return 1;
> >> +}
> >> +__setup("ima_integrity_policy=", integrity_policy_setup);
> >> +
> >>   /*
> >>    * is_ima_appraise_enabled - return appraise status
> >>    *
> >> @@ -189,6 +202,54 @@ int ima_read_xattr(struct dentry *dentry,
> >>   	return ret;
> >>   }
> >>
> >> +bool ima_inode_is_appraised(struct dentry *dentry, struct inode *inode)
> >> +{
> >> +	ssize_t len;
> >> +
> >> +	len =  __vfs_getxattr(dentry, inode, XATTR_NAME_IMA, NULL, 0);
> >> +
> >> +	return (len > 0 && len >= sizeof(struct evm_ima_xattr_data));
> >> +}
> >> +
> >> +/*
> >> + * ima_appraise_biba_check - detect violations of a Biba policy
> >> + *
> >> + * The appraisal policy identifies which subjects belong to the TCB. Files
> >> + * with a valid digital signature or HMAC are also part of the TCB. This
> >> + * function detects attempts to write appraised files by subjects outside
> >> + * the TCB. The Biba strict policy denies this operation.
> >> + *
> >> + * Return: true if current operation violates a Biba policy, false otherwise
> >> + */
> >> +bool ima_appraise_biba_check(struct file *file,
> >> +			     struct integrity_iint_cache *iint,
> >> +			     int must_appraise, char **pathbuf,
> >> +			     const char **pathname, char *namebuf)
> >> +{
> >> +	struct inode *inode = file_inode(file);
> >> +	fmode_t mode = file->f_mode;
> >> +	char *cause = "write_down";
> >> +
> >> +	/* check write up, ima_appraise_measurement() checks read down */
> >> +	if (!must_appraise && (mode & FMODE_WRITE)) {
> >> +		if (IS_IMA(inode)) {
> >> +			if (!iint)
> >> +				iint = integrity_iint_find(inode);
> >> +			if (iint->flags & IMA_APPRAISE)
> >> +				goto out_violation;
> >> +		} else if (ima_inode_is_appraised(file_dentry(file), inode)) {
> >> +			goto out_violation;
> >> +		}
> >> +	}
> >> +	return false;
> >> +out_violation:
> >> +	*pathname = ima_d_path(&file->f_path, pathbuf, namebuf);
> >> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, *pathname,
> >> +			    ima_integrity_policies_str[ima_integrity_policy],
> >> +			    cause, 0, 0);
> >> +	return true;
> >> +}
> >> +
> >>   /*
> >>    * ima_appraise_measurement - appraise file measurement
> >>    *
> >> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> >> index bb7e36e90c79..6e85ea8e2373 100644
> >> --- a/security/integrity/ima/ima_main.c
> >> +++ b/security/integrity/ima/ima_main.c
> >> @@ -31,6 +31,7 @@ int ima_initialized;
> >>
> >>   #ifdef CONFIG_IMA_APPRAISE
> >>   int ima_appraise = IMA_APPRAISE_ENFORCE;
> >> +int ima_integrity_policy;
> >>   #else
> >>   int ima_appraise;
> >>   #endif
> >> @@ -103,7 +104,8 @@ static void ima_rdwr_violation_check(struct file *file,
> >>   	if (!send_tomtou && !send_writers)
> >>   		return;
> >>
> >> -	*pathname = ima_d_path(&file->f_path, pathbuf, filename);
> >> +	if (!*pathname)
> >> +		*pathname = ima_d_path(&file->f_path, pathbuf, filename);
> >>
> >>   	if (send_tomtou)
> >>   		ima_add_violation(file, *pathname, iint,
> >> @@ -168,7 +170,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> >>   	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> >>   	struct evm_ima_xattr_data *xattr_value = NULL;
> >>   	int xattr_len = 0;
> >> -	bool violation_check;
> >> +	bool violation_check, policy_violation = false;
> >>   	enum hash_algo hash_algo;
> >>
> >>   	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> >> @@ -181,7 +183,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> >>   	action = ima_get_action(inode, mask, func, &pcr);
> >>   	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
> >>   			   (ima_policy_flag & IMA_MEASURE));
> >> -	if (!action && !violation_check)
> >> +	if (!action && !violation_check && !ima_integrity_policy)
> >>   		return 0;
> >>
> >>   	must_appraise = action & IMA_APPRAISE;
> >> @@ -198,13 +200,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> >>   			goto out;
> >>   	}
> >>
> >> -	if (violation_check) {
> >> +	if (ima_integrity_policy)
> >> +		policy_violation = ima_appraise_biba_check(file, iint,
> >> +						must_appraise, &pathbuf,
> >> +						&pathname, filename);
> >> +	if (violation_check)
> >>   		ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
> >>   					 &pathbuf, &pathname);
> >> -		if (!action) {
> >> -			rc = 0;
> >> -			goto out_free;
> >> -		}
> >> +	if (!action) {
> >> +		rc = 0;
> >> +		goto out_free;
> >>   	}
> >>
> >>   	/* Determine if already appraised/measured based on bitmask
> >> @@ -260,7 +265,9 @@ 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_integrity_policy && policy_violation)) &&
> >> +	    (ima_appraise & IMA_APPRAISE_ENFORCE))
> >>   		return -EACCES;
> >>   	return 0;
> >>   }
> > 
> 

Re: [RFC][PATCH 1/2] ima: preserve the integrity of appraised files

[Roberto Sassu]

On 10/23/2017 10:30 PM, Mimi Zohar wrote:
> On Mon, 2017-10-23 at 15:41 +0200, Roberto Sassu wrote:
>> On 10/23/2017 1:46 PM, Mimi Zohar wrote:
>>> On Fri, 2017-10-20 at 17:41 +0200, Roberto Sassu wrote:
>>>> The existing 'ima_appraise_tcb' policy aims at protecting the integrity
>>>> of files owned by root against offline attacks, while LSMs should decide
>>>> if those files can be modified, and new files can be created. However,
>>>> LSMs cannot take this decision independently, if IMA appraises only
>>>> a subset of files that a process is allowed to access. A process can
>>>> become compromised due to reading files not appraised by IMA.
>>>>
>>>> To avoid this issue, the IMA policy should contain as criteria process
>>>> credentials rather than files attributes. Then, when a process matches
>>>> those criteria, files will be always appraised by IMA, regardless of
>>>> file attributes.
>>>>
>>>> The IMA policy with process credentials will define which process belongs
>>>> to the TCB and which not. With this information, IMA would be be able
>>>> to preserve the integrity of appraised files, without an LSM, for example
>>>> by denying writes by processes outside the TCB (Biba strict policy).
>>>>
>>>> This patch adds support for enforcing the Biba strict policy. More
>>>> policies will be introduced later. Enforcement will be enabled by
>>>> adding 'ima_integrity_policy=biba-strict' to the kernel command line.
>>>
>>> Way, way back, before the any of the integrity code was upstreamed,
>>> the original integrity design had LSMs calling exported integrity
>>> functions to verify file integrity (eg. evm_verifyxattr).  A decision
>>> was made, at the time, to have a clear delineation between Mandatory
>>> Access Control (MAC) and integrity.  There have been recent
>>> discussions about LSMs blurring this line and calling
>>> evm_verifyxattr() directly.
>>
>> If IMA/EVM were used to check the integrity of every file (content +
>> labels) before any other LSMs makes security decisions, I would agree
>> with you that there are two distinct layers: IMA/EVM at the bottom,
>> that protect the system against offline attacks (the association
>> between file content and LSM labels); LSMs at the top, protect it
>> against runtime attacks (i.e. preserve the integrity of the TCB).
>>
>> If instead IMA appraises a subset of the system, e.g. when the default
>> appraisal policy (called appraise_tcb) is selected, then LSMs alone
>> cannot guarantee anymore the runtime integrity of the system if subjects
>> in the LSMs TCB are allowed to read files not verified by IMA (read up
>> violation in the Biba strict model, because the integrity of files not
>> verified by IMA is low).
>>
>> Then, in order to preserve the runtime integrity of the system, IMA must
>> complement LSMs and prevent the non-appraised portion of the system
>> from interacting with the appraised portion.
> 
> Instead of adding MAC to IMA, reverse it.  Have the LSMs call the
> integrity subsystem to verify a file's integrity before granting
> permissions.

If not all files are appraised, but only those that belong to the TCB,
LSMs should take responsibility of denying access to a file with missing
or invalid HMAC. Suppose that an LSM is enforcing the Biba strict policy
and a process outside the TCB is accessing that file. From the LSM point
of view this operation should be allowed, because it does not violate
Biba rules. If IMA appraisal denies access, it is acting as a MAC.


>> For example, the recent work by Matthew Garrett (IMA: Support using new
>> creds in appraisal policy) goes in this direction. With the policy:
>>
>> appraise euid=0 func=CREDS_CHECK
>>
>> IMA enforces the Biba strict policy (read up) because it prevents bad
>> code, loaded through execve(), from being executed by the TCB (root
>> processes).
>>
>> As I mentioned in the patch set description, it could be possible to
>> enforce a more generic policy, which includes also FILE_CHECK and
>> MMAP_CHECK.
>>
>> With digital signatures, the enforcement of the write down rule is
>> guaranteed because signed files are immutable. This patch set adds
>> support for enforcing the write down rule on mutable files.
>>
>> Roberto
>>
>>
>>> Never was there any thought or discussion of adding MAC to the
>>> integrity subsystem.  A Biba implementation doesn't belong in IMA, but
>>> should be defined as a separate LSM.  (Years ago we implemented a low-
>>> water mark LSM named SLIM, based on LOMAC.)
>>>
>>> Mimi
>>>
>>>> To enforce this policy, it is necessary to upload to IMA a new policy
>>>> which defines the subjects part of the TCB. For example, the rule
>>>> 'appraise fowner=0' could be replaced with two rules: 'appraise uid=0'
>>>> and 'appraise euid=0'.
>>>>
>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>> ---
>>>>    Documentation/admin-guide/kernel-parameters.txt |  4 ++
>>>>    security/integrity/ima/ima.h                    | 23 ++++++++++
>>>>    security/integrity/ima/ima_appraise.c           | 61 +++++++++++++++++++++++++
>>>>    security/integrity/ima/ima_main.c               | 25 ++++++----
>>>>    4 files changed, 104 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>>> index 05496622b4ef..37810c6a3b28 100644
>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>>> @@ -1532,6 +1532,10 @@
>>>>    	                [IMA] Define a custom template format.
>>>>    			Format: { "field1|...|fieldN" }
>>>>
>>>> +	ima_integrity_policy=
>>>> +			[IMA] Select an integrity policy to enforce.
> 
> The boot command line "ima_policy=" already adds support for loading
> different builtin policies.  The different policies can be
> concatenated together (eg. ima_policy="tcb | appraise_tcb |
> secure_boot").  There's no need for a new mechanism for loading
> builtin policies.

These parameters have different meanings: ima_policy= defines the TCB,
ima_integrity_policy= defines which policy is enforced on the TCB.

Roberto


>>>> +			Policies: { "biba-strict" }
>>>> +
>>>>    	ima.ahash_minsize= [IMA] Minimum file size for asynchronous hash usage
>>>>    			Format: <min_file_size>
>>>>    			Set the minimal file size for using asynchronous hash.
>>>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>>>> index d52b487ad259..377e1d8c2afb 100644
>>>> --- a/security/integrity/ima/ima.h
>>>> +++ b/security/integrity/ima/ima.h
>>>> @@ -36,6 +36,8 @@ enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN,
>>>>    		     IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII };
>>>>    enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
>>>>
>>>> +enum integrity_policies { BIBA_STRICT = 1, INTEGRITY_POLICY__LAST };
>>>> +
>>>>    /* digest size for IMA, fits SHA1 or MD5 */
>>>>    #define IMA_DIGEST_SIZE		SHA1_DIGEST_SIZE
>>>>    #define IMA_EVENT_NAME_LEN_MAX	255
>>>> @@ -57,6 +59,7 @@ extern int ima_initialized;
>>>>    extern int ima_used_chip;
>>>>    extern int ima_hash_algo;
>>>>    extern int ima_appraise;
>>>> +extern int ima_integrity_policy;
>>>>
>>>>    /* IMA event related data */
>>>>    struct ima_event_data {
>>>> @@ -247,6 +250,11 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
>>>>    				 int xattr_len);
>>>>    int ima_read_xattr(struct dentry *dentry,
>>>>    		   struct evm_ima_xattr_data **xattr_value);
>>>> +bool ima_inode_is_appraised(struct dentry *dentry, struct inode *inode);
>>>> +bool ima_appraise_biba_check(struct file *file,
>>>> +			     struct integrity_iint_cache *iint,
>>>> +			     int must_appraise, char **pathbuf,
>>>> +			     const char **pathname, char *namebuf);
>>>>
>>>>    #else
>>>>    static inline int ima_appraise_measurement(enum ima_hooks func,
>>>> @@ -289,6 +297,21 @@ static inline int ima_read_xattr(struct dentry *dentry,
>>>>    	return 0;
>>>>    }
>>>>
>>>> +static inline bool ima_inode_is_appraised(struct dentry *dentry,
>>>> +					  struct inode *inode);
>>>> +{
>>>> +	return false;
>>>> +}
>>>> +
>>>> +static inline bool ima_appraise_biba_check(struct file *file,
>>>> +					   struct integrity_iint_cache *iint,
>>>> +					   int must_appraise, char **pathbuf,
>>>> +					   const char **pathname,
>>>> +					   char *namebuf)
>>>> +{
>>>> +	return false;
>>>> +}
>>>> +
>>>>    #endif /* CONFIG_IMA_APPRAISE */
>>>>
>>>>    /* LSM based policy rules require audit */
>>>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>>>> index 809ba70fbbbf..c24824ef64c4 100644
>>>> --- a/security/integrity/ima/ima_appraise.c
>>>> +++ b/security/integrity/ima/ima_appraise.c
>>>> @@ -18,6 +18,10 @@
>>>>
>>>>    #include "ima.h"
>>>>
>>>> +static char *ima_integrity_policies_str[INTEGRITY_POLICY__LAST] = {
>>>> +	[BIBA_STRICT] = "biba-strict",
>>>> +};
>>>> +
>>>>    static int __init default_appraise_setup(char *str)
>>>>    {
>>>>    #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
>>>> @@ -33,6 +37,15 @@ static int __init default_appraise_setup(char *str)
>>>>
>>>>    __setup("ima_appraise=", default_appraise_setup);
>>>>
>>>> +static int __init integrity_policy_setup(char *str)
>>>> +{
>>>> +	if (strcmp(str, ima_integrity_policies_str[BIBA_STRICT]) == 0)
>>>> +		ima_integrity_policy = BIBA_STRICT;
>>>> +
>>>> +	return 1;
>>>> +}
>>>> +__setup("ima_integrity_policy=", integrity_policy_setup);
>>>> +
>>>>    /*
>>>>     * is_ima_appraise_enabled - return appraise status
>>>>     *
>>>> @@ -189,6 +202,54 @@ int ima_read_xattr(struct dentry *dentry,
>>>>    	return ret;
>>>>    }
>>>>
>>>> +bool ima_inode_is_appraised(struct dentry *dentry, struct inode *inode)
>>>> +{
>>>> +	ssize_t len;
>>>> +
>>>> +	len =  __vfs_getxattr(dentry, inode, XATTR_NAME_IMA, NULL, 0);
>>>> +
>>>> +	return (len > 0 && len >= sizeof(struct evm_ima_xattr_data));
>>>> +}
>>>> +
>>>> +/*
>>>> + * ima_appraise_biba_check - detect violations of a Biba policy
>>>> + *
>>>> + * The appraisal policy identifies which subjects belong to the TCB. Files
>>>> + * with a valid digital signature or HMAC are also part of the TCB. This
>>>> + * function detects attempts to write appraised files by subjects outside
>>>> + * the TCB. The Biba strict policy denies this operation.
>>>> + *
>>>> + * Return: true if current operation violates a Biba policy, false otherwise
>>>> + */
>>>> +bool ima_appraise_biba_check(struct file *file,
>>>> +			     struct integrity_iint_cache *iint,
>>>> +			     int must_appraise, char **pathbuf,
>>>> +			     const char **pathname, char *namebuf)
>>>> +{
>>>> +	struct inode *inode = file_inode(file);
>>>> +	fmode_t mode = file->f_mode;
>>>> +	char *cause = "write_down";
>>>> +
>>>> +	/* check write up, ima_appraise_measurement() checks read down */
>>>> +	if (!must_appraise && (mode & FMODE_WRITE)) {
>>>> +		if (IS_IMA(inode)) {
>>>> +			if (!iint)
>>>> +				iint = integrity_iint_find(inode);
>>>> +			if (iint->flags & IMA_APPRAISE)
>>>> +				goto out_violation;
>>>> +		} else if (ima_inode_is_appraised(file_dentry(file), inode)) {
>>>> +			goto out_violation;
>>>> +		}
>>>> +	}
>>>> +	return false;
>>>> +out_violation:
>>>> +	*pathname = ima_d_path(&file->f_path, pathbuf, namebuf);
>>>> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, *pathname,
>>>> +			    ima_integrity_policies_str[ima_integrity_policy],
>>>> +			    cause, 0, 0);
>>>> +	return true;
>>>> +}
>>>> +
>>>>    /*
>>>>     * ima_appraise_measurement - appraise file measurement
>>>>     *
>>>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>>>> index bb7e36e90c79..6e85ea8e2373 100644
>>>> --- a/security/integrity/ima/ima_main.c
>>>> +++ b/security/integrity/ima/ima_main.c
>>>> @@ -31,6 +31,7 @@ int ima_initialized;
>>>>
>>>>    #ifdef CONFIG_IMA_APPRAISE
>>>>    int ima_appraise = IMA_APPRAISE_ENFORCE;
>>>> +int ima_integrity_policy;
>>>>    #else
>>>>    int ima_appraise;
>>>>    #endif
>>>> @@ -103,7 +104,8 @@ static void ima_rdwr_violation_check(struct file *file,
>>>>    	if (!send_tomtou && !send_writers)
>>>>    		return;
>>>>
>>>> -	*pathname = ima_d_path(&file->f_path, pathbuf, filename);
>>>> +	if (!*pathname)
>>>> +		*pathname = ima_d_path(&file->f_path, pathbuf, filename);
>>>>
>>>>    	if (send_tomtou)
>>>>    		ima_add_violation(file, *pathname, iint,
>>>> @@ -168,7 +170,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>>>>    	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
>>>>    	struct evm_ima_xattr_data *xattr_value = NULL;
>>>>    	int xattr_len = 0;
>>>> -	bool violation_check;
>>>> +	bool violation_check, policy_violation = false;
>>>>    	enum hash_algo hash_algo;
>>>>
>>>>    	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
>>>> @@ -181,7 +183,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>>>>    	action = ima_get_action(inode, mask, func, &pcr);
>>>>    	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
>>>>    			   (ima_policy_flag & IMA_MEASURE));
>>>> -	if (!action && !violation_check)
>>>> +	if (!action && !violation_check && !ima_integrity_policy)
>>>>    		return 0;
>>>>
>>>>    	must_appraise = action & IMA_APPRAISE;
>>>> @@ -198,13 +200,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>>>>    			goto out;
>>>>    	}
>>>>
>>>> -	if (violation_check) {
>>>> +	if (ima_integrity_policy)
>>>> +		policy_violation = ima_appraise_biba_check(file, iint,
>>>> +						must_appraise, &pathbuf,
>>>> +						&pathname, filename);
>>>> +	if (violation_check)
>>>>    		ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
>>>>    					 &pathbuf, &pathname);
>>>> -		if (!action) {
>>>> -			rc = 0;
>>>> -			goto out_free;
>>>> -		}
>>>> +	if (!action) {
>>>> +		rc = 0;
>>>> +		goto out_free;
>>>>    	}
>>>>
>>>>    	/* Determine if already appraised/measured based on bitmask
>>>> @@ -260,7 +265,9 @@ 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_integrity_policy && policy_violation)) &&
>>>> +	    (ima_appraise & IMA_APPRAISE_ENFORCE))
>>>>    		return -EACCES;
>>>>    	return 0;
>>>>    }
>>>
>>
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

[RFC][PATCH 2/2] ima: don't measure files in the TCB if Biba strict policy is enforced

[Roberto Sassu]

The Biba strict policy prevents processes outside the TCB from modifying
appraised files. Then, since the integrity of those files is preserved,
because only processes in the TCB can write appraised files, it is not
necessary to measure them each time they are accessed by the TCB.

This solves one of the main problems of binary attestation: when a
modified file is accessed by the TCB, it was necessary to measure it
because verifiers cannot determine from the measurement list if the
writer belong or not to the TCB. Verifiers find an unknown digest
and have to consider the whole system as compromised.

If the Biba strict policy has been selected, and appraisal is in enforce
mode, IMA measures files at first access, if they have a digital signature.
Then, for subsequent accesses, files are not measured again, unless the
appraisal status changes.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_main.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6e85ea8e2373..16c2da0e32d9 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -200,10 +200,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 			goto out;
 	}
 
-	if (ima_integrity_policy)
+	if (ima_integrity_policy) {
 		policy_violation = ima_appraise_biba_check(file, iint,
 						must_appraise, &pathbuf,
 						&pathname, filename);
+		/* do not measure mutable files, if they are appraised */
+		if (ima_integrity_policy == BIBA_STRICT &&
+		    (ima_appraise & IMA_APPRAISE_ENFORCE))
+			if (iint && (iint->flags & IMA_APPRAISED))
+				action &= ~IMA_MEASURE;
+	}
 	if (violation_check)
 		ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
 					 &pathbuf, &pathname);
@@ -246,9 +252,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
 		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
 
-	if (rc == 0 && (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 (!rc && ima_integrity_policy == BIBA_STRICT &&
+		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+			iint->flags &= ~IMA_MEASURE;
+			if (!(iint->flags & IMA_DIGSIG))
+				action &= ~IMA_MEASURE;
+		}
+	}
 	if (action & IMA_MEASURE)
 		ima_store_measurement(iint, file, pathname,
 				      xattr_value, xattr_len, pcr);
-- 
2.11.0

Re: [RFC][PATCH 2/2] ima: don't measure files in the TCB if Biba strict policy is enforced

[Mimi Zohar]

On Fri, 2017-10-20 at 17:41 +0200, Roberto Sassu wrote:
> The Biba strict policy prevents processes outside the TCB from modifying
> appraised files. Then, since the integrity of those files is preserved,
> because only processes in the TCB can write appraised files, it is not
> necessary to measure them each time they are accessed by the TCB.

The builtin appraise_tcb appraises all files owned by root.  With this
patch you've redefined TCB to be any currently loaded IMA policy.

> This solves one of the main problems of binary attestation: when a
> modified file is accessed by the TCB, it was necessary to measure it
> because verifiers cannot determine from the measurement list if the
> writer belong or not to the TCB. Verifiers find an unknown digest
> and have to consider the whole system as compromised.
> 
> If the Biba strict policy has been selected, and appraisal is in enforce
> mode, IMA measures files at first access, if they have a digital signature.
> Then, for subsequent accesses, files are not measured again, unless the
> appraisal status changes.

Signed files aren't changing, so there should only be one file
measurement in the measurement list.  So this only affects mutable
files.  We're going through a lot of effort to re-measure mutable
files after they change.

> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/ima/ima_main.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 6e85ea8e2373..16c2da0e32d9 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -200,10 +200,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>  			goto out;
>  	}
> 
> -	if (ima_integrity_policy)
> +	if (ima_integrity_policy) {
>  		policy_violation = ima_appraise_biba_check(file, iint,
>  						must_appraise, &pathbuf,
>  						&pathname, filename);
> +		/* do not measure mutable files, if they are appraised */
> +		if (ima_integrity_policy == BIBA_STRICT &&
> +		    (ima_appraise & IMA_APPRAISE_ENFORCE))
> +			if (iint && (iint->flags & IMA_APPRAISED))
> +				action &= ~IMA_MEASURE;
> +	}
>  	if (violation_check)
>  		ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
>  					 &pathbuf, &pathname);
> @@ -246,9 +252,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>  	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
>  		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
> 
> -	if (rc == 0 && (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 (!rc && ima_integrity_policy == BIBA_STRICT &&
> +		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> +			iint->flags &= ~IMA_MEASURE;
> +			if (!(iint->flags & IMA_DIGSIG))
> +				action &= ~IMA_MEASURE;
> +		}
> +	}
>  	if (action & IMA_MEASURE)
>  		ima_store_measurement(iint, file, pathname,
>  				      xattr_value, xattr_len, pcr);

Re: [RFC][PATCH 2/2] ima: don't measure files in the TCB if Biba strict policy is enforced

[Roberto Sassu]

On 10/23/2017 10:40 PM, Mimi Zohar wrote:
> On Fri, 2017-10-20 at 17:41 +0200, Roberto Sassu wrote:
>> The Biba strict policy prevents processes outside the TCB from modifying
>> appraised files. Then, since the integrity of those files is preserved,
>> because only processes in the TCB can write appraised files, it is not
>> necessary to measure them each time they are accessed by the TCB.
> 
> The builtin appraise_tcb appraises all files owned by root.  With this
> patch you've redefined TCB to be any currently loaded IMA policy.

Root processes are part of the TCB. Since a policy can be uploaded only
by a root process, it will be always appraised. Digital signature could
be required specifically for the POLICY_CHECK hook.


>> This solves one of the main problems of binary attestation: when a
>> modified file is accessed by the TCB, it was necessary to measure it
>> because verifiers cannot determine from the measurement list if the
>> writer belong or not to the TCB. Verifiers find an unknown digest
>> and have to consider the whole system as compromised.
>>
>> If the Biba strict policy has been selected, and appraisal is in enforce
>> mode, IMA measures files at first access, if they have a digital signature.
>> Then, for subsequent accesses, files are not measured again, unless the
>> appraisal status changes.
> 
> Signed files aren't changing, so there should only be one file
> measurement in the measurement list.  So this only affects mutable
> files.  We're going through a lot of effort to re-measure mutable
> files after they change.

If appraised files can be written only by processes in the TCB, it is
not necessary to report the new digest, after their content changes,
because their integrity is preserved. It is sufficient to correctly
update the extended attribute.

Roberto


>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   security/integrity/ima/ima_main.c | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 6e85ea8e2373..16c2da0e32d9 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -200,10 +200,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>>   			goto out;
>>   	}
>>
>> -	if (ima_integrity_policy)
>> +	if (ima_integrity_policy) {
>>   		policy_violation = ima_appraise_biba_check(file, iint,
>>   						must_appraise, &pathbuf,
>>   						&pathname, filename);
>> +		/* do not measure mutable files, if they are appraised */
>> +		if (ima_integrity_policy == BIBA_STRICT &&
>> +		    (ima_appraise & IMA_APPRAISE_ENFORCE))
>> +			if (iint && (iint->flags & IMA_APPRAISED))
>> +				action &= ~IMA_MEASURE;
>> +	}
>>   	if (violation_check)
>>   		ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
>>   					 &pathbuf, &pathname);
>> @@ -246,9 +252,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>>   	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
>>   		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
>>
>> -	if (rc == 0 && (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 (!rc && ima_integrity_policy == BIBA_STRICT &&
>> +		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
>> +			iint->flags &= ~IMA_MEASURE;
>> +			if (!(iint->flags & IMA_DIGSIG))
>> +				action &= ~IMA_MEASURE;
>> +		}
>> +	}
>>   	if (action & IMA_MEASURE)
>>   		ima_store_measurement(iint, file, pathname,
>>   				      xattr_value, xattr_len, pcr);
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

Re: [RFC][PATCH 0/2] ima: preserve integrity of dynamic data

[Mimi Zohar]

On Fri, 2017-10-20 at 17:41 +0200, Roberto Sassu wrote:
> One of the most challenging tasks for remote attestation is how to
> handle the measurement of dynamic data (mutable files). When the default
> policy is selected, IMA measures files accessed by root processes (TCB).

Agreed.  The original ima_policy="appraise_tcb" (formerly known as
"ima_appraise_tcb") is a builtin policy, which can be and normally is
replaced by a custom policy.  This builtin IMA policy starts
immediately and is normally replaced after the LSMs have been
initialized, allowing for a finer grain policy.

> However, if a file was previously modified by another process, the digest
> included in the new measurement list is unknown, and verifiers must assume,
> during a remote attestation, that the system was compromised, because they
> don't know if that file was written by a process in the TCB or not.
> 
> The goal of this patch set is to enforce an integrity policy on appraised
> files, to avoid reporting measurements of dynamic data after they have been
> modified. Only the initial state should be reported (e.g. the file
> signature, or a digest list).
> 
> In order to properly enforce an integrity policy, it is necessary to
> specify in the IMA policy process credentials rather than file attributes.
> 
> For example, the rule:
> 
> appraise fowner=0
> 
> could be replaced with:
> 
> appraise uid=0
> appraise euid=0

The difference between these two policies is the ability to know which
files must be hashed/signed apriori.  In the first case, only those
files owned by root must be hashed/signed, while in the latter case
all files on the file system must be hashed/signed.

When making changes, please keep in mind the bigger picture of who is
doing what and how the integrity subsystem is currently being used.
 Adding new functionality or extending the existing subsystem is all
good, but these changes cannot break existing systems or change its
meaning.

Mimi