diff mbox

[V3,05/10] capabilities: use intuitive names for id changes

Message ID 1360ed3437f87ac0b9e076ff5ea05c67ee8a7ed8.1503459890.git.rgb@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Richard Guy Briggs Aug. 23, 2017, 10:12 a.m. UTC
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@redhat.com>
---
 security/commoncap.c |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

Comments

Serge E. Hallyn Aug. 24, 2017, 4:17 p.m. UTC | #1
Quoting Richard Guy Briggs (rgb@redhat.com):
> 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@redhat.com>

Reviewed-by: Serge Hallyn <serge@hallyn.com>

> ---
>  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); }
> +
> +static inline bool is_eff(kuid_t uid, struct cred *cred)
> +{ return uid_eq(cred->euid, uid); }
> +
> +static inline bool is_suid(kuid_t uid, struct cred *cred)
> +{ return !is_real(uid, cred) && is_eff(uid, cred); }
> +
>  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)) {
>  		warn_setuid_and_fcaps_mixed(bprm->filename);
>  		return;
>  	}
> @@ -504,12 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
>  	 *
>  	 * 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)) {
> +	if (is_eff(root_uid, new) || is_real(root_uid, new)) {
>  		/* pP' = (cap_bset & ~0) | (pI & ~0) */
>  		new->cap_permitted = cap_combine(old->cap_bset,
>  						 old->cap_inheritable);
>  	}
> -	if (uid_eq(new->euid, root_uid))
> +	if (is_eff(root_uid, new))
>  		*effective = true;
>  }
>  
> @@ -519,6 +528,13 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
>  	!cap_issubset(cred->cap_##target, cred->cap_##source)
>  #define cap_full(field, cred) \
>  	cap_issubset(CAP_FULL_SET, cred->cap_##field)
> +
> +static inline bool is_setuid(struct cred *new, const struct cred *old)
> +{ return !uid_eq(new->euid, old->uid); }
> +
> +static inline bool is_setgid(struct cred *new, const struct cred *old)
> +{ return !gid_eq(new->egid, old->gid); }
> +
>  /**
>   * cap_bprm_set_creds - Set up the proposed credentials for execve().
>   * @bprm: The execution parameters, including the proposed creds
> @@ -556,7 +572,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  	 *
>  	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
>  	 */
> -	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
> +	is_setid = is_setuid(new, old) || is_setgid(new, old);
>  
>  	if ((is_setid || cap_gained(permitted, new, old)) &&
>  	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
> @@ -612,7 +628,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) ||
> +		    !is_eff(root_uid, new) || !is_real(root_uid, new) ||
>  		    !root_privileged()) {
>  			ret = audit_log_bprm_fcaps(bprm, new, old);
>  			if (ret < 0)
> -- 
> 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
James Morris Aug. 25, 2017, 5:59 a.m. UTC | #2
On Wed, 23 Aug 2017, Richard Guy Briggs 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@redhat.com>
> ---
>  security/commoncap.c |   26 +++++++++++++++++++++-----
>  1 files changed, 21 insertions(+), 5 deletions(-)


Acked-by: James Morris <james.l.morris@oracle.com>
Andy Lutomirski Aug. 25, 2017, 3:06 p.m. UTC | #3
On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> 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@redhat.com>
> ---
>  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.

> @@ -519,6 +528,13 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
>         !cap_issubset(cred->cap_##target, cred->cap_##source)
>  #define cap_full(field, cred) \
>         cap_issubset(CAP_FULL_SET, cred->cap_##field)
> +
> +static inline bool is_setuid(struct cred *new, const struct cred *old)
> +{ return !uid_eq(new->euid, old->uid); }
> +
> +static inline bool is_setgid(struct cred *new, const struct cred *old)
> +{ return !gid_eq(new->egid, old->gid); }

Please don't.  This logic is fragile, and these helpers are pretending
it's not fragile even though it's still fragile.
--
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
Serge E. Hallyn Aug. 25, 2017, 6:51 p.m. UTC | #4
Quoting Andy Lutomirski (luto@kernel.org):
> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> 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@redhat.com>
> > ---
> >  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.

> > +
> > +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.
That's what it's checking for, the name of the new helper makes
that clear, and the code becomes clearer because we only see the
thing we care about checking for rather than the intent being
hidden.

> 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.

In what way does not do what you'd expect?
--
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
Andy Lutomirski Aug. 25, 2017, 7:45 p.m. UTC | #5
--Andy
> On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> 
> Quoting Andy Lutomirski (luto@kernel.org):
>>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> 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@redhat.com>
>>> ---
>>> 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.

> That's what it's checking for, the name of the new helper makes
> that clear, and the code becomes clearer because we only see the
> thing we care about checking for rather than the intent being
> hidden.


> 
>> 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.
> 
> In what way does not do what you'd expect?

It doesn't look at cred->suid.--
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
Serge E. Hallyn Aug. 25, 2017, 8:06 p.m. UTC | #6
Quoting Andy Lutomirski (luto@amacapital.net):
> 
> 
> 
> --Andy
> > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> > 
> > Quoting Andy Lutomirski (luto@kernel.org):
> >>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> 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@redhat.com>
> >>> ---
> >>> 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
--
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
James Morris Aug. 28, 2017, 1:32 a.m. UTC | #7
On Fri, 25 Aug 2017, Serge E. Hallyn wrote:

> 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.

Perhaps document them better and maybe prefix them with __ to emphasize 
that they're internal only?
Richard Guy Briggs Aug. 28, 2017, 9:12 a.m. UTC | #8
On 2017-08-25 15:06, Serge E. Hallyn wrote:
> Quoting Andy Lutomirski (luto@amacapital.net):
> > 
> > --Andy
> > > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> > > 
> > > Quoting Andy Lutomirski (luto@kernel.org):
> > >>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> 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@redhat.com>
> > >>> ---
> > >>> 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.

I was trying to get away entirely from "uid_eq" because I didn't find it
at all helpful in understanding what that function did, so I don't see
real_uid_eq() as an improvement.  (More below.)

> > >>> +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()?

Except that *someone* earlier had come up with the local variable is_setid():

	58319057b784 luto 2015-09-04 ("capabilities: ambient capabilities")
 
So I'm finding this particular objection and renaming scheme a bit hard
to swallow.

I simply extended the convention and made the two conditions that feed
it follow the same naming pattern but for the fact that it is a function
name with the minimum necessary arguments in the order that made the
most sense to me to read aloud to understand its usage.  Making it a
macro that hides the arguments was my original effort.

I was trying to find another unique function name that got across the
intent of what I needed to express.

> -serge

- 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
Andy Lutomirski Aug. 28, 2017, 8:12 p.m. UTC | #9
On Mon, Aug 28, 2017 at 2:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-08-25 15:06, Serge E. Hallyn wrote:
>> Quoting Andy Lutomirski (luto@amacapital.net):
>> >
>> > --Andy
>> > > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
>> > >
>> > > Quoting Andy Lutomirski (luto@kernel.org):
>> > >>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> 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@redhat.com>
>> > >>> ---
>> > >>> 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.
>
> I was trying to get away entirely from "uid_eq" because I didn't find it
> at all helpful in understanding what that function did, so I don't see
> real_uid_eq() as an improvement.  (More below.)
>
>> > >>> +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()?
>
> Except that *someone* earlier had come up with the local variable is_setid():
>
>         58319057b784 luto 2015-09-04 ("capabilities: ambient capabilities")
>
> So I'm finding this particular objection and renaming scheme a bit hard
> to swallow.

At least that thing is a variable that's local to the function (and
hence to the context in which that function is called).  I'm not
saying it's a work of art.
--
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 mbox

Patch

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); }
+
+static inline bool is_eff(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->euid, uid); }
+
+static inline bool is_suid(kuid_t uid, struct cred *cred)
+{ return !is_real(uid, cred) && is_eff(uid, cred); }
+
 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)) {
 		warn_setuid_and_fcaps_mixed(bprm->filename);
 		return;
 	}
