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