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)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla57
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)
22.57 KB,
patch
|
francois
:
review+
|
Details | Diff | Splinter Review |
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 hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 2•7 years ago
|
||
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.
Updated•7 years ago
|
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)]
Comment hidden (Intermittent Failures Robot) |
Updated•7 years ago
|
Priority: P5 → P2
Whiteboard: #sbv4-m10
Assignee | ||
Comment 6•7 years ago
|
||
Looks like we have uaf bug in nsNSSShutDownList
Assignee | ||
Comment 7•7 years ago
|
||
(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
Assignee | ||
Comment 8•7 years ago
|
||
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)
Assignee | ||
Comment 10•7 years ago
|
||
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
Assignee | ||
Comment 11•7 years ago
|
||
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?
Assignee | ||
Updated•7 years ago
|
Attachment #8906496 -
Flags: review?(francois)
Assignee | ||
Comment 12•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Attachment #8906496 -
Attachment is obsolete: true
Attachment #8906496 -
Flags: review?(francois)
Assignee | ||
Comment 13•7 years ago
|
||
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)
Updated•7 years ago
|
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Updated•7 years ago
|
Attachment #8906528 -
Flags: review?(francois) → review+
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a5a4ca7b82047b0a55fb8b0b1fa331c2adaf1e7
Assignee | ||
Comment 15•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb5d9d656d27a1cf2b88d62a16395ac471711cb
https://hg.mozilla.org/mozilla-central/rev/9cb5d9d656d2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Updated•7 years ago
|
Keywords: csectype-uaf,
sec-moderate
Updated•7 years ago
|
Group: toolkit-core-security → core-security-release
Updated•7 years ago
|
Whiteboard: #sbv4-m10 → [adv-main57+] #sbv4-m10
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main57+] #sbv4-m10 → [adv-main57+][post-critsmash-triage] #sbv4-m10
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•