[PATCH][SRU][xenial] UBUNTU: SAUCE: apparmor: fix nnp subset check failure, when stacking

[Tyler Hicks]

On 2019-08-05 16:31:33, John Johansen wrote:
> This is a backport of a fix that landed as part of a larger patch
> in 4.17 commit 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp")

Same question as I had on the Bionic patch. Do we also need to remove
the no new privs check from change_profile_perms_wrapper()?

Tyler

> 
> Domain transitions that add a new profile to the confinement stack
> when under NO NEW PRIVS is allowed as it can not expand privileges.
> 
> However such transitions are failing due to how/where the subset
> test is being applied. Applying the test per profile in the
> profile transition and profile_onexec call backs is incorrect as
> it disregards the other profiles in the stack so it can not
> correctly determine if the old confinement stack is a subset of
> the new confinement stack.
> 
> Move the test to after the new confinement stack is constructed.
> 
> BugLink: http://bugs.launchpad.net/bugs/1839037
> Signed-off-by: John Johansen <john.johansen at canonical.com>
> ---
>  security/apparmor/domain.c | 47 ++++++++++++--------------------------
>  1 file changed, 14 insertions(+), 33 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index d9e29b2fdda2..38595ca7e642 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -560,22 +560,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) {
> @@ -653,22 +637,6 @@ static int profile_onexec(struct aa_profile *profile, struct aa_label *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) {
>  			dbg_printk("appaarmor: scrubbing environment "
> @@ -788,7 +756,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, label)) {
> +		error = -EPERM;
> +		info = "no new privs";
> +		goto audit;
> +	}
>  
>  	if (bprm->unsafe & LSM_UNSAFE_SHARE) {
>  		/* FIXME: currently don't mediate shared state */
> -- 
> 2.17.1
>