1744165 - Intermittent browser/components/preferences/tests/siteData/browser_siteData2.js | application crashed [@ mozilla::dom::SessionStorageManager::ClearStorages(mozilla::OriginAttributesPattern const&, nsTSubstring<c | application terminated with exit code 11

Status: RESOLVED FIXED

(Core :: Storage: localStorage & sessionStorage, defect, P1)

Description

[Treeherder Bug Filer]

Filed by: smolnar [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=360024488&repo=autoland
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Icn_RKOZRnqDNs4wETMETw/runs/0/artifacts/public/logs/live_backing.log

---

 INFO - mozcrash Saved app info as /builds/worker/workspace/build/blobber_upload_dir/4972b5b2-fd41-6e66-e59a-a8006c282b7e.extra
[task 2021-12-02T22:55:23.527Z] 22:55:23     INFO - PROCESS-CRASH | browser/components/preferences/tests/siteData/browser_siteData2.js | application crashed [@ mozilla::dom::SessionStorageManager::ClearStorages(mozilla::OriginAttributesPattern const&, nsTSubstring<char> const&)]
[task 2021-12-02T22:55:23.527Z] 22:55:23     INFO - Crash dump filename: /tmp/tmpa5q_fhgv.mozrunner/minidumps/4972b5b2-fd41-6e66-e59a-a8006c282b7e.dmp
[task 2021-12-02T22:55:23.527Z] 22:55:23     INFO - Operating system: Linux
[task 2021-12-02T22:55:23.527Z] 22:55:23     INFO -                   0.0.0 Linux 4.4.0-1014-aws #14taskcluster1-Ubuntu SMP Tue Apr 3 10:27:00 UTC 2018 x86_64
[task 2021-12-02T22:55:23.528Z] 22:55:23     INFO - CPU: amd64
[task 2021-12-02T22:55:23.528Z] 22:55:23     INFO -      family 6 model 85 stepping 7
[task 2021-12-02T22:55:23.528Z] 22:55:23     INFO -      2 CPUs
[task 2021-12-02T22:55:23.528Z] 22:55:23     INFO - Linux Ubuntu 18.04 - bionic (Ubuntu 18.04.6 LTS)
[task 2021-12-02T22:55:23.528Z] 22:55:23     INFO - 
[task 2021-12-02T22:55:23.528Z] 22:55:23     INFO - Crash reason:  SIGSEGV / SI_KERNEL
[task 2021-12-02T22:55:23.528Z] 22:55:23     INFO - Crash address: 0x0
[task 2021-12-02T22:55:23.529Z] 22:55:23     INFO - Process uptime: not available
[task 2021-12-02T22:55:23.529Z] 22:55:23     INFO - 
[task 2021-12-02T22:55:23.529Z] 22:55:23     INFO - Thread 0  (crashed)
[task 2021-12-02T22:55:23.529Z] 22:55:23     INFO -  0  libxul.so!mozilla::dom::SessionStorageManager::ClearStorages(mozilla::OriginAttributesPattern const&, nsTSubstring<char> const&) [SessionStorageManager.cpp:394b3415b476edd81fc8728abb82c25ded4a7d16 : 583 + 0x9]
[task 2021-12-02T22:55:23.530Z] 22:55:23     INFO -     rax = 0xe5e5e5e5e5e5e5e5   rdx = 0x00007ffed2155240
[task 2021-12-02T22:55:23.530Z] 22:55:23     INFO -     rcx = 0x0000000000000000   rbx = 0x00007f787577a7e0
[task 2021-12-02T22:55:23.530Z] 22:55:23     INFO -     rsi = 0x00007ffed2154f70   rdi = 0x00007f787577a7e0
[task 2021-12-02T22:55:23.530Z] 22:55:23     INFO -     rbp = 0x00007ffed2154f50   rsp = 0x00007ffed2154f10
[task 2021-12-02T22:55:23.531Z] 22:55:23     INFO -      r8 = 0x00007ffed2154e40    r9 = 0x0000000000000000
[task 2021-12-02T22:55:23.531Z] 22:55:23     INFO -     r10 = 0x00007ffed2154d38   r11 = 0x00007f7898516100
[task 2021-12-02T22:55:23.531Z] 22:55:23     INFO -     r12 = 0x0000000080004005   r13 = 0x00007f78a093a894
[task 2021-12-02T22:55:23.531Z] 22:55:23     INFO -     r14 = 0x00007ffed2155240   r15 = 0x00007ffed2154f70
[task 2021-12-02T22:55:23.531Z] 22:55:23     INFO -     rip = 0x00007f78a5409a90
[task 2021-12-02T22:55:23.531Z] 22:55:23     INFO -     Found by: given as instruction pointer in context
[task 2021-12-02T22:55:23.531Z] 22:55:23     INFO -  1  libxul.so!mozilla::dom::SessionStorageManager::Observe(char const*, nsTSubstring<char16_t> const&, nsTSubstring<char> const&) [SessionStorageManager.cpp:394b3415b476edd81fc8728abb82c25ded4a7d16 : 606 + 0x34]
[task 2021-12-02T22:55:23.531Z] 22:55:23     INFO -     rbx = 0x00007ffed21552a0   rbp = 0x00007ffed2155050
[task 2021-12-02T22:55:23.532Z] 22:55:23     INFO -     rsp = 0x00007ffed2154f60   r12 = 0x0000000080004005
[task 2021-12-02T22:55:23.532Z] 22:55:23     INFO -     r13 = 0x00007f78a093a894   r14 = 0x00007f787577a7e0
[task 2021-12-02T22:55:23.532Z] 22:55:23     INFO -     r15 = 0x00007ffed2155240   rip = 0x00007f78a5409c81
[task 2021-12-02T22:55:23.533Z] 22:55:23     INFO -     Found by: call frame info
[task 2021-12-02T22:55:23.533Z] 22:55:23     INFO -  2  libxul.so!mozilla::dom::StorageObserver::Notify(char const*, nsTSubstring<char16_t> const&, nsTSubstring<char> const&) [StorageObserver.cpp:394b3415b476edd81fc8728abb82c25ded4a7d16 : 136 + 0xe]
[task 2021-12-02T22:55:23.533Z] 22:55:23     INFO -     rbx = 0x00007f78a093a894   rbp = 0x00007ffed21550a0
[task 2021-12-02T22:55:23.534Z] 22:55:23     INFO -     rsp = 0x00007ffed2155060   r12 = 0x00007f788c3afac0
[task 2021-12-02T22:55:23.534Z] 22:55:23     INFO -     r13 = 0x00007f78783abfa0   r14 = 0x00007ffed2155240
[task 2021-12-02T22:55:23.534Z] 22:55:23     INFO -     r15 = 0x00007ffed21552a0   rip = 0x00007f78a5411592
[task 2021-12-02T22:55:23.535Z] 22:55:23     INFO -     Found by: call frame info
[task 2021-12-02T22:55:23.535Z] 22:55:23     INFO -  3  libxul.so!mozilla::dom::StorageObserver::Observe(nsISupports*, char const*, char16_t const*) [StorageObserver.cpp:394b3415b476edd81fc8728abb82c25ded4a7d16 : 425 + 0x3e]
[task 2021-12-02T22:55:23.535Z] 22:55:23     INFO -     rbx = 0x00007ffed21552a0   rbp = 0x00007ffed2155330
INFO -     rsp = 0x00007ffed21550b0   r12 = 0x00007f788c3afac0
[task 2021-12-02T22:55:23.536Z] 22:55:23     INFO -     r13 = 0x00007f78783abfa0   r14 = 0x00007f789f494420
[task 2021-12-02T22:55:23.536Z] 22:55:23     INFO -     r15 = 0x0000000080004005   rip = 0x00007f78a5416dc2
[task 2021-12-02T22:55:23.536Z] 22:55:23     INFO -     Found by: call frame info
[task 2021-12-02T22:55:23.536Z] 22:55:23     INFO -  4  libxul.so!nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) [nsObserverList.cpp:394b3415b476edd81fc8728abb82c25ded4a7d16 : 70 + 0xe]
[task 2021-12-02T22:55:23.536Z] 22:55:23     INFO -     rbx = 0x0000000000000001   rbp = 0x00007ffed2155370
[task 2021-12-02T22:55:23.536Z] 22:55:23     INFO -     rsp = 0x00007ffed2155340   r12 = 0x0000000000000000
[task 2021-12-02T22:55:23.536Z] 22:55:23     INFO -     r13 = 0x00007f78b27957e0   r14 = 0x00007f788c3afac0
[task 2021-12-02T22:55:23.537Z] 22:55:23     INFO -     r15 = 0x00007f78783abfa0   rip = 0x00007f78a2f8a193
[task 2021-12-02T22:55:23.537Z] 22:55:23     INFO -     Found by: call frame info
[task 2021-12-02T22:55:23.537Z] 22:55:23     INFO -  5  libxul.so!nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) [nsObserverService.cpp:394b3415b476edd81fc8728abb82c25ded4a7d16 : 292 + 0x14]
[task 2021-12-02T22:55:23.537Z] 22:55:23     INFO -     rbx = 0x00007f788c3afac0   rbp = 0x00007ffed2155440
[task 2021-12-02T22:55:23.537Z] 22:55:23     INFO -     rsp = 0x00007ffed2155380   r12 = 0x00007f78783abfa0
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     r13 = 0x00007f78b27957e0   r14 = 0x00007f788c3afac0
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     r15 = 0x00007f789e22e0e0   rip = 0x00007f78a2f8c1b1
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     Found by: call frame info
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -  6  libxul.so!NS_InvokeByIndex + 0x8d
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     rbx = 0x0000000000000003   rbp = 0x00007ffed2155480
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     rsp = 0x00007ffed2155450   r12 = 0x00007f789e22e0c0
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     r13 = 0x0000000000000005   r14 = 0x00007ffed2155540
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     r15 = 0x0000000000000003   rip = 0x00007f78a3009466
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     Found by: call frame info
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -  7  libxul.so!XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [XPCWrappedNative.cpp:394b3415b476edd81fc8728abb82c25ded4a7d16 : 1130 + 0xeca]
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     rbx = 0x0000000000000003   rbp = 0x00007ffed2155690
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     rsp = 0x00007ffed2155490   r12 = 0x00007f78a0c483e0
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     r13 = 0x00007ffed2155578   r14 = 0x00007ffed2155540
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     r15 = 0x0000000000000003   rip = 0x00007f78a3a0460d
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     Found by: call frame info
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -  8  libxul.so!XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) [XPCWrappedNativeJSOps.cpp:394b3415b476edd81fc8728abb82c25ded4a7d16 : 921 + 0x7]
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     rbx = 0x00007ffed21556f8   rbp = 0x00007ffed21557c0
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     rsp = 0x00007ffed21556a0   r12 = 0x00000c3f79348240
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     r13 = 0x00000c3f79348240   r14 = 0x00007f789f4a3c10
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     r15 = 0x00007f78a88f3038   rip = 0x00007f78a3a05aa3
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     Found by: call frame info
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -  9  libxul.so!js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [Interpreter.cpp:394b3415b476edd81fc8728abb82c25ded4a7d16 : 475 + 0x148]
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     rbx = 0x00007f789857f000   rbp = 0x00007ffed2155860
 INFO -     rsp = 0x00007ffed21557d0   r12 = 0x00007f78a3a05820
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     r13 = 0x00007ffed2155928   r14 = 0x00007f7898516100
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     r15 = 0x00007ffed21557f8   rip = 0x00007f78a6dae941
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO -     Found by: call frame info
[task 2021-12-02T22:55:23.538Z] 22:55:23     INFO - 10  libxul.so!Interpret(JSContext*, js::RunState&) [Interpreter.cpp:394b3415b476edd81fc8728abb82c25ded4a7d16 : 3243 + 0x73]
<...>

