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

[Tyler Hicks]

On 2019-08-09 11:37:59, Tyler Hicks wrote:
> 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()?

Same situation as the Bionic patch. change_profile_perms_wrapper() also
needs tweaking but it isn't important to fix the bug that k8s is
triggering. Lets land this patch ASAP and then fix
change_profile_perms_wrapper() in a future SRU.

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

Tyler

> 
> 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
> >