diff mbox

[saucy] UBUNTU: SAUCE: (no-up) apparmor: Fix tasks not subject to, reloaded policy

Message ID 52855AE5.3040401@canonical.com
State New
Headers show

Commit Message

John Johansen Nov. 14, 2013, 11:21 p.m. UTC
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 <john.johansen@canonical.com>
---
 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(-)

Comments

Stefan Bader Nov. 15, 2013, 8:23 a.m. UTC | #1
The apparent changes are consistent with the description and those replacing
direct access by the get function seem to be paired with puts. Good test results
in the bug report.
Tim Gardner Nov. 15, 2013, 2:19 p.m. UTC | #2

diff mbox

Patch

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)