apparmor: update domain transitions that are subsets of confinement at nnp

Domain transition so far have been largely blocked by no new privs,
unless the transition has been provably a subset of the previous
confinement. There was a couple problems with the previous
implementations,

- transitions that weren't explicitly a stack but resulted in a subset
  of confinement were disallowed

- confinement subsets were only calculated from the previous
  confinement instead of the confinement being enforced at the time of
  no new privs, so transitions would have to get progressively
  tighter.

Fix this by detecting and storing a reference to the task's
confinement at the "time" no new privs is set. This reference is then
used to determine whether a transition is a subsystem of the
confinement at the time no new privs was set.

Unfortunately the implementation is less than ideal in that we have to
detect no new privs after the fact when a task attempts a domain
transition. This is adequate for the currently but will not work in a
stacking situation where no new privs could be conceivably be set in
both the "host" and in the container.

Signed-off-by: John Johansen <john.johansen@canonical.com>

4 files changed, 110 insertions, 65 deletions

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 874c1bf6b84a..07623fb41e32 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -2156,6 +2156,7 @@ static struct aa_sfs_entry aa_sfs_entry_domain[] = {
 	AA_SFS_FILE_BOOLEAN("change_profile",	1),
 	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_FILE_STRING("version", "1.2"),
 	{ }
 };
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index cd58eef4eb8d..9d1936519cfd 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -592,22 +592,6 @@ static struct aa_label *profile_transition(struct aa_profile *profile,
 	if (!new)
 		goto audit;
 
-	/* Policy has specified a domain transitions. if no_new_privs and
-	 * confined and not transitioning to the current domain fail.
-	 *
-	 * NOTE: Domain transitions from unconfined and to stritly stacked
-	 * subsets are allowed even when no_new_privs is set because this
-	 * aways results in a further reduction of permissions.
-	 */
-	if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) &&
-	    !profile_unconfined(profile) &&
-	    !aa_label_is_subset(new, &profile->label)) {
-		error = -EPERM;
-		info = "no new privs";
-		nonewprivs = true;
-		perms.allow &= ~MAY_EXEC;
-		goto audit;
-	}
 
 	if (!(perms.xindex & AA_X_UNSAFE)) {
 		if (DEBUG_ON) {
@@ -684,21 +668,6 @@ static int profile_onexec(struct aa_profile *profile, struct aa_label *onexec,
 		perms.allow &= ~AA_MAY_ONEXEC;
 		goto audit;
 	}
-	/* Policy has specified a domain transitions. if no_new_privs and
-	 * confined and not transitioning to the current domain fail.
-	 *
-	 * NOTE: Domain transitions from unconfined and to stritly stacked
-	 * subsets are allowed even when no_new_privs is set because this
-	 * aways results in a further reduction of permissions.
-	 */
-	if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) &&
-	    !profile_unconfined(profile) &&
-	    !aa_label_is_subset(onexec, &profile->label)) {
-		error = -EPERM;
-		info = "no new privs";
-		perms.allow &= ~AA_MAY_ONEXEC;
-		goto audit;
-	}
 
 	if (!(perms.xindex & AA_X_UNSAFE)) {
 		if (DEBUG_ON) {
@@ -800,6 +769,17 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 
 	label = aa_get_newest_label(cred_label(bprm->cred));
 
+	/*
+	 * Detect no new privs being set, and store the label it
+	 * occurred under. Ideally this would happen when nnp
+	 * is set but there isn't a good way to do that yet.
+	 *
+	 * Testing for unconfined must be done before the subset test
+	 */
+	if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) && !unconfined(label) &&
+	    !ctx->nnp)
+		ctx->nnp = aa_get_label(label);
+
 	/* buffer freed below, name is pointer into buffer */
 	get_buffers(buffer);
 	/* Test for onexec first as onexec override other x transitions. */
@@ -820,7 +800,20 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 		goto done;
 	}
 
