| Message ID | 9e3f99d3c2cbfee24fb3af7ec9b4cf93e4430112.1503459890.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Richard Guy Briggs (rgb@redhat.com): > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > negation of is_subset() easier to read and analyse. > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > security/commoncap.c | 16 ++++++++++------ > 1 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/security/commoncap.c b/security/commoncap.c > index b7fbf77..6f05ec0 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec > *effective = true; > } > It's subjective and so might be just me, but I think I'd find it easier to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target) This looks correct though, so either way Reviewed-by: Serge Hallyn <serge@hallyn.com> > +#define cap_gained(field, target, source) \ > + !cap_issubset(target->cap_##field, source->cap_##field) > +#define cap_grew(target, source, cred) \ > + !cap_issubset(cred->cap_##target, cred->cap_##source) > +#define cap_full(field, cred) \ > + cap_issubset(CAP_FULL_SET, cred->cap_##field) > /** > * cap_bprm_set_creds - Set up the proposed credentials for execve(). > * @bprm: The execution parameters, including the proposed creds > @@ -541,10 +547,9 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) > handle_privileged_root(bprm, has_cap, &effective, root_uid); > > /* if we have fs caps, clear dangerous personality flags */ > - if (!cap_issubset(new->cap_permitted, old->cap_permitted)) > + if (cap_gained(permitted, new, old)) > bprm->per_clear |= PER_CLEAR_ON_SETID; > > - > /* Don't let someone trace a set[ug]id/setpcap binary with the revised > * credentials unless they have the appropriate permit. > * > @@ -552,8 +557,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) > */ > is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid); > > - if ((is_setid || > - !cap_issubset(new->cap_permitted, old->cap_permitted)) && > + if ((is_setid || cap_gained(permitted, new, old)) && > ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) || > !ptracer_capable(current, new->user_ns))) { > /* downgrade; they get no more than they had, and maybe less */ > @@ -605,8 +609,8 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) > * Number 1 above might fail if you don't have a full bset, but I think > * that is interesting information to audit. > */ > - if (!cap_issubset(new->cap_effective, new->cap_ambient)) { > - if (!cap_issubset(CAP_FULL_SET, new->cap_effective) || > + if (cap_grew(effective, ambient, new)) { > + if (!cap_full(effective, new) || > !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || > issecure(SECURE_NOROOT)) { > ret = audit_log_bprm_fcaps(bprm, new, old); > -- > 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-08-24 11:03, Serge E. Hallyn wrote: > Quoting Richard Guy Briggs (rgb@redhat.com): > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > > negation of is_subset() easier to read and analyse. > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > security/commoncap.c | 16 ++++++++++------ > > 1 files changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > index b7fbf77..6f05ec0 100644 > > --- a/security/commoncap.c > > +++ b/security/commoncap.c > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec > > *effective = true; > > } > > > > It's subjective and so might be just me, but I think I'd find it easier > to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target) In more than one place, I wanted to put the parameter that I was trying to read aloud closest to the function name to make reading it flow better, leaving the parameters less critical to comprehension towards the end. > This looks correct though, so either way > > Reviewed-by: Serge Hallyn <serge@hallyn.com> Thanks. Did you want to put this through, or send it through Paul's audit tree? > > +#define cap_gained(field, target, source) \ > > + !cap_issubset(target->cap_##field, source->cap_##field) > > +#define cap_grew(target, source, cred) \ > > + !cap_issubset(cred->cap_##target, cred->cap_##source) > > +#define cap_full(field, cred) \ > > + cap_issubset(CAP_FULL_SET, cred->cap_##field) > > /** > > * cap_bprm_set_creds - Set up the proposed credentials for execve(). > > * @bprm: The execution parameters, including the proposed creds > > @@ -541,10 +547,9 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) > > handle_privileged_root(bprm, has_cap, &effective, root_uid); > > > > /* if we have fs caps, clear dangerous personality flags */ > > - if (!cap_issubset(new->cap_permitted, old->cap_permitted)) > > + if (cap_gained(permitted, new, old)) > > bprm->per_clear |= PER_CLEAR_ON_SETID; > > > > - > > /* Don't let someone trace a set[ug]id/setpcap binary with the revised > > * credentials unless they have the appropriate permit. > > * > > @@ -552,8 +557,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) > > */ > > is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid); > > > > - if ((is_setid || > > - !cap_issubset(new->cap_permitted, old->cap_permitted)) && > > + if ((is_setid || cap_gained(permitted, new, old)) && > > ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) || > > !ptracer_capable(current, new->user_ns))) { > > /* downgrade; they get no more than they had, and maybe less */ > > @@ -605,8 +609,8 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) > > * Number 1 above might fail if you don't have a full bset, but I think > > * that is interesting information to audit. > > */ > > - if (!cap_issubset(new->cap_effective, new->cap_ambient)) { > > - if (!cap_issubset(CAP_FULL_SET, new->cap_effective) || > > + if (cap_grew(effective, ambient, new)) { > > + if (!cap_full(effective, new) || > > !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || > > issecure(SECURE_NOROOT)) { > > ret = audit_log_bprm_fcaps(bprm, new, old); > > -- > > 1.7.1 - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Richard Guy Briggs (rgb@redhat.com): > On 2017-08-24 11:03, Serge E. Hallyn wrote: > > Quoting Richard Guy Briggs (rgb@redhat.com): > > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > > > negation of is_subset() easier to read and analyse. > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > --- > > > security/commoncap.c | 16 ++++++++++------ > > > 1 files changed, 10 insertions(+), 6 deletions(-) > > > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > > index b7fbf77..6f05ec0 100644 > > > --- a/security/commoncap.c > > > +++ b/security/commoncap.c > > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec > > > *effective = true; > > > } > > > > > > > It's subjective and so might be just me, but I think I'd find it easier > > to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target) > > In more than one place, I wanted to put the parameter that I was trying > to read aloud closest to the function name to make reading it flow > better, leaving the parameters less critical to comprehension towards > the end. And I see that in the final patch it looks nicer the way you have it. > > This looks correct though, so either way > > > > Reviewed-by: Serge Hallyn <serge@hallyn.com> > > Thanks. Did you want to put this through, or send it through Paul's > audit tree? If Paul's around I'm happy to have it go through his tree. thanks, -serge -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 24, 2017 at 9:37 AM, Serge E. Hallyn <serge@hallyn.com> wrote: > Quoting Richard Guy Briggs (rgb@redhat.com): >> On 2017-08-24 11:03, Serge E. Hallyn wrote: >> > Quoting Richard Guy Briggs (rgb@redhat.com): >> > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the >> > > negation of is_subset() easier to read and analyse. >> > > >> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> >> > > --- >> > > security/commoncap.c | 16 ++++++++++------ >> > > 1 files changed, 10 insertions(+), 6 deletions(-) >> > > >> > > diff --git a/security/commoncap.c b/security/commoncap.c >> > > index b7fbf77..6f05ec0 100644 >> > > --- a/security/commoncap.c >> > > +++ b/security/commoncap.c >> > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec >> > > *effective = true; >> > > } >> > > >> > >> > It's subjective and so might be just me, but I think I'd find it easier >> > to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target) >> >> In more than one place, I wanted to put the parameter that I was trying >> to read aloud closest to the function name to make reading it flow >> better, leaving the parameters less critical to comprehension towards >> the end. > > And I see that in the final patch it looks nicer the way you have it. > >> > This looks correct though, so either way >> > >> > Reviewed-by: Serge Hallyn <serge@hallyn.com> >> >> Thanks. Did you want to put this through, or send it through Paul's >> audit tree? > > If Paul's around I'm happy to have it go through his tree. Is this series based against -next with the changes that touch commoncap.c? Also, did you validate this with the existing LTP tests and selftests? https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283 -Kees
On Thu, Aug 24, 2017 at 3:06 PM, Kees Cook <keescook@chromium.org> wrote: > On Thu, Aug 24, 2017 at 9:37 AM, Serge E. Hallyn <serge@hallyn.com> wrote: >> Quoting Richard Guy Briggs (rgb@redhat.com): >>> On 2017-08-24 11:03, Serge E. Hallyn wrote: >>> > Quoting Richard Guy Briggs (rgb@redhat.com): >>> > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the >>> > > negation of is_subset() easier to read and analyse. >>> > > >>> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> >>> > > --- >>> > > security/commoncap.c | 16 ++++++++++------ >>> > > 1 files changed, 10 insertions(+), 6 deletions(-) >>> > > >>> > > diff --git a/security/commoncap.c b/security/commoncap.c >>> > > index b7fbf77..6f05ec0 100644 >>> > > --- a/security/commoncap.c >>> > > +++ b/security/commoncap.c >>> > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec >>> > > *effective = true; >>> > > } >>> > > >>> > >>> > It's subjective and so might be just me, but I think I'd find it easier >>> > to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target) >>> >>> In more than one place, I wanted to put the parameter that I was trying >>> to read aloud closest to the function name to make reading it flow >>> better, leaving the parameters less critical to comprehension towards >>> the end. >> >> And I see that in the final patch it looks nicer the way you have it. >> >>> > This looks correct though, so either way >>> > >>> > Reviewed-by: Serge Hallyn <serge@hallyn.com> >>> >>> Thanks. Did you want to put this through, or send it through Paul's >>> audit tree? >> >> If Paul's around I'm happy to have it go through his tree. Since Serge is okay with these I'll take a closer look and if it all looks good I can pull it in to the audit tree (no objections from me on the last revision, although I remember it being much smaller). That said, since we are already at -rc6, I'm going to defer merging this into audit/next *after* the upcoming merge window. We are right at where I normally draw the line and considering the scope and nature of this patchset I think having a full RC cycle in linux-next would be a good thing. > Is this series based against -next with the changes that touch commoncap.c? > > Also, did you validate this with the existing LTP tests and selftests? > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283 Another reason for keeping this in the queue a bit longer. Richard, can you do this testing before the upcoming merge window closes?
On Wed, 23 Aug 2017, Richard Guy Briggs wrote: > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > negation of is_subset() easier to read and analyse. > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > security/commoncap.c | 16 ++++++++++------ > 1 files changed, 10 insertions(+), 6 deletions(-) Acked-by: James Morris <james.l.morris@oracle.com>
On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > negation of is_subset() easier to read and analyse. > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > security/commoncap.c | 16 ++++++++++------ > 1 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/security/commoncap.c b/security/commoncap.c > index b7fbf77..6f05ec0 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec > *effective = true; > } > > +#define cap_gained(field, target, source) \ > + !cap_issubset(target->cap_##field, source->cap_##field) > +#define cap_grew(target, source, cred) \ > + !cap_issubset(cred->cap_##target, cred->cap_##source) > +#define cap_full(field, cred) \ > + cap_issubset(CAP_FULL_SET, cred->cap_##field) I read this several times and I can't figure out what "gain" means, what "grew" means, and why it's more intuitive that they signify weird permutations of the parameters. I also don't know what "source" and "target" mean in this context. Can you clearly articulate how this is better than the status quo? --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Andy Lutomirski (luto@kernel.org): > On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > > negation of is_subset() easier to read and analyse. > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > security/commoncap.c | 16 ++++++++++------ > > 1 files changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > index b7fbf77..6f05ec0 100644 > > --- a/security/commoncap.c > > +++ b/security/commoncap.c > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec > > *effective = true; > > } > > > > +#define cap_gained(field, target, source) \ > > + !cap_issubset(target->cap_##field, source->cap_##field) > > +#define cap_grew(target, source, cred) \ > > + !cap_issubset(cred->cap_##target, cred->cap_##source) > > +#define cap_full(field, cred) \ > > + cap_issubset(CAP_FULL_SET, cred->cap_##field) > > I read this several times and I can't figure out what "gain" means, > what "grew" means, and why it's more intuitive that they signify weird gain means the new set has bits which the old didn't. It's clearer precisely because it describes the intent rather than the mechanism. > permutations of the parameters. I also don't know what "source" and > "target" mean in this context. Source and target might be better named 'old' and 'new' or something like that, though I didn't found source and target to be confusing. > Can you clearly articulate how this is better than the status quo? Yes, it describes what we're checking for, making the code more self-documenting. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-08-24 12:06, Kees Cook wrote: > On Thu, Aug 24, 2017 at 9:37 AM, Serge E. Hallyn <serge@hallyn.com> wrote: > > Quoting Richard Guy Briggs (rgb@redhat.com): > >> On 2017-08-24 11:03, Serge E. Hallyn wrote: > >> > Quoting Richard Guy Briggs (rgb@redhat.com): > >> > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > >> > > negation of is_subset() easier to read and analyse. > >> > > > >> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > >> > > --- > >> > > security/commoncap.c | 16 ++++++++++------ > >> > > 1 files changed, 10 insertions(+), 6 deletions(-) > >> > > > >> > > diff --git a/security/commoncap.c b/security/commoncap.c > >> > > index b7fbf77..6f05ec0 100644 > >> > > --- a/security/commoncap.c > >> > > +++ b/security/commoncap.c > >> > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec > >> > > *effective = true; > >> > > } > >> > > > >> > > >> > It's subjective and so might be just me, but I think I'd find it easier > >> > to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target) > >> > >> In more than one place, I wanted to put the parameter that I was trying > >> to read aloud closest to the function name to make reading it flow > >> better, leaving the parameters less critical to comprehension towards > >> the end. > > > > And I see that in the final patch it looks nicer the way you have it. > > > >> > This looks correct though, so either way > >> > > >> > Reviewed-by: Serge Hallyn <serge@hallyn.com> > >> > >> Thanks. Did you want to put this through, or send it through Paul's > >> audit tree? > > > > If Paul's around I'm happy to have it go through his tree. > > Is this series based against -next with the changes that touch commoncap.c? This series is against pcmoore's audit/next tree (I know I'm missing two commits but they pose no conflict.). Which -next tree are you talking about? I might guess linux-security/next or linux-next/master (I have at least a dozen "next" in my git repo config.) I did eventually find your patches in sfr's tree and in your for-next/kspp branch. I'll have a look at the commoncap.c changes including the elimination of cap_effective. > Also, did you validate this with the existing LTP tests and selftests? > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283 No. I will look into doing that. Thanks for the suggestion. I see that bprm->cap_effective has vanished, so that will affect at least one hunk. > Kees Cook - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-08-28 05:19, Richard Guy Briggs wrote: > On 2017-08-24 12:06, Kees Cook wrote: > > On Thu, Aug 24, 2017 at 9:37 AM, Serge E. Hallyn <serge@hallyn.com> wrote: > > > Quoting Richard Guy Briggs (rgb@redhat.com): > > >> On 2017-08-24 11:03, Serge E. Hallyn wrote: > > >> > Quoting Richard Guy Briggs (rgb@redhat.com): > > >> > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > > >> > > negation of is_subset() easier to read and analyse. > > >> > > > > >> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > >> > > --- > > >> > > security/commoncap.c | 16 ++++++++++------ > > >> > > 1 files changed, 10 insertions(+), 6 deletions(-) > > >> > > > > >> > > diff --git a/security/commoncap.c b/security/commoncap.c > > >> > > index b7fbf77..6f05ec0 100644 > > >> > > --- a/security/commoncap.c > > >> > > +++ b/security/commoncap.c > > >> > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec > > >> > > *effective = true; > > >> > > } > > >> > > > > >> > > > >> > It's subjective and so might be just me, but I think I'd find it easier > > >> > to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target) > > >> > > >> In more than one place, I wanted to put the parameter that I was trying > > >> to read aloud closest to the function name to make reading it flow > > >> better, leaving the parameters less critical to comprehension towards > > >> the end. > > > > > > And I see that in the final patch it looks nicer the way you have it. > > > > > >> > This looks correct though, so either way > > >> > > > >> > Reviewed-by: Serge Hallyn <serge@hallyn.com> > > >> > > >> Thanks. Did you want to put this through, or send it through Paul's > > >> audit tree? > > > > > > If Paul's around I'm happy to have it go through his tree. > > > > Is this series based against -next with the changes that touch commoncap.c? > > This series is against pcmoore's audit/next tree (I know I'm missing two > commits but they pose no conflict.). > > Which -next tree are you talking about? I might guess > linux-security/next or linux-next/master (I have at least a dozen "next" > in my git repo config.) > > I did eventually find your patches in sfr's tree and in your for-next/kspp branch. > > I'll have a look at the commoncap.c changes including the elimination of cap_effective. > > > Also, did you validate this with the existing LTP tests and selftests? > > > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283 > > No. I will look into doing that. Thanks for the suggestion. > > I see that bprm->cap_effective has vanished, so that will affect at least one hunk. And I spoke too soon and didn't notice this was in the bprm struct and not the cred struct, so I think were're fine there. I'll go ahead and rebase on your commoncap.c changes as well as run the ltp and kernel self-test validation tests. > > Kees Cook > > - RGB - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-08-28 07:08, Richard Guy Briggs wrote: > On 2017-08-28 05:19, Richard Guy Briggs wrote: > > On 2017-08-24 12:06, Kees Cook wrote: > > > On Thu, Aug 24, 2017 at 9:37 AM, Serge E. Hallyn <serge@hallyn.com> wrote: > > > > Quoting Richard Guy Briggs (rgb@redhat.com): > > > >> On 2017-08-24 11:03, Serge E. Hallyn wrote: > > > >> > Quoting Richard Guy Briggs (rgb@redhat.com): > > > >> > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > > > >> > > negation of is_subset() easier to read and analyse. > > > >> > > > > > >> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > >> > > --- > > > >> > > security/commoncap.c | 16 ++++++++++------ > > > >> > > 1 files changed, 10 insertions(+), 6 deletions(-) > > > >> > > > > > >> > > diff --git a/security/commoncap.c b/security/commoncap.c > > > >> > > index b7fbf77..6f05ec0 100644 > > > >> > > --- a/security/commoncap.c > > > >> > > +++ b/security/commoncap.c > > > >> > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec > > > >> > > *effective = true; > > > >> > > } > > > >> > > > > > >> > > > > >> > It's subjective and so might be just me, but I think I'd find it easier > > > >> > to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target) > > > >> > > > >> In more than one place, I wanted to put the parameter that I was trying > > > >> to read aloud closest to the function name to make reading it flow > > > >> better, leaving the parameters less critical to comprehension towards > > > >> the end. > > > > > > > > And I see that in the final patch it looks nicer the way you have it. > > > > > > > >> > This looks correct though, so either way > > > >> > > > > >> > Reviewed-by: Serge Hallyn <serge@hallyn.com> > > > >> > > > >> Thanks. Did you want to put this through, or send it through Paul's > > > >> audit tree? > > > > > > > > If Paul's around I'm happy to have it go through his tree. > > > > > > Is this series based against -next with the changes that touch commoncap.c? > > > > This series is against pcmoore's audit/next tree (I know I'm missing two > > commits but they pose no conflict.). > > > > Which -next tree are you talking about? I might guess > > linux-security/next or linux-next/master (I have at least a dozen "next" > > in my git repo config.) > > > > I did eventually find your patches in sfr's tree and in your for-next/kspp branch. > > > > I'll have a look at the commoncap.c changes including the elimination of cap_effective. > > > > > Also, did you validate this with the existing LTP tests and selftests? > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283 > > > > No. I will look into doing that. Thanks for the suggestion. Ok, I'm running the kernel self-test make TARGETS="capabilities" kselftest and getting a good way through it and then hit this on an unmodified kernel: [RUN] +++ Tests with uid != 0 +++ [NOTE] Using global UIDs for tests [OK] Child succeeded test_execve: chdir to private tmpfs: Permission denied [FAIL] Child failed selftests: test_execve [FAIL] Is this a known limitation or have I got something weird in my runtime environment that is killing it at this part of the test? > > I see that bprm->cap_effective has vanished, so that will affect at least one hunk. > > And I spoke too soon and didn't notice this was in the bprm struct and > not the cred struct, so I think were're fine there. I'll go ahead and > rebase on your commoncap.c changes as well as run the ltp and kernel > self-test validation tests. > > > > Kees Cook > > > > - RGB > > - RGB - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 01, 2017 at 06:18:43AM -0400, Richard Guy Briggs wrote: > On 2017-08-28 07:08, Richard Guy Briggs wrote: > > On 2017-08-28 05:19, Richard Guy Briggs wrote: > > > On 2017-08-24 12:06, Kees Cook wrote: > > > > On Thu, Aug 24, 2017 at 9:37 AM, Serge E. Hallyn <serge@hallyn.com> wrote: > > > > > Quoting Richard Guy Briggs (rgb@redhat.com): > > > > >> On 2017-08-24 11:03, Serge E. Hallyn wrote: > > > > >> > Quoting Richard Guy Briggs (rgb@redhat.com): > > > > >> > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > > > > >> > > negation of is_subset() easier to read and analyse. > > > > >> > > > > > > >> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > >> > > --- > > > > >> > > security/commoncap.c | 16 ++++++++++------ > > > > >> > > 1 files changed, 10 insertions(+), 6 deletions(-) > > > > >> > > > > > > >> > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > >> > > index b7fbf77..6f05ec0 100644 > > > > >> > > --- a/security/commoncap.c > > > > >> > > +++ b/security/commoncap.c > > > > >> > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec > > > > >> > > *effective = true; > > > > >> > > } > > > > >> > > > > > > >> > > > > > >> > It's subjective and so might be just me, but I think I'd find it easier > > > > >> > to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target) > > > > >> > > > > >> In more than one place, I wanted to put the parameter that I was trying > > > > >> to read aloud closest to the function name to make reading it flow > > > > >> better, leaving the parameters less critical to comprehension towards > > > > >> the end. > > > > > > > > > > And I see that in the final patch it looks nicer the way you have it. > > > > > > > > > >> > This looks correct though, so either way > > > > >> > > > > > >> > Reviewed-by: Serge Hallyn <serge@hallyn.com> > > > > >> > > > > >> Thanks. Did you want to put this through, or send it through Paul's > > > > >> audit tree? > > > > > > > > > > If Paul's around I'm happy to have it go through his tree. > > > > > > > > Is this series based against -next with the changes that touch commoncap.c? > > > > > > This series is against pcmoore's audit/next tree (I know I'm missing two > > > commits but they pose no conflict.). > > > > > > Which -next tree are you talking about? I might guess > > > linux-security/next or linux-next/master (I have at least a dozen "next" > > > in my git repo config.) > > > > > > I did eventually find your patches in sfr's tree and in your for-next/kspp branch. > > > > > > I'll have a look at the commoncap.c changes including the elimination of cap_effective. > > > > > > > Also, did you validate this with the existing LTP tests and selftests? > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283 > > > > > > No. I will look into doing that. Thanks for the suggestion. > > Ok, I'm running the kernel self-test > > make TARGETS="capabilities" kselftest > > and getting a good way through it and then hit this on an unmodified kernel: > > [RUN] +++ Tests with uid != 0 +++ > [NOTE] Using global UIDs for tests > [OK] Child succeeded > test_execve: chdir to private tmpfs: Permission denied Hm, just a hunch, anything in syslog? The fact that you can mount the private tmp but not chdir to it just sounds like selinux contexts. Might run it under strace... Boy, that's an interesting testcase. > [FAIL] Child failed > selftests: test_execve [FAIL] > > Is this a known limitation or have I got something weird in my runtime > environment that is killing it at this part of the test? -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-09-02 00:37, Serge E. Hallyn wrote: > On Fri, Sep 01, 2017 at 06:18:43AM -0400, Richard Guy Briggs wrote: > > On 2017-08-28 07:08, Richard Guy Briggs wrote: > > > On 2017-08-28 05:19, Richard Guy Briggs wrote: > > > > On 2017-08-24 12:06, Kees Cook wrote: > > > > > On Thu, Aug 24, 2017 at 9:37 AM, Serge E. Hallyn <serge@hallyn.com> wrote: > > > > > > Quoting Richard Guy Briggs (rgb@redhat.com): > > > > > >> On 2017-08-24 11:03, Serge E. Hallyn wrote: > > > > > >> > Quoting Richard Guy Briggs (rgb@redhat.com): > > > > > >> > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > > > > > >> > > negation of is_subset() easier to read and analyse. > > > > > >> > > > > > > > >> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > > >> > > --- > > > > > >> > > security/commoncap.c | 16 ++++++++++------ > > > > > >> > > 1 files changed, 10 insertions(+), 6 deletions(-) > > > > > >> > > > > > > > >> > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > >> > > index b7fbf77..6f05ec0 100644 > > > > > >> > > --- a/security/commoncap.c > > > > > >> > > +++ b/security/commoncap.c > > > > > >> > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec > > > > > >> > > *effective = true; > > > > > >> > > } > > > > > >> > > > > > > > >> > > > > > > >> > It's subjective and so might be just me, but I think I'd find it easier > > > > > >> > to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target) > > > > > >> > > > > > >> In more than one place, I wanted to put the parameter that I was trying > > > > > >> to read aloud closest to the function name to make reading it flow > > > > > >> better, leaving the parameters less critical to comprehension towards > > > > > >> the end. > > > > > > > > > > > > And I see that in the final patch it looks nicer the way you have it. > > > > > > > > > > > >> > This looks correct though, so either way > > > > > >> > > > > > > >> > Reviewed-by: Serge Hallyn <serge@hallyn.com> > > > > > >> > > > > > >> Thanks. Did you want to put this through, or send it through Paul's > > > > > >> audit tree? > > > > > > > > > > > > If Paul's around I'm happy to have it go through his tree. > > > > > > > > > > Is this series based against -next with the changes that touch commoncap.c? > > > > > > > > This series is against pcmoore's audit/next tree (I know I'm missing two > > > > commits but they pose no conflict.). > > > > > > > > Which -next tree are you talking about? I might guess > > > > linux-security/next or linux-next/master (I have at least a dozen "next" > > > > in my git repo config.) > > > > > > > > I did eventually find your patches in sfr's tree and in your for-next/kspp branch. > > > > > > > > I'll have a look at the commoncap.c changes including the elimination of cap_effective. > > > > > > > > > Also, did you validate this with the existing LTP tests and selftests? > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283 > > > > > > > > No. I will look into doing that. Thanks for the suggestion. > > > > Ok, I'm running the kernel self-test > > > > make TARGETS="capabilities" kselftest > > > > and getting a good way through it and then hit this on an unmodified kernel: > > > > [RUN] +++ Tests with uid != 0 +++ > > [NOTE] Using global UIDs for tests > > [OK] Child succeeded > > test_execve: chdir to private tmpfs: Permission denied > > Hm, just a hunch, anything in syslog? The fact that you can mount the > private tmp but not chdir to it just sounds like selinux contexts. Nothing in journalctl -b, nothing in /var/log > Might run it under strace... strace reports: chdir("/root/rgb/git/linux-2.6/tools/testing/selftests/capabilities") = -1 EACCES (Permission denied) An audit rule only reports the previous success. Problem looks to be the nfs mount on which the test is run. Running it locally works fine. > Boy, that's an interesting testcase. > > > [FAIL] Child failed > > selftests: test_execve [FAIL] > > > > Is this a known limitation or have I got something weird in my runtime > > environment that is killing it at this part of the test? Ok, cleared that up... Next I'm trying to run the ltp and it hangs on several tests and eventually trashes my nfs mount and can't make further progress. Kees, what version of the LTP do you run and what subset of tests do you suggest to validate things? I'm running the upstream clone git. - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-09-04 02:57, Richard Guy Briggs wrote: > On 2017-09-02 00:37, Serge E. Hallyn wrote: > > On Fri, Sep 01, 2017 at 06:18:43AM -0400, Richard Guy Briggs wrote: > > > On 2017-08-28 07:08, Richard Guy Briggs wrote: > > > > On 2017-08-28 05:19, Richard Guy Briggs wrote: > > > > > On 2017-08-24 12:06, Kees Cook wrote: > > > > > > On Thu, Aug 24, 2017 at 9:37 AM, Serge E. Hallyn <serge@hallyn.com> wrote: > > > > > > > Quoting Richard Guy Briggs (rgb@redhat.com): > > > > > > >> On 2017-08-24 11:03, Serge E. Hallyn wrote: > > > > > > >> > Quoting Richard Guy Briggs (rgb@redhat.com): > > > > > > >> > > Introduce macros cap_gained, cap_grew, cap_full to make the use of the > > > > > > >> > > negation of is_subset() easier to read and analyse. > > > > > > >> > > > > > > > > >> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > > > >> > > --- > > > > > > >> > > security/commoncap.c | 16 ++++++++++------ > > > > > > >> > > 1 files changed, 10 insertions(+), 6 deletions(-) > > > > > > >> > > > > > > > > >> > > diff --git a/security/commoncap.c b/security/commoncap.c > > > > > > >> > > index b7fbf77..6f05ec0 100644 > > > > > > >> > > --- a/security/commoncap.c > > > > > > >> > > +++ b/security/commoncap.c > > > > > > >> > > @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec > > > > > > >> > > *effective = true; > > > > > > >> > > } > > > > > > >> > > > > > > > > >> > > > > > > > >> > It's subjective and so might be just me, but I think I'd find it easier > > > > > > >> > to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target) > > > > > > >> > > > > > > >> In more than one place, I wanted to put the parameter that I was trying > > > > > > >> to read aloud closest to the function name to make reading it flow > > > > > > >> better, leaving the parameters less critical to comprehension towards > > > > > > >> the end. > > > > > > > > > > > > > > And I see that in the final patch it looks nicer the way you have it. > > > > > > > > > > > > > >> > This looks correct though, so either way > > > > > > >> > > > > > > > >> > Reviewed-by: Serge Hallyn <serge@hallyn.com> > > > > > > >> > > > > > > >> Thanks. Did you want to put this through, or send it through Paul's > > > > > > >> audit tree? > > > > > > > > > > > > > > If Paul's around I'm happy to have it go through his tree. > > > > > > > > > > > > Is this series based against -next with the changes that touch commoncap.c? > > > > > > > > > > This series is against pcmoore's audit/next tree (I know I'm missing two > > > > > commits but they pose no conflict.). > > > > > > > > > > Which -next tree are you talking about? I might guess > > > > > linux-security/next or linux-next/master (I have at least a dozen "next" > > > > > in my git repo config.) > > > > > > > > > > I did eventually find your patches in sfr's tree and in your for-next/kspp branch. > > > > > > > > > > I'll have a look at the commoncap.c changes including the elimination of cap_effective. > > > > > > > > > > > Also, did you validate this with the existing LTP tests and selftests? > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283 > > > > > > > > > > No. I will look into doing that. Thanks for the suggestion. > > > > > > Ok, I'm running the kernel self-test > > > > > > make TARGETS="capabilities" kselftest > > > > > > and getting a good way through it and then hit this on an unmodified kernel: > > > > > > [RUN] +++ Tests with uid != 0 +++ > > > [NOTE] Using global UIDs for tests > > > [OK] Child succeeded > > > test_execve: chdir to private tmpfs: Permission denied > > > > Hm, just a hunch, anything in syslog? The fact that you can mount the > > private tmp but not chdir to it just sounds like selinux contexts. > > Nothing in journalctl -b, nothing in /var/log > > > Might run it under strace... > > strace reports: chdir("/root/rgb/git/linux-2.6/tools/testing/selftests/capabilities") = -1 EACCES (Permission denied) > > An audit rule only reports the previous success. > > Problem looks to be the nfs mount on which the test is run. Running it > locally works fine. > > > Boy, that's an interesting testcase. > > > > > [FAIL] Child failed > > > selftests: test_execve [FAIL] > > > > > > Is this a known limitation or have I got something weird in my runtime > > > environment that is killing it at this part of the test? > > Ok, cleared that up... > > Next I'm trying to run the ltp and it hangs on several tests and > eventually trashes my nfs mount and can't make further progress. > Kees, what version of the LTP do you run and what subset of tests do you > suggest to validate things? I'm running the upstream clone git. And I got so absorbed in installing and running ltp that I lost sight of the original goal and forgot about the specific tests you listed in your patch description. They all pass. All tests looking good now. > - RGB - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/security/commoncap.c b/security/commoncap.c index b7fbf77..6f05ec0 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec *effective = true; } +#define cap_gained(field, target, source) \ + !cap_issubset(target->cap_##field, source->cap_##field) +#define cap_grew(target, source, cred) \ + !cap_issubset(cred->cap_##target, cred->cap_##source) +#define cap_full(field, cred) \ + cap_issubset(CAP_FULL_SET, cred->cap_##field) /** * cap_bprm_set_creds - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds @@ -541,10 +547,9 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) handle_privileged_root(bprm, has_cap, &effective, root_uid); /* if we have fs caps, clear dangerous personality flags */ - if (!cap_issubset(new->cap_permitted, old->cap_permitted)) + if (cap_gained(permitted, new, old)) bprm->per_clear |= PER_CLEAR_ON_SETID; - /* Don't let someone trace a set[ug]id/setpcap binary with the revised * credentials unless they have the appropriate permit. * @@ -552,8 +557,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) */ is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid); - if ((is_setid || - !cap_issubset(new->cap_permitted, old->cap_permitted)) && + if ((is_setid || cap_gained(permitted, new, old)) && ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) || !ptracer_capable(current, new->user_ns))) { /* downgrade; they get no more than they had, and maybe less */ @@ -605,8 +609,8 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) * Number 1 above might fail if you don't have a full bset, but I think * that is interesting information to audit. */ - if (!cap_issubset(new->cap_effective, new->cap_ambient)) { - if (!cap_issubset(CAP_FULL_SET, new->cap_effective) || + if (cap_grew(effective, ambient, new)) { + if (!cap_full(effective, new) || !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || issecure(SECURE_NOROOT)) { ret = audit_log_bprm_fcaps(bprm, new, old);
Introduce macros cap_gained, cap_grew, cap_full to make the use of the negation of is_subset() easier to read and analyse. Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- security/commoncap.c | 16 ++++++++++------ 1 files changed, 10 insertions(+), 6 deletions(-)