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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> >> ---
> >>   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;
> >>   }
> > 
> 




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux