[PATCH][eoan] UBUNTU: SAUCE: apparmor: fix nnp subset test for unconfined
Tyler Hicks
tyhicks at canonical.com
Tue Oct 8 17:21:44 UTC 2019
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 at 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 at lists.ubuntu.com
> >> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
More information about the kernel-team
mailing list