@@ -504,12 +513,12 @@  void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
 	 *
 	 * 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)) {
+	if (is_eff(root_uid, new) || is_real(root_uid, new)) {
 		/* pP' = (cap_bset & ~0) | (pI & ~0) */
 		new->cap_permitted = cap_combine(old->cap_bset,
 						 old->cap_inheritable);
 	}
-	if (uid_eq(new->euid, root_uid))
+	if (is_eff(root_uid, new))
 		*effective = true;
 }
 
@@ -519,6 +528,13 @@  void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
 	!cap_issubset(cred->cap_##target, cred->cap_##source)
 #define cap_full(field, cred) \
 	cap_issubset(CAP_FULL_SET, cred->cap_##field)
+
+static inline bool is_setuid(struct cred *new, const struct cred *old)
+{ return !uid_eq(new->euid, old->uid); }
+
+static inline bool is_setgid(struct cred *new, const struct cred *old)
+{ return !gid_eq(new->egid, old->gid); }
+
 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().
  * @bprm: The execution parameters, including the proposed creds
@@ -556,7 +572,7 @@  int cap_bprm_set_creds(struct linux_binprm *bprm)
 	 *
 	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
 	 */
-	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
+	is_setid = is_setuid(new, old) || is_setgid(new, old);
 
 	if ((is_setid || cap_gained(permitted, new, old)) &&
 	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
@@ -612,7 +628,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) ||
+		    !is_eff(root_uid, new) || !is_real(root_uid, new) ||
 		    !root_privileged()) {
 			ret = audit_log_bprm_fcaps(bprm, new, old);
 			if (ret < 0)