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

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

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

John Johansen-2
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 <[hidden email]>
---
 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
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Tyler Hicks-2
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 <[hidden email]>
> ---
>  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
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

John Johansen-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 <[hidden email]>
>> ---
>>  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
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Tyler Hicks-2
In reply to this post by Tyler Hicks-2
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 <[hidden email]>

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 <[hidden email]>
> > ---
> >  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
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Stefan Bader-2
In reply to this post by John Johansen-2
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 <[hidden email]>
Acked-by: Stefan Bader <[hidden email]>

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


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Khaled Elmously
In reply to this post by John Johansen-2
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 <[hidden email]>
> ---
>  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
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Khaled Elmously
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 <[hidden email]>
> > ---
> >  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
> > [hidden email]
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Kleber Souza
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 <[hidden email]>
>>> ---
>>>  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
>>> [hidden email]
>>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

John Johansen-2
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 <[hidden email]>
>>>> ---
>>>>  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
>>>> [hidden email]
>>>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>>
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team