Comment 1

[Daniel Veditz [:dveditz]]

Briefly looking at the test it's hard to know if content could trigger this (clear-site-data header?). It might be OK to call this UAF sec-moderate if this test is emulating a cleanup that could only happen under user initiation.

Comment 2

[Jens Stutte [:jstutte]]

From the stack it seems we try to access an already freed SessionStorageManager through this raw pointer array.

Comment 3

[Jari Jalkanen [:jari]]

Attachment: Bug 1744165 - Clear sinks and block further use after storage observer shutdown. r=#dom-storage-reviewers

Comment 4

[Jari Jalkanen [:jari]]

We crash during the storage cleaning operation at the end of the test when the unprotected and shared browsingContext suddenly becomes null. The corresponding member is stored in a RefPtr but this does not help because it is not owning the object and not all access goes through it.

The suggested solution is to store the id values so that we never need to use the unsafe browsingContext pointer. In order to not cause object lifetime related regressions, the pointer is otherwise left as before.

Comment 5

[Andreas Farre [:farre]]

So I agree with Jens in Comment 2, that the real issue here seems to be that it we're trying to access a dead SessionStorageManager, not that the BrowsingContext is dead. SessionStorageManager::mBrowsingContext is refcounted and accessed on main thread only, so as long as the SessionStorageManager lives, so should SessionStorageManager::mBrowsingContext. Which means that SessionStorageManager::CanLoad should be totally fine in doing mBrowsingContext && !mBrowsingContext->IsDiscarded().

