Bugzilla

## Vulnerability Analysis: Global Buffer Overflow via Empty Animation Segments in PWebRenderBridge IPC

### Bug Title
Global buffer overflow in parent process via `OpAddCompositorAnimations` IPC message with empty animation segments

### 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:
   ```cpp
   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:

```cpp
// 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:
```cpp
if (animation.mSegments.IsEmpty()) {
  continue;  // Skip animations with no segments
}
```

### 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:
   ```cpp
   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:

```cpp
// 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:
```cpp
if (animation.mSegments.IsEmpty()) {
  continue;  // Skip animations with no segments
}
```