lkml.org 
[lkml]   [2018]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.16 101/136] apparmor: ensure that undecidable profile attachments fail
    3.16.54-rc1 review patch.  If anyone has any objections, please let me know.

    ------------------

    From: John Johansen <john.johansen@canonical.com>

    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@canonical.com>
    [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@decadent.org.uk>
    ---
    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
    \
     
     \ /
      Last update: 2018-02-11 05:47    [W:4.066 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site