Access violation on mozilla::dom::Element::BindToTree after realloc failure on AttrArray::GrowBy
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: sourc7, Assigned: smaug)
Details
(4 keywords, Whiteboard: [reporter-external] [client-bounty-form] [verif?] [adv-main116+] [adv-ESR115.1+] [adv-ESR102.14+])
Attachments
(7 files, 1 obsolete file)
|
1.92 KB,
text/html
|
Details | |
|
9.15 KB,
text/plain
|
Details | |
|
8.83 KB,
text/plain
|
Details | |
|
9.15 KB,
text/plain
|
Details | |
|
2.78 KB,
text/plain
|
Details | |
|
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
diannaS
:
approval-mozilla-esr102+
diannaS
:
approval-mozilla-esr115+
dveditz
:
sec-approval+
|
Details | Review |
|
166 bytes,
text/plain
|
Details |
When using DOMParser.parseFromString to parse HTML innerHTML with mimeType text/html on low virtual memory, it able to crash on mozilla::dom::Element::BindToTree at address e.g. 0x002ff004, 0x003ff004, 0x004ff004, 0x008ff004, 0xe11ff004, 0xffdff004, and more.., from the CPU register and crash address it looks like out-of-bound read.
From my observation the crash occur after mozilla::dom::DOMParser::ParseFromString it went through mozilla::dom::Element::SetAttrAndNotify then on AttrArray::GrowBy it do realloc but fail, then when newImpl is nullptr the NS_ENSURE_TRUE will return false.
The reduced testcase.html has very low success rate, for now it only for demonstrate HTML code that able to trigger BindToTree AV_READ crash. It's still unknown to me what key point that make the unreduced testcase more reliable, I decided to submit this for now, then figure that later.
Tested on:
- Firefox Nightly 116.0a1 (2023-06-30) (32-bit) on Windows 11
- Firefox 114.0.2 (32-bit)
| Reporter | ||
Comment 1•2 years ago
|
||
| Reporter | ||
Comment 2•2 years ago
|
||
| Reporter | ||
Comment 3•2 years ago
|
||
Updated•2 years ago
|
Updated•2 years ago
|
| Reporter | ||
Comment 4•2 years ago
|
||
Sorry, I forgot to mention, it's also reproducible on Firefox Nightly 117.0a1 (2023-07-05) (64-bit)
Updated•2 years ago
|
Updated•2 years ago
|
Comment 5•2 years ago
|
||
Looks like all calls to SetAttr in https://searchfox.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp should propagate failure.
Comment 6•2 years ago
|
||
Updated•2 years ago
|
Comment 7•2 years ago
|
||
The patch that I just uploaded should fix this, but I haven't actually tried the test case on a 32-bit system.
smaug, this patch adds branches, albeit prediction-hinted ones, on the innerHTML code path. It might be worthwhile to assess the performance impact, though I'm not sure what the right next step would be if the performance impact doesn't look OK.
Comment 8•2 years ago
|
||
needinfoing smaug to highlight the performance note above.
| Assignee | ||
Comment 10•2 years ago
|
||
We need to tweak https://searchfox.org/mozilla-central/rev/f29deb388a7675b93f040b0e89a37822cdbd8d58/dom/base/AttrArray.cpp#316-319 to not leak the memory when allocation fails.
Updated•2 years ago
|
| Assignee | ||
Comment 12•2 years ago
|
||
| Assignee | ||
Comment 13•2 years ago
|
||
Comment on attachment 9343363 [details]
Bug 1841368, don't leak the old AttrArray::Impl, r=peterv
Security Approval Request
- How easily could an exploit be constructed based on the patch?: I think it is non-trivial, since it requires out of memory situation
- 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?: Yes
- If not, how different, hard to create, and risky will they be?: The same patch seems to apply to mozilla-beta and esr102
- How likely is this patch to cause regressions; how much testing does it need?: This is hard to test, because it requires close to out of memory situation. The patch should be rather safe.
But, it is based on code inspection. - Is Android affected?: Yes
| Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
| Reporter | ||
Comment 14•2 years ago
|
||
When I tried to reproduce the testcase on local build, I able to reproduce the BindToTree crash signature, but the SEGV address is always hit 0x0, and the top crash stack code is also different than on Firefox official build.
I already tried build Firefox with different OS (Windows, Linux), different hardware, various optimization flag (-O2, -O1, -O0), --enable --disable debug, and other mozconfig ac_add_options flags, but unfortunately the SEGV address still hit 0x0.
Then when looking Firefox taskcluster live_backing.log at building Linux32 Shippable, from the "Adding configure options" I tried add ac_add_options --enable-profile-use=cross and export MOZ_LTO=cross to my mozconfig, then downloaded the profdata.tar.xz from the taskcluster, extracted the tar.xz into MOZ_OBJDIR/instrumented, after Firefox was compiled, I finally able to hit SEGV on address e.g. 0xffdff004 as on attached minidump.txt! I'll try verify the patch on the PGO build.
Comment 15•2 years ago
|
||
glandium, maybe this isn't surprising to you, but you might be interested in this case of LTO apparently turning an OOM crash from a null crash into something else. Maybe the timing is just very tricky.
Comment 16•2 years ago
|
||
(In reply to Irvan Kurniawan (:sourc7) from comment #14)
Then when looking Firefox taskcluster live_backing.log at building Linux32 Shippable, from the "Adding configure options" I tried add
ac_add_options --enable-profile-use=crossandexport MOZ_LTO=crossto my mozconfig, then downloaded the profdata.tar.xz from the taskcluster, extracted the tar.xz into MOZ_OBJDIR/instrumented, after Firefox was compiled, I finally able to hit SEGV on address e.g.0xffdff004as on attached minidump.txt! I'll try verify the patch on the PGO build.
Could you do this experiment again with only MOZ_LTO=cross and not --enable-profile-use=cross?
| Reporter | ||
Comment 17•2 years ago
|
||
Could you do this experiment again with only MOZ_LTO=cross and not --enable-profile-use=cross?
Ok, I recently compiled Firefox with the PGO configuration on Windows 11, the mozconfig is as follow:
ac_add_options --target=i686-pc-windows-msvc
ac_add_options --enable-rust-simd
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-ff-i686-pgo
ac_add_options --enable-profile-use=cross
export MOZ_LTO=cross
export ENABLE_CLANG_PLUGIN=1
export LINKER=lld-link
I had to extract the profdata.tar.xz to MOZ_OBJDIR/instrumented folder to build the Firefox, after Firefox was compiled, then when try reproduce the testcase, I able to reproduce the access-violation on 0x009ff004, 0xffdff004, and etc..
To try with only MOZ_LTO=cross, I removed the ac_add_options --enable-profile-use=cross, and change the MOZ_OBJDIR to different directory, it doesn't require me to extract the profdata.tar.xz to instrumented folder, after Firefox was compiled when reproduce the testcase, the mozilla::dom::Element::BindToTree crash address is always hit 0x00000000, even tried multiple times.
I've analyzed the crash on WinDbg, on Firefox official build (PGO build), at the Locals tab the mAttrMap variable is exist, on the local build (non PGO) the mAttrMap variable is missing, then at Stack tab, the upper Call Site (call stack) is also different between both build.
Comment 18•2 years ago
|
||
Could you do another attempt with --enable-profile-use=cross but not MOZ_LTO=cross?
| Reporter | ||
Comment 19•2 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #18)
Could you do another attempt with --enable-profile-use=cross but not MOZ_LTO=cross?
Ok, I've removed the MOZ_LTO=cross from mozconfig, then rebuilt the Firefox, but I still can't reproduce the access violation as on official build.
I see it do require ac_add_options --enable-profile-use=cross and export MOZ_LTO=cross for lld-link process to work properly. When both is enabled, the lld-link process is make ./mach build and ./mach buildsymbols process take longer.
| Reporter | ||
Comment 20•2 years ago
|
||
Ok, after applied the patch, I no longer able to reproduce the "BindToTree" crash on normal build and PGO build.
| Assignee | ||
Comment 21•2 years ago
|
||
Irvan, thanks for testing the patch!
Comment 22•2 years ago
|
||
Comment on attachment 9343363 [details]
Bug 1841368, don't leak the old AttrArray::Impl, r=peterv
sec-approval+ = dveditz
Comment 23•2 years ago
|
||
Comment 24•2 years ago
|
||
Comment 25•2 years ago
|
||
The patch landed in nightly and beta is affected.
:smaug, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- If no, please set
status-firefox116towontfix.
For more information, please visit BugBot documentation.
| Assignee | ||
Comment 26•2 years ago
|
||
Comment on attachment 9343363 [details]
Bug 1841368, don't leak the old AttrArray::Impl, r=peterv
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration:
- User impact if declined: security sensitive crashes
- Fix Landed on Version:
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Adds better error handling for allocation failure
Beta/Release Uplift Approval Request
- User impact if declined:
- Is this code covered by automated tests?: Yes
- 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):
- String changes made/needed:
- Is Android affected?: Yes
Comment 27•2 years ago
|
||
Comment on attachment 9343363 [details]
Bug 1841368, don't leak the old AttrArray::Impl, r=peterv
Approved for 116.0b8
Updated•2 years ago
|
Comment 28•2 years ago
|
||
| uplift | ||
Updated•2 years ago
|
Comment 29•2 years ago
|
||
Comment on attachment 9343363 [details]
Bug 1841368, don't leak the old AttrArray::Impl, r=peterv
Approved for 102.14esr
Approved for 115.1esr
Comment 30•2 years ago
|
||
| uplift | ||
Comment 31•2 years ago
|
||
| uplift | ||
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 32•2 years ago
|
||
Updated•1 year ago
|
Updated•1 year ago
|
Updated•11 months ago
|
Description
•