[SRU,disco] UBUNTU: SAUCE: apparmor: fix nnp subset test for unconfined
diff mbox series

Message ID bf25962f-85d3-1290-80d3-ccd5cffb9d2a@canonical.com
State New
Headers show
Series
  • [SRU,disco] UBUNTU: SAUCE: apparmor: fix nnp subset test for unconfined
Related show

Commit Message

John Johansen Oct. 3, 2019, 7:14 p.m. UTC
The subset test is not taking into account the unconfined exception
which will cause profile transitions in the stacked confinement
case to fail when no_new_privs is applied.

This fixes a regression introduced in the 4.17 kernel caused by the
reworking of domain transitions.

Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp")
BugLink: https://bugs.launchpad.net/bugs/1844186
Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/domain.c        |  8 ++++----
 security/apparmor/include/label.h |  1 +
 security/apparmor/label.c         | 33 +++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 4 deletions(-)

Comments

Tyler Hicks Oct. 3, 2019, 9:07 p.m. UTC | #1
On 2019-10-03 12:14:46, John Johansen wrote:
> The subset test is not taking into account the unconfined exception
> which will cause profile transitions in the stacked confinement
> case to fail when no_new_privs is applied.
> 
> This fixes a regression introduced in the 4.17 kernel caused by the
> reworking of domain transitions.
> 
> Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp")
> BugLink: https://bugs.launchpad.net/bugs/1844186
> Signed-off-by: John Johansen <john.johansen@canonical.com>

Acked-by: Tyler Hicks <tyhicks@canonical.com>

Thanks!

Tyler

