Incorrect check for SUID/SGID/fscaps programs

RESOLVED FIXED in 4.12

Status

defect
RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: fweimer, Assigned: jld)

Tracking

({csectype-priv-escalation, sec-moderate})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: doesn't affect Firefox)

Attachments

(2 attachments, 2 obsolete attachments)

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150806132111

Steps to reproduce:

Bug  365703 and bug 351470 (CVE-2006-4842) are still only fixed partially.  fscaps are not considered on GNU/Linux, and a check like getuid() == geteuid() misses cases where the program has switched UIDs completely.

Please use secure_getenv on GNU/Linux to read all environment variables, as explained here: https://sourceware.org/glibc/wiki/Tips_and_Tricks/secure_getenv
Assignee: wtc → nobody
Status: UNCONFIRMED → NEW
Ever confirmed: true
Group: core-security → core-security-release
Tim, is this within your purview now?
Flags: needinfo?(ttaubert)
Think I'm not the best person to do this. David, any thoughts on who can help here?
Flags: needinfo?(ttaubert) → needinfo?(dkeeler)
It is best to find someone familiar with operating system security, such as
a developer working on Firefox OS or sandboxing. But anyone interested in
Unix/Linux system programming can tackle this bug.
Jed is this something you could look at?
Flags: needinfo?(jld)
Relevant things I've dealt with recently: Linux capabilities, the code in glibc that decides whether a process needs this kind of security, and the NSPR env var routines.  So I'm probably the right person to take this.
Assignee: nobody → jld
Flags: needinfo?(jld)
One subtlety: secure_getenv is a glibc extension, so if there's anyone building NSPR-based programs for Linux with a different libc and installing them with file capabilities, they could still be affected.
This exposes secure_getenv() plus NSPR's env lock as PR_GetEnvSecure, and uses it instead of uid/gid tests.  In the absence of secure_getenv, the fallback is to uid/gid comparison if XP_UNIX, and otherwise it's just PR_GetEnv.  This is a little more “fail open” than I'd normally like, but it might be the best option under the circumstances.

Important side-effect: the behavior of logging: previously logging would be disabled entirely if set[ug]id, but with this patch it just ignores the env var and logs to stderr as usual.  I don't know if we care enough about that difference to try to preserve it; from the bug history it looks as if that was just a principle-of-least-privilege thing, not for a specific attack.  I also don't know if we want to announce that in the commit message; I made it a little vague (as usual for security bugs) but that might be defeated by the code comments in any case.
Comment on attachment 8670574 [details] [diff] [review]
Expose secure_getenv as PR_GetEnvSecure

Ted, Bugzilla suggests you as reviewer, but feel free to reassign if needed.
Attachment #8670574 - Flags: review?(ted)
(In reply to Jed Davis [:jld] from comment #7)

> This exposes secure_getenv() plus NSPR's env lock as PR_GetEnvSecure, and
> uses it instead of uid/gid tests.  In the absence of secure_getenv, the
> fallback is to uid/gid comparison if XP_UNIX, and otherwise it's just
> PR_GetEnv.

You could add a check for issetugid and use that instead of the UID/GID comparison.  It's available on a couple of other systems (Solaris, some BSDs).
Comment on attachment 8670574 [details] [diff] [review]
Expose secure_getenv as PR_GetEnvSecure

Review of attachment 8670574 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with two little nits. If you can fix those I'll land this for you. (Ping me on IRC and I'll get it done.)

::: pr/src/misc/prenv.c
@@ +83,5 @@
> +
> +  return ev;
> +#else
> +#ifdef XP_UNIX
> +  if (getuid() != geteuid() || getgid() != getegid()) {

This could stand an additional comment saying that this our best-effort fallback.

::: pr/tests/env.c
@@ +63,5 @@
> +                ** copy of it) setuid / setgid / otherwise inherently
> +                ** privileged (e.g., file capabilities) and run it
> +                ** with this flag.
> +                */
> +                secure = 1;

This should probably have a check that the binary is actually running setuid/setgid and error if you pass -s, so that we don't try to run a broken test.
Attachment #8670574 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> ::: pr/tests/env.c
> @@ +63,5 @@
> > +                ** copy of it) setuid / setgid / otherwise inherently
> > +                ** privileged (e.g., file capabilities) and run it
> > +                ** with this flag.
> > +                */
> > +                secure = 1;
> 
> This should probably have a check that the binary is actually running
> setuid/setgid and error if you pass -s, so that we don't try to run a broken
> test.

