3.16.54-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: John Johansen <john.johansen@xxxxxxxxxxxxx>
commit 844b8292b6311ecd30ae63db1471edb26e01d895 upstream.
Profiles that have an undecidable overlap in their attachments are
being incorrectly handled. Instead of failing to attach the first one
encountered is being used.
eg.
profile A /** { .. }
profile B /*foo { .. }
have an unresolvable longest left attachment, they both have an exact
match on / and then have an overlapping expression that has no clear
winner.
Currently the winner will be the profile that is loaded first which
can result in non-deterministic behavior. Instead in this situation
the exec should fail.
Fixes: 898127c34ec0 ("AppArmor: functions for domain transitions")
Signed-off-by: John Johansen <john.johansen@xxxxxxxxxxxxx>
[bwh: Backported to 3.16:
- Add 'info' parameter to x_to_profile(), done upstream in commit
93c98a484c49 "apparmor: move exec domain mediation to using labels"
- Adjust context]
Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
---
security/apparmor/domain.c | 46 ++++++++++++++++++++++++++++++++--------------
1 file changed, 32 insertions(+), 14 deletions(-)
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -126,6 +126,7 @@ static struct file_perms change_profile_
* __attach_match_ - find an attachment match
* @name - to match against (NOT NULL)
* @head - profile list to walk (NOT NULL)
+ * @info - info message if there was an error (NOT NULL)
*
* Do a linear search on the profiles in the list. There is a matching
* preference where an exact match is preferred over a name which uses
@@ -137,28 +138,44 @@ static struct file_perms change_profile_
* Returns: profile or NULL if no match found
*/
static struct aa_profile *__attach_match(const char *name,
- struct list_head *head)
+ struct list_head *head,
+ const char **info)
{
int len = 0;
+ bool conflict = false;
struct aa_profile *profile, *candidate = NULL;
list_for_each_entry_rcu(profile, head, base.list) {
if (profile->flags & PFLAG_NULL)
continue;
- if (profile->xmatch && profile->xmatch_len > len) {
- unsigned int state = aa_dfa_match(profile->xmatch,
- DFA_START, name);
- u32 perm = dfa_user_allow(profile->xmatch, state);
- /* any accepting state means a valid match. */
- if (perm & MAY_EXEC) {
- candidate = profile;
- len = profile->xmatch_len;
+ if (profile->xmatch) {
+ if (profile->xmatch_len == len) {
+ conflict = true;
+ continue;
+ } else if (profile->xmatch_len > len) {
+ unsigned int state;
+ u32 perm;
+
+ state = aa_dfa_match(profile->xmatch,
+ DFA_START, name);
+ perm = dfa_user_allow(profile->xmatch, state);
+ /* any accepting state means a valid match. */
+ if (perm & MAY_EXEC) {
+ candidate = profile;
+ len = profile->xmatch_len;
+ conflict = false;
+ }
}
} else if (!strcmp(profile->base.name, name))
/* exact non-re match, no more searching required */
return profile;
}
+ if (conflict) {
+ *info = "conflicting profile attachments";
+ return NULL;
+ }
+
return candidate;
}
@@ -167,16 +184,18 @@ static struct aa_profile *__attach_match
* @ns: the current namespace (NOT NULL)
* @list: list to search (NOT NULL)
* @name: the executable name to match against (NOT NULL)
+ * @info: info message if there was an error
*
* Returns: profile or NULL if no match found
*/
static struct aa_profile *find_attach(struct aa_namespace *ns,
- struct list_head *list, const char *name)
+ struct list_head *list, const char *name,
+ const char **info)
{
struct aa_profile *profile;
rcu_read_lock();
- profile = aa_get_profile(__attach_match(name, list));
+ profile = aa_get_profile(__attach_match(name, list, info));
rcu_read_unlock();
return profile;
@@ -298,7 +317,8 @@ static struct aa_profile *x_table_lookup
* Returns: refcounted profile or NULL if not found available
*/
static struct aa_profile *x_to_profile(struct aa_profile *profile,
- const char *name, u32 xindex)
+ const char *name, u32 xindex,
+ const char **info)
{
struct aa_profile *new_profile = NULL;
struct aa_namespace *ns = profile->ns;
@@ -312,11 +332,11 @@ static struct aa_profile *x_to_profile(s
if (xindex & AA_X_CHILD)
/* released by caller */
new_profile = find_attach(ns, &profile->base.profiles,
- name);
+ name, info);
else
/* released by caller */
new_profile = find_attach(ns, &ns->base.profiles,
- name);
+ name, info);
break;
case AA_X_TABLE:
/* released by caller */
@@ -385,7 +405,8 @@ int apparmor_bprm_set_creds(struct linux
/* change_profile on exec already been granted */
new_profile = aa_get_profile(cxt->onexec);
else
- new_profile = find_attach(ns, &ns->base.profiles, name);
+ new_profile = find_attach(ns, &ns->base.profiles, name,
+ &info);
if (!new_profile)
goto cleanup;
/*
@@ -421,7 +442,7 @@ int apparmor_bprm_set_creds(struct linux
if (perms.allow & MAY_EXEC) {
/* exec permission determine how to transition */
- new_profile = x_to_profile(profile, name, perms.xindex);
+ new_profile = x_to_profile(profile, name, perms.xindex, &info);
if (!new_profile) {
if (perms.xindex & AA_X_INHERIT) {
/* (p|c|n)ix - don't change profile but do