1730156 - (CVE-2021-38504) AddressSanitizer: SEGV on unknown address in mozilla::dom::Directory::GetFullRealPath(nsTSubstring<char16_t>&) -> operator-> get

Status: VERIFIED FIXED

(Core :: DOM: File, defect)

Description

[Irvan Kurniawan [:sourc7]]

Attachment: testcase.html

When using <input type="file"> then set attribute webkitdirectory to true then repeatedly reload the page with form submit or location.reload(). When page reload while holding down "Enter" key after file select dialog appears then select file, the tab crashes with AddressSanitizer: SEGV on unknown address in mozilla::dom::Directory::GetFullRealPath -> operator-> get

After holding down the "Enter" key, the file dialog still allow to choose file even set webkitdirectory to true. I think the HTMLInputElement still refer to previous pointer, one of the crash address on Windows 10 show e5e5e5e5 (probably uaf?)

When testing this on Firefox ASan 64-bit on Linux it shows AddressSanitizer: SEGV on unknown address with Hint: this fault was caused by a dereference of a high value address (see register values below) on Firefox ASan 32-bit it shows the address SEGV address on unknown address 0x270edfe3 (same address every-time I tried the testcase)

Steps to reproduce:

1. Visit attached testcase.html
2. Hold down "Enter" key
3. After file open dialog appears, then select file
4. Click "Open" or press "Enter" key
5. The tab crashed

ASan output:

