951354 - ASan: Crash with heap-use-after-free when running xpcshell test getHSTSPreloadList.js

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: stack.txt

With a local repository, run the getHSTSPreloadList.js test on an ASan build of nightly. 

./run-mozilla.sh [path to]/xpcshell [path to]/getHSTSPreloadList.js [path to]/nsSTSPreloadList.inc

See attached stack.

The stack I'm providing has no symbol names, because I can't figure out how to use the ASan symbolizer script with the above. If anyone has advice, I'd appreciate it.

Comment 1

[Matt Wobensmith [:mwobensmith][:matt:]]

I should also mention that the test seems to run all of the way through, before it crashes at the very end.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

After bisecting, the first bad changeset I get is https://hg.mozilla.org/mozilla-central/rev/f78f52c8c9ff (bug 911336).

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Looks like any nsNSSShutDownObject needs to call shutdown(calledFromObject) in its destructor.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

All other classes that inherit from nsNSSShutdownObject that I could find do call shutdown(calledFromObject). One question is there seems to be some inconsistencies in whether or not the nsNSSShutdownPreventionLock is used and/or isAlreadyShutDown() is called. I'll investigate further to see if this matters.

Comment 5

[Camilo Viecco (:cviecco)]

Comment on attachment 8350229 [details] [diff] [review]
patch

I missed that one.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: test

Turns out a compiled code test was the easiest way to test this. Unfortunately, I had to add another test directory ("compiled") under security/manager/ssl/tests/ (it was that or change all of the moz.build files in that subtree to say TEST_TOOL_DIRS instead of TEST_DIRS or DIRS). I'm open to suggestions for alternatives to either of these.

Comment 7

[Camilo Viecco (:cviecco)]

Comment on attachment 8350805 [details] [diff] [review]
test

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

I like this.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8350229 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I don't think it is possible to trigger this bug in Firefox itself. nsNSSComponent causes all NSS resources to be released when it receives the "profile-before-change" notification or in its destructor. When using a ScopedXPCOMStartup (as it appears Firefox does, but xpcshell doesn't), it will always be notified before receiving the "xpcom-shutdown" notification, which is when services are destructed. If there is no profile, however (as can be the case with xpcshell), "profile-before-change" is never emitted, and if nsNSSCertificateDB is destructed before nsNSSComponent, when nsNSSComponent tries to free all NSS resources, it will use the now dead nsNSSCertificateDB object and segfault.

So, in short, I don't think this is exploitable in Firefox.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Not really - determining the security problem would require familiarity with how NSS shutdown works

Which older supported branches are affected by this flaw?

27 and 28 (beta and aurora)

If not all supported branches, which bug introduced the flaw?

bug 911336

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch applies cleanly to beta and aurora.

How likely is this patch to cause regressions; how much testing does it need?

Very unlikely.

Comment 9

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

Comment on attachment 8350229 [details] [diff] [review]
patch

sec-approval+ for trunk. Please check in and, assuming all is green, nominate for Aurora and Beta so we can avoid ever shipping this issue.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0aced80aa98

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d0aced80aa98

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8350229 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 911336
User impact if declined: potentially exploitable
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7863c4ddd409
https://hg.mozilla.org/releases/mozilla-beta/rev/fba7bb9253f1

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

ni? to myself so I don't forget to land the test when appropriate.

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8350805 [details] [diff] [review]
test

[Security approval request comment]
This is just the test for the fix. The fix itself has shipped on all affected versions, I believe.

Comment 16

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

Comment on attachment 8350805 [details] [diff] [review]
test

sec-approval+ to check in the test.

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thanks, Al.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8526c7a38761
https://hg.mozilla.org/releases/mozilla-aurora/rev/24f26d63819a
https://hg.mozilla.org/releases/mozilla-beta/rev/c26f88fa740f

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And backed out from all three for not passing the tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41a0bf080410
https://hg.mozilla.org/releases/mozilla-aurora/rev/1f9d7af12960
https://hg.mozilla.org/releases/mozilla-beta/rev/e75d291a556f

Example failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=33202802&tree=Mozilla-Beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=33202668&tree=Mozilla-Aurora

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: test v1.1

OS X and Windows were complaining because a pref wasn't set (which was because this test can't rely on some services, etc. that are commonly available, because they would keep the certificate db alive past the scope of the pointer in the test, which defeats the whole point of the test (which tests that if the db is destructed before NSS is shut down, it does the appropriate cleanup)).
Carrying over r+.
https://tbpl.mozilla.org/?tree=Try&rev=f47f89cfab86

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da1b1e747466

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da1b1e747466

Comment 22

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ad0044f9976
https://hg.mozilla.org/releases/mozilla-beta/rev/1bca9c9e0ae7

Comment 23

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: m-c_asan_crash.txt

Both FF27 and FF28 look good, but FF29 ASan produces this crash.

Comment 24

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

That looks like a new bug - I filed bug 965379.

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Keeler. I'll go ahead and mark this one verified for 29, based on that. Much appreciated.