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

Message ID 27796354-3326-0dab-27d8-35ffc1a74d8c@canonical.com
State New
Headers show
Series
  • [SRU,xenial] UBUNTU: SAUCE: apparmor: fix nnp subset check failure, when stacking
Related show

Commit Message

John Johansen Aug. 5, 2019, 11:31 p.m. UTC
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")

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@canonical.com>
---
 security/apparmor/domain.c | 47 ++++++++++++--------------------------
 1 file changed, 14 insertions(+), 33 deletions(-)

Comments

Tyler Hicks Aug. 9, 2019, 4:38 p.m. UTC | #1
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@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
>
John Johansen Aug. 9, 2019, 9:53 p.m. UTC | #2
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@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
>>
Tyler Hicks Aug. 9, 2019, 9:57 p.m. UTC | #3
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@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@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
> >
Stefan Bader Aug. 12, 2019, 2:59 p.m. UTC | #4
On 06.08.19 01:31, 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")
> 
> 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@canonical.com>
Acked-by: Stefan Bader <stefan.bader@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 */
>
Khaled Elmously Aug. 13, 2019, 4 a.m. UTC | #5
Applied to Xenial, but note: This patch didn't apply cleanly to xenial/master-next.

The first 2 delta chunks remove code that is expected to contain this line:

perms.allow &= ~AA_MAY_ONEXEC

but in xenial/master-next that line doesn't exist.


In any case, I applied the patch by removing those 2 chunks of code anyway.

The third delta chunk had no issues.

Khaled


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")
> 
> 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@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
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Khaled Elmously Aug. 13, 2019, 4:02 a.m. UTC | #6
Sigh..It's because there's a dependency patch, which I just saw.

Pleaase ignore my previous comment.



On 2019-08-13 00:00:11 , Khaled Elmously wrote:
> Applied to Xenial, but note: This patch didn't apply cleanly to xenial/master-next.
> 
> The first 2 delta chunks remove code that is expected to contain this line:
> 
> perms.allow &= ~AA_MAY_ONEXEC
> 
> but in xenial/master-next that line doesn't exist.
> 
> 
> In any case, I applied the patch by removing those 2 chunks of code anyway.
> 
> The third delta chunk had no issues.
> 
> Khaled
> 
> 
> 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")
> > 
> > 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@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
> > 
> > 
> > -- 
> > kernel-team mailing list
> > kernel-team@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Kleber Sacilotto de Souza Aug. 13, 2019, 9:51 a.m. UTC | #7
On 8/13/19 6:02 AM, Khaled Elmously wrote:
> Sigh..It's because there's a dependency patch, which I just saw.
> 
> Pleaase ignore my previous comment.

Hi John,

When a submitted patch was not based on the master-next branch
and cannot be applied cleanly because it's based on other
changes, please send all the patches in a single patch set
or state the dependency in the patch submission so we know in
which order they need to be applied.


Thank you,
Kleber

> 
> 
> 
> On 2019-08-13 00:00:11 , Khaled Elmously wrote:
>> Applied to Xenial, but note: This patch didn't apply cleanly to xenial/master-next.
>>
>> The first 2 delta chunks remove code that is expected to contain this line:
>>
>> perms.allow &= ~AA_MAY_ONEXEC
>>
>> but in xenial/master-next that line doesn't exist.
>>
>>
>> In any case, I applied the patch by removing those 2 chunks of code anyway.
>>
>> The third delta chunk had no issues.
>>
>> Khaled
>>
>>
>> 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")
>>>
>>> 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@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
>>>
>>>
>>> -- 
>>> kernel-team mailing list
>>> kernel-team@lists.ubuntu.com
>>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
John Johansen Aug. 13, 2019, 10:57 a.m. UTC | #8
On 8/13/19 2:51 AM, Kleber Souza wrote:
> On 8/13/19 6:02 AM, Khaled Elmously wrote:
>> Sigh..It's because there's a dependency patch, which I just saw.
>>
>> Pleaase ignore my previous comment.
> 
> Hi John,
> 
> When a submitted patch was not based on the master-next branch
> and cannot be applied cleanly because it's based on other
> changes, please send all the patches in a single patch set
> or state the dependency in the patch submission so we know in
> which order they need to be applied.
> 
oops sorry, I make sure to use git send-email on the set next time



> 
> Thank you,
> Kleber
> 
>>
>>
>>
>> On 2019-08-13 00:00:11 , Khaled Elmously wrote:
>>> Applied to Xenial, but note: This patch didn't apply cleanly to xenial/master-next.
>>>
>>> The first 2 delta chunks remove code that is expected to contain this line:
>>>
>>> perms.allow &= ~AA_MAY_ONEXEC
>>>
>>> but in xenial/master-next that line doesn't exist.
>>>
>>>
>>> In any case, I applied the patch by removing those 2 chunks of code anyway.
>>>
>>> The third delta chunk had no issues.
>>>
>>> Khaled
>>>
>>>
>>> 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")
>>>>
>>>> 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@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
>>>>
>>>>
>>>> -- 
>>>> kernel-team mailing list
>>>> kernel-team@lists.ubuntu.com
>>>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>>
>

Patch
diff mbox series

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