=================================================================
==1971273==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x7f8aa549f80f bp 0x7fff19db80f0 sp 0x7fff19db80f0 T0)
==1971273==The signal is caused by a READ memory access.
==1971273==Hint: this fault was caused by a dereference of a high value address (see register values below).  Disassemble the provided pc to learn which register was used.
    #0 0x7f8aa549f80f in get /home/sourc7/git/gecko-dev-asan/objdir-ff-asan/dist/include/nsCOMPtr.h:851:48
    #1 0x7f8aa549f80f in operator-> /home/sourc7/git/gecko-dev-asan/objdir-ff-asan/dist/include/nsCOMPtr.h:872:12
    #2 0x7f8aa549f80f in mozilla::dom::Directory::GetFullRealPath(nsTSubstring<char16_t>&) /home/sourc7/git/gecko-dev-asan/dom/filesystem/Directory.cpp:133:17
    #3 0x7f8aa55f00be in GetDOMFileOrDirectoryPath /home/sourc7/git/gecko-dev-asan/dom/html/HTMLInputElement.cpp:433:29
    #4 0x7f8aa55f00be in mozilla::dom::HTMLInputElement::AfterSetFilesOrDirectories(bool) /home/sourc7/git/gecko-dev-asan/dom/html/HTMLInputElement.cpp:2511:5
    #5 0x7f8aa561e661 in mozilla::dom::DispatchChangeEventCallback::Callback(nsresult, FallibleTArray<RefPtr<mozilla::dom::BlobImpl> > const&) /home/sourc7/git/gecko-dev-asan/dom/html/HTMLInputElement.cpp:234:20
    #6 0x7f8aa55c9ccb in mozilla::dom::HTMLInputElement::nsFilePickerShownCallback::Done(short) /home/sourc7/git/gecko-dev-asan/dom/html/HTMLInputElement.cpp:559:13
    #7 0x7f8aa7bb8e9f in nsFilePickerProxy::Recv__delete__(mozilla::dom::MaybeInputData const&, short const&) /home/sourc7/git/gecko-dev-asan/widget/nsFilePickerProxy.cpp:191:16
    #8 0x7f8a9fe561f3 in mozilla::dom::PFilePickerChild::OnMessageReceived(IPC::Message const&) /home/sourc7/git/gecko-dev-asan/objdir-ff-asan/ipc/ipdl/PFilePickerChild.cpp:186:28
    #9 0x7f8a9fc441e5 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /home/sourc7/git/gecko-dev-asan/objdir-ff-asan/ipc/ipdl/PContentChild.cpp:8336:32
    #10 0x7f8a9f96b45c in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /home/sourc7/git/gecko-dev-asan/ipc/glue/MessageChannel.cpp:2051:25
    #11 0x7f8a9f967af6 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /home/sourc7/git/gecko-dev-asan/ipc/glue/MessageChannel.cpp:1978:9
    #12 0x7f8a9f96997a in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /home/sourc7/git/gecko-dev-asan/ipc/glue/MessageChannel.cpp:1826:3
    #13 0x7f8a9f96a2c1 in mozilla::ipc::MessageChannel::MessageTask::Run() /home/sourc7/git/gecko-dev-asan/ipc/glue/MessageChannel.cpp:1857:13
    #14 0x7f8a9e39ad5a in mozilla::RunnableTask::Run() /home/sourc7/git/gecko-dev-asan/xpcom/threads/TaskController.cpp:502:16
    #15 0x7f8a9e35640f in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /home/sourc7/git/gecko-dev-asan/xpcom/threads/TaskController.cpp:805:26
    #16 0x7f8a9e352c88 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /home/sourc7/git/gecko-dev-asan/xpcom/threads/TaskController.cpp:641:15
    #17 0x7f8a9e353358 in mozilla::TaskController::ProcessPendingMTTask(bool) /home/sourc7/git/gecko-dev-asan/xpcom/threads/TaskController.cpp:425:36
    #18 0x7f8a9e38f361 in operator() /home/sourc7/git/gecko-dev-asan/xpcom/threads/TaskController.cpp:135:37
    #19 0x7f8a9e38f361 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_0>::Run() /home/sourc7/git/gecko-dev-asan/xpcom/threads/nsThreadUtils.h:532:5
    #20 0x7f8a9e37599b in nsThread::ProcessNextEvent(bool, bool*) /home/sourc7/git/gecko-dev-asan/xpcom/threads/nsThread.cpp:1148:16
    #21 0x7f8a9e381e81 in NS_ProcessNextEvent(nsIThread*, bool) /home/sourc7/git/gecko-dev-asan/xpcom/threads/nsThreadUtils.cpp:466:10
    #22 0x7f8a9f974907 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/sourc7/git/gecko-dev-asan/ipc/glue/MessagePump.cpp:85:21
    #23 0x7f8a9f7d6741 in RunInternal /home/sourc7/git/gecko-dev-asan/ipc/chromium/src/base/message_loop.cc:331:10
    #24 0x7f8a9f7d6741 in RunHandler /home/sourc7/git/gecko-dev-asan/ipc/chromium/src/base/message_loop.cc:324:3
    #25 0x7f8a9f7d6741 in MessageLoop::Run() /home/sourc7/git/gecko-dev-asan/ipc/chromium/src/base/message_loop.cc:306:3
    #26 0x7f8aa7ba9c3a in nsBaseAppShell::Run() /home/sourc7/git/gecko-dev-asan/widget/nsBaseAppShell.cpp:137:27
    #27 0x7f8aac8c8a5f in XRE_RunAppShell() /home/sourc7/git/gecko-dev-asan/toolkit/xre/nsEmbedFunctions.cpp:923:20
    #28 0x7f8a9f7d6741 in RunInternal /home/sourc7/git/gecko-dev-asan/ipc/chromium/src/base/message_loop.cc:331:10
    #29 0x7f8a9f7d6741 in RunHandler /home/sourc7/git/gecko-dev-asan/ipc/chromium/src/base/message_loop.cc:324:3
    #30 0x7f8a9f7d6741 in MessageLoop::Run() /home/sourc7/git/gecko-dev-asan/ipc/chromium/src/base/message_loop.cc:306:3
    #31 0x7f8aac8c7908 in XRE_InitChildProcess(int, char**, XREChildData const*) /home/sourc7/git/gecko-dev-asan/toolkit/xre/nsEmbedFunctions.cpp:755:34
    #32 0x55e150df0b2c in content_process_main /home/sourc7/git/gecko-dev-asan/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
    #33 0x55e150df0b2c in main /home/sourc7/git/gecko-dev-asan/browser/app/nsBrowserApp.cpp:327:18
    #34 0x7f8ab7ce8b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
    #35 0x55e150d417d8 in _start (/home/sourc7/git/gecko-dev-asan/objdir-ff-asan/dist/bin/firefox+0xa97d8)

Comment 1

[Irvan Kurniawan [:sourc7]]

Attachment: asan.txt

Comment 2

[Irvan Kurniawan [:sourc7]]

Attachment: asan.32bit.txt

Comment 3

[Irvan Kurniawan [:sourc7]]

After checking with mozregression it showing to this pushlog, I can reproduce the crash after checking out to commit Bug 1585284 - Force global in Blob CTOR.

Comment 4

