Closed Bug 1841368 (CVE-2023-4048) Opened 1 year ago Closed 11 months ago

Access violation on mozilla::dom::Element::BindToTree after realloc failure on AttrArray::GrowBy

Categories

(Core :: DOM: Core & HTML, defect)

All
Unspecified
defect

Tracking

()

RESOLVED FIXED
117 Branch
Tracking Status
firefox-esr102 116+ fixed
firefox-esr115 116+ fixed
firefox115 --- wontfix
firefox116 + fixed
firefox117 + fixed

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)

Attached file testcase1.html

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)
Flags: sec-bounty?
Group: firefox-core-security → dom-core-security
Component: Security → DOM: HTML Parser
Keywords: csectype-oom
Product: Firefox → Core
Hardware: Unspecified → x86
Summary: Access violation on mozilla::dom::Element::BindToTree after realloc failure on AttrArray::GrowBy → Access violation on mozilla::dom::Element::BindToTree after realloc failure on AttrArray::GrowBy (32-bit builds)
Attached file minidump_64bit.txt

Sorry, I forgot to mention, it's also reproducible on Firefox Nightly 117.0a1 (2023-07-05) (64-bit)

Hardware: x86 → All
Summary: Access violation on mozilla::dom::Element::BindToTree after realloc failure on AttrArray::GrowBy (32-bit builds) → Access violation on mozilla::dom::Element::BindToTree after realloc failure on AttrArray::GrowBy
Severity: -- → S2
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED

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.

needinfoing smaug to highlight the performance note above.

Flags: needinfo?(smaug)

I don't think we want that patch

Flags: needinfo?(smaug)

Olli said he could fix this.

Assignee: hsivonen → smaug
Component: DOM: HTML Parser → DOM: Core & HTML

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
Attachment #9343363 - Flags: sec-approval?
Attachment #9342585 - Attachment is obsolete: true

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.

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.

Flags: needinfo?(mh+mozilla)

(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=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.

Could you do this experiment again with only MOZ_LTO=cross and not --enable-profile-use=cross?

Flags: needinfo?(mh+mozilla) → needinfo?(susah.yak)

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.

Flags: needinfo?(susah.yak)

Could you do another attempt with --enable-profile-use=cross but not MOZ_LTO=cross?

Flags: needinfo?(susah.yak)

(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.

Flags: needinfo?(susah.yak)

Ok, after applied the patch, I no longer able to reproduce the "BindToTree" crash on normal build and PGO build.

Irvan, thanks for testing the patch!

Comment on attachment 9343363 [details]
Bug 1841368, don't leak the old AttrArray::Impl, r=peterv

sec-approval+ = dveditz

Attachment #9343363 - Flags: sec-approval? → sec-approval+
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e190c6549a75
don't leak the old AttrArray::Impl, r=peterv
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch

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-firefox116 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(smaug)

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
Flags: needinfo?(smaug)
Attachment #9343363 - Flags: approval-mozilla-esr115?
Attachment #9343363 - Flags: approval-mozilla-beta?

Comment on attachment 9343363 [details]
Bug 1841368, don't leak the old AttrArray::Impl, r=peterv

Approved for 116.0b8

Attachment #9343363 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9343363 - Flags: approval-mozilla-esr102?

Comment on attachment 9343363 [details]
Bug 1841368, don't leak the old AttrArray::Impl, r=peterv

Approved for 102.14esr
Approved for 115.1esr

Attachment #9343363 - Flags: approval-mozilla-esr115?
Attachment #9343363 - Flags: approval-mozilla-esr115+
Attachment #9343363 - Flags: approval-mozilla-esr102?
Attachment #9343363 - Flags: approval-mozilla-esr102+
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Flags: sec-bounty? → sec-bounty+
Keywords: testcase
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?] [adv-main116+] [adv-ESR115.1+] [adv-ESR102.14+]
Group: core-security-release
Alias: CVE-2023-4048
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: