Factor out the case of privileged root from the function cap_bprm_set_creds() to make the latter easier to read and analyse. Suggested-by: Serge Hallyn <serge(a)hallyn.com&gt;

Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; --- security/commoncap.c | 62 +++++++++++++++++++++++++++---------------------- 1 files changed, 34 insertions(+), 28 deletions(-) diff --git a/security/commoncap.c b/security/commoncap.c index 78b3783..b7fbf77 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -481,6 +481,38 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c return rc; } +void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effective, kuid_t root_uid) +{ + const struct cred *old = current_cred(); + struct cred *new = bprm->cred; + + if (issecure(SECURE_NOROOT)) + return; + /* + * If the legacy file capability is set, then don't set privs + * for a setuid root binary run by a non-root user. Do set it + * for a root user just to cause least surprise to an admin. + */ + if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { + warn_setuid_and_fcaps_mixed(bprm->filename); + return; + } + /* + * To support inheritance of root-permissions and suid-root + * executables under compatibility mode, we override the + * capability sets for the file. + * + * If only the real uid is 0, we do not set the effective bit. + */ + if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) { + /* pP' = (cap_bset & ~0) | (pI & ~0) */ + new->cap_permitted = cap_combine(old->cap_bset, + old->cap_inheritable); + } + if (uid_eq(new->euid, root_uid)) + *effective = true; +} + /** * cap_bprm_set_creds - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds @@ -493,46 +525,20 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) { const struct cred *old = current_cred(); struct cred *new = bprm->cred; - bool effective, has_cap = false, is_setid; + bool effective = false, has_cap = false, is_setid; int ret; kuid_t root_uid; if (WARN_ON(!cap_ambient_invariant_ok(old))) return -EPERM; - effective = false; ret = get_file_caps(bprm, &effective, &has_cap); if (ret < 0) return ret; root_uid = make_kuid(new->user_ns, 0); - if (!issecure(SECURE_NOROOT)) { - /* - * If the legacy file capability is set, then don't set privs - * for a setuid root binary run by a non-root user. Do set it - * for a root user just to cause least surprise to an admin. - */ - if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { - warn_setuid_and_fcaps_mixed(bprm->filename); - goto skip; - } - /* - * To support inheritance of root-permissions and suid-root - * executables under compatibility mode, we override the - * capability sets for the file. - * - * If only the real uid is 0, we do not set the effective bit. - */ - if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) { - /* pP' = (cap_bset & ~0) | (pI & ~0) */ - new->cap_permitted = cap_combine(old->cap_bset, - old->cap_inheritable); - } - if (uid_eq(new->euid, root_uid)) - effective = true; - } -skip: + 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)) -- 1.7.1

On Wed, 23 Aug 2017, Richard Guy Briggs wrote: > Factor out the case of privileged root from the function > cap_bprm_set_creds() to make the latter easier to read and analyse. > > Suggested-by: Serge Hallyn <serge(a)hallyn.com&gt; > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > --- > security/commoncap.c | 62 +++++++++++++++++++++++++++---------------------- > 1 files changed, 34 insertions(+), 28 deletions(-) > > diff --git a/security/commoncap.c b/security/commoncap.c > index 78b3783..b7fbf77 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -481,6 +481,38 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c > return rc; > } > > +void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effective, kuid_t root_uid) Can this be static?

James Morris

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(a)redhat.com&gt; --- 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) /** * 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

On 2017-08-24 11:03, Serge E. Hallyn wrote: > Quoting Richard Guy Briggs (rgb(a)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(a)redhat.com&gt; > > --- > > 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(a)hallyn.com&gt; Thanks. Did you want to put this through, or send it through Paul's audit tree?

On Thu, Aug 24, 2017 at 9:37 AM, Serge E. Hallyn <serge(a)hallyn.com&gt; wrote: > Quoting Richard Guy Briggs (rgb(a)redhat.com): >> On 2017-08-24 11:03, Serge E. Hallyn wrote: >> > Quoting Richard Guy Briggs (rgb(a)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(a)redhat.com&gt; >> > > --- >> > > 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(a)hallyn.com&gt; >> >> 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=...

On 2017-08-24 12:06, Kees Cook wrote: > On Thu, Aug 24, 2017 at 9:37 AM, Serge E. Hallyn <serge(a)hallyn.com&gt; wrote: > > Quoting Richard Guy Briggs (rgb(a)redhat.com): > >> On 2017-08-24 11:03, Serge E. Hallyn wrote: > >> > Quoting Richard Guy Briggs (rgb(a)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(a)redhat.com&gt; > >> > > --- > >> > > 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(a)hallyn.com&gt; > >> > >> 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=... 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

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(a)hallyn.com&gt; wrote: > > > > Quoting Richard Guy Briggs (rgb(a)redhat.com): > > > >> On 2017-08-24 11:03, Serge E. Hallyn wrote: > > > >> > Quoting Richard Guy Briggs (rgb(a)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(a)redhat.com&gt; > > > >> > > --- > > > >> > > 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(a)hallyn.com&gt; > > > >> > > > >> 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=... > > > > 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?

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(a)hallyn.com&gt; wrote: > > > > > > Quoting Richard Guy Briggs (rgb(a)redhat.com): > > > > > >> On 2017-08-24 11:03, Serge E. Hallyn wrote: > > > > > >> > Quoting Richard Guy Briggs (rgb(a)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(a)redhat.com&gt; > > > > > >> > > --- > > > > > >> > > 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(a)hallyn.com&gt; > > > > > >> > > > > > >> 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=... > > > > > > > > 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

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(a)redhat.com&gt; --- 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)

Introduce inline root_privileged() to make use of SECURE_NONROOT easier to read. Suggested-by: Serge Hallyn <serge(a)hallyn.com&gt;

Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; --- security/commoncap.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/security/commoncap.c b/security/commoncap.c index 028d4e4..36c38a1 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -481,13 +481,13 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f return rc; } +static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } + void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effective, kuid_t root_uid) { const struct cred *old = current_cred(); struct cred *new = bprm->cred; - if (issecure(SECURE_NOROOT)) - return; /* * If the legacy file capability is set, then don't set privs * for a setuid root binary run by a non-root user. Do set it @@ -544,7 +544,8 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) root_uid = make_kuid(new->user_ns, 0); - handle_privileged_root(bprm, has_fcap, &effective, root_uid); + if (root_privileged()) + handle_privileged_root(bprm, has_fcap, &effective, root_uid); /* if we have fs caps, clear dangerous personality flags */ if (cap_gained(permitted, new, old)) @@ -612,7 +613,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) 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)) { + !root_privileged()) { ret = audit_log_bprm_fcaps(bprm, new, old); if (ret < 0) return ret; -- 1.7.1

On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > Introduce a number of inlines to make the use of the negation of > uid_eq() easier to read and analyse. > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > --- > security/commoncap.c | 26 +++++++++++++++++++++----- > 1 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/security/commoncap.c b/security/commoncap.c > index 36c38a1..1af7dec 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f > > static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } > > +static inline bool is_real(kuid_t uid, struct cred *cred) > +{ return uid_eq(cred->uid, uid); } OK I guess, but this just seems like a way to obfuscate the code a bit and save typing "->uid".

> + > +static inline bool is_eff(kuid_t uid, struct cred *cred) > +{ return uid_eq(cred->euid, uid); } Ditto. > + > +static inline bool is_suid(kuid_t uid, struct cred *cred) > +{ return !is_real(uid, cred) && is_eff(uid, cred); } Please no. This is IMO insane. You're hiding really weird, nonintuitive logic in an oddly named helper.

Also, this is going to cause massive confusion and severe bugs: given the same, the only remotely sensible guess as to what this function does is uid_eq(cred->suid, uid). So NAK to this. > + > void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effective, kuid_t root_uid) > { > const struct cred *old = current_cred(); > @@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe > * for a setuid root binary run by a non-root user. Do set it > * for a root user just to cause least surprise to an admin. > */ > - if (has_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { > + if (has_fcap && is_suid(root_uid, new)) { e.g. this. The logic used to be obviously slightly dicey. Now it looks sane but doesn't do what you'd naively expect it to do, which is far worse.

--Andy > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge(a)hallyn.com&gt; wrote: > > Quoting Andy Lutomirski (luto(a)kernel.org): >>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: >>> Introduce a number of inlines to make the use of the negation of >>> uid_eq() easier to read and analyse. >>> >>> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; >>> --- >>> security/commoncap.c | 26 +++++++++++++++++++++----- >>> 1 files changed, 21 insertions(+), 5 deletions(-) >>> >>> diff --git a/security/commoncap.c b/security/commoncap.c >>> index 36c38a1..1af7dec 100644 >>> --- a/security/commoncap.c >>> +++ b/security/commoncap.c >>> @@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f >>> >>> static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } >>> >>> +static inline bool is_real(kuid_t uid, struct cred *cred) >>> +{ return uid_eq(cred->uid, uid); } >> >> OK I guess, but this just seems like a way to obfuscate the code a bit >> and save typing "->uid". > > Personally I find the new to be far more readable. In the old, the > distinction between uid and euid is one character hidden in the middle > of the expression. Would real_uid_eq be better?

>>> +static inline bool is_eff(kuid_t uid, struct cred *cred) >>> +{ return uid_eq(cred->euid, uid); } >> >> Ditto. >> >>> + >>> +static inline bool is_suid(kuid_t uid, struct cred *cred) >>> +{ return !is_real(uid, cred) && is_eff(uid, cred); } >> >> Please no. This is IMO insane. You're hiding really weird, >> nonintuitive logic in an oddly named helper. > > How is it nonintuitive? It's very precisely checking that a > nonroot user is executing something that results in euid=0. I can think of several sensible predicated: 1. Are we execing a setuid-root program, where the setuod bit wasn't suppressed by nnp, mount options, trace, etc? 2. Same as 1, but also require that we weren't root. 3. Is the new euid 0 and old uid != 0? 4. Does suid == 0? This helper checks something equivalent to 3, but only once were far enough through exec and before user code starts. This is probably equivalent to 2 as well. This is quite subtle and deserves an open-coded check, a much more carefully named helper, or, better yet, something that looks at binprm instead of cred.

is_suid sounds like #4.

>>> @@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe >>> * for a setuid root binary run by a non-root user. Do set it >>> * for a root user just to cause least surprise to an admin. >>> */ >>> - if (has_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { >>> + if (has_fcap && is_suid(root_uid, new)) { >> >> e.g. this. The logic used to be obviously slightly dicey. Now it >> looks sane but doesn't do what you'd naively expect it to do, which is >> far worse. > > In what way does not do what you'd expect? It doesn't look at cred->suid.

Quoting Andy Lutomirski (luto(a)amacapital.net): > > --Andy > > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge(a)hallyn.com&gt; wrote: > > > > Quoting Andy Lutomirski (luto(a)kernel.org): > >>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > >>> Introduce a number of inlines to make the use of the negation of > >>> uid_eq() easier to read and analyse. > >>> > >>> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > >>> --- > >>> security/commoncap.c | 26 +++++++++++++++++++++----- > >>> 1 files changed, 21 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/security/commoncap.c b/security/commoncap.c > >>> index 36c38a1..1af7dec 100644 > >>> --- a/security/commoncap.c > >>> +++ b/security/commoncap.c > >>> @@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f > >>> > >>> static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } > >>> > >>> +static inline bool is_real(kuid_t uid, struct cred *cred) > >>> +{ return uid_eq(cred->uid, uid); } > >> > >> OK I guess, but this just seems like a way to obfuscate the code a bit > >> and save typing "->uid". > > > > Personally I find the new to be far more readable. In the old, the > > distinction between uid and euid is one character hidden in the middle > > of the expression. > > Would real_uid_eq be better? Replacing is_real() with real_uid_eq() would be good. I still think that a is_setuid() is worthwhile.

> >>> +static inline bool is_eff(kuid_t uid, struct cred *cred) > >>> +{ return uid_eq(cred->euid, uid); } > >> > >> Ditto. > >> > >>> + > >>> +static inline bool is_suid(kuid_t uid, struct cred *cred) > >>> +{ return !is_real(uid, cred) && is_eff(uid, cred); } > >> > >> Please no. This is IMO insane. You're hiding really weird, > >> nonintuitive logic in an oddly named helper. > > > > How is it nonintuitive? It's very precisely checking that a > > nonroot user is executing something that results in euid=0. > > I can think of several sensible predicated: > > 1. Are we execing a setuid-root program, where the setuod bit wasn't suppressed by nnp, mount options, trace, etc? > > 2. Same as 1, but also require that we weren't root. > > 3. Is the new euid 0 and old uid != 0? > > 4. Does suid == 0? > > This helper checks something equivalent to 3, but only once were far enough through exec and before user code starts. This is probably equivalent to 2 as well. This is quite subtle and deserves an open-coded check, a much more carefully named helper, or, better yet, something that looks at binprm instead of cred. Part of the motivation here is that the things we are checking for are some rather baroque combinations of conditions, so having each piece of those be as simple and clear as possible helps to better reason about what is going on (which helped Richard to find the bug he is fixing). These helpers are local (should all be static, as James pointed out). Making helpers to simplify the final checks is the right way to clarify code. I'm all for making sure they are as clear as possible, but I do think their existence is justified. > is_suid sounds like #4. [...] > >>> @@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe > >>> * for a setuid root binary run by a non-root user. Do set it > >>> * for a root user just to cause least surprise to an admin. > >>> */ > >>> - if (has_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { > >>> + if (has_fcap && is_suid(root_uid, new)) { > >> > >> e.g. this. The logic used to be obviously slightly dicey. Now it > >> looks sane but doesn't do what you'd naively expect it to do, which is > >> far worse. > > > > In what way does not do what you'd expect? > > It doesn't look at cred->suid. Heh, good point. How about is_setuid()?

-serge

Remove a layer of conditional logic to make the use of conditions easier to read and analyse. Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt;

--- security/commoncap.c | 13 ++++++------- 1 files changed, 6 insertions(+), 7 deletions(-) diff --git a/security/commoncap.c b/security/commoncap.c index 5d81354..ffcaff0 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -551,13 +551,12 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root) { bool ret = false; - if (cap_grew(effective, ambient, cred)) { - if (!cap_full(effective, cred) || - !is_eff(root, cred) || !is_real(root, cred) || - !root_privileged()) { - ret = true; - } - } + if (cap_grew(effective, ambient, cred) && + (!cap_full(effective, cred) || + !is_eff(root, cred) || + !is_real(root, cred) || + !root_privileged())) + ret = true; return ret; } -- 1.7.1

Remove a layer of conditional logic to make the use of conditions easier to read and analyse. Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; --- security/commoncap.c | 13 ++++++------- 1 files changed, 6 insertions(+), 7 deletions(-) diff --git a/security/commoncap.c b/security/commoncap.c index 5d81354..ffcaff0 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -551,13 +551,12 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root) { bool ret = false; - if (cap_grew(effective, ambient, cred)) { - if (!cap_full(effective, cred) || - !is_eff(root, cred) || !is_real(root, cred) || - !root_privileged()) { - ret = true; - } - } + if (cap_grew(effective, ambient, cred) && + (!cap_full(effective, cred) || + !is_eff(root, cred) || + !is_real(root, cred) || + !root_privileged())) + ret = true;

return ret; } -- 1.7.1

The way the logic was presented, it was awkward to read and verify. Invert the logic using DeMorgan's Law to be more easily able to read and understand. Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt;

--- security/commoncap.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/security/commoncap.c b/security/commoncap.c index ffcaff0..eb2da69 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -552,10 +552,10 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root) bool ret = false; if (cap_grew(effective, ambient, cred) && - (!cap_full(effective, cred) || - !is_eff(root, cred) || - !is_real(root, cred) || - !root_privileged())) + !(cap_full(effective, cred) && + is_eff(root, cred) && + is_real(root, cred) && + root_privileged())) ret = true; return ret; }

