From patchwork Thu Nov 14 23:21:09 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: John Johansen X-Patchwork-Id: 291411 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) by ozlabs.org (Postfix) with ESMTP id ADBE92C00BC for ; Fri, 15 Nov 2013 10:21:27 +1100 (EST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1Vh6Dx-0003Jb-0q; Thu, 14 Nov 2013 23:21:21 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1Vh6Dr-0003Hq-Ql for kernel-team@lists.ubuntu.com; Thu, 14 Nov 2013 23:21:15 +0000 Received: from static-50-53-33-184.bvtn.or.frontiernet.net ([50.53.33.184] helo=[192.168.192.110]) by youngberry.canonical.com with esmtpsa (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1Vh6Dr-00084R-KI for kernel-team@lists.ubuntu.com; Thu, 14 Nov 2013 23:21:15 +0000 Message-ID: <52855AE5.3040401@canonical.com> Date: Thu, 14 Nov 2013 15:21:09 -0800 From: John Johansen Organization: Canonical User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0 MIME-Version: 1.0 To: Kernel team list Subject: [saucy] [PATCH] UBUNTU: SAUCE: (no-up) apparmor: Fix tasks not subject to, reloaded policy X-Enigmail-Version: 1.5.2 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.14 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: kernel-team-bounces@lists.ubuntu.com This patch is against code that is not upstream. BugLink: http://bugs.launchpad.net/bugs/1236455 When profile replacement is used the cred may not be updated for various reasons (cached, locking ...). In these cases directly using the cred without checking for an updated profile is wrong, and can result in inconsistent application of old vs. new policy. This is not just an issue of when the new policy takes over from the old but both old and new being applied similtaneously in inconsistent ways. Signed-off-by: John Johansen --- security/apparmor/domain.c | 7 +++++-- security/apparmor/include/context.h | 11 +++++++++++ security/apparmor/include/policy.h | 1 - security/apparmor/lsm.c | 18 ++++++++++++------ security/apparmor/resource.c | 2 +- 5 files changed, 29 insertions(+), 10 deletions(-) diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 8cc0472..90f5c87 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -641,7 +641,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest) /* released below */ cred = get_current_cred(); cxt = cred_cxt(cred); - label = aa_cred_label(cred); + label = aa_get_newest_cred_label(cred); previous = cxt->previous; profile = labels_profile(label); @@ -739,6 +739,7 @@ audit: out: aa_put_profile(hat); + aa_put_label(label); kfree(name); put_cred(cred); @@ -784,7 +785,7 @@ int aa_change_profile(const char *ns_name, const char *hname, bool onexec, } cred = get_current_cred(); - label = aa_cred_label(cred); + label = aa_get_newest_cred_label(cred); profile = labels_profile(label); /* @@ -795,6 +796,7 @@ int aa_change_profile(const char *ns_name, const char *hname, bool onexec, * of permissions. */ if (current->no_new_privs && !unconfined(label)) { + aa_put_label(label); put_cred(cred); return -EPERM; } @@ -866,6 +868,7 @@ audit: aa_put_namespace(ns); aa_put_profile(target); + aa_put_label(label); put_cred(cred); return error; diff --git a/security/apparmor/include/context.h b/security/apparmor/include/context.h index 7641967..c3d8fe4 100644 --- a/security/apparmor/include/context.h +++ b/security/apparmor/include/context.h @@ -103,6 +103,17 @@ static inline struct aa_label *aa_cred_label(const struct cred *cred) } /** + * aa_get_newest_cred_label - obtain the newest version of the label on a cred + * @cred: cred to obtain label from (NOT NULL) + * + * Returns: newest version of confining label + */ +static inline struct aa_label *aa_get_newest_cred_label(const struct cred *cred) +{ + return aa_get_newest_label(aa_cred_label(cred)); +} + +/** * __aa_task_label - retrieve another task's label * @task: task to query (NOT NULL) * diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h index 09d3ccf..f5053da 100644 --- a/security/apparmor/include/policy.h +++ b/security/apparmor/include/policy.h @@ -255,7 +255,6 @@ static inline struct aa_profile *aa_get_newest_profile(struct aa_profile *p) return labels_profile(aa_get_newest_label(&p->label)); } - static inline struct aa_profile *aa_deref_parent(struct aa_profile *p) { return rcu_dereference_protected(p->parent, diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index f2e233b..2eb665e 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -126,7 +126,7 @@ static int apparmor_capget(struct task_struct *target, kernel_cap_t *effective, rcu_read_lock(); cred = __task_cred(target); - label = aa_cred_label(cred); + label = aa_get_newest_cred_label(cred); *effective = cred->cap_effective; *inheritable = cred->cap_inheritable; @@ -145,6 +145,7 @@ static int apparmor_capget(struct task_struct *target, kernel_cap_t *effective, } } rcu_read_unlock(); + aa_put_label(label); return 0; } @@ -159,15 +160,17 @@ static int apparmor_capable(const struct cred *cred, struct user_namespace *ns, if (error) return error; - label = aa_cred_label(cred); + label = aa_get_newest_cred_label(cred); if (unconfined(label)) - return 0; + goto out; label_for_each_confined(i, label, profile) { int e = aa_capable(current, profile, cap, audit); if (e) error = e; } +out: + aa_put_label(label); return error; } @@ -426,7 +429,7 @@ static int apparmor_file_open(struct file *file, const struct cred *cred) return 0; } - label = aa_cred_label(cred); + label = aa_get_newest_cred_label(cred); if (!unconfined(label)) { struct inode *inode = file_inode(file); struct path_cond cond = { inode->i_uid, inode->i_mode }; @@ -436,6 +439,7 @@ static int apparmor_file_open(struct file *file, const struct cred *cred) /* todo cache full allowed permissions set and state */ fcxt->allow = aa_map_file_to_perms(file); } + aa_put_label(label); return error; } @@ -460,14 +464,14 @@ static void apparmor_file_free_security(struct file *file) static int common_file_perm(int op, struct file *file, u32 mask) { struct aa_file_cxt *fcxt = file->f_security; - struct aa_label *label, *flabel = aa_cred_label(file->f_cred); + struct aa_label *label, *flabel = aa_get_newest_cred_label(file->f_cred); int error = 0; BUG_ON(!flabel); if (!file->f_path.mnt || !mediated_filesystem(file_inode(file))) - return 0; + goto out; label = __aa_get_current_label(); @@ -482,6 +486,8 @@ static int common_file_perm(int op, struct file *file, u32 mask) ((flabel != label) || (mask & ~fcxt->allow))) error = aa_file_perm(op, label, file, mask); __aa_put_current_label(label); +out: + aa_put_label(flabel); return error; } diff --git a/security/apparmor/resource.c b/security/apparmor/resource.c index 3614b62..c32b33f 100644 --- a/security/apparmor/resource.c +++ b/security/apparmor/resource.c @@ -96,7 +96,7 @@ int aa_task_setrlimit(struct aa_label *label, struct task_struct *task, int i, error = 0; rcu_read_lock(); - task_label = aa_get_label(aa_cred_label(__task_cred(task))); + task_label = aa_get_newest_cred_label(__task_cred(task)); rcu_read_unlock(); /* TODO: extend resource control to handle other (non current)