Comment 6

[Jens Stutte [:jstutte]]

(In reply to Andreas Farre [:farre] from comment #5)

So I agree with Jens in Comment 2, that the real issue here seems to be that it we're trying to access a dead SessionStorageManager, not that the BrowsingContext is dead. SessionStorageManager::mBrowsingContext is refcounted and accessed on main thread only, so as long as the SessionStorageManager lives, so should SessionStorageManager::mBrowsingContext. Which means that SessionStorageManager::CanLoad should be totally fine in doing mBrowsingContext && !mBrowsingContext->IsDiscarded().

A possible reason for this dead SessionStorageManager pointer could be that we do not unregister all registered observers on StorageObserver::Shutdown ? Could this lead to a situation where we will not try to remove the sink in SessionStorageManager::~SessionStorageManager as StorageObserver::Self() returns nullptr ?

Comment 7

[Jan Varga [:janv]]

Yeah that's also possible. The observer service can keep StorageObserver alive even after StorageObserver::Shutdown. So it seems the shutdown method should clear mSinks. It would be also nice to call RemoveObserver for all the topics, but that is not strictly required to fix this bug.

Anyway, CheckedUnsafePtr would catch the shutdown problem. So it might be good to do add that too.

Comment 8

[Jan Varga [:janv]]

Oh, another alternative may be to hold a strong ref to StorageObserver in SessionStorageManager .

Comment 9

[Jens Stutte [:jstutte]]

(In reply to Jan Varga [:janv] from comment #7)

It would be also nice to call RemoveObserver for all the topics, but that is not strictly required to fix this bug.

Are you sure? We pass a raw pointer to our StorageObserver instance into AddObserver which could do any kind of funny things after our destruction. I would definitely want to unregister all topics, too.

Comment 10

[Jan Varga [:janv]]

And add a shutdown flag to StorageObserver to prevent handling of notifications after StorageObserver::Shutdown.

Again, it would be nice to correctly call RemoveObserver in StorageObserver::Shutdown, but that seems to be non-trivial work given that some AddObserver calls are conditional.

Comment 11

[Jan Varga [:janv]]

(In reply to Jens Stutte [:jstutte] from comment #9)

(In reply to Jan Varga [:janv] from comment #7)

It would be also nice to call RemoveObserver for all the topics, but that is not strictly required to fix this bug.

Are you sure? We pass a raw pointer to our StorageObserver instance into AddObserver which could do any kind of funny things after our destruction. I would definitely want to unregister all topics, too.

It would be safe if we set a flag in StorageObserver::Shutdown and then checked it in Observe.

Regarding the raw pointer passed to AddObserver, see https://searchfox.org/mozilla-central/rev/83e67336083df9f9a3d1e0c33f2ba19703d57161/xpcom/ds/nsIObserverService.idl#32
StorageObserver supports nsIWeakReference, so we should be safe.

That actually means that ObserverService shouldn't keep StorageObserver alive after StorageObserver::Shutdown.
Maybe something else does.

Comment 12

[Jens Stutte [:jstutte]]

I see, thanks. Then adding a flag would be indeed safe.

Comment 13

[Jens Stutte [:jstutte]]

Actually removing the sinks on shutdown as you propose earlier in comment 7 could be just enough to solve this issue.

Comment 14

[Jan Varga [:janv]]

Yeah, but StorageObserver::Observe does other things besides notifying sinks. For example it tries to send IPC messages over PBackground (in the same process). I guess it shouldn't do that after shutdown.

Comment 15

[Jari Jalkanen [:jari]]

Comment on attachment 9258274 [details]
Bug 1744165 - Clear sinks and block further use after storage observer shutdown. r=#dom-storage-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The issue occurs when StorageObserver receives a notify after it has been shut down. Users don't have access to the ClearStorages method used in the test which exposed the issue, and this has not been seen during the regular shutdown sequence. It would probably be hard to exploit this from the content process.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The relevant parts of the code have not changed much recently so backports are not expected to be hard to create.
• How likely is this patch to cause regressions; how much testing does it need?: The change mainly clears an array at shutdown and adds more checks that our object is not used after shutdown. Regressions are unlikely as this is merely adding redundancy to guarantee the old functionality under special circumstances.

Comment 16

[Tom Ritter [:tjr]]

Comment on attachment 9258274 [details]
Bug 1744165 - Clear sinks and block further use after storage observer shutdown. r=#dom-storage-reviewers

Approved to land after relman has indicated it can uplift before Jan 27th. I'm setting the flag, but you should fill out the form.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

OK to land.

Comment 18

[Jari Jalkanen [:jari]]

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #16)

Comment on attachment 9258274 [details]
Bug 1744165 - Clear sinks and block further use after storage observer shutdown. r=#dom-storage-reviewers

Approved to land after relman has indicated it can uplift before Jan 27th. I'm setting the flag, but you should fill out the form.

Filling out the form below.

Comment 19

[Jari Jalkanen [:jari]]

Comment on attachment 9258274 [details]
Bug 1744165 - Clear sinks and block further use after storage observer shutdown. r=#dom-storage-reviewers

Approval Request Comment

• Feature/Bug causing the regression: Clearing storages during browser shutdown
• User impact if declined: None
• Is this code covered by automated tests?: The issue was found by a test on CI but reproducibility is not 100%
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• List of other uplifts needed for the feature/fix: None
• Is the change risky?: Low
• Why is the change risky/not risky?: Change merely adds extra checking and cleanup to shutdown phase
• String changes made/needed: None

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/03dd276a193e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9258274 [details]
Bug 1744165 - Clear sinks and block further use after storage observer shutdown. r=#dom-storage-reviewers

Approved for 97.0b9 and 91.6esr.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/beaadf9015af
https://hg.mozilla.org/releases/mozilla-esr91/rev/e12588f4908d

Comment 23

[BugBot [:suhaib / :marco]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 24

[Jari Jalkanen [:jari]]

The survey was answered.

Comment 25

[BugBot [:suhaib / :marco]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.