That's a little tricky — to pick up the case this bug is filed for I'd also need to call getauxval(), which is a GNU extension (and also looks ELF-specific, which may or may not matter), with the same fallback to uid/gid, and I don't know if it makes sense to add that much logic to the test, given that it's meant to be run manually with special setup anyway.
Ok, that's fair.
Now with more comment.
Attachment #8670574 - Attachment is obsolete: true
Attachment #8670932 - Flags: review+
Do NSPR patches need sec-approval?  This one is fairly obvious about where vulnerabilities might be hiding in downstream applications.
Flags: needinfo?(dveditz)
(In reply to Jed Davis [:jld] from comment #14)
> Do NSPR patches need sec-approval?  This one is fairly obvious about where
> vulnerabilities might be hiding in downstream applications.

Yes, it's still part of Firefox.
Flags: needinfo?(dveditz)
(In reply to Daniel Veditz [:dveditz] from comment #15)
> (In reply to Jed Davis [:jld] from comment #14)
> > Do NSPR patches need sec-approval?  This one is fairly obvious about where
> > vulnerabilities might be hiding in downstream applications.

It turns out that Bugzilla seems to think sec-approval isn't relevant to patches in this component, so I can't ask for it even if I wanted to.  (I'd forgotten that was configurable per-product/component, or I might have checked *before* needinfo'ing.)

> Yes, it's still part of Firefox.

This bug doesn't affect Firefox — as far as I know, we don't support installing it with fscaps (which should generally be treated with the same level of caution as being setuid root).
Florian, do you know if there are already programs affected by this?  (Bug 1194680 comment #0 suggests that there might be.)  I'm trying to understand the security implications of landing this patch, because having it in a public repository in its current state effectively means disclosing the vulnerability to anyone who's watching for security-related commits.
Flags: needinfo?(fweimer)
(In reply to Jed Davis [:jld] from comment #17)
> Florian, do you know if there are already programs affected by this?

I'm pretty sure we have programs which perform an SELinux transition on start and are technically affected by the imprecise check (although actual security impact would be minor).
Flags: needinfo?(fweimer)
Kai, can/should we get this into the next NSPR release, or would it make sense to hold it until after that?
Flags: needinfo?(kaie)
Well, it's definitely not getting into that NSPR release now.  (I asked because bug 1194680 will need a release with this that NSS can depend on.)
Flags: needinfo?(kaie)
Comment on attachment 8670932 [details] [diff] [review]
Expose secure_getenv as PR_GetEnvSecure [v2]

Review of attachment 8670932 [details] [diff] [review]:
-----------------------------------------------------------------

This also needs a change to nspr.def; see bug 1209397.
Attachment #8670932 - Flags: review+ → review-
Fixed nspr.def, but the build doesn't use it on any of the OSes I have easy access to, so more eyes on it would be good.
Attachment #8670932 - Attachment is obsolete: true
Attachment #8685608 - Flags: review?(ted)
Comment on attachment 8685608 [details] [diff] [review]
Expose secure_getenv as PR_GetEnvSecure [v3]

Review of attachment 8685608 [details] [diff] [review]:
-----------------------------------------------------------------

Honestly this is just a rubber-stamp.
Attachment #8685608 - Flags: review?(ted) → review+
Keywords: sec-highsec-moderate
Whiteboard: doesn't affect Firefox
https://hg.mozilla.org/projects/nspr/rev/09f5287f368d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.12
Who's willing to contribute text for the NSPR 4.12 release notes?
Florian, do you want to?
Flags: needinfo?(fweimer)
Also, should this get a new CVE number?
(In reply to Kai Engert (:kaie) from comment #26)
> Who's willing to contribute text for the NSPR 4.12 release notes?
> Florian, do you want to?

How about:

"Added a PR_GetEnvSecure function, which returns NULL if the program was run with elevated privileges. It is recommended to use this function in general purpose library code."
Or, because the function documentation has comments, which says the detection might not work everywhere:

* Added a PR_GetEnvSecure function, which attempts to detect if the program
  is being executed with elevated privileges, and returns NULL if detected.
  It is recommended to use this function in general purpose library code.
(In reply to Kai Engert (:kaie) from comment #29)
> Or, because the function documentation has comments, which says the
> detection might not work everywhere:
> 
> * Added a PR_GetEnvSecure function, which attempts to detect if the program
>   is being executed with elevated privileges, and returns NULL if detected.
>   It is recommended to use this function in general purpose library code.

Looks good to me.
Flags: needinfo?(fweimer)
Group: core-security-release
QA Contact: jjones
You need to log in before you can comment on or make changes to this bug.