[apparmor] [PATCH 2/3] apparmor: convert attaching profiles via xattrs to use, dfa matching

Seth Arnold seth.arnold at canonical.com
Thu Feb 8 22:11:56 UTC 2018


On Thu, Feb 08, 2018 at 12:38:57PM -0800, John Johansen wrote:
> This converts profile attachment based on xattrs to a fixed extended
> conditional using dfa matching.
> 
> This has a couple of advantages
> - pattern matching can be used for the xattr match
> 
> - xattrs can be optional for an attachment or marked as required
> 
> - the xattr attachment conditional will be able to be combined with
>   other extended conditionals when the flexible extended conditional
>   work lands.
> 
> The xattr fixed extended conditional is appended to the xmatch
> conditional. If an xattr attachment is specified the profile xmatch
> will be generated regardless of whether there is a pattern match on
> the executable name.
> 
> Signed-off-by: John Johansen <john.johansen at canonical.com>

Acked-by: Seth Arnold <seth.arnold at canonical.com>

Thanks

> ---
>  security/apparmor/apparmorfs.c     |  5 ++++
>  security/apparmor/domain.c         | 52 ++++++++++++++++++++++++++------------
>  security/apparmor/include/policy.h |  2 --
>  security/apparmor/policy.c         |  6 +----
>  security/apparmor/policy_unpack.c  | 32 -----------------------
>  5 files changed, 42 insertions(+), 55 deletions(-)
> 
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 1e63ff2e5b85..35e6b240fb14 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -2147,6 +2147,10 @@ static struct aa_sfs_entry aa_sfs_entry_signal[] = {
>  	{ }
>  };
>  
> +static struct aa_sfs_entry aa_sfs_entry_attach[] = {
> +	AA_SFS_FILE_BOOLEAN("xattr", 1),
> +	{ }
> +};
>  static struct aa_sfs_entry aa_sfs_entry_domain[] = {
>  	AA_SFS_FILE_BOOLEAN("change_hat",	1),
>  	AA_SFS_FILE_BOOLEAN("change_hatv",	1),
> @@ -2155,6 +2159,7 @@ static struct aa_sfs_entry aa_sfs_entry_domain[] = {
>  	AA_SFS_FILE_BOOLEAN("stack",		1),
>  	AA_SFS_FILE_BOOLEAN("fix_binfmt_elf_mmap",	1),
>  	AA_SFS_FILE_BOOLEAN("post_nnp_subset",	1),
> +	AA_SFS_DIR("attach_conditions",		aa_sfs_entry_attach),
>  	AA_SFS_FILE_STRING("version", "1.2"),
>  	{ }
>  };
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index ca8353f3e7a0..9651e18cbae5 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -306,11 +306,12 @@ static int change_profile_perms(struct aa_profile *profile,
>   * aa_xattrs_match - check whether a file matches the xattrs defined in profile
>   * @bprm: binprm struct for the process to validate
>   * @profile: profile to match against (NOT NULL)
> + * @state: state to start match in
>   *
>   * Returns: number of extended attributes that matched, or < 0 on error
>   */
>  static int aa_xattrs_match(const struct linux_binprm *bprm,
> -			   struct aa_profile *profile)
> +			   struct aa_profile *profile, unsigned int state)
>  {
>  	int i;
>  	size_t size;
> @@ -321,27 +322,40 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
>  	if (!bprm || !profile->xattr_count)
>  		return 0;
>  
> +	/* transition from exec match to xattr set */
> +	state = aa_dfa_null_transition(profile->xmatch, state);
> +
>  	d = bprm->file->f_path.dentry;
>  
>  	for (i = 0; i < profile->xattr_count; i++) {
>  		size = vfs_getxattr_alloc(d, profile->xattrs[i], &value,
>  					  value_size, GFP_KERNEL);
> -		if (size < 0) {
> -			ret = -EINVAL;
> -			goto out;
> -		}
> +		if (size >= 0) {
> +			u32 perm;
>  
> -		/* Check the xattr value, not just presence */
> -		if (profile->xattr_lens[i]) {
> -			if (profile->xattr_lens[i] != size) {
> +			/* Check the xattr value, not just presence */
> +			state = aa_dfa_match_len(profile->xmatch, state, value,
> +						 size);
> +			perm = dfa_user_allow(profile->xmatch, state);
> +			if (!(perm & MAY_EXEC)) {
>  				ret = -EINVAL;
>  				goto out;
>  			}
> -
> -			if (memcmp(value, profile->xattr_values[i], size)) {
> +		}
> +		/* transition to next element */
> +		state = aa_dfa_null_transition(profile->xmatch, state);
> +		if (size < 0) {
> +			/*
> +			 * No xattr match, so verify if transition to
> +			 * next element was valid. IFF so the xattr
> +			 * was optional.
> +			 */
> +			if (!state) {
>  				ret = -EINVAL;
>  				goto out;
>  			}
> +			/* don't count missing optional xattr as matched */
> +			ret--;
>  		}
>  	}
>  
> @@ -402,13 +416,16 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
>  			perm = dfa_user_allow(profile->xmatch, state);
>  			/* any accepting state means a valid match. */
>  			if (perm & MAY_EXEC) {
> -				int ret = aa_xattrs_match(bprm, profile);
> +				int ret = aa_xattrs_match(bprm, profile, state);
>  
>  				/* Fail matching if the xattrs don't match */
>  				if (ret < 0)
>  					continue;
>  
> -				/* The new match isn't more specific
> +				/*
> +				 * TODO: allow for more flexible best match
> +				 *
> +				 * The new match isn't more specific
>  				 * than the current best match
>  				 */
>  				if (profile->xmatch_len == len &&
> @@ -427,9 +444,11 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
>  				xattrs = ret;
>  				conflict = false;
>  			}
> -		} else if (!strcmp(profile->base.name, name) &&
> -			   aa_xattrs_match(bprm, profile) >= 0)
> -			/* exact non-re match, no more searching required */
> +		} else if (!strcmp(profile->base.name, name))
> +			/*
> +			 * old exact non-re match, without conditionals such
> +			 * as xattrs. no more searching required
> +			 */
>  			return profile;
>  	}
>  
> @@ -651,7 +670,8 @@ static struct aa_label *profile_transition(struct aa_profile *profile,
>  			 * met, and fail execution otherwise
>  			 */
>  			label_for_each(i, new, component) {
> -				if (aa_xattrs_match(bprm, component) < 0) {
> +				if (aa_xattrs_match(bprm, component, state) <
> +				    0) {
>  					error = -EACCES;
>  					info = "required xattrs not present";
>  					perms.allow &= ~MAY_EXEC;
> diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
> index 9eab920b68eb..15f2404e70ba 100644
> --- a/security/apparmor/include/policy.h
> +++ b/security/apparmor/include/policy.h
> @@ -153,8 +153,6 @@ struct aa_profile {
>  
>  	int xattr_count;
>  	char **xattrs;
> -	size_t *xattr_lens;
> -	char **xattr_values;
>  
>  	struct aa_rlimit rlimits;
>  
> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
> index 437cc917a5eb..69af9c6a6089 100644
> --- a/security/apparmor/policy.c
> +++ b/security/apparmor/policy.c
> @@ -229,13 +229,9 @@ void aa_free_profile(struct aa_profile *profile)
>  	aa_free_cap_rules(&profile->caps);
>  	aa_free_rlimit_rules(&profile->rlimits);
>  
> -	for (i=0; i<profile->xattr_count; i++) {
> +	for (i = 0; i < profile->xattr_count; i++)
>  		kzfree(profile->xattrs[i]);
> -		kzfree(profile->xattr_values[i]);
> -	}
>  	kzfree(profile->xattrs);
> -	kzfree(profile->xattr_lens);
> -	kzfree(profile->xattr_values);
>  	kzfree(profile->dirname);
>  	aa_put_dfa(profile->xmatch);
>  	aa_put_dfa(profile->policy.dfa);
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index 81c52cd26126..7d4f79884846 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -556,38 +556,6 @@ static bool unpack_xattrs(struct aa_ext *e, struct aa_profile *profile)
>  			goto fail;
>  	}
>  
> -	if (unpack_nameX(e, AA_STRUCT, "xattr_values")) {
> -		int i, size;
> -
> -		size = unpack_array(e, NULL);
> -
> -		/* Must be the same number of xattr values as xattrs */
> -		if (size != profile->xattr_count)
> -			goto fail;
> -
> -		profile->xattr_lens = kmalloc_array(size, sizeof(size_t),
> -						    GFP_KERNEL);
> -		if (!profile->xattr_lens)
> -			goto fail;
> -
> -		profile->xattr_values = kmalloc_array(size, sizeof(char *),
> -						      GFP_KERNEL);
> -		if (!profile->xattr_values)
> -			goto fail;
> -
> -		for (i = 0; i < size; i++) {
> -			profile->xattr_lens[i] = unpack_blob(e,
> -					      &profile->xattr_values[i], NULL);
> -			profile->xattr_values[i] =
> -				kvmemdup(profile->xattr_values[i],
> -					 profile->xattr_lens[i]);
> -		}
> -
> -		if (!unpack_nameX(e, AA_ARRAYEND, NULL))
> -			goto fail;
> -		if (!unpack_nameX(e, AA_STRUCTEND, NULL))
> -			goto fail;
> -	}
>  	return 1;
>  
>  fail:
> -- 
> 2.14.1
> 
> 
> -- 
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20180208/fe10d185/attachment.sig>


More information about the AppArmor mailing list