| Message ID | f04dff4f-1630-dc08-4b94-36c42a25e8a6@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [eoan] UBUNTU: SAUCE: apparmor: fix nnp subset test for unconfined | expand |
On 2019-10-03 12:14:35, John Johansen wrote: > The subset test is not taking into account the unconfined exception > which will cause profile transitions in the stacked confinement > case to fail when no_new_privs is applied. > > This fixes a regression introduced in the 4.17 kernel caused by the > reworking of domain transitions. > > Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp") > BugLink: https://bugs.launchpad.net/bugs/1844186 > Signed-off-by: John Johansen <john.johansen@canonical.com> This looks correct to me. Acked-by: Tyler Hicks <tyhicks@canonical.com> I also reviewed the regression tests that you created for this bug: https://gitlab.com/apparmor/apparmor/merge_requests/424 Thanks! Tyler > --- > security/apparmor/domain.c | 8 ++++---- > security/apparmor/include/label.h | 1 + > security/apparmor/label.c | 33 +++++++++++++++++++++++++++++++ > 3 files changed, 38 insertions(+), 4 deletions(-) > > diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c > index 9e0492795267..776e8f76aef3 100644 > --- a/security/apparmor/domain.c > +++ b/security/apparmor/domain.c > @@ -931,7 +931,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) > * aways results in a further reduction of permissions. > */ > if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) && > - !unconfined(label) && !aa_label_is_subset(new, ctx->nnp)) { > + !unconfined(label) && !aa_label_is_unconfined_subset(new, ctx->nnp)) { > error = -EPERM; > info = "no new privs"; > goto audit; > @@ -1209,7 +1209,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(new, ctx->nnp)) { > + !aa_label_is_unconfined_subset(new, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > @@ -1230,7 +1230,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(previous, ctx->nnp)) { > + !aa_label_is_unconfined_subset(previous, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > @@ -1426,7 +1426,7 @@ int aa_change_profile(const char *fqname, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(new, ctx->nnp)) { > + !aa_label_is_unconfined_subset(new, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h > index 56be4412d84e..b1aeb282ca22 100644 > --- a/security/apparmor/include/label.h > +++ b/security/apparmor/include/label.h > @@ -281,6 +281,7 @@ bool aa_label_init(struct aa_label *label, int size); > struct aa_label *aa_label_alloc(int size, struct aa_proxy *proxy, gfp_t gfp); > > bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub); > +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub); > struct aa_profile *__aa_label_next_not_in_set(struct label_it *I, > struct aa_label *set, > struct aa_label *sub); > diff --git a/security/apparmor/label.c b/security/apparmor/label.c > index 3fa3e56f0e05..d94cf3d10877 100644 > --- a/security/apparmor/label.c > +++ b/security/apparmor/label.c > @@ -551,6 +551,39 @@ bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub) > return __aa_label_next_not_in_set(&i, set, sub) == NULL; > } > > +/** > + * aa_label_is_unconfined_subset - test if @sub is a subset of @set > + * @set: label to test against > + * @sub: label to test if is subset of @set > + * > + * This checks for subset but taking into account unconfined. IF > + * @sub contains an unconfined profile that does not have a matching > + * unconfined in @set then this will not cause the test to fail. > + * Conversely we don't care about an unconfined in @set that is not in > + * @sub > + * > + * Returns: true if @sub is special_subset of @set > + * else false > + */ > +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub) > +{ > + struct label_it i = { }; > + struct aa_profile *p; > + > + AA_BUG(!set); > + AA_BUG(!sub); > + > + if (sub == set) > + return true; > + > + do { > + p = __aa_label_next_not_in_set(&i, set, sub); > + if (p && !profile_unconfined(p)) > + break; > + } while (p); > + > + return p == NULL; > +} > > > /** > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 2019-10-03 12:14:35, John Johansen wrote: > The subset test is not taking into account the unconfined exception > which will cause profile transitions in the stacked confinement > case to fail when no_new_privs is applied. > > This fixes a regression introduced in the 4.17 kernel caused by the > reworking of domain transitions. > > Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp") > BugLink: https://bugs.launchpad.net/bugs/1844186 > Signed-off-by: John Johansen <john.johansen@canonical.com> > --- > security/apparmor/domain.c | 8 ++++---- > security/apparmor/include/label.h | 1 + > security/apparmor/label.c | 33 +++++++++++++++++++++++++++++++ > 3 files changed, 38 insertions(+), 4 deletions(-) > > diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c > index 9e0492795267..776e8f76aef3 100644 > --- a/security/apparmor/domain.c > +++ b/security/apparmor/domain.c > @@ -931,7 +931,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) > * aways results in a further reduction of permissions. > */ > if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) && > - !unconfined(label) && !aa_label_is_subset(new, ctx->nnp)) { > + !unconfined(label) && !aa_label_is_unconfined_subset(new, ctx->nnp)) { > error = -EPERM; > info = "no new privs"; > goto audit; > @@ -1209,7 +1209,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(new, ctx->nnp)) { > + !aa_label_is_unconfined_subset(new, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > @@ -1230,7 +1230,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(previous, ctx->nnp)) { > + !aa_label_is_unconfined_subset(previous, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > @@ -1426,7 +1426,7 @@ int aa_change_profile(const char *fqname, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(new, ctx->nnp)) { > + !aa_label_is_unconfined_subset(new, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h > index 56be4412d84e..b1aeb282ca22 100644 > --- a/security/apparmor/include/label.h > +++ b/security/apparmor/include/label.h > @@ -281,6 +281,7 @@ bool aa_label_init(struct aa_label *label, int size); > struct aa_label *aa_label_alloc(int size, struct aa_proxy *proxy, gfp_t gfp); > > bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub); > +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub); > struct aa_profile *__aa_label_next_not_in_set(struct label_it *I, > struct aa_label *set, > struct aa_label *sub); > diff --git a/security/apparmor/label.c b/security/apparmor/label.c > index 3fa3e56f0e05..d94cf3d10877 100644 > --- a/security/apparmor/label.c > +++ b/security/apparmor/label.c > @@ -551,6 +551,39 @@ bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub) > return __aa_label_next_not_in_set(&i, set, sub) == NULL; > } > > +/** > + * aa_label_is_unconfined_subset - test if @sub is a subset of @set > + * @set: label to test against > + * @sub: label to test if is subset of @set > + * > + * This checks for subset but taking into account unconfined. IF > + * @sub contains an unconfined profile that does not have a matching > + * unconfined in @set then this will not cause the test to fail. Something caused me to take another look at this patch and now I'm wondering why it is safe, when considering transitions where NNP is set, for @sub to contain an unconfined profile that's not present in @set. Doesn't that violate NNP? > + * Conversely we don't care about an unconfined in @set that is not in > + * @sub I agree that this should be safe. However, I don't see where this condition is special-cased in the code. If @set contains an unconfined profile that's not present in @sub, I would think that __aa_label_next_not_in_set() would just skip over that profile. Tyler > + * > + * Returns: true if @sub is special_subset of @set > + * else false > + */ > +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub) > +{ > + struct label_it i = { }; > + struct aa_profile *p; > + > + AA_BUG(!set); > + AA_BUG(!sub); > + > + if (sub == set) > + return true; > + > + do { > + p = __aa_label_next_not_in_set(&i, set, sub); > + if (p && !profile_unconfined(p)) > + break; > + } while (p); > + > + return p == NULL; > +} > > > /** > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 10/8/19 7:55 AM, Tyler Hicks wrote: > On 2019-10-03 12:14:35, John Johansen wrote: >> The subset test is not taking into account the unconfined exception >> which will cause profile transitions in the stacked confinement >> case to fail when no_new_privs is applied. >> >> This fixes a regression introduced in the 4.17 kernel caused by the >> reworking of domain transitions. >> >> Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp") >> BugLink: https://bugs.launchpad.net/bugs/1844186 >> Signed-off-by: John Johansen <john.johansen@canonical.com> >> --- >> security/apparmor/domain.c | 8 ++++---- >> security/apparmor/include/label.h | 1 + >> security/apparmor/label.c | 33 +++++++++++++++++++++++++++++++ >> 3 files changed, 38 insertions(+), 4 deletions(-) >> >> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c >> index 9e0492795267..776e8f76aef3 100644 >> --- a/security/apparmor/domain.c >> +++ b/security/apparmor/domain.c >> @@ -931,7 +931,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) >> * aways results in a further reduction of permissions. >> */ >> if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) && >> - !unconfined(label) && !aa_label_is_subset(new, ctx->nnp)) { >> + !unconfined(label) && !aa_label_is_unconfined_subset(new, ctx->nnp)) { >> error = -EPERM; >> info = "no new privs"; >> goto audit; >> @@ -1209,7 +1209,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) >> * reduce restrictions. >> */ >> if (task_no_new_privs(current) && !unconfined(label) && >> - !aa_label_is_subset(new, ctx->nnp)) { >> + !aa_label_is_unconfined_subset(new, ctx->nnp)) { >> /* not an apparmor denial per se, so don't log it */ >> AA_DEBUG("no_new_privs - change_hat denied"); >> error = -EPERM; >> @@ -1230,7 +1230,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) >> * reduce restrictions. >> */ >> if (task_no_new_privs(current) && !unconfined(label) && >> - !aa_label_is_subset(previous, ctx->nnp)) { >> + !aa_label_is_unconfined_subset(previous, ctx->nnp)) { >> /* not an apparmor denial per se, so don't log it */ >> AA_DEBUG("no_new_privs - change_hat denied"); >> error = -EPERM; >> @@ -1426,7 +1426,7 @@ int aa_change_profile(const char *fqname, int flags) >> * reduce restrictions. >> */ >> if (task_no_new_privs(current) && !unconfined(label) && >> - !aa_label_is_subset(new, ctx->nnp)) { >> + !aa_label_is_unconfined_subset(new, ctx->nnp)) { >> /* not an apparmor denial per se, so don't log it */ >> AA_DEBUG("no_new_privs - change_hat denied"); >> error = -EPERM; >> diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h >> index 56be4412d84e..b1aeb282ca22 100644 >> --- a/security/apparmor/include/label.h >> +++ b/security/apparmor/include/label.h >> @@ -281,6 +281,7 @@ bool aa_label_init(struct aa_label *label, int size); >> struct aa_label *aa_label_alloc(int size, struct aa_proxy *proxy, gfp_t gfp); >> >> bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub); >> +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub); >> struct aa_profile *__aa_label_next_not_in_set(struct label_it *I, >> struct aa_label *set, >> struct aa_label *sub); >> diff --git a/security/apparmor/label.c b/security/apparmor/label.c >> index 3fa3e56f0e05..d94cf3d10877 100644 >> --- a/security/apparmor/label.c >> +++ b/security/apparmor/label.c >> @@ -551,6 +551,39 @@ bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub) >> return __aa_label_next_not_in_set(&i, set, sub) == NULL; >> } >> >> +/** >> + * aa_label_is_unconfined_subset - test if @sub is a subset of @set >> + * @set: label to test against >> + * @sub: label to test if is subset of @set >> + * >> + * This checks for subset but taking into account unconfined. IF >> + * @sub contains an unconfined profile that does not have a matching >> + * unconfined in @set then this will not cause the test to fail. > > Something caused me to take another look at this patch and now I'm > wondering why it is safe, when considering transitions where NNP is set, > for @sub to contain an unconfined profile that's not present in @set. > Doesn't that violate NNP? > No. sub is the set of profiles being enforced at the time nnp was requested. The unconfined exception is that transitions from unconfined to another profile are guaranteed to not increase privileges so they were allowed. This has existed since the beginning of nnp. so unconfined -> some confined P the issue that we are trying to fix here is the stacking case unconfined//&P -> P or unconfined//&P1 -> P2//&P1 the simple if (unconfined) test we use to do won't work on the stack. And while doing that simple test in the profile permission callback works for the unconfined exception, doesn't work for the label as a whole because it ends up blocking things that would be allowed when examined wholistically. For the purposes of this particular subset test we can just ignore unconfined because unconfined//&P1 == P1 the reason we don't just drop unconfined from the nnp set profile build is its a bigger patch, and we have to deal with the case where the nnp subset is empty. We in fact can always enforce the nnp restriction through stacking. That is get rid of the subset test and always merge the nnp subset against the new label. Doing so would give us the most flexibility to allow profile transitions under nnp, but to handle it correctly we need to track not just the nnp subset, but the transition set with the nnp subset merged in. Which again is a bigger patch >> + * Conversely we don't care about an unconfined in @set that is not in >> + * @sub > > I agree that this should be safe. However, I don't see where this > condition is special-cased in the code. If @set contains an unconfined > profile that's not present in @sub, I would think that > __aa_label_next_not_in_set() would just skip over that profile. > yep, just wanted to note that it didn't matter because at first I was thinking we would have to deal with that one as well. if anyone else is following along the logic is The reverse case where the profile set contains an unconfined that nnp doesn't is also okay due to the nature of stacking and the subset test. That is if we have nnp subset of P and the confinement set unconfined//&P P must exist in the current confinement set otherwise the subset test will fail, and P existing in the confinement stack restricts unconfined so that it doesn't have new permissions. > Tyler > >> + * >> + * Returns: true if @sub is special_subset of @set >> + * else false >> + */ >> +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub) >> +{ >> + struct label_it i = { }; >> + struct aa_profile *p; >> + >> + AA_BUG(!set); >> + AA_BUG(!sub); >> + >> + if (sub == set) >> + return true; >> + >> + do { >> + p = __aa_label_next_not_in_set(&i, set, sub); >> + if (p && !profile_unconfined(p)) >> + break; >> + } while (p); >> + >> + return p == NULL; >> +} >> >> >> /** >> -- >> 2.17.1 >> >> >> -- >> kernel-team mailing list >> kernel-team@lists.ubuntu.com >> https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 2019-10-08 10:14:29, John Johansen wrote: > On 10/8/19 7:55 AM, Tyler Hicks wrote: > > On 2019-10-03 12:14:35, John Johansen wrote: > >> The subset test is not taking into account the unconfined exception > >> which will cause profile transitions in the stacked confinement > >> case to fail when no_new_privs is applied. > >> > >> This fixes a regression introduced in the 4.17 kernel caused by the > >> reworking of domain transitions. > >> > >> Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp") > >> BugLink: https://bugs.launchpad.net/bugs/1844186 > >> Signed-off-by: John Johansen <john.johansen@canonical.com> > >> --- > >> security/apparmor/domain.c | 8 ++++---- > >> security/apparmor/include/label.h | 1 + > >> security/apparmor/label.c | 33 +++++++++++++++++++++++++++++++ > >> 3 files changed, 38 insertions(+), 4 deletions(-) > >> > >> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c > >> index 9e0492795267..776e8f76aef3 100644 > >> --- a/security/apparmor/domain.c > >> +++ b/security/apparmor/domain.c > >> @@ -931,7 +931,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) > >> * aways results in a further reduction of permissions. > >> */ > >> if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) && > >> - !unconfined(label) && !aa_label_is_subset(new, ctx->nnp)) { > >> + !unconfined(label) && !aa_label_is_unconfined_subset(new, ctx->nnp)) { > >> error = -EPERM; > >> info = "no new privs"; > >> goto audit; > >> @@ -1209,7 +1209,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) > >> * reduce restrictions. > >> */ > >> if (task_no_new_privs(current) && !unconfined(label) && > >> - !aa_label_is_subset(new, ctx->nnp)) { > >> + !aa_label_is_unconfined_subset(new, ctx->nnp)) { > >> /* not an apparmor denial per se, so don't log it */ > >> AA_DEBUG("no_new_privs - change_hat denied"); > >> error = -EPERM; > >> @@ -1230,7 +1230,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) > >> * reduce restrictions. > >> */ > >> if (task_no_new_privs(current) && !unconfined(label) && > >> - !aa_label_is_subset(previous, ctx->nnp)) { > >> + !aa_label_is_unconfined_subset(previous, ctx->nnp)) { > >> /* not an apparmor denial per se, so don't log it */ > >> AA_DEBUG("no_new_privs - change_hat denied"); > >> error = -EPERM; > >> @@ -1426,7 +1426,7 @@ int aa_change_profile(const char *fqname, int flags) > >> * reduce restrictions. > >> */ > >> if (task_no_new_privs(current) && !unconfined(label) && > >> - !aa_label_is_subset(new, ctx->nnp)) { > >> + !aa_label_is_unconfined_subset(new, ctx->nnp)) { > >> /* not an apparmor denial per se, so don't log it */ > >> AA_DEBUG("no_new_privs - change_hat denied"); > >> error = -EPERM; > >> diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h > >> index 56be4412d84e..b1aeb282ca22 100644 > >> --- a/security/apparmor/include/label.h > >> +++ b/security/apparmor/include/label.h > >> @@ -281,6 +281,7 @@ bool aa_label_init(struct aa_label *label, int size); > >> struct aa_label *aa_label_alloc(int size, struct aa_proxy *proxy, gfp_t gfp); > >> > >> bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub); > >> +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub); > >> struct aa_profile *__aa_label_next_not_in_set(struct label_it *I, > >> struct aa_label *set, > >> struct aa_label *sub); > >> diff --git a/security/apparmor/label.c b/security/apparmor/label.c > >> index 3fa3e56f0e05..d94cf3d10877 100644 > >> --- a/security/apparmor/label.c > >> +++ b/security/apparmor/label.c > >> @@ -551,6 +551,39 @@ bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub) > >> return __aa_label_next_not_in_set(&i, set, sub) == NULL; > >> } > >> > >> +/** > >> + * aa_label_is_unconfined_subset - test if @sub is a subset of @set > >> + * @set: label to test against > >> + * @sub: label to test if is subset of @set > >> + * > >> + * This checks for subset but taking into account unconfined. IF > >> + * @sub contains an unconfined profile that does not have a matching > >> + * unconfined in @set then this will not cause the test to fail. > > > > Something caused me to take another look at this patch and now I'm > > wondering why it is safe, when considering transitions where NNP is set, > > for @sub to contain an unconfined profile that's not present in @set. > > Doesn't that violate NNP? > > > > No. sub is the set of profiles being enforced at the time nnp was requested. Bah, this is what I had gotten backwards when I took a second look earlier this morning. I mistakenly thought @set was the initial set when NNP was flicked on. Thanks for clarifying. Tyler > The unconfined exception is that transitions from unconfined to another > profile are guaranteed to not increase privileges so they were allowed. > This has existed since the beginning of nnp. so > > unconfined -> some confined P > > the issue that we are trying to fix here is the stacking case > > unconfined//&P -> P > or > unconfined//&P1 -> P2//&P1 > > the simple if (unconfined) test we use to do won't work on the stack. And > while doing that simple test in the profile permission callback works > for the unconfined exception, doesn't work for the label as a whole because > it ends up blocking things that would be allowed when examined wholistically. > > For the purposes of this particular subset test we can just ignore unconfined > because > > unconfined//&P1 == P1 > > the reason we don't just drop unconfined from the nnp set profile build > is its a bigger patch, and we have to deal with the case where the nnp > subset is empty. > > We in fact can always enforce the nnp restriction through stacking. That > is get rid of the subset test and always merge the nnp subset against > the new label. Doing so would give us the most flexibility to allow > profile transitions under nnp, but to handle it correctly we need to > track not just the nnp subset, but the transition set with the nnp > subset merged in. Which again is a bigger patch > > >> + * Conversely we don't care about an unconfined in @set that is not in > >> + * @sub > > > > I agree that this should be safe. However, I don't see where this > > condition is special-cased in the code. If @set contains an unconfined > > profile that's not present in @sub, I would think that > > __aa_label_next_not_in_set() would just skip over that profile. > > > > yep, just wanted to note that it didn't matter because at first I > was thinking we would have to deal with that one as well. > > if anyone else is following along the logic is > > The reverse case where the profile set contains an unconfined that nnp > doesn't is also okay due to the nature of stacking and the subset test. > That is if we have > > nnp subset of P and the confinement set unconfined//&P > > P must exist in the current confinement set otherwise the subset test will > fail, and P existing in the confinement stack restricts unconfined so that > it doesn't have new permissions. > > > > Tyler > > > >> + * > >> + * Returns: true if @sub is special_subset of @set > >> + * else false > >> + */ > >> +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub) > >> +{ > >> + struct label_it i = { }; > >> + struct aa_profile *p; > >> + > >> + AA_BUG(!set); > >> + AA_BUG(!sub); > >> + > >> + if (sub == set) > >> + return true; > >> + > >> + do { > >> + p = __aa_label_next_not_in_set(&i, set, sub); > >> + if (p && !profile_unconfined(p)) > >> + break; > >> + } while (p); > >> + > >> + return p == NULL; > >> +} > >> > >> > >> /** > >> -- > >> 2.17.1 > >> > >> > >> -- > >> kernel-team mailing list > >> kernel-team@lists.ubuntu.com > >> https://lists.ubuntu.com/mailman/listinfo/kernel-team >
On 03.10.19 21:14, John Johansen wrote: > The subset test is not taking into account the unconfined exception > which will cause profile transitions in the stacked confinement > case to fail when no_new_privs is applied. > > This fixes a regression introduced in the 4.17 kernel caused by the > reworking of domain transitions. > > Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp") > BugLink: https://bugs.launchpad.net/bugs/1844186 > Signed-off-by: John Johansen <john.johansen@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- With some more background info it is still mind bending due to the many levels of inversion in the logic. Like aa_subset_is_unconfined_subset() which takes quite some time to parse. I think in the end it is a "subset does not add further restrictions". This is tested with !confined. Then that result is taken inverted again to fail when switching profiles. So in the end its a "allow additional unconfined profiles" thing. -Stefan > security/apparmor/domain.c | 8 ++++---- > security/apparmor/include/label.h | 1 + > security/apparmor/label.c | 33 +++++++++++++++++++++++++++++++ > 3 files changed, 38 insertions(+), 4 deletions(-) > > diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c > index 9e0492795267..776e8f76aef3 100644 > --- a/security/apparmor/domain.c > +++ b/security/apparmor/domain.c > @@ -931,7 +931,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) > * aways results in a further reduction of permissions. > */ > if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) && > - !unconfined(label) && !aa_label_is_subset(new, ctx->nnp)) { > + !unconfined(label) && !aa_label_is_unconfined_subset(new, ctx->nnp)) { > error = -EPERM; > info = "no new privs"; > goto audit; > @@ -1209,7 +1209,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(new, ctx->nnp)) { > + !aa_label_is_unconfined_subset(new, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > @@ -1230,7 +1230,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(previous, ctx->nnp)) { > + !aa_label_is_unconfined_subset(previous, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > @@ -1426,7 +1426,7 @@ int aa_change_profile(const char *fqname, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(new, ctx->nnp)) { > + !aa_label_is_unconfined_subset(new, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h > index 56be4412d84e..b1aeb282ca22 100644 > --- a/security/apparmor/include/label.h > +++ b/security/apparmor/include/label.h > @@ -281,6 +281,7 @@ bool aa_label_init(struct aa_label *label, int size); > struct aa_label *aa_label_alloc(int size, struct aa_proxy *proxy, gfp_t gfp); > > bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub); > +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub); > struct aa_profile *__aa_label_next_not_in_set(struct label_it *I, > struct aa_label *set, > struct aa_label *sub); > diff --git a/security/apparmor/label.c b/security/apparmor/label.c > index 3fa3e56f0e05..d94cf3d10877 100644 > --- a/security/apparmor/label.c > +++ b/security/apparmor/label.c > @@ -551,6 +551,39 @@ bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub) > return __aa_label_next_not_in_set(&i, set, sub) == NULL; > } > > +/** > + * aa_label_is_unconfined_subset - test if @sub is a subset of @set > + * @set: label to test against > + * @sub: label to test if is subset of @set > + * > + * This checks for subset but taking into account unconfined. IF > + * @sub contains an unconfined profile that does not have a matching > + * unconfined in @set then this will not cause the test to fail. > + * Conversely we don't care about an unconfined in @set that is not in > + * @sub > + * > + * Returns: true if @sub is special_subset of @set > + * else false > + */ > +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub) > +{ > + struct label_it i = { }; > + struct aa_profile *p; > + > + AA_BUG(!set); > + AA_BUG(!sub); > + > + if (sub == set) > + return true; > + > + do { > + p = __aa_label_next_not_in_set(&i, set, sub); > + if (p && !profile_unconfined(p)) > + break; > + } while (p); > + > + return p == NULL; > +} > > > /** >
On 03.10.19 21:14, John Johansen wrote: > The subset test is not taking into account the unconfined exception > which will cause profile transitions in the stacked confinement > case to fail when no_new_privs is applied. > > This fixes a regression introduced in the 4.17 kernel caused by the > reworking of domain transitions. > > Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp") > BugLink: https://bugs.launchpad.net/bugs/1844186 > Signed-off-by: John Johansen <john.johansen@canonical.com> Applied to eoan/master-next branch. Thanks, Kleber > --- > security/apparmor/domain.c | 8 ++++---- > security/apparmor/include/label.h | 1 + > security/apparmor/label.c | 33 +++++++++++++++++++++++++++++++ > 3 files changed, 38 insertions(+), 4 deletions(-) > > diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c > index 9e0492795267..776e8f76aef3 100644 > --- a/security/apparmor/domain.c > +++ b/security/apparmor/domain.c > @@ -931,7 +931,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) > * aways results in a further reduction of permissions. > */ > if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) && > - !unconfined(label) && !aa_label_is_subset(new, ctx->nnp)) { > + !unconfined(label) && !aa_label_is_unconfined_subset(new, ctx->nnp)) { > error = -EPERM; > info = "no new privs"; > goto audit; > @@ -1209,7 +1209,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(new, ctx->nnp)) { > + !aa_label_is_unconfined_subset(new, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > @@ -1230,7 +1230,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(previous, ctx->nnp)) { > + !aa_label_is_unconfined_subset(previous, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > @@ -1426,7 +1426,7 @@ int aa_change_profile(const char *fqname, int flags) > * reduce restrictions. > */ > if (task_no_new_privs(current) && !unconfined(label) && > - !aa_label_is_subset(new, ctx->nnp)) { > + !aa_label_is_unconfined_subset(new, ctx->nnp)) { > /* not an apparmor denial per se, so don't log it */ > AA_DEBUG("no_new_privs - change_hat denied"); > error = -EPERM; > diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h > index 56be4412d84e..b1aeb282ca22 100644 > --- a/security/apparmor/include/label.h > +++ b/security/apparmor/include/label.h > @@ -281,6 +281,7 @@ bool aa_label_init(struct aa_label *label, int size); > struct aa_label *aa_label_alloc(int size, struct aa_proxy *proxy, gfp_t gfp); > > bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub); > +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub); > struct aa_profile *__aa_label_next_not_in_set(struct label_it *I, > struct aa_label *set, > struct aa_label *sub); > diff --git a/security/apparmor/label.c b/security/apparmor/label.c > index 3fa3e56f0e05..d94cf3d10877 100644 > --- a/security/apparmor/label.c > +++ b/security/apparmor/label.c > @@ -551,6 +551,39 @@ bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub) > return __aa_label_next_not_in_set(&i, set, sub) == NULL; > } > > +/** > + * aa_label_is_unconfined_subset - test if @sub is a subset of @set > + * @set: label to test against > + * @sub: label to test if is subset of @set > + * > + * This checks for subset but taking into account unconfined. IF > + * @sub contains an unconfined profile that does not have a matching > + * unconfined in @set then this will not cause the test to fail. > + * Conversely we don't care about an unconfined in @set that is not in > + * @sub > + * > + * Returns: true if @sub is special_subset of @set > + * else false > + */ > +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub) > +{ > + struct label_it i = { }; > + struct aa_profile *p; > + > + AA_BUG(!set); > + AA_BUG(!sub); > + > + if (sub == set) > + return true; > + > + do { > + p = __aa_label_next_not_in_set(&i, set, sub); > + if (p && !profile_unconfined(p)) > + break; > + } while (p); > + > + return p == NULL; > +} > > > /** >
On Thu, Oct 03, 2019 at 12:14:35PM -0700, John Johansen wrote: > The subset test is not taking into account the unconfined exception > which will cause profile transitions in the stacked confinement > case to fail when no_new_privs is applied. > > This fixes a regression introduced in the 4.17 kernel caused by the > reworking of domain transitions. > > Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp") > BugLink: https://bugs.launchpad.net/bugs/1844186 > Signed-off-by: John Johansen <john.johansen@canonical.com> Applied to unstable/master, thanks!
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 9e0492795267..776e8f76aef3 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -931,7 +931,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) * aways results in a further reduction of permissions. */ if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) && - !unconfined(label) && !aa_label_is_subset(new, ctx->nnp)) { + !unconfined(label) && !aa_label_is_unconfined_subset(new, ctx->nnp)) { error = -EPERM; info = "no new privs"; goto audit; @@ -1209,7 +1209,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) * reduce restrictions. */ if (task_no_new_privs(current) && !unconfined(label) && - !aa_label_is_subset(new, ctx->nnp)) { + !aa_label_is_unconfined_subset(new, ctx->nnp)) { /* not an apparmor denial per se, so don't log it */ AA_DEBUG("no_new_privs - change_hat denied"); error = -EPERM; @@ -1230,7 +1230,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) * reduce restrictions. */ if (task_no_new_privs(current) && !unconfined(label) && - !aa_label_is_subset(previous, ctx->nnp)) { + !aa_label_is_unconfined_subset(previous, ctx->nnp)) { /* not an apparmor denial per se, so don't log it */ AA_DEBUG("no_new_privs - change_hat denied"); error = -EPERM; @@ -1426,7 +1426,7 @@ int aa_change_profile(const char *fqname, int flags) * reduce restrictions. */ if (task_no_new_privs(current) && !unconfined(label) && - !aa_label_is_subset(new, ctx->nnp)) { + !aa_label_is_unconfined_subset(new, ctx->nnp)) { /* not an apparmor denial per se, so don't log it */ AA_DEBUG("no_new_privs - change_hat denied"); error = -EPERM; diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h index 56be4412d84e..b1aeb282ca22 100644 --- a/security/apparmor/include/label.h +++ b/security/apparmor/include/label.h @@ -281,6 +281,7 @@ bool aa_label_init(struct aa_label *label, int size); struct aa_label *aa_label_alloc(int size, struct aa_proxy *proxy, gfp_t gfp); bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub); +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub); struct aa_profile *__aa_label_next_not_in_set(struct label_it *I, struct aa_label *set, struct aa_label *sub); diff --git a/security/apparmor/label.c b/security/apparmor/label.c index 3fa3e56f0e05..d94cf3d10877 100644 --- a/security/apparmor/label.c +++ b/security/apparmor/label.c @@ -551,6 +551,39 @@ bool aa_label_is_subset(struct aa_label *set, struct aa_label *sub) return __aa_label_next_not_in_set(&i, set, sub) == NULL; } +/** + * aa_label_is_unconfined_subset - test if @sub is a subset of @set + * @set: label to test against + * @sub: label to test if is subset of @set + * + * This checks for subset but taking into account unconfined. IF + * @sub contains an unconfined profile that does not have a matching + * unconfined in @set then this will not cause the test to fail. + * Conversely we don't care about an unconfined in @set that is not in + * @sub + * + * Returns: true if @sub is special_subset of @set + * else false + */ +bool aa_label_is_unconfined_subset(struct aa_label *set, struct aa_label *sub) +{ + struct label_it i = { }; + struct aa_profile *p; + + AA_BUG(!set); + AA_BUG(!sub); + + if (sub == set) + return true; + + do { + p = __aa_label_next_not_in_set(&i, set, sub); + if (p && !profile_unconfined(p)) + break; + } while (p); + + return p == NULL; +} /**
The subset test is not taking into account the unconfined exception which will cause profile transitions in the stacked confinement case to fail when no_new_privs is applied. This fixes a regression introduced in the 4.17 kernel caused by the reworking of domain transitions. Fixes: 9fcf78cca1986 ("apparmor: update domain transitions that are subsets of confinement at nnp") BugLink: https://bugs.launchpad.net/bugs/1844186 Signed-off-by: John Johansen <john.johansen@canonical.com> --- security/apparmor/domain.c | 8 ++++---- security/apparmor/include/label.h | 1 + security/apparmor/label.c | 33 +++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 4 deletions(-)