Now that the logic is inverted, it is much easier to see that both real root and effective root conditions had to be met to avoid printing the BPRM_FCAPS record with audit syscalls. This meant that any setuid root applications would print a full BPRM_FCAPS record when it wasn't necessary, cluttering the event output, since the SYSCALL and PATH records indicated the presence of the setuid bit and effective root user id. Require only one of effective root or real root to avoid printing the unnecessary record. Ref: 3fc689e96c0c (Add audit_log_bprm_fcaps/AUDIT_BPRM_FCAPS) See: https://github.com/linux-audit/audit-kernel/issues/16 Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt;

--- security/commoncap.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/security/commoncap.c b/security/commoncap.c index eb2da69..49cce06 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -540,7 +540,7 @@ static inline bool is_setgid(struct cred *new, const struct cred *old) * * We do not bother to audit if 3 things are true: * 1) cap_effective has all caps - * 2) we are root + * 2) we became root *OR* are root

* 3) root is supposed to have all caps (SECURE_NOROOT) * Since this is just a normal root execing a process. * @@ -553,8 +553,8 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root) if (cap_grew(effective, ambient, cred) && !(cap_full(effective, cred) && - is_eff(root, cred) && - is_real(root, cred) && + (is_eff(root, cred) || + is_real(root, cred)) && root_privileged())) ret = true; return ret; -- 1.7.1

On 2017-08-24 11:29, Serge E. Hallyn wrote: > Quoting Richard Guy Briggs (rgb(a)redhat.com): > > Now that the logic is inverted, it is much easier to see that both real root > > and effective root conditions had to be met to avoid printing the BPRM_FCAPS > > record with audit syscalls. This meant that any setuid root applications would > > print a full BPRM_FCAPS record when it wasn't necessary, cluttering the event > > output, since the SYSCALL and PATH records indicated the presence of the setuid > > bit and effective root user id. > > > > Require only one of effective root or real root to avoid printing the > > unnecessary record. > > > > Ref: 3fc689e96c0c (Add audit_log_bprm_fcaps/AUDIT_BPRM_FCAPS) > > See: https://github.com/linux-audit/audit-kernel/issues/16 > > > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > > Reviewed-by: Serge Hallyn <serge(a)hallyn.com&gt; > > I wonder whether, > > > --- > > security/commoncap.c | 6 +++--- > > 1 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > index eb2da69..49cce06 100644 > > --- a/security/commoncap.c > > +++ b/security/commoncap.c > > @@ -540,7 +540,7 @@ static inline bool is_setgid(struct cred *new, const struct cred *old) > > * > > * We do not bother to audit if 3 things are true: > > * 1) cap_effective has all caps > > - * 2) we are root > > + * 2) we became root *OR* are root > > For clarity, what do you think about adding "(because fcaps were not used)"? Possibly. Is it possible to become root without fcaps other than logging in on a console as root from the get-go? But I see your point. Even if su or sudo were used to gain root, it would have been on a previous operation and not the immediate one being audited. The intention behind the change in the comment wording was to emphasize that the original comment hand-waved a bit about effective root vs real root without being explicit that it could be one or the other rather than requiring both, which affected the logic used to express it.

> > * 3) root is supposed to have all caps (SECURE_NOROOT) > > * Since this is just a normal root execing a process. > > * > > @@ -553,8 +553,8 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root) > > > > if (cap_grew(effective, ambient, cred) && > > !(cap_full(effective, cred) && > > - is_eff(root, cred) && > > - is_real(root, cred) && > > + (is_eff(root, cred) || > > + is_real(root, cred)) && > > root_privileged())) > > ret = true; > > return ret; > > -- > > 1.7.1 - RGB -- Richard Guy Briggs <rgb(a)redhat.com&gt; 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