[Irvan Kurniawan [:sourc7]]

Attachment: testcase-click.html

I simplified the testcase to trigger a crash with just one click to the <input type=file> element then once the user selects a file the tab will crash. It works perfectly on non-fission, however it require couple of reload on Fission, I'll share the simpler testcase for fission soon.

Steps to reproduce:

1. Visit attached testcase-click.html
2. Click "Browse..." to upload file
3. Select file
4. Tab will crashed

Comment 5

[Andrew McCreight [:mccr8]]

In a debug mozilla-central build (from about a month ago... I haven't updated recently) I get this assertion:

Assertion failure: aGlobal, at dom/file/File.cpp:32
#01: mozilla::dom::File::Create(nsIGlobalObject*, mozilla::dom::BlobImpl*) (dom/file/File.cpp:32)
#02: mozilla::dom::DispatchChangeEventCallback::Callback(nsresult, FallibleTArray<RefPtr<mozilla::dom::BlobImpl> > const&) (dom/html/HTMLInputElement.cpp:226)
#03: mozilla::dom::HTMLInputElement::nsFilePickerShownCallback::Done(short) (dom/html/HTMLInputElement.cpp:0)

(the rest of the stack looks similar to the above)

Comment 6

[Andrew McCreight [:mccr8]]

That's with testcase-click.html, following the STR from comment 4.

Comment 7

[Andrew McCreight [:mccr8]]

Bug 1585284 landed in 71, so marking affected flags.

Comment 8

[Andrew McCreight [:mccr8]]

I found some crashes in the wild that look the same as this, including a poison value, including this one from mid-August: bp-4cbbef9c-5137-44c4-a3a9-a45090210820

Comment 9

[Irvan Kurniawan [:sourc7]]

On another testcase I got another ASan results. On 64-bit it crash on operator! with READ memory access (this fault was caused by a dereference of a high value address) same as on comment 1.

More interestingly on 32-bit it crash on operator++ with WRITE memory access (after running this couple of times, I notice the SEGV address can be changed from 0x5e8b0cf0 to 0x83fffff1).

I will also share the testcase soon.

Comment 10

[Irvan Kurniawan [:sourc7]]

Attachment: asan-operator!.64bit.txt

Comment 11

[Irvan Kurniawan [:sourc7]]

Attachment: asan-operator++.32bit.txt

Comment 12

[Andrew McCreight [:mccr8]]

If you have a debug build handy, you could try running the other test cases in it to see if they might be different than the assertion I got in comment 5.

Comment 13

[Andrew McCreight [:mccr8]]

(FWIW, testcase.html asserts in a debug build the same as the stack in comment 5.)

Comment 14

[Andrew McCreight [:mccr8]]

Ok, I guess the assert isn't as much of an indicator of the issue as I thought. File::Create() does detect that in an opt build and returns null.

If I comment out that assert, it gets a little further and hits another assert:

Assertion failure: aData.IsDirectory(), at /Users/andrewmccreight/mc/dom/html/HTMLInputElement.cpp:432
#01: mozilla::dom::(anonymous namespace)::GetDOMFileOrDirectoryPath(mozilla::dom::OwningFileOrDirectory const&, nsTSubstring<char16_t>&, mozilla::ErrorResult&) (dom/html/HTMLInputElement.cpp:432)
#02: mozilla::dom::HTMLInputElement::AfterSetFilesOrDirectories(bool) (dom/html/HTMLInputElement.cpp:2513)
#03: mozilla::dom::DispatchChangeEventCallback::Callback(nsresult, FallibleTArray<RefPtr<mozilla::dom::BlobImpl> > const&) (dom/html/HTMLInputElement.cpp:235)

Comment 15

[Andrew McCreight [:mccr8]]

Jens, is there somebody who is familiar with this File code who could take a look at this? Thanks.

Comment 16

[Jens Stutte [:jstutte]]

(In reply to Andrew McCreight [:mccr8] from comment #14)

Ok, I guess the assert isn't as much of an indicator of the issue as I thought. File::Create() does detect that in an opt build and returns null.

If I comment out that assert, it gets a little further and hits another assert:

Assertion failure: aData.IsDirectory(), at /Users/andrewmccreight/mc/dom/html/HTMLInputElement.cpp:432
#01: mozilla::dom::(anonymous namespace)::GetDOMFileOrDirectoryPath(mozilla::dom::OwningFileOrDirectory const&, nsTSubstring<char16_t>&, mozilla::ErrorResult&) (dom/html/HTMLInputElement.cpp:432)
#02: mozilla::dom::HTMLInputElement::AfterSetFilesOrDirectories(bool) (dom/html/HTMLInputElement.cpp:2513)
#03: mozilla::dom::DispatchChangeEventCallback::Callback(nsresult, FallibleTArray<RefPtr<mozilla::dom::BlobImpl> > const&) (dom/html/HTMLInputElement.cpp:235)

From what I read in GetDOMFileOrDirectoryPath that MOZ_ASSERT indicates we have an uninitialized OwningFileOrDirectory object in mFilesOrDirectories[0].

But the stack does not reveal how it got there.

Comment 17

[Irvan Kurniawan [:sourc7]]

(In reply to Andrew McCreight [:mccr8] from comment #12)

If you have a debug build handy, you could try running the other test cases in it to see if they might be different than the assertion I got in comment 5.

Thanks Andrew for pointing out, currently the operator++ crash testcase is still intermittent, after couple of tries sometimes it also crash at GetFullRealPath (as testcase on comment 0 and comment 4).

I've added null check to below the assertion line, then build the i686 ASan build, after the tab crashed on operator++, the browser output shows "aGlobal is null" (the null check) before ASan print the crash stack trace. Then yes it also hit the same assertion on comment 5.

Comment 18

[Eden Chuang[:edenchuang]]

The root cause of the crash in comment 4's testcase_click.html is location.reload() happens before the file picker finishes.

The crash flow is

1. click the "browse" the file picker pop up
2. main() is triggered, then location.reload() is called.
3. location.reload() makes the HTMLInputElement's scriptGlobalObject be freed. HTMLInputElement still be alive but invalid since the file picker has a strong reference RefPtr<HTMLInputElement>. And the file picker is still there.
4. Select a file and click "open", DispatchChangeEventCallback is created with the invalid HTMLInputElement.
5. DispatchChangeEventCallback::Callback() is called, but try to access the freed scriptGlobalObject through HTMLInputElement::GetOwnerGlobal(), then crash the tab.

I am not sure if it is easy to make the file picker detect the page reload happens and close itself.
But a simple solution is not to create the DispatchChangeEventCallback if the HTMLInputElement has no scriptGlobalObject anymore.

Comment 19

[Eden Chuang[:edenchuang]]

Attachment: Bug 1730156 - Do not create DispatchChangeEventCallback if associated HTMLInputElement has no scriptGlobalObject. r=smaug

Comment 20

[Eden Chuang[:edenchuang]]

Comment on attachment 9242049 [details]
Bug 1730156 - Do not create DispatchChangeEventCallback if associated HTMLInputElement has no scriptGlobalObject. r=smaug

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It is not easy to understand how to create the case from the patch since the patch did not expose too much information on how to achieve it.
• 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?: Bug 1585284
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: It should not cause any regression.

Comment 21

[Peter Van der Beken [:peterv]]

I filed bug 1731879 to make it harder to have uninitialized WebIDL unions.

Comment 22

[Andrew McCreight [:mccr8]]

I filed bug 1731886 about making the WebIDL union getters use release asserts.

Comment 23

[Irvan Kurniawan [:sourc7]]

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

however it require couple of reload on Fission, I'll share the simpler testcase for fission soon.

Ok, I recently tried the testcase-click.html (on comment 4) with Fission enabled, it turns out it also crashes immediately after selecting the file (as on Fission disabled).

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

I will also share the testcase soon.

I couldn't found a way to reliably hit operator++ all the time, it also often hit GetFullRealPath signature. When retrying STR at comment 0 multiple times I found at some time I also able hit the operator++ crash. I think the underlying testcase code is same and it also hit same assertion Assertion failure: aGlobal, but when combined with appropriate timing it will hit the operator crash.

(In reply to Eden Chuang[:edenchuang] from comment #20)

Comment on attachment 9242049 [details]
Bug 1730156 - Do not create DispatchChangeEventCallback if associated HTMLInputElement has no scriptGlobalObject. r=smaug

Thanks for the patch Eden, I've applied the patches on Firefox ASan, after tried the STR multiple times, I no longer experience the both crashes.

Comment 24

[Tom Ritter [:tjr]]

Comment on attachment 9242049 [details]
Bug 1730156 - Do not create DispatchChangeEventCallback if associated HTMLInputElement has no scriptGlobalObject. r=smaug

Approved to land and request uplift

Comment 25

[Ryan VanderMeulen [:RyanVM]]

We're a few hours away from building the final Fx93 beta this cycle. How badly do we need to get this fix shipped in the upcoming releases vs. waiting until next cycle?

Comment 26

[Tom Ritter [:tjr]]

No; we can hold it. I'll retract sec-approval :)

Comment 27

[Tom Ritter [:tjr]]

Comment on attachment 9242049 [details]
Bug 1730156 - Do not create DispatchChangeEventCallback if associated HTMLInputElement has no scriptGlobalObject. r=smaug

Holding sec-approval until next version

Comment 28

[Eden Chuang[:edenchuang]]

Keep me ni? for tracking sec-approval?

Comment 29

[Tom Ritter [:tjr]]

Comment on attachment 9242049 [details]
Bug 1730156 - Do not create DispatchChangeEventCallback if associated HTMLInputElement has no scriptGlobalObject. r=smaug

approved to land

Comment 30

[Andrew McCreight [:mccr8]]

Looks like somebody hit the release assert I added in bug 1731886 on a similar stack: bp-cde82cf8-bc08-4a96-93a1-f77eb0211010

It is getting to GetDOMFileOrDirectoryPath via ContentChild::RecvGetFilesResponse instead of nsFilePickerProxy::Recv__delete__, but that seems similar.

Comment 31

[Andrew McCreight [:mccr8]]

Irvan, any chance you were testing similar test cases on 94 beta? I'm just curious about the crash report in comment 30. Thanks.

Comment 32

[Irvan Kurniawan [:sourc7]]

(In reply to Andrew McCreight [:mccr8] from comment #31)

Irvan, any chance you were testing similar test cases on 94 beta? I'm just curious about the crash report in comment 30. Thanks.

No, it wasn't my crash report, when testing the testcase for a while, I usually got nsFilePickerProxy::Recv__delete__ on the stack, after testing further I wasn't received ContentChild::RecvGetFilesResponse on the stack previously.

Fortunately after modify input1.webkitdirectory from false to true on testcase-click.html it also able to trigger mozilla::dom::ContentChild::RecvGetFilesResponse as on the crash report in comment 30.

Comment 33

[Irvan Kurniawan [:sourc7]]

Attachment: testcase-click-webkitdirectory.html

The difference is that this one needs to upload a directory instead of a file.

Comment 34

[Irvan Kurniawan [:sourc7]]

Hereby my recent crash report d56d7e45-c466-4c99-abfd-62b7e0211012 which also show ContentChild::RecvGetFilesResponse on the stack.

Comment 35

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

Do not create DispatchChangeEventCallback if associated HTMLInputElement has no scriptGlobalObject. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6f44d3823a2ee88bbf64404a85c72ea0e3170850
https://hg.mozilla.org/mozilla-central/rev/6f44d3823a2e

Comment 36

[Eden Chuang[:edenchuang]]

Comment on attachment 9242049 [details]
Bug 1730156 - Do not create DispatchChangeEventCallback if associated HTMLInputElement has no scriptGlobalObject. r=smaug

Beta/Release Uplift Approval Request

• User impact if declined: Since scriptGlobalObject could be freed before creating DispatchChangeEventCallback, users could meet the tab crash if this situation happens.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch only adds a simple check on the depending scriptGlobalObject when creating DispatchChangeEventCallback, so it is not risky to make regressions or bugs.
• String changes made/needed: No

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9242049 [details]
Bug 1730156 - Do not create DispatchChangeEventCallback if associated HTMLInputElement has no scriptGlobalObject. r=smaug

Approved for 94.0b7 and 91.3esr.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a3e1316cc29b

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/1e024e57bb82

Comment 40

[Ciprian Georgiu, Desktop QA]

Reproduced the tab crash using testcase.html from comment 0, on an affected Nightly asan build.

The issue is verified as fixed on the latest asan builds: Beta 94.0b7, Nightly 95.01 and Esr 91.3. Tested with Win 10 x64 and Ubuntu 18.04 x64.

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

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 42

[Daniel Veditz [:dveditz]]

Possibly should have rated this sec-moderate because of the file upload interaction required... the crash itself is bad though.

Comment 43

[Tom Ritter [:tjr]]

Attachment: advisory.txt