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

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

[PATCH][SRU][bionic] 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 | 46 ++++++++++++--------------------------
 1 file changed, 14 insertions(+), 32 deletions(-)

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 6a54d2ffa840..e596d2d425bc 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -592,22 +592,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) {
@@ -684,21 +668,6 @@ static int profile_onexec(struct aa_profile *profile, struct aa_label *onexec,
  perms.allow &= ~AA_MAY_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) {
@@ -819,7 +788,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][bionic] UBUNTU: SAUCE: apparmor: fix nnp subset check failure when, stacking

Tyler Hicks-2
On 2019-08-05 16:39:50, 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")

That patch also removes the no new privs check from
change_profile_perms_wrapper() but this patch does not. Is that
intentional?

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 | 46 ++++++++++++--------------------------
>  1 file changed, 14 insertions(+), 32 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 6a54d2ffa840..e596d2d425bc 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -592,22 +592,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) {
> @@ -684,21 +668,6 @@ static int profile_onexec(struct aa_profile *profile, struct aa_label *onexec,
>   perms.allow &= ~AA_MAY_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) {
> @@ -819,7 +788,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][bionic] UBUNTU: SAUCE: apparmor: fix nnp subset check failure when, stacking

Tyler Hicks-2
On 2019-08-09 11:36:59, Tyler Hicks wrote:
> On 2019-08-05 16:39:50, 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")
>
> That patch also removes the no new privs check from
> change_profile_perms_wrapper() but this patch does not. Is that
> intentional?

Speaking with John over IRC, it sounds like
change_profile_perms_wrapper() likely also needs its no new privs check
removed to fix that code path.

However, this patch allows the new nnp.sh apparmor regression test to
pass without triggering any WARNINGs and it also fixes the k8s bug. I
think it is important to land this patch in the upcoming SRU cycle and
then fixup change_profile_perms_wrapper() in a future cycle when we're
not in as much of a rush.

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 | 46 ++++++++++++--------------------------
> >  1 file changed, 14 insertions(+), 32 deletions(-)
> >
> > diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> > index 6a54d2ffa840..e596d2d425bc 100644
> > --- a/security/apparmor/domain.c
> > +++ b/security/apparmor/domain.c
> > @@ -592,22 +592,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) {
> > @@ -684,21 +668,6 @@ static int profile_onexec(struct aa_profile *profile, struct aa_label *onexec,
> >   perms.allow &= ~AA_MAY_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) {
> > @@ -819,7 +788,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][bionic] 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:39, 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 | 46 ++++++++++++--------------------------
>  1 file changed, 14 insertions(+), 32 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 6a54d2ffa840..e596d2d425bc 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -592,22 +592,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) {
> @@ -684,21 +668,6 @@ static int profile_onexec(struct aa_profile *profile, struct aa_label *onexec,
>   perms.allow &= ~AA_MAY_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) {
> @@ -819,7 +788,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: [PATCH][SRU][bionic] UBUNTU: SAUCE: apparmor: fix nnp subset check failure when, stacking

Khaled Elmously
In reply to this post by John Johansen-2
On 2019-08-05 16:39:50 , 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 | 46 ++++++++++++--------------------------
>  1 file changed, 14 insertions(+), 32 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 6a54d2ffa840..e596d2d425bc 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -592,22 +592,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) {
> @@ -684,21 +668,6 @@ static int profile_onexec(struct aa_profile *profile, struct aa_label *onexec,
>   perms.allow &= ~AA_MAY_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) {
> @@ -819,7 +788,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