<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Andrew McCreight [:mccr8]

Reporter

Comment 1

•

4 years ago

Attached file asan.windows10.txt — Details

Comment 2

•

4 years ago

It looks like NotificationController::Shutdown is being called on a freed mPressShell.

Group: firefox-core-security → dom-core-security

Component: Security → Disability Access APIs

Keywords: csectype-uaf

Product: Firefox → Core

Andrew McCreight [:mccr8]

Comment 3

•

4 years ago

•

Edited

Just looking at the stack, it kind of looks like mPresShell should be rooted on this line in nsDocumentViewer::LoadComplete: EventDispatcher::Dispatch(window, mPresContext, &event, nullptr, &status);. There's an earlier scope in this method that holds a strong stack ref to the pres shell with the comment "Hold strong ref because this could conceivably run script" but I'm not sure why it wouldn't be needed here.

Comment 4

•

4 years ago

What puzzles me about this is that PresShell::Destroy should call DocAccessible::Shutdown. So, if NotificationController has a freed PresShell, that means PresShell::Destroy never got called? Or a DocAccessible was somehow created with a freed PresShell? Or a DocAccessible was created with a PresShell after the PresShell was destroyed (but before it was freed)?

It's probably worth at least adding an assertion to the DocAccessible constructor to ensure it never gets passed a destroyed PresShell.

Comment 5

•

4 years ago

It is possible that the test case is doing something weird like calling FuzzingFunctions.enableAccessibility() during an event or something. Hopefully if we get a test case it will shed some light on the situation.

Reporter

Comment 6

•

4 years ago

Alright I've refactored the code, it require responsive design mode + multiple fast click to trigger the UAF. I think it's still possible to trigger the UAF without RDM which is interesting to find out.

There are 3 ways to trigger the crash, with ./mach run (e10s enabled) and ./mach run --disable-e10s (e10s disabled). On e10s disabled, it doesn't require launcher or reload to trigger the crash (which is more easy to trigger).

Steps to Reproduce:

e10s enabled (with launcher):

Open Firefox with fuzzing build (ac_add_options --enable-fuzzing)
Go to about:config
Set fuzzing.enabled to true
Set dom.disable_open_during_load to false
Restart Firefox
Visit attached launcher.bundle.html
Click "Launch" button
Repeatedly click the text then simultaneously press ctrl+shift+m every ~2s
After repeated tries, the tab is crashed

e10s enabled (close the tab):

Open Firefox with fuzzing build (ac_add_options --enable-fuzzing)
Go to about:config
Set fuzzing.enabled to true
Restart Firefox
Visit attached testcase.main.html
Click "Launch" button
Repeatedly click the text then simultaneously press ctrl+shift+m every ~2s
After trying for a few seconds, close the tab
ASan will show heap-use-after-free

e10s disabled:

Open Firefox with fuzzing build (ac_add_options --enable-fuzzing) then ./mach run --disable-e10s
Go to about:config
Set fuzzing.enabled to true
Visit attached testcase.main.html
Repeatedly click the text then simultaneously press ctrl+shift+m every ~2s
The tab is crashed.

(I also will attach steps to reproduce video to demonstrate the crash)

Reporter

Comment 7

•

4 years ago

Attached file launcher.bundle.html (obsolete) — Details

Reporter

Comment 8

•

4 years ago

Attached file testcase.reload.html (obsolete) — Details

Reporter

Comment 9

•

4 years ago

Attached file testcase.main.html (obsolete) — Details

Reporter

Comment 10

•

4 years ago

Attached video Firefox - UAF in DocAccessible::Shutdown - e10s enabled launcher.mp4 (obsolete) — Details

Reporter

Comment 11

•

4 years ago

Attached video Firefox - UAF in DocAccessible::Shutdown - e10s disabled.mp4 — Details

Reporter

Comment 12

•

4 years ago

Attached file asan.e10senabled.txt — Details

Reporter

Comment 13

•

4 years ago

Attached file asan.e10sdisabled.txt — Details

Comment 14

•

4 years ago

