Closed Bug 1394031 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: tnguyen)

References

Details

(4 keywords, Whiteboard: [adv-main57+][post-critsmash-triage] #sbv4-m10)

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
Summary: Intermittent toolkit/components/url-classifier/tests/unit/test_safebrowsing_protobuf.js | application crashed [@ nsNSSShutDownObject::shutdown(nsNSSShutDownObject::ShutdownCalledFrom)] → Intermittent test_platform_specific_threats.js,test_pref.js ,test_safebrowsing_protobuf.js | application crashed [@ nsNSSShutDownObject::shutdown(nsNSSShutDownObject::ShutdownCalledFrom)]
Priority: P5 → P2
Whiteboard: #sbv4-m10
Looks like we have uaf bug in nsNSSShutDownList
(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
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?
Flags: needinfo?(dkeeler)
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.
Group: toolkit-core-security
Flags: needinfo?(dkeeler)
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
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?
Attachment #8906496 - Flags: review?(francois)
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
Attachment #8906496 - Attachment is obsolete: true
Attachment #8906496 - Flags: review?(francois)
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.
Attachment #8906528 - Flags: review?(francois)
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Attachment #8906528 - Flags: review?(francois) → review+
https://hg.mozilla.org/mozilla-central/rev/9cb5d9d656d2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: toolkit-core-security → core-security-release
Whiteboard: #sbv4-m10 → [adv-main57+] #sbv4-m10
Flags: qe-verify-
Whiteboard: [adv-main57+] #sbv4-m10 → [adv-main57+][post-critsmash-triage] #sbv4-m10
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.