1394031 - Intermittent test_platform_specific_threats.js,test_pref.js ,test_safebrowsing_protobuf.js | application crashed [@ nsNSSShutDownObject::shutdown(nsNSSShutDownObject::ShutdownCalledFrom)]

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Treeherder Bug Filer]

Filed by: wkocher [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=126004174&repo=mozilla-inbound

https://queue.taskcluster.net/v1/task/cvT6VZ6kSsiufWxmfImPgA/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[Intermittent Failures Robot]

2 failures in 908 pushes (0.002 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 1
* autoland: 1

Platform breakdown:
* windows7-32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1394031&startday=2017-08-21&endday=2017-08-27&tree=all

Comment 2

[Thomas Nguyen (:tnguyen)]

I guess the problem is in the line.
http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#812

I think we don't need to keep a ref of hasher like that. The hash's usage only consists of a complete set of Init, Update, and Finish, there's no reason to keep it around.

Comment 4

[Intermittent Failures Robot]

7 failures in 939 pushes (0.007 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 4
* autoland: 2
* mozilla-central: 1

Platform breakdown:
* windows7-32: 7

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1394031&startday=2017-08-28&endday=2017-09-03&tree=all

Comment 6

[Thomas Nguyen (:tnguyen)]

Looks like we have uaf bug in nsNSSShutDownList

Comment 7

[Thomas Nguyen (:tnguyen)]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #2)
> I guess the problem is in the line.
> http://searchfox.org/mozilla-central/rev/
> 18c16ebf818abb86805ce08a6e537e4cd826f044/toolkit/components/url-classifier/
> nsUrlClassifierDBService.cpp#812

I thought null out mCryptoHash would be a problem but it seems not right. nsCryptoHash implements nsNSSShutDownObject and obtain a nsNSSShutDownPreventionLock when we are trying to remove it. It should be ok

I see we have an asan failures and the uaf comes from freeing LoadLoadableRootsTask.
http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/security/manager/ssl/nsNSSComponent.cpp#2111

Comment 8

[Thomas Nguyen (:tnguyen)]

Hmm, I took a closer look at nsNSSShutDown, it's interesting. We may have a case
evaporateAllNSSResourcesAndShutDown() in Main thread
- (a) acquire sListLock then set 
http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/security/manager/ssl/nsNSSShutDown.cpp#165
- (b) release sListLock
- (c) Do restrictActivityToCurrentThread
http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/security/manager/ssl/nsNSSShutDown.cpp#168

In other thread, destruct an object like 
http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/security/manager/ssl/nsNSSComponent.cpp#1081
- (d) acquire sListLock then nsNSSActivityState::enter()
- (e) release sListLock and do shutdown(ShutdownCalledFrom::Object);

If the order is a -> b -> d -> c, shutdown(ShutdownCalledFrom::Object) will not be called in e because we will bail out at IsAlreadyShutdown(). And it will be an uaf.
I may still need to consult NSS expert about that. Hi David, am I missing anything?

Comment 9

[Dana Keeler (she/her) [:keeler]]

Yes - you're correct. See also bug 1379846 (cc'd you on it). Unfortunately we don't have a good solution right now (open to suggestions, though). What we've been doing in the meantime is working on removing the need for this infrastructure entirely, but there are a number of prerequisites (e.g. moving to a different database format for certificates and keys). For right now I think we can avoid this particular instance by removing the mCryptoHash member like you said in comment 2.

Comment 10

[Thomas Nguyen (:tnguyen)]

Attachment: Remove mCryptoHash members of nsUrlClassifierDBServiceWorker and ProtocolParser

The usage of cryptoHash consists of a complete set of Init, Update, and Finish, there's
no reason to keep it around

MozReview-Commit-ID: 7bT9IsWEM5m

Comment 11

[Thomas Nguyen (:tnguyen)]

I don't have a good solution for now, I think we may not need to use IsAlreadyShutdown in object dtor, but could add a restriction between nsNSSShutDownObject.shutdown called from dtor and the one called from shutdown observer (evaporateAllNSSResourcesAndShutDown). It seems to be more complicated.
Francois, could you please take a look at the simpler approach that removes mCryptoHash?

Comment 12

[Thomas Nguyen (:tnguyen)]

Attachment: Remove mCryptoHash members of nsUrlClassifierDBServiceWorker and ProtocolParser

The usage of cryptoHash consists of a complete set of Init, Update, and Finish, there's
no reason to keep it around

MozReview-Commit-ID: 7bT9IsWEM5m

Comment 13

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8906528 [details] [diff] [review]
Remove mCryptoHash members of nsUrlClassifierDBServiceWorker and ProtocolParser

Add a slight change, we should make sure psm initialized before the test.
That is the case : 
toolkit/components/url-classifier/tests/unit/test_bug1274685_unowned_list.js
Psm is initialised and shut downed immediately (when calling nsIOService setOffline). It is the same uaf case as we mentioned in comment 8.

Comment 14

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a5a4ca7b82047b0a55fb8b0b1fa331c2adaf1e7

Comment 15

[Thomas Nguyen (:tnguyen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb5d9d656d27a1cf2b88d62a16395ac471711cb

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/9cb5d9d656d2