| Message ID | 07f7a8ea-d711-9e3e-7021-8f50d4331695@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,bionic] UBUNTU: SAUCE: apparmor: fix nnp subset check failure when, stacking | expand |
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 <john.johansen@canonical.com> > --- > 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 >
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 <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 | 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 > >
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 <john.johansen@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > 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 */ >
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 <john.johansen@canonical.com> > --- > 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 > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
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 */
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 | 46 ++++++++++++-------------------------- 1 file changed, 14 insertions(+), 32 deletions(-)