-	/* TODO: Add ns level no_new_privs subset test */
+	/* Policy has specified a domain transitions. If no_new_privs and
+	 * confined ensure the transition is to confinement that is subset
+	 * of the confinement when the task entered no new privs.
+	 *
+	 * NOTE: Domain transitions from unconfined and to stacked
+	 * subsets are allowed even when no_new_privs is set because this
+	 * aways results in a further reduction of permissions.
+	 */
+	if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) &&
+	    !unconfined(label) && !aa_label_is_subset(new, ctx->nnp)) {
+		error = -EPERM;
+		info = "no new privs";
+		goto audit;
+	}
 
 	if (bprm->unsafe & LSM_UNSAFE_SHARE) {
 		/* FIXME: currently don't mediate shared state */
@@ -1047,30 +1040,28 @@ build:
 int aa_change_hat(const char *hats[], int count, u64 token, int flags)
 {
 	const struct cred *cred;
-	struct aa_task_ctx *ctx;
+	struct aa_task_ctx *ctx = task_ctx(current);
 	struct aa_label *label, *previous, *new = NULL, *target = NULL;
 	struct aa_profile *profile;
 	struct aa_perms perms = {};
 	const char *info = NULL;
 	int error = 0;
 
-	/*
-	 * Fail explicitly requested domain transitions if no_new_privs.
-	 * There is no exception for unconfined as change_hat is not
-	 * available.
-	 */
-	if (task_no_new_privs(current)) {
-		/* not an apparmor denial per se, so don't log it */
-		AA_DEBUG("no_new_privs - change_hat denied");
-		return -EPERM;
-	}
-
 	/* released below */
 	cred = get_current_cred();
-	ctx = task_ctx(current);
 	label = aa_get_newest_cred_label(cred);
 	previous = aa_get_newest_label(ctx->previous);
 
+	/*
+	 * Detect no new privs being set, and store the label it
+	 * occurred under. Ideally this would happen when nnp
+	 * is set but there isn't a good way to do that yet.
+	 *
+	 * Testing for unconfined must be done before the subset test
+	 */
+	if (task_no_new_privs(current) && !unconfined(label) && !ctx->nnp)
+		ctx->nnp = aa_get_label(label);
+
 	if (unconfined(label)) {
 		info = "unconfined can not change_hat";
 		error = -EPERM;
@@ -1091,6 +1082,18 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags)
 		if (error)
 			goto fail;
 
+		/*
+		 * no new privs prevents domain transitions that would
+		 * reduce restrictions.
+		 */
+		if (task_no_new_privs(current) && !unconfined(label) &&
+		    !aa_label_is_subset(new, ctx->nnp)) {
+			/* not an apparmor denial per se, so don't log it */
+			AA_DEBUG("no_new_privs - change_hat denied");
+			error = -EPERM;
+			goto out;
+		}
+
 		if (flags & AA_CHANGE_TEST)
 			goto out;
 
@@ -1100,6 +1103,18 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags)
 			/* kill task in case of brute force attacks */
 			goto kill;
 	} else if (previous && !(flags & AA_CHANGE_TEST)) {
+		/*
+		 * no new privs prevents domain transitions that would
+		 * reduce restrictions.
+		 */
+		if (task_no_new_privs(current) && !unconfined(label) &&
+		    !aa_label_is_subset(previous, ctx->nnp)) {
+			/* not an apparmor denial per se, so don't log it */
+			AA_DEBUG("no_new_privs - change_hat denied");
+			error = -EPERM;
+			goto out;
+		}
+
 		/* Return to saved label.  Kill task if restore fails
 		 * to avoid brute force attacks
 		 */
@@ -1142,21 +1157,6 @@ static int change_profile_perms_wrapper(const char *op, const char *name,
 	const char *info = NULL;
 	int error = 0;
 
-	/*
-	 * Fail explicitly requested domain transitions when no_new_privs
-	 * and not unconfined OR the transition results in a stack on
-	 * the current label.
-	 * Stacking domain transitions and transitions from unconfined are
-	 * allowed even when no_new_privs is set because this aways results
-	 * in a reduction of permissions.
-	 */
-	if (task_no_new_privs(current) && !stack &&
-	    !profile_unconfined(profile) &&
-	    !aa_label_is_subset(target, &profile->label)) {
-		info = "no new privs";
-		error = -EPERM;
-	}
-
 	if (!error)
 		error = change_profile_perms(profile, target, stack, request,
 					     profile->file.start, perms);
@@ -1190,10 +1190,23 @@ int aa_change_profile(const char *fqname, int flags)
 	const char *info = NULL;
 	const char *auditname = fqname;		/* retain leading & if stack */
 	bool stack = flags & AA_CHANGE_STACK;
+	struct aa_task_ctx *ctx = task_ctx(current);
 	int error = 0;
 	char *op;
 	u32 request;
 
+	label = aa_get_current_label();
+
+	/*
+	 * Detect no new privs being set, and store the label it
+	 * occurred under. Ideally this would happen when nnp
+	 * is set but there isn't a good way to do that yet.
+	 *
+	 * Testing for unconfined must be done before the subset test
+	 */
+	if (task_no_new_privs(current) && !unconfined(label) && !ctx->nnp)
+		ctx->nnp = aa_get_label(label);
+
 	if (!fqname || !*fqname) {
 		AA_DEBUG("no profile name");
 		return -EINVAL;
@@ -1281,14 +1294,28 @@ check:
 	if (flags & AA_CHANGE_TEST)
 		goto out;
 
+	/* stacking is always a subset, so only check the nonstack case */
+	if (!stack) {
+		new = fn_label_build_in_ns(label, profile, GFP_KERNEL,
+					   aa_get_label(target),
+					   aa_get_label(&profile->label));
+		/*
+		 * no new privs prevents domain transitions that would
+		 * reduce restrictions.
+		 */
+		if (task_no_new_privs(current) && !unconfined(label) &&
+		    !aa_label_is_subset(new, ctx->nnp)) {
+			/* not an apparmor denial per se, so don't log it */
+			AA_DEBUG("no_new_privs - change_hat denied");
+			error = -EPERM;
+			goto out;
+		}
+	}
+
 	if (!(flags & AA_CHANGE_ONEXEC)) {
 		/* only transition profiles in the current ns */
 		if (stack)
 			new = aa_label_merge(label, target, GFP_KERNEL);
-		else
-			new = fn_label_build_in_ns(label, profile, GFP_KERNEL,
-					aa_get_label(target),
-					aa_get_label(&profile->label));
 		if (IS_ERR_OR_NULL(new)) {
 			info = "failed to build target label";
 			error = PTR_ERR(new);
@@ -1297,9 +1324,15 @@ check:
 			goto audit;
 		}
 		error = aa_replace_current_label(new);
-	} else
+	} else {
+		if (new) {
+			aa_put_label(new);
+			new = NULL;
+		}
+
 		/* full transition will be built in exec path */
 		error = aa_set_current_onexec(target, stack);
+	}
 
 audit:
 	error = fn_for_each_in_ns(label, profile,
diff --git a/security/apparmor/include/task.h b/security/apparmor/include/task.h
index d222197db299..55edaa1d83f8 100644
--- a/security/apparmor/include/task.h
+++ b/security/apparmor/include/task.h
@@ -18,11 +18,13 @@
 
 /*
  * struct aa_task_ctx - information for current task label change
+ * @nnp: snapshot of label at time of no_new_privs
  * @onexec: profile to transition to on next exec  (MAY BE NULL)
  * @previous: profile the task may return to     (MAY BE NULL)
  * @token: magic value the task must know for returning to @previous_profile
  */
 struct aa_task_ctx {
+	struct aa_label *nnp;
 	struct aa_label *onexec;
 	struct aa_label *previous;
 	u64 token;
@@ -52,6 +54,7 @@ static inline struct aa_task_ctx *aa_alloc_task_ctx(gfp_t flags)
 static inline void aa_free_task_ctx(struct aa_task_ctx *ctx)
 {
 	if (ctx) {
+		aa_put_label(ctx->nnp);
 		aa_put_label(ctx->previous);
 		aa_put_label(ctx->onexec);
 
@@ -68,6 +71,7 @@ static inline void aa_dup_task_ctx(struct aa_task_ctx *new,
 				   const struct aa_task_ctx *old)
 {
 	*new = *old;
+	aa_get_label(new->nnp);
 	aa_get_label(new->previous);
 	aa_get_label(new->onexec);
 }
diff --git a/security/apparmor/task.c b/security/apparmor/task.c
index 44b9b938e06d..c6b78a14da91 100644
--- a/security/apparmor/task.c
+++ b/security/apparmor/task.c
@@ -45,6 +45,7 @@ struct aa_label *aa_get_task_label(struct task_struct *task)
 int aa_replace_current_label(struct aa_label *label)
 {
 	struct aa_label *old = aa_current_raw_label();
+	struct aa_task_ctx *ctx = task_ctx(current);
 	struct cred *new;
 
 	AA_BUG(!label);
@@ -59,6 +60,12 @@ int aa_replace_current_label(struct aa_label *label)
 	if (!new)
 		return -ENOMEM;
 
+	if (ctx->nnp && label_is_stale(ctx->nnp)) {
+		struct aa_label *tmp = ctx->nnp;
+
+		ctx->nnp = aa_get_newest_label(tmp);
+		aa_put_label(tmp);
+	}
 	if (unconfined(label) || (labels_ns(old) != labels_ns(label)))
 		/*
 		 * if switching to unconfined or a different label namespace