1194680 - (CVE-2016-5286) Environment variables are unsafe in SUID/SGID/fscaps programs

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Florian Weimer]

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

Steps to reproduce:

We have some binaries which use NSS and are SUID/SGID/fscaps.

Some of them suffer from bug 1194678 in NSPR.  Others are exposed through NSS directly, where no such checks currently seem to exist.

As explained in bug 1194678 all environment variables should be accessed through secure_getenv on GNU/Linux.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

As a side-effect of bug 1194678 I'm exporting secure_getenv from NSPR (with thread safety vs. PR_SetEnv, and fallback to the old uid/gid checks if secure_getenv isn't available) as PR_GetEnvSecure, so NSS should be able to use that after updating its NSPR dependency.

Comment 2

[Kai Engert (:KaiE:)]

Florian, let's clarify what needs to be done, and the suggesting timing.

I see that NSS has many calls to both plain "getenv" and NSPR "PR_GetEnv".

If I understand correctly, you are suggesting that we replace all those existing calls with calls to the new API PR_GetEnvSecure.

Can you confirm that my understanding is correct, and that the replacement task is exactly what's necessary to be done in this bug?


The next question is, what is the suggested timing of these actions.

Can we do the slow approach, release NSPR first, and then fix NSS a bit later?

Or - do you think we really should do everything at once, land the NSPR patch in public at the same time as NSS patch with all replacements, and attempt to release both NSPR and NSS at the same time, very quickly afterwards?

Comment 3

[Florian Weimer]

Yes, changing the function called seems reasonable to me.

Regarding scheduling, it's probably best for Huzaifa to comment.  Huzaifa, do you think disclosure of this issue needs to be coordinated with Red Hat?

Comment 4

[Kai Engert (:KaiE:)]

(In reply to Florian Weimer from comment #3)
> Regarding scheduling, it's probably best for Huzaifa to comment.  Huzaifa,
> do you think disclosure of this issue needs to be coordinated with Red Hat?

The question wasn't related to Red Hat, but more general.

Is it OK to commit the NSPR patch first, and commit the NSS patches at a later time?

In the meantime, Florian gave his assessment on IRC, that an embargo might not be required at all, because the issue can only be exploited locally, and it requires special configuration.

Comment 5

[Kai Engert (:KaiE:)]

We have several places inside NSS that don't use NSPR.

Those places cannot be changed to use the NSPR replacement function, without introducing dependenices on NSPR.

We could in theory change them to use secure_getenv, but that's gnu libc, only, and would require the introduction of #ifdef.


The following places are affected, and I'm not sure what to do about them.

nss/external_tests/google_test/gtest/include/gtest/internal/gtest-port.h
  portable helper, could read anything

lib/dbm/src/h_page.c - reads TMP/TMPDIR/TEMP

sqlite - TMPDIR

mkdepend - reading include path, 
           should be ok, only used by developers for dependency generation

Comment 6

[Kai Engert (:KaiE:)]

Attachment: 1194680-v1.patch

Comment 7

[Huzaifa Sidhpurwala]

(In reply to Florian Weimer from comment #3)
> Yes, changing the function called seems reasonable to me.
> 
> Regarding scheduling, it's probably best for Huzaifa to comment.  Huzaifa,
> do you think disclosure of this issue needs to be coordinated with Red Hat?

I know its a bit late to reply, but disclosure does not need co-ordination with Red Hat.

Comment 8

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #5)
> The following places are affected, and I'm not sure what to do about them.
> 
> nss/external_tests/google_test/gtest/include/gtest/internal/gtest-port.h
>   portable helper, could read anything
> 
> lib/dbm/src/h_page.c - reads TMP/TMPDIR/TEMP
> 
> sqlite - TMPDIR
> 
> mkdepend - reading include path, 
>            should be ok, only used by developers for dependency generation


Martin, what do you think about gtest-port.h ?

Bob, what do you think about dbm h_page.c and sqlite?

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 8692999 [details] [diff] [review]
1194680-v1.patch

This patch builds and passes the tests on my local linux machine.

(It doesn't fix the places that I mentioned in the previous comment.)

Comment 10

[Martin Thomson [:mt:]]

I don't think that we need to worry about getenv() in test code.  If we have to suppress warnings (I don't know what sort of checking is detecting these), then we should suppress them.  But if we can find a general solution, that would be good.

lib/dbm and sqlite are linked in: those could take a dependency on NSPR.  I wonder if the following addition could be made to each package:

#define getenv PR_GetEnvSecure
char* PR_GetEnvSecure(const char *var);

That might minimize the patching needed.

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8692999 [details] [diff] [review]
1194680-v1.patch

There were one or two env vars that looked like they just controlled amount of debug output, but I didn't look very closely to confirm that, because I have no objection to securing them just in case there's an input-dependent bug in the debugging code or some other way to use them in an exploit.

Comment 12

[Kai Engert (:KaiE:)]

Today, a suggestion has been made to use a two-step approach to this bug.

As a first step, an unsuspicious patch for NSS could be use, which changes all calls to getenv to a helper function within NSS, and that function still calls the old PR_GetEnv.

We want to do that early in the development process of NSS 3.23 (which will likely start in around 10 days).

Then, we want to plan a coordinated NSPR and NSS release, where both releases will be made at the same time.

The commits to both NSPR and NSS are made very close to the release date. The change to NSS can then be minimal. Only the helper function will have to be changed to call the new secure function, instead of calling the old function.

I'm not sure if this issue is sufficiently bad to use this two step approach, but it probably can't hurt to do it that way.

Comment 13

[Kai Engert (:KaiE:)]

During the next 4 weeks, Mozilla should make a decision if it wants this patch to be uplifted to older branches.

Given that this bug has the whiteboard status "doesn't affect firefox", I conclude that we don't need to worry about older firefox branches.

Comment 14

[Florian Weimer]

(In reply to Kai Engert (:kaie) from comment #12)

> I'm not sure if this issue is sufficiently bad to use this two step
> approach, but it probably can't hurt to do it that way.

I don't think this approach is needed (see also comment 7), and it appears to make the fix more complicated.

Comment 15

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/4f727a27da00

Comment 16

[Kai Engert (:KaiE:)]

Should this get a new CVE number?

Comment 17

[Kai Engert (:KaiE:)]

Suggsted wording for release notes:

* Use the new PR_GetEnvSecure function, which is the preferred way to read
  environment variables in general purpose libraries.

Comment 18

[Florian Weimer]

Where does the CVE-2016-5286 assignment come from?  Does it cover both NSPR and NSS?  Thanks.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

The CVE comes from me assigning it. There is a checkin in comment 15 so one would assume it covers that fix of this bug. That's an NSS checkin.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

This didn't affect Firefox so I didn't assign a CVE until Mitre emailed Mozilla security asking for one. Redhat could have assigned one, I guess, but clearly didn't.