New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for privilleged execution with getauxval. #6993

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@paulidale
Contributor

paulidale commented Aug 17, 2018

Check getauxval on systems that have it when checking for setuid execution.

# ifdef AT_SECURE
if (getauxval(AT_SECURE) != 0)
return 1;
# endif

This comment has been minimized.

@mspncp

mspncp Aug 17, 2018

Contributor

In the getauxval(AT_SECURE) == 0 case, is it really necessary to do the getuid() and getgid() tests as fallback? Don't we trust getauxval(AT_SECURE)? Couldn't we just replace the latter tests with the former if if it exists?

# ifdef AT_SECURE
    if (getauxval(AT_SECURE) != 0)
        return 1;
# else
    if (getuid() != geteuid())
        return 1;
    if (getgid() != getegid())
         return 1;
# endif
     return 0;

This comment has been minimized.

@paulidale

paulidale Aug 17, 2018

Contributor

I also suspect it isn't necessary to check the uid and gid, but the additional tests can't hurt and shouldn't cost much since this function isn't called on any critical path that I noticed (caveat: I didn't look too hard). I did consider making it an either/or as suggested but erred on the (overly?) cautious side. This is the sort of code where playing safe can pay off.

Regardless, I'm willing to change this.

This comment has been minimized.

@kroeckx

kroeckx Aug 19, 2018

Member

You could also return getauxval(AT_SECURE) != 0;

But I really don't have a preference either, just pick one.

This comment has been minimized.

@mspncp

mspncp Aug 19, 2018

Contributor

I like Kurt's suggestion, but if you choose it, you should do it for the else part, too:

# ifdef AT_SECURE
    return getauxval(AT_SECURE) != 0;
# else
    return getuid() != geteuid() || getgid() != getegid();
# endif

This comment has been minimized.

@paulidale

paulidale Aug 19, 2018

Contributor

I'm going with the final suggestion by @mspncp here.

@paulidale paulidale force-pushed the paulidale:getauxval branch from 248be47 to c884a20 Aug 19, 2018

@paulidale

This comment has been minimized.

Contributor

paulidale commented Aug 19, 2018

I also cleaned up the preprocessor directive guarding the include line.
That's enough changes to justify a review I think.

@mspncp

mspncp approved these changes Aug 19, 2018

LGTM

return 1;
return 0;
# ifdef AT_SECURE
return getauxval(AT_SECURE);

This comment has been minimized.

@kroeckx

kroeckx Aug 19, 2018

Member

Should this be != 0? It's documented to return a non-zero value.

This comment has been minimized.

@kroeckx

kroeckx Aug 19, 2018

Member

issetugid() is documented to return 1. This function seems to be undocumented.

This comment has been minimized.

@kroeckx

kroeckx Aug 19, 2018

Member

I guess it doesn't really matter

This comment has been minimized.

@paulidale

paulidale Aug 19, 2018

Contributor

I've added the != 0.

This comment has been minimized.

@paulidale

paulidale Aug 20, 2018

Contributor

It is documented in recent glibc's. It returns an unsigned long and in this case a non-zero value means privileged. The not zero check is appropriate and necessary.

@paulidale paulidale force-pushed the paulidale:getauxval branch from c884a20 to f3210b1 Aug 19, 2018

@paulidale

This comment has been minimized.

Contributor

paulidale commented Aug 20, 2018

Merged thanks.

@paulidale paulidale closed this Aug 20, 2018

@paulidale paulidale deleted the paulidale:getauxval branch Aug 20, 2018

levitte pushed a commit that referenced this pull request Aug 20, 2018

Check getauxval on systems that have it when checking for setuid exec…
…ution.

Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from #6993)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment