| 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 | expand |
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 >
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 >>
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 > >
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 */ >
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
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
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 >
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 >> >
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 */
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(-)