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

[John Johansen]

On 8/9/19 9:38 AM, 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()?
> 

change_profile_perms_wrapper() will need a similar change, but that
can come as a separate patch. We didn't catch that one as we didn't
have a test for it, and so I would like to be able to test it before
we push the change.


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