This could be more of a fuzzing harness problem than something that could be reproduced in the wild, given that fuzzing hook function wasn't really designed to be called at random times repeatedly, and wouldn't be in a real web page.

but needs more investigation to be sure of that.

Reporter

Comment 15

•

4 years ago

(In reply to Daniel Veditz [:dveditz] from comment #14)

This could be more of a fuzzing harness problem than something that could be reproduced in the wild, given that fuzzing hook function wasn't really designed to be called at random times repeatedly, and wouldn't be in a real web page.

but needs more investigation to be sure of that.

I've installed NVDA on Windows 10, then run the launcher.bundle.html (with steps to reproduce above) on Firefox 90.0a1 (2021-05-12) (64-bit) official build, the Firefox crashes on the first try with signature mozilla::PresShell::RemoveRefreshObserver (as on attached ASan stack above).

Hereby the crash reports: https://crash-stats.mozilla.org/report/index/4bad5f58-da2c-4262-b017-7f82a0210512

Reporter

Comment 16

•

4 years ago

In the crash report above, one of the CPU registers shows "0xe5e5e5e5e5e5e5e5" indicating UAF.

Reporter

Comment 17

•

4 years ago

I can also reproduce this on Arch Linux with Orca screen reader activated using Firefox Nightly 90.0a1 (2021-05-17) (64-bit) (Official Build) then run launcher.bundle.html without FuzzingFunctions.enableAccessibility() on the code.

Interestingly I got 3 different crash reports as follows:

Reporter

Comment 18

•

4 years ago

(In reply to Irvan Kurniawan (:sourc7) from comment #15)

I've installed NVDA on Windows 10, then run the launcher.bundle.html (with steps to reproduce above) on Firefox 90.0a1 (2021-05-12) (64-bit) official build, the Firefox crashes on the first try with signature mozilla::PresShell::RemoveRefreshObserver (as on attached ASan stack above).

It turns out that I also able to reproduce this when using Microsoft Narrator (Windows built in screen reader).

I'm sure that as long the Accessibility shows Activated: true on about:support it able to reproduce on Firefox official build (same as using FuzzingFunctions.enableAccessibility() on Firefox fuzzing build)

Comment 19

•

4 years ago

I can't reproduce this here with the NVDA screen reader despite many attempts. :(

I can think of some assertions we could add, but that's probably not helpful because catching local build assertions in content processes is currently really difficult, at least on Windows. I guess we could land some diagnostic assertions to try to catch the problem on crash-stats, but that might expose the bug.

In addition to my thoughts in comment 4, I wonder whether somehow a second DocAccessible is being created for the same PresShell. If that happens, PresShell::SetDocAccessible would get called, overriding its DocAccessible. When the PresShell gets destroyed, it would shut down the second DocAccessible, but not the first, leaving the first with a destroyed DocSAccessible. That said, I don't see how a second DocAccessible could be created; we always ask the PresShell for its DocAccessible before creating a new one.

:dveditz, do you think it'd make sense to land these as DIAGNOSTIC_ASSERTs? If so, how should I go about doing that without exposing a potential UAF?

Flags: needinfo?(dveditz)

Reporter

Comment 20

•

4 years ago

(In reply to James Teh [:Jamie] from comment #19)

In addition to my thoughts in comment 4, I wonder whether somehow a second DocAccessible is being created for the same PresShell. If that happens, PresShell::SetDocAccessible would get called, overriding its DocAccessible. When the PresShell gets destroyed, it would shut down the second DocAccessible, but not the first, leaving the first with a destroyed DocSAccessible. That said, I don't see how a second DocAccessible could be created; we always ask the PresShell for its DocAccessible before creating a new one.

In another testcase it throw assertion as follow Assertion failure: !mDocument->GetPresShell() (Where did this shell come from?), at layout/base/PresShell.cpp:4121.

From the assertion message, I think it's related to this bug. I also found the assertion and UAF occur because of combination of ctrl+shift+m and mouse click. Without mouse click the RemoveRefreshObserver won't use the freed mPressShell.

Reporter

Comment 21

•

4 years ago

(In reply to Irvan Kurniawan (:sourc7) from comment #20)

In another testcase it throw assertion as follow Assertion failure: !mDocument->GetPresShell() (Where did this shell come from?), at layout/base/PresShell.cpp:4121.

From the assertion message, I think it's related to this bug. I also found the assertion and UAF occur because of combination of ctrl+shift+m and mouse click. Without mouse click the RemoveRefreshObserver won't use the freed mPressShell.

Hmm, I'm not sure whether it is related or not, hereby the signature report for mozilla::PresShell::DoFlushPendingNotifications.

I think someone who knows about PresShell could look into this bug, hopefully it will solve this problem too.

Comment 22

•

4 years ago

If you're able to see assertions, perhaps I can provide a patch which adds assertions I'd like to verify for the accessibility crash?

diff --git a/accessible/generic/DocAccessible.cpp b/accessible/generic/DocAccessible.cpp
index c7411cb66f724..e19674c6d9556 100644
--- a/accessible/generic/DocAccessible.cpp
+++ b/accessible/generic/DocAccessible.cpp
@@ -98,6 +98,10 @@ DocAccessible::DocAccessible(dom::Document* aDocument,
   mDoc = this;
 
   MOZ_ASSERT(mPresShell, "should have been given a pres shell");
+  MOZ_ASSERT(!mPresShell->IsDestroying(),
+      "Should never get a destroying PresShell");
+  MOZ_ASSERT(!mPresShell->GetDocAccessible(),
+      "PresShell shouldn't already have a DocAccessible");
   mPresShell->SetDocAccessible(this);
 }

Reporter

Comment 23

•

4 years ago

(In reply to James Teh [:Jamie] from comment #22)

If you're able to see assertions, perhaps I can provide a patch which adds assertions I'd like to verify for the accessibility crash?

diff --git a/accessible/generic/DocAccessible.cpp b/accessible/generic/DocAccessible.cpp
index c7411cb66f724..e19674c6d9556 100644
--- a/accessible/generic/DocAccessible.cpp
+++ b/accessible/generic/DocAccessible.cpp
@@ -98,6 +98,10 @@ DocAccessible::DocAccessible(dom::Document* aDocument,
   mDoc = this;
 
   MOZ_ASSERT(mPresShell, "should have been given a pres shell");
+  MOZ_ASSERT(!mPresShell->IsDestroying(),
+      "Should never get a destroying PresShell");
+  MOZ_ASSERT(!mPresShell->GetDocAccessible(),
+      "PresShell shouldn't already have a DocAccessible");
   mPresShell->SetDocAccessible(this);
 }

Sorry I forgot to add, the assertion on comment 20 is from another testcase, it not required Accessiblity and more easily triggered just by switch to Responsive Design Mode (ctrl+shift+m) then click the text. I'm still not sure whether that related or not, but it give a clue as PresShell is also on the stack.

Thanks for the patch, I've added both assertion to my Firefox ASan build, but unfortunately the browser still crashes with heap-use-after-free.

Comment 24

•

4 years ago

Okay. Thanks for trying it. In that case, no point in landing these assertions for this bug.

Back to the drawing board.

Flags: needinfo?(dveditz)

Andrew McCreight [:mccr8]

Comment 25

•

4 years ago

Oh, I see the assertions in comment 20 are DIAGNOSTIC_ASSERTs. Do ASan builds have debug asserts enabled? It might be worth changing the MOZ_ASSERTs in my patch to MOZ_DIAGNOSTIC_ASSERT.

Comment 26

•

4 years ago

The usual ASan builds are opt, so they don't have MOZ_ASSERT enabled.

Reporter

Comment 27

•

4 years ago

(In reply to James Teh [:Jamie] from comment #25)

Oh, I see the assertions in comment 20 are DIAGNOSTIC_ASSERTs. Do ASan builds have debug asserts enabled? It might be worth changing the MOZ_ASSERTs in my patch to MOZ_DIAGNOSTIC_ASSERT.

Still same, it still crash with heap-use-after-free.

Reporter

Comment 28

•

4 years ago

Alright I've captured the UAF with RR, here the pernosco debugging session link: https://pernos.co/debug/2EryKI_CiVGymWBqd-qw-A/index.html

Comment 29

•

4 years ago

Thanks.

Eitan, Pernosco is still mostly unusable with a screen reader. :( Would you mind taking a look at this?

Flags: needinfo?(eitan)

Assignee

Comment 30

•

4 years ago

A DocAccessible is not supposed to outlive its PresShell. The DocAccessible indirectly holds a weak ref to the PresShell (via NotificationController). When the doc is constructed, it passes its pointer to its PresShell so that when the PresShell is destroyed it will shutdown the doc, and thus a weak reference is assumed to be safe.

This crash happens when a doc is shutdown after its presshell is destroyed. This happens because a doc is constructed, and sets its reference on its presshell via SetDocAccessible, but then when the doc is inserted into its outerdoc, the previous doc is shutdown, and calls SetDocAccessible with nullptr on the same PresShell. So the PresShell never has a valid reverence to the current DocAccessible. This typically is not a problem unless the PresShell is destroyed before the DocAccessible is shutdown, which is what happens here.

Flags: needinfo?(eitan)

Comment 31

•

4 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #30)

but then when the doc is inserted into its outerdoc, the previous doc is shutdown,

But how did we create a new doc for that PresShell without shutting the other one down first? As I understand it, that's never supposed to happen. DocManager::GetDocAccessible always checks for an existing DocAccessible on the PresShell first.

Even if a doc is bound to its OuterDoc late, we just bind the doc; we don't create a new one.

Flags: needinfo?(eitan)

Assignee

Comment 32

•

4 years ago

A DOM document is not guaranteed to have the same presshell thru its entire lifetime, see Document::CreatePresShell and Document::DeletePresShell. So if the DocManager tries to find a DocAccessilble with a Document that had its PresShell swapped, it will fail. This will create a new DocAccessible.

Flags: needinfo?(eitan)

Assignee

Comment 33

•

4 years ago

To add to that, you would think PresShell::Destroy would be called on the old presshell to shutdown the previous DocAccessible, but that seems to not be the case.

Comment 34

•

4 years ago

Right... and that's the really problematic piece here. If that's the cause of the problem, why isn't PresShell::Destroy being called? And if that's intentional, where else are we supposed to shut down the DocAccessible?

Reporter

Comment 35

•

4 years ago

Attached file valgrind.txt — Details

From Valgrind report it show 2 invalid read and 1 invalid write on following function:

Invalid read of size 8 on mozilla::PresShell::RemoveRefreshObserver -> GetPresContext
Invalid read of size 8 on mozilla::a11y::SelectionManager::RemoveDocSelectionListener -> nsFrameSelection
Invalid write of size 8 on mozilla::a11y::DocAccessible::Shutdown()-> SetDocAccessible

Comment 36

•

4 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #30)

This happens because a doc is constructed, and sets its reference on its presshell via SetDocAccessible

If there were a previous doc, this should have triggered the assertion I suggested adding in comment 22, but it didn't.

We could also try this assertion, but I don't think it'll help if the other one didn't:

diff --git a/accessible/generic/DocAccessible.cpp b/accessible/generic/DocAccessible.cpp
index e19674c6d9556..20486bf7fdcc9 100644
--- a/accessible/generic/DocAccessible.cpp
+++ b/accessible/generic/DocAccessible.cpp
@@ -415,6 +415,8 @@ void DocAccessible::Shutdown() {
     MOZ_ASSERT(!mParent, "Parent has to be null!");
   }
 
+  MOZ_ASSERT(mPresShell->GetDocAccessible() == this,
+      "PresShell should reference this DocAccessible");
   mPresShell->SetDocAccessible(nullptr);
   mPresShell = nullptr;  // Avoid reentrancy

Comment 37

•

4 years ago

(Of course, if testing with an opt build, that should be MOZ_DIAGNOSTIC_ASSERT)

Reporter

Comment 38

•

4 years ago

(In reply to James Teh [:Jamie] from comment #36)

(In reply to Eitan Isaacson [:eeejay] from comment #30)

This happens because a doc is constructed, and sets its reference on its presshell via SetDocAccessible

If there were a previous doc, this should have triggered the assertion I suggested adding in comment 22, but it didn't.

We could also try this assertion, but I don't think it'll help if the other one didn't:
diff --git a/accessible/generic/DocAccessible.cpp b/accessible/generic/DocAccessible.cpp
index e19674c6d9556..20486bf7fdcc9 100644
--- a/accessible/generic/DocAccessible.cpp
+++ b/accessible/generic/DocAccessible.cpp
@@ -415,6 +415,8 @@ void DocAccessible::Shutdown() {
     MOZ_ASSERT(!mParent, "Parent has to be null!");
   }
 
+  MOZ_ASSERT(mPresShell->GetDocAccessible() == this,
+      "PresShell should reference this DocAccessible");
   mPresShell->SetDocAccessible(nullptr);
   mPresShell = nullptr;  // Avoid reentrancy
 

I've added the assertion using MOZ_DIAGNOSTIC_ASSERT on ASan build, but unfortunately it still crashes with UAF on RemoveRefreshObserver.

Reporter

Comment 39

•

4 years ago

In the Arch Linux and Windows 10 when using debug build it hit assertion as follow:
Assertion failure: ObserverCount() == mEarlyRunners.Length() (observers, except pending selection scrolls, should have been unregistered), at /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:1153

Assignee

Comment 40

•

4 years ago

OK. I was just kidding about all that and I was way off. Looks like yes indeed presshell/docacc pairs get out of sync, that is why docaccessibles keep on getting created when there should already be a live one.

But i needed to figure out the origin of the mixup.. looks like during the Destroy method of a presshell, its DocAccessible is correctly shut down, but if there is a selection change that needs to get flushed it happens via MaybeReleaseCapturingContent in the PresShell's destroy method after the current DocAccessible was shut down. This triggers our selection manager to spawn a new DocAccessible in the Destroy phase of the presshell, and things get out of whack.

This shouldn't happen because the old DocAccessible's shutdown should remove the doc's/presshell's selection listeners. But this isn't the case because we add special control listeners via SetControlSelectionListener and don't clear those outside of focus changes. I think those should be cleared in doc shutdown as well.

This is a similar issue to bug 1330739, I think the weak references might still be of use there in case of SelectionManager/DocManager shutdown.

Assignee

Comment 41

•

4 years ago

Attached file Bug 1709976 - Remove selection listeners from shutting down PresShell. r?Jamie — Details

Phabricator Automation

Updated

•

4 years ago

Assignee: nobody → eitan

Status: NEW → ASSIGNED

Assignee

Comment 42

•

4 years ago

Since this crash is hard to reproduce, i would rely on the reporter to verify that this is actually remedied.

Comment 43

•

4 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #40)

This triggers our selection manager to spawn a new DocAccessible in the Destroy phase of the presshell, and things get out of whack.

That was the purpose of the mPresShell->IsDestroying() assertion I suggested, but it turns out that mIsDestroying is set pretty late in PresShell::Destroy(), after the DocAccessible is shut down and MaybeReleaseCapturingContent is called. Ug.

Assignee

Comment 44

•

4 years ago

I didn't realize that was your suggestion. I thought of moving it earlier but don't know what that would screw with.

Assignee

Comment 45

•

4 years ago

Irvan,

Is there a chance you can try this patch and verify it remedies the issue?

Flags: needinfo?(susah.yak)

Comment 46

•

4 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #44)

I didn't realize that was your suggestion.

It was earlier in the bug; see comment 22. Anyway, this wouldn't have fixed the bug; it just would have made it easier to catch. :)

Thanks for figuring this out (and patching it).

Reporter

Comment 47

•

4 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #45)

Irvan,

Is there a chance you can try this patch and verify it remedies the issue?

Thanks! It finally solve the issue!

After applied the patch (on Arch Linux), I no longer able to reproduce the crash, and on debug build it no longer hit the assertion on comment 39.

Flags: needinfo?(susah.yak)

Reporter

Comment 48

•

4 years ago

Attached file launcher.bundlewfx.html (obsolete) — Details

(In reply to Eitan Isaacson [:eeejay] from comment #42)

Since this crash is hard to reproduce, i would rely on the reporter to verify that this is actually remedied.

Eitan, can you reproduce with steps to reproduce below:

Open Microsoft Narrator
Open Firefox Nightly
Go to about:config
Set dom.disable_open_during_load to false (to disable pop-up blocker)
Visit attached launcher.bundlewfx.html
Click "Launch" button
Repeatedly fast click the text as possible (my click per second: 7) then simultaneously press ctrl+shift+m every ~1s (as on attached video below)
The tab will crashed.

Reporter

Comment 49

•

4 years ago

Attached video Firefox - UAF in DocAccessibleShutdown - Windows 10 with Narrator.mp4 (obsolete) — Details

Reporter

Updated

•

4 years ago

Summary: AddressSanitizer: heap-use-after-free in [mozilla::PresShell::RemoveRefreshObserver] and [@ get] → AddressSanitizer: heap-use-after-free in [@ mozilla::PresShell::RemoveRefreshObserver]

Comment 50

•

4 years ago

Given the user interaction required to mostly-reliably trigger this race this is sec-moderate in severity

Keywords: sec-moderate

Julien Cristau [:jcristau]

Assignee

Comment 51

•

4 years ago

Comment on attachment 9223266 [details]
Bug 1709976 - Remove selection listeners from shutting down PresShell. r?Jamie

Security Approval Request

How easily could an exploit be constructed based on the patch?: Not at all. This fixes an issue well before any potential UAF.
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?:
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?: Since this patch has no test coverage, and since this is a hard to reproduce UAF I propose that this shouldn't be backported.
How likely is this patch to cause regressions; how much testing does it need?:

Attachment #9223266 - Flags: sec-approval?

Comment 52

•

4 years ago

The sec-moderate rating means you can land without sec-approval.

Flags: needinfo?(eitan)

Reporter

Comment 53

•

4 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #51)

If not, how different, hard to create, and risky will they be?: Since this patch has no test coverage, and since this is a hard to reproduce UAF I propose that this shouldn't be backported.

Glady! I found new test case that gets triggered by pressing ctrl+shift+m then just with 1-click which is very easy to reproduce. I'll attach the new testcase in a moment.

Reporter

Comment 54

•

4 years ago

Attached file launcher.bundle.html — Details

Hereby I attach the new testcase "launcher.bundle.html" which triggers UAF more easily and reliably (works every time).

When in responsive design mode then one click inside RDM viewport it will crash with use-after-free.

Steps to reproduce

e10s enabled:

Visit attached launcher.bundle.html
Click "Launch Testcase"
After switched to new window, press Ctrl + Shift + M to switch to Responsive Design Mode
Click duration section on <audio> element or click anywhere inside RDM viewport
The tab will crash with use-after-free

e10s disabled:

Visit attached testcase.main.html
Press Ctrl + Shift + M to switch to Responsive Design Mode
Click duration section on <audio> element or click anywhere inside RDM viewport
The tab will crash with use-after-free

Attachment #9221225 - Attachment is obsolete: true

Attachment #9223279 - Attachment is obsolete: true

Reporter

Comment 55

•

4 years ago

Attached file testcase.main.html — Details

Attachment #9221226 - Attachment is obsolete: true

Attachment #9221227 - Attachment is obsolete: true

Reporter

Comment 56

•

4 years ago

Sorry I forgot to mention on new STR above, it require screen reader e.g. Microsoft Narrator, NVDA (on Windows 10), or Orca (on Linux) to activate Accesibility Activated: true on about:support in order to reproduce the UAF.

Reporter

Comment 57

•

4 years ago

Attached video Firefox - UAF in DocAccessibleShutdown with one-click - Windows 10 using Microsoft Narrator.mp4 — Details

Attachment #9221231 - Attachment is obsolete: true

Attachment #9223280 - Attachment is obsolete: true

Updated

•

4 years ago

Crash Signature: [@ nsRefreshDriver::RemoveRefreshObserver ]

Updated

•

4 years ago

Crash Signature: [@ nsRefreshDriver::RemoveRefreshObserver ] → [@ nsRefreshDriver::RemoveRefreshObserver ] [@ mozilla::detail::MutexImpl::lock ] [@ mozilla::PresShell::RemoveRefreshObserver ]

Updated

•

4 years ago

Type: task → defect

status-firefox88: --- → wontfix

status-firefox89: --- → wontfix

status-firefox90: --- → affected

status-firefox91: --- → affected

status-firefox-esr78: --- → affected

tracking-firefox90: --- → +

tracking-firefox91: --- → +

tracking-firefox-esr78: --- → 91+

Comment 58

•

4 years ago

Which older supported branches are affected by this flaw?:

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

Presumably all branches are affected based on those answers, and I did reproduce a crash on ESR and Release.

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: Since this patch has no test coverage, and since this is a hard to reproduce UAF I propose that this shouldn't be backported.

The code being patched is identical to what's on ESR-78 and Release so it should apply easily, though I can't say whether other relevant context changed around it. It was not hard to reproduce the crash with Irvan's latest testcases. They were very consistent, though less obviously UAF crashes in a release build. The ESR-78 crash behavior was different from the others -- it hung up my browser (partially responsive but mostly dead) and I had to force-kill it from the task manager. The ultimately reported crash had the same signature though -- presumably sent from the crashing tab before the browser became unresponsive as a whole.

ESR-78: bp-a4946ab8-9039-468c-b580-d10880210602
Release (88.0.1): bp-cc018aad-9d65-4b2d-84d5-729300210602
Nightly: bp-a3a23889-d9f0-4294-b0d3-29c240210602

It would be easy to verify the fix if this is backported, but I don't know about regressions.

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

This is an important question to get your opinion on.

status-firefox89: wontfix → affected

tracking-firefox-esr78: 91+ → ---

Assignee

Comment 59

•

4 years ago

I think the chance that this patch causes a regression is very small but isn't 0. Maybe we can uplift to release, and wait a bit before ESR?

Flags: needinfo?(eitan)

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 60

•

4 years ago

Comment on attachment 9223266 [details]
Bug 1709976 - Remove selection listeners from shutting down PresShell. r?Jamie

Approved to land and uplift. I'm not sure about delaying the ESR patch one release; we try to avoid doing that.

Attachment #9223266 - Flags: sec-approval?

Attachment #9223266 - Flags: sec-approval+

Attachment #9223266 - Flags: approval-mozilla-esr78+

Attachment #9223266 - Flags: approval-mozilla-beta+

Comment 61

•

4 years ago

Remove selection listeners from shutting down PresShell. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/65d3494db521b3d15112c092d0647a2899808381
https://hg.mozilla.org/mozilla-central/rev/65d3494db521

Group: dom-core-security → core-security-release

Status: ASSIGNED → RESOLVED

Closed: 4 years ago

status-firefox91: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → 91 Branch

Ryan VanderMeulen [:RyanVM]

Updated

•

4 years ago

status-firefox89: affected → wontfix

tracking-firefox-esr78: --- → 90+

Julien Cristau [:jcristau]

Comment 62

•

4 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/81a6b75e5224

status-firefox90: affected → fixed

Ryan VanderMeulen [:RyanVM]

Comment 63

•

4 years ago

uplift

https://hg.mozilla.org/releases/mozilla-esr78/rev/1c6290c3e4e1

status-firefox-esr78: affected → fixed

Comment 64

•

4 years ago

Somewhere around 4.7% of users (beta users, at least) have accessibility enabled: https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2021-06-08&include_spill=0&keys=__none__!__none__!__none__&max_channel_version=beta%252F90&measure=A11Y_INSTANTIATED_FLAG&min_channel_version=beta%252F88&processType=*&product=Firefox&sanitize=1&sort_by_value=0&sort_keys=submissions&start_date=2021-05-31&table=0&trim=1&use_submission_date=0

That's several million people every day so seems worth calling this one sec-high after all.

Keywords: sec-moderate → sec-high

BugBot [:suhaib / :marco/ :calixte]

Updated

•

4 years ago

Flags: sec-bounty? → sec-bounty+

Comment 65

•

4 years ago

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.

Flags: needinfo?(eitan)

Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][sec-survey]

Cornel Ionce [:noni] [Hubs QA]

Assignee

Updated

•

4 years ago

Flags: needinfo?(eitan)

Updated

•

4 years ago

QA Whiteboard: [post-critsmash-triage]

Flags: qe-verify+

Rares Doghi, Desktop QA

Comment 66

•

4 years ago

Hi Irvan, I have been using your steps from Comment 54 and your attached test case in order to verify the fix in our latest Nightly Asan Fuzzing build i got from :

https://treeherder.mozilla.org/jobs?repo=mozilla-central&selectedTaskRun=NCAkqN8eTBuc7_dJsX3f7Q.0

I started Narrator on Windows 10 and opened the ASAN Fuzzing Build (here tried both ways with fuzzing.enabled = true/false ) and loaded the attached case in a new tab.
Clicked Launch Testcase wich opened the audio player in a new tab.
Hit Ctrl Shift M in order to start RDM and click the seek bar for the audio a few times

Tab Crashed
Please also note that I was able to reproduce the Tab crash in older Nightly asan fuzzing builds.

I also tried this with Beta 90.0b1 and Firefox didn't crash it would just close the tab , also tried the same thing with non asan Fuzzing builds , normal builds would not crash, they would just close the tab.

Am I missing some steps ? or Prefs I might need to set before launching the test case ? Is the Expected result to simply Close the Tab ? or it's suppose to keep the tab open without crashing it ?

Flags: needinfo?(susah.yak)

Reporter

Comment 67

•

4 years ago

Attached video Firefox 89.0.2 - RemoveRefreshObserver Crash.mp4 — Details

(In reply to Rares Doghi from comment #66)

I also tried this with Beta 90.0b1 and Firefox didn't crash it would just close the tab , also tried the same thing with non asan Fuzzing builds , normal builds would not crash, they would just close the tab.

Am I missing some steps ? or Prefs I might need to set before launching the test case ? Is the Expected result to simply Close the Tab ? or it's suppose to keep the tab open without crashing it ?

I still able to reproduce this in Firefox 89.0.2 (64-bit) on Windows 10 (video attached), make sure to click the duration section (0:00 / 0:00) (not the seek bar section) or the white page section as on attached video. I hope this helps!

Flags: needinfo?(susah.yak)

Rares Doghi, Desktop QA

Comment 68

•

4 years ago

Thank you Irvan, I was able to reproduce the issue in older builds and Verify the fix in ESR 78.12.0esr, Beta 90.0b12 and our latest Nightly build on Windows 10 using Narrator and Ubuntu 20.04 using Screen Reader (orca)

Status: RESOLVED → VERIFIED

status-firefox90: fixed → verified

status-firefox91: fixed → verified

status-firefox-esr78: fixed → verified

Flags: qe-verify+

Updated

•

4 years ago

Whiteboard: [reporter-external] [client-bounty-form] [verif?][sec-survey] → [reporter-external] [client-bounty-form] [verif?][sec-survey][adv-main90+]

Updated

•

4 years ago

Whiteboard: [reporter-external] [client-bounty-form] [verif?][sec-survey][adv-main90+] → [reporter-external] [client-bounty-form] [verif?][sec-survey][adv-main90+][adv-esr78.12+]

Comment 69

•

4 years ago

Attached file advisory.txt (obsolete) — Details

Comment 70

•

4 years ago

Attached file advisory.txt — Details

Attachment #9230315 - Attachment is obsolete: true

Updated

•

4 years ago

Alias: CVE-2021-29970