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.johan...@canonical.com>
Acked-by: Seth Arnold <seth.arn...@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@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
signature.asc
Description: PGP signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
