2017521 - Global Buffer Overflow via Empty Animation Segments in PWebRenderBridge IPC

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jason Kratzer [:jkratzer]]

Affected Component

• Protocol: PWebRenderBridge (gfx/layers/ipc/PWebRenderBridge.ipdl)
• Parent-side handler: WebRenderBridgeParent::ProcessWebRenderParentCommands() → CompositorAnimationStorage::SetAnimations() → AnimationHelper::ExtractAnimations() → SampleAnimationForProperty()
• Vulnerable code: gfx/layers/AnimationHelper.cpp, lines 249-256

Technical Details

Root Cause

The PWebRenderBridge IPC protocol accepts OpAddCompositorAnimations commands via SetDisplayList or EmptyTransaction messages. These commands contain Animation structs with an AnimationSegment[] segments array. The parent process does not validate that the segments array is non-empty before processing animations.

When an Animation with isNotAnimating=false and an empty segments array is sent from the content process:

1. ExtractAnimations() (AnimationHelper.cpp:527-528): Creates a PropertyAnimation object and appends it to PropertyAnimationGroup::mAnimations, but the mSegments sub-array remains empty because the loop at line 573 (for (const AnimationSegment& segment : animation.segments())) iterates zero times.

2. SampleAnimationForProperty() (AnimationHelper.cpp:249-252): When the compositor samples this animation:

   size_t segmentSize = animation.mSegments.Length();  // = 0
   PropertyAnimation::SegmentData* segment = animation.mSegments.Elements();
   // Elements() returns Hdr()+1, which points past sEmptyTArrayHeader
   while (segment->mEndPortion < computedTiming.mProgress.Value() &&
          segmentIndex < segmentSize - 1) {  // segmentSize-1 = SIZE_MAX
   

   • segmentSize is 0, so segmentSize - 1 wraps to SIZE_MAX (unsigned underflow)
   • segment points past sEmptyTArrayHeader, a 12-byte static global variable
   • segment->mEndPortion reads 4 bytes from an out-of-bounds global memory location

Attack Vector

A compromised content process constructs a malicious OpAddCompositorAnimations IPC command containing an Animation with:

• isNotAnimating = false (so the animation is processed, not skipped)
• isNotPlaying = false (so timing produces non-null progress)
• Valid timing parameters (duration, delay, fill mode) ensuring the animation is in its active phase
• An empty segments array (the vulnerability trigger)
• A valid animation ID with the content process PID in the upper 32 bits (passes PID validation at WebRenderBridgeParent.cpp:1678)

This command is embedded in a SetDisplayList IPC message as a WebRenderParentCommand.

Memory Corruption Details

The ASAN report confirms:

• Error type: global-buffer-overflow
• Operation: READ of size 4
• Location: AnimationHelper.cpp:252 — segment->mEndPortion dereference
• Process: Parent process (PID 2064929), Thread T79 (WebRender backend thread)
• Read address: 0x7733f0d333dc — 25 bytes after a 3-byte global string variable, in global redzone memory

Exploitability Assessment

This is an information disclosure vulnerability with potential for further exploitation:

• The read is from global memory adjacent to sEmptyTArrayHeader, accessing whatever static data follows in the binary layout
• An attacker could potentially use this to leak ASLR information or other sensitive data from the parent process
• The read value influences control flow (the while loop condition) and further computation (positionInSegment calculation at line 258-260), providing an oracle for leaked values
• With careful manipulation of the animation timing parameters, an attacker could probe different portions of the leaked data

Proof of Concept

HTML Testcase (anim_exploit.html)

Triggers multiple display list transactions via DOM style changes to invoke EndTransaction in the content process.

C++ Patch (exploit.patch)

Modifies WebRenderBridgeChild::EndTransaction() to inject a malicious OpAddCompositorAnimations command with an empty segments array on the 3rd transaction (to avoid crashing before the browser is fully initialized).

Recommended Fix

Add a validation check in AnimationHelper::ExtractAnimations() or WebRenderBridgeParent::ProcessWebRenderParentCommands() to reject animations with empty segments arrays when isNotAnimating is false:

// In ExtractAnimations(), after line 524 (the isNotAnimating continue):
if (animation.segments().IsEmpty()) {
  // An animation that is supposed to be animating must have segments.
  // Skip it to prevent OOB read in SampleAnimationForProperty.
  continue;
}

Alternatively, add a guard in SampleAnimationForProperty() at line 249:

if (animation.mSegments.IsEmpty()) {
  continue;  // Skip animations with no segments
}

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: exploit.patch

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: anim_exploit.html

Comment 3

[Jason Kratzer [:jkratzer]]

Attachment: crash_stack.txt

Comment 4

[Andrew McCreight [:mccr8]]

Nika, how bad is it to read right past sEmptyTArrayHeader? I see in bug 1243463 that we did some hardening here, but I see that bug 1763691 and bug 1240838 didn't go anywhere so maybe there's still a problem?

I think this code should be changed to use IndexOf to safely compute segmentIndex, and then segment could be derived from that, thus avoiding the dangerous address arithmetic altogether.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

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

Nika, how bad is it to read right past sEmptyTArrayHeader? I see in bug 1243463 that we did some hardening here, but I see that bug 1763691 and bug 1240838 didn't go anywhere so maybe there's still a problem?

I expect that sEmptyTArray is mapped next to other rodata when loaded, meaning that reading past the end will read other predictable bytes from memory. This is obviously not ideal (hence those bugs you mentioned). I don't know what exactly will be after sEmptyTArray in any particular build, and it probably varies between platforms.

The hardening from bug 1243463 was just making sure that we can't write to sEmptyTArrayHeader and make it look like a non-empty array (as that could cause all kinds of non-local and hard to diagnose problems).

I think this code should be changed to use IndexOf to safely compute segmentIndex, and then segment could be derived from that, thus avoiding the dangerous address arithmetic altogether.

Yes, this is clearly a bug in the GPU code where data being received over this API is not being validated. If there is an invariant that the animations array must be non-empty, this must be checked somewhere as we're processing untrusted data from a content process.

Comment 6

[Brad Werth [:bradwerth]]

I'll take this and try to build a fix.

Comment 7

[Brad Werth [:bradwerth]]

Attachment: (secure)

Comment 8

[Brad Werth [:bradwerth]]

Comment on attachment 9546653 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: fairly easily as long as attacker has an IPC exploit ready to send malformed animation data to parent.
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: 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?: Easy to create.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions. This should not be exercised in normal web content.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 9546653 [details]
(secure)

sec-approval+

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/dd29b0683b2a
https://hg.mozilla.org/integration/autoland/rev/946bd1168575
Make SampleAnimationForProperty bail out when an animation has no segments. r=hiro

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/946bd1168575

Comment 12

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

The patch landed in nightly and beta is affected.
:bradwerth, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox149 to wontfix.

For more information, please visit BugBot documentation.

Comment 13

[Daniel Veditz [:dveditz]]

Bugbot is wrong for security bugs and bugs that already have a release tracking flag. Please check with release managers or the security team before wontfixing such bugs.

New analysis tools have led to a new batch of security bug hunters finding previously undiscovered bugs. This bug was reported in the first few wave of reports from people using those tools so presumably it is low-hanging fruit and will be re-discovered multiple times

Comment 14

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Parent process OOB read. AI code analysis tools can easily find this Bug and we are getting duplicate reports against ESR.
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Rejects dangerous IPC payloads using an existing failure mechanism.
• String changes made/needed: None
• Is Android affected?: yes

Comment 15

[Brad Werth [:bradwerth]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D284193

Comment 16

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined: Parent process OOB read. AI code analysis tools can easily find this Bug and we are getting duplicate reports against ESR.
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Rejects dangerous IPC payloads using an existing failure mechanism.
• String changes made/needed: None
• Is Android affected?: yes

Comment 17

[Brad Werth [:bradwerth]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D284193

Comment 18

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Parent process OOB read. AI code analysis tools can easily find this Bug and we are getting duplicate reports against ESR.
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Rejects dangerous IPC payloads using an existing failure mechanism.
• String changes made/needed: None
• Is Android affected?: yes

Comment 19

[Brad Werth [:bradwerth]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D284193

Comment 20

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/5b15a33c351d
https://hg.mozilla.org/releases/mozilla-beta/rev/c1309d9a9b33

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/bc25a6f11e7b

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/126f3c211de1