> ---
>  security/apparmor/domain.c        |  8 ++++----
>  security/apparmor/include/label.h |  1 +
>  security/apparmor/label.c         | 33 +++++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 3b266a438776..1c3cd1228e97 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -935,7 +935,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	 * 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)) {
> +	    !unconfined(label) && !aa_label_is_unconfined_subset(new, ctx->nnp)) {
>  		error = -EPERM;
>  		info = "no new privs";
>  		goto audit;
> @@ -1213,7 +1213,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags)
>  		 * reduce restrictions.
>  		 */
>  		if (task_no_new_privs(current) && !unconfined(label) &&
> -		    !aa_label_is_subset(new, ctx->nnp)) {
> +		    !aa_label_is_unconfined_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;
> @@ -1234,7 +1234,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags)
>  		 * reduce restrictions.
>  		 */
>  		if (task_no_new_privs(current) && !unconfined(label) &&
> -		    !aa_label_is_subset(previous, ctx->nnp)) {
> +		    !aa_label_is_unconfined_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;
> @@ -1430,7 +1430,7 @@ int aa_change_profile(const char *fqname, int flags)
>  		 * reduce restrictions.
>  		 */
>  		if (task_no_new_privs(current) && !unconfined(label) &&
> -		    !aa_label_is_subset(new, ctx->nnp)) {
> +		    !aa_label_is_unconfined_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;
> diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h
> index d871e7ff0952..71af9f6f58ed 100644
> --- a/security/apparmor/include/label.h
> +++ b/security/apparmor/include/label.h
> @@ -285,6 +285,7 @@ bool aa_label_init(struct aa_label *label, int size);
>  struct aa_label *aa_label_alloc(int size, struct aa_proxy *proxy, gfp_t gfp);
>  
>  bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub);
> +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub);
>  struct aa_profile *__aa_label_next_not_in_set(struct label_it *I,
>  					     struct aa_label *set,
>  					     struct aa_label *sub);
> diff --git a/security/apparmor/label.c b/security/apparmor/label.c
> index 83e4f6d62c66..8de6f1655bb1 100644
> --- a/security/apparmor/label.c
> +++ b/security/apparmor/label.c
> @@ -555,6 +555,39 @@ bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub)
>  	return __aa_label_next_not_in_set(&i, set, sub) == NULL;
>  }
>  
> +/**
> + * aa_label_is_unconfined_subset - test if @sub is a subset of @set
> + * @set: label to test against
> + * @sub: label to test if is subset of @set
> + *
> + * This checks for subset but taking into account unconfined. IF
> + * @sub contains an unconfined profile that does not have a matching
> + * unconfined in @set then this will not cause the test to fail.
> + * Conversely we don't care about an unconfined in @set that is not in
> + * @sub
> + *
> + * Returns: true if @sub is special_subset of @set
> + *     else false
> + */
> +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub)
> +{
> +	struct label_it i = { };
> +	struct aa_profile *p;
> +
> +	AA_BUG(!set);
> +	AA_BUG(!sub);
> +
> +	if (sub == set)
> +		return true;
> +
> +	do {
> +		p = __aa_label_next_not_in_set(&i, set, sub);
> +		if (p && !profile_unconfined(p))
> +			break;
> +	} while (p);
> +
> +	return p == NULL;
> +}
>  
>  
>  /**
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Stefan Bader Oct. 9, 2019, 10:44 a.m. UTC | #2
On 03.10.19 21:14, John Johansen wrote:
> The subset test is not taking into account the unconfined exception
> which will cause profile transitions in the stacked confinement
> case to fail when no_new_privs is applied.
> 
> This fixes a regression introduced in the 4.17 kernel caused by the
> reworking of domain transitions.
> 
> Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp")
> BugLink: https://bugs.launchpad.net/bugs/1844186
> Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---

Any "complaints" already in the Eoan reply.

>  security/apparmor/domain.c        |  8 ++++----
>  security/apparmor/include/label.h |  1 +
>  security/apparmor/label.c         | 33 +++++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 3b266a438776..1c3cd1228e97 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -935,7 +935,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	 * 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)) {
> +	    !unconfined(label) && !aa_label_is_unconfined_subset(new, ctx->nnp)) {
>  		error = -EPERM;
>  		info = "no new privs";
>  		goto audit;
> @@ -1213,7 +1213,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags)
>  		 * reduce restrictions.
>  		 */
>  		if (task_no_new_privs(current) && !unconfined(label) &&
> -		    !aa_label_is_subset(new, ctx->nnp)) {
> +		    !aa_label_is_unconfined_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;
> @@ -1234,7 +1234,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags)
>  		 * reduce restrictions.
>  		 */
>  		if (task_no_new_privs(current) && !unconfined(label) &&
> -		    !aa_label_is_subset(previous, ctx->nnp)) {
> +		    !aa_label_is_unconfined_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;
> @@ -1430,7 +1430,7 @@ int aa_change_profile(const char *fqname, int flags)
>  		 * reduce restrictions.
>  		 */
>  		if (task_no_new_privs(current) && !unconfined(label) &&
> -		    !aa_label_is_subset(new, ctx->nnp)) {
> +		    !aa_label_is_unconfined_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;
> diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h
> index d871e7ff0952..71af9f6f58ed 100644
> --- a/security/apparmor/include/label.h
> +++ b/security/apparmor/include/label.h
> @@ -285,6 +285,7 @@ bool aa_label_init(struct aa_label *label, int size);
>  struct aa_label *aa_label_alloc(int size, struct aa_proxy *proxy, gfp_t gfp);
>  
>  bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub);
> +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub);
>  struct aa_profile *__aa_label_next_not_in_set(struct label_it *I,
>  					     struct aa_label *set,
>  					     struct aa_label *sub);
> diff --git a/security/apparmor/label.c b/security/apparmor/label.c
> index 83e4f6d62c66..8de6f1655bb1 100644
> --- a/security/apparmor/label.c
> +++ b/security/apparmor/label.c
> @@ -555,6 +555,39 @@ bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub)
>  	return __aa_label_next_not_in_set(&i, set, sub) == NULL;
>  }
>  
> +/**
> + * aa_label_is_unconfined_subset - test if @sub is a subset of @set
> + * @set: label to test against
> + * @sub: label to test if is subset of @set
> + *
> + * This checks for subset but taking into account unconfined. IF
> + * @sub contains an unconfined profile that does not have a matching
> + * unconfined in @set then this will not cause the test to fail.
> + * Conversely we don't care about an unconfined in @set that is not in
> + * @sub
> + *
> + * Returns: true if @sub is special_subset of @set
> + *     else false
> + */
> +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub)
> +{
> +	struct label_it i = { };
> +	struct aa_profile *p;
> +
> +	AA_BUG(!set);
> +	AA_BUG(!sub);
> +
> +	if (sub == set)
> +		return true;
> +
> +	do {
> +		p = __aa_label_next_not_in_set(&i, set, sub);
> +		if (p && !profile_unconfined(p))
> +			break;
> +	} while (p);
> +
> +	return p == NULL;
> +}
>  
>  
>  /**
>
Kleber Souza Oct. 16, 2019, 2:25 p.m. UTC | #3
On 03.10.19 21:14, John Johansen wrote:
> The subset test is not taking into account the unconfined exception
> which will cause profile transitions in the stacked confinement
> case to fail when no_new_privs is applied.
> 
> This fixes a regression introduced in the 4.17 kernel caused by the
> reworking of domain transitions.
> 
> Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp")
> BugLink: https://bugs.launchpad.net/bugs/1844186
> Signed-off-by: John Johansen <john.johansen@canonical.com>

Applied to disco/master-next branch.

Thanks,
Kleber

> ---
>  security/apparmor/domain.c        |  8 ++++----
>  security/apparmor/include/label.h |  1 +
>  security/apparmor/label.c         | 33 +++++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 3b266a438776..1c3cd1228e97 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -935,7 +935,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	 * 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)) {
> +	    !unconfined(label) && !aa_label_is_unconfined_subset(new, ctx->nnp)) {
>  		error = -EPERM;
>  		info = "no new privs";
>  		goto audit;
> @@ -1213,7 +1213,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags)
>  		 * reduce restrictions.
>  		 */
>  		if (task_no_new_privs(current) && !unconfined(label) &&
> -		    !aa_label_is_subset(new, ctx->nnp)) {
> +		    !aa_label_is_unconfined_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;
> @@ -1234,7 +1234,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags)
>  		 * reduce restrictions.
>  		 */
>  		if (task_no_new_privs(current) && !unconfined(label) &&
> -		    !aa_label_is_subset(previous, ctx->nnp)) {
> +		    !aa_label_is_unconfined_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;
> @@ -1430,7 +1430,7 @@ int aa_change_profile(const char *fqname, int flags)
>  		 * reduce restrictions.
>  		 */
>  		if (task_no_new_privs(current) && !unconfined(label) &&
> -		    !aa_label_is_subset(new, ctx->nnp)) {
> +		    !aa_label_is_unconfined_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;
> diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h
> index d871e7ff0952..71af9f6f58ed 100644
> --- a/security/apparmor/include/label.h
> +++ b/security/apparmor/include/label.h
> @@ -285,6 +285,7 @@ bool aa_label_init(struct aa_label *label, int size);
>  struct aa_label *aa_label_alloc(int size, struct aa_proxy *proxy, gfp_t gfp);
>  
>  bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub);
> +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub);
>  struct aa_profile *__aa_label_next_not_in_set(struct label_it *I,
>  					     struct aa_label *set,
>  					     struct aa_label *sub);
> diff --git a/security/apparmor/label.c b/security/apparmor/label.c
> index 83e4f6d62c66..8de6f1655bb1 100644
> --- a/security/apparmor/label.c
> +++ b/security/apparmor/label.c
> @@ -555,6 +555,39 @@ bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub)
>  	return __aa_label_next_not_in_set(&i, set, sub) == NULL;
>  }
>  
> +/**
> + * aa_label_is_unconfined_subset - test if @sub is a subset of @set
> + * @set: label to test against
> + * @sub: label to test if is subset of @set
> + *
> + * This checks for subset but taking into account unconfined. IF
> + * @sub contains an unconfined profile that does not have a matching
> + * unconfined in @set then this will not cause the test to fail.
> + * Conversely we don't care about an unconfined in @set that is not in
> + * @sub
> + *
> + * Returns: true if @sub is special_subset of @set
> + *     else false
> + */
> +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub)
> +{
> +	struct label_it i = { };
> +	struct aa_profile *p;
> +
> +	AA_BUG(!set);
> +	AA_BUG(!sub);
> +
> +	if (sub == set)
> +		return true;
> +
> +	do {
> +		p = __aa_label_next_not_in_set(&i, set, sub);
> +		if (p && !profile_unconfined(p))
> +			break;
> +	} while (p);
> +
> +	return p == NULL;
> +}
>  
>  
>  /**
>

Patch
diff mbox series

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 3b266a438776..1c3cd1228e97 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -935,7 +935,7 @@  int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 	 * 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)) {
+	    !unconfined(label) && !aa_label_is_unconfined_subset(new, ctx->nnp)) {
 		error = -EPERM;
 		info = "no new privs";
 		goto audit;
@@ -1213,7 +1213,7 @@  int aa_change_hat(const char *hats[], int count, u64 token, int flags)
 		 * reduce restrictions.
 		 */
 		if (task_no_new_privs(current) && !unconfined(label) &&
-		    !aa_label_is_subset(new, ctx->nnp)) {
+		    !aa_label_is_unconfined_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;
@@ -1234,7 +1234,7 @@  int aa_change_hat(const char *hats[], int count, u64 token, int flags)
 		 * reduce restrictions.
 		 */
 		if (task_no_new_privs(current) && !unconfined(label) &&
-		    !aa_label_is_subset(previous, ctx->nnp)) {
+		    !aa_label_is_unconfined_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;
@@ -1430,7 +1430,7 @@  int aa_change_profile(const char *fqname, int flags)
 		 * reduce restrictions.
 		 */
 		if (task_no_new_privs(current) && !unconfined(label) &&
-		    !aa_label_is_subset(new, ctx->nnp)) {
+		    !aa_label_is_unconfined_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;
diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h
index d871e7ff0952..71af9f6f58ed 100644
--- a/security/apparmor/include/label.h
+++ b/security/apparmor/include/label.h
@@ -285,6 +285,7 @@  bool aa_label_init(struct aa_label *label, int size);
 struct aa_label *aa_label_alloc(int size, struct aa_proxy *proxy, gfp_t gfp);
 
 bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub);
+bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub);
 struct aa_profile *__aa_label_next_not_in_set(struct label_it *I,
 					     struct aa_label *set,
 					     struct aa_label *sub);
diff --git a/security/apparmor/label.c b/security/apparmor/label.c
index 83e4f6d62c66..8de6f1655bb1 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -555,6 +555,39 @@  bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub)
 	return __aa_label_next_not_in_set(&i, set, sub) == NULL;
 }
 
+/**
+ * aa_label_is_unconfined_subset - test if @sub is a subset of @set
+ * @set: label to test against
+ * @sub: label to test if is subset of @set
+ *
+ * This checks for subset but taking into account unconfined. IF
+ * @sub contains an unconfined profile that does not have a matching
+ * unconfined in @set then this will not cause the test to fail.
+ * Conversely we don't care about an unconfined in @set that is not in
+ * @sub
+ *
+ * Returns: true if @sub is special_subset of @set
+ *     else false
+ */
+bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub)
+{
+	struct label_it i = { };
+	struct aa_profile *p;
+
+	AA_BUG(!set);
+	AA_BUG(!sub);
+
+	if (sub == set)
+		return true;
+
+	do {
+		p = __aa_label_next_not_in_set(&i, set, sub);
+		if (p && !profile_unconfined(p))
+			break;
+	} while (p);
+
+	return p == NULL;
+}
 
 
 /**