[PATCH][eoan] UBUNTU: SAUCE: apparmor: fix nnp subset test for unconfined

[Tyler Hicks]

On 2019-10-03 12:14:35, 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 at 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(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 9e0492795267..776e8f76aef3 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -931,7 +931,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;
> @@ -1209,7 +1209,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;
> @@ -1230,7 +1230,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;
> @@ -1426,7 +1426,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 56be4412d84e..b1aeb282ca22 100644
> --- a/security/apparmor/include/label.h
> +++ b/security/apparmor/include/label.h
> @@ -281,6 +281,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 3fa3e56f0e05..d94cf3d10877 100644
> --- a/security/apparmor/label.c
> +++ b/security/apparmor/label.c
> @@ -551,6 +551,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.

Something caused me to take another look at this patch and now I'm
wondering why it is safe, when considering transitions where NNP is set,
for @sub to contain an unconfined profile that's not present in @set.
Doesn't that violate NNP?

> + * Conversely we don't care about an unconfined in @set that is not in
> + * @sub

I agree that this should be safe. However, I don't see where this
condition is special-cased in the code. If @set contains an unconfined
profile that's not present in @sub, I would think that
__aa_label_next_not_in_set() would just skip over that profile.

Tyler

> + *
> + * 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 at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team