24545f4fcbd7c5ed088348ec64e7e039da43eb00  [non-cocoa][fuzz] Frame cache evicted or unparsed frames dereferenced

The bug at a glance

A batch of attacker-reachable out-of-bounds reads and null-pointer dereferences across WebCore’s APNG/PNG, GIF, ICO, WEBP and JPEG-XL image decoders, all reachable by serving a crafted image to an /CSS background in the WebContent (or GPU image-decoding) process. The sharpest root cause is that PNGImageDecoder stored a malformed APNG acTL frame count (m_frameCount) BEFORE validating it and clamped the requested frame index against that advertised count rather than against the actual m_frameBufferCache.size(), so a file advertising more frames than it ships drove reads past the end of the frame-buffer Vector; sibling null-deref fixes cover frames whose backing store was evicted or never allocated. Severity high is justified for an OOB read in the renderer (heap info-leak into decoded pixels that script can read back via canvas), but the diff proves OOB-read / null-deref (info-disclosure + DoS); it does not by itself prove a write primitive or RCE (inferred).

Classic image-decoder desync: the animation metadata (acTL/anim chunk) and the real number of decoded frame buffers are populated on different paths, and every decoder trusted the attacker-controlled count to index a container sized from reality.

Root cause

The violated invariant is that an index used to subscript m_frameBufferCache (a Vector) must be < m_frameBufferCache.size(), and that any ScalableImageDecoderFrame dereferenced for its backing store must actually own one. PNGImageDecoder::readChunks parsed the APNG acTL with m_frameCount = png_get_uint_32(chunk->data) and m_playCount = png_get_uint_32(chunk->data+4) and only THEN tested the limits; on rejection it called fallbackNotAnimated() and returned, but m_frameCount had already been written. Because frameCount() reports m_frameCount and callers size per-frame containers from it, the advertised-but-unbacked count leaks forward.

Pre-patch PNGImageDecoder::frameBufferAtIndex went: 1. if index >= decodeIfNeededAndGetFrameCount() it clamped index to decodeIfNeededAndGetFrameCount()-1 (the ADVERTISED count, derived from m_frameCount); 2. if the cache was empty it grew it to 1; 3. it then did auto& frame = m_frameBufferCache[index]. When a malformed acTL advertised more frames than buffers were ever allocated, step 1 clamped to a large index while step 2 only guaranteed size>=1, so step 3 read past the Vector end. The patch drops the advertised-count clamp and instead does index = std::min(index, m_frameBufferCache.size() - 1), clamping against the real container, and adds RELEASE_ASSERT(m_currentFrame < m_frameBufferCache.size()) on the rowAvailable / frameComplete write paths.

The companion defects are null backing-store dereferences. A frame evicted from the cache (or not yet decoded) has prevBuffer->backingStore() == nullptr, yet GIFImageDecoder::frameComplete / PNGImageDecoder::frameComplete called prevBuffer->backingStore()->frameRect() unconditionally, and initFrameBuffer asserted prevBuffer->isComplete() (a no-op in release) before copying from it. ICOImageDecoder::decodeAtIndex dereferenced static_cast<PNGImageDecoder*>(…)->firstFrameBuffer() which is null until the embedded PNG sub-decoder has parsed its own header. JPEGXLImageDecoder::clearDecodedPixelDataIfNeeded built an iterator m_frameBufferCache.begin()+clearBeforeFrame with an unclamped clearBeforeFrame, walking past end(). WEBPImageDecoder::frameBufferAtIndex returned &m_frameBufferCache[index] after a decode() that may have left the cache untouched while m_frameCount came from a demuxer that since rejected the file.

Key code

PNGImageDecoder::frameBufferAtIndex clamps to the real container, not the advertised frame count

-    if (index >= decodeIfNeededAndGetFrameCount())
-        index = decodeIfNeededAndGetFrameCount() - 1;
-
     if (m_frameBufferCache.isEmpty())
         m_frameBufferCache.grow(1);

+    // A malformed acTL can advertise more frames than there are buffers.
+    index = std::min(index, m_frameBufferCache.size() - 1);
+
     auto& frame = m_frameBufferCache[index];

Fix approach

Every decoder is re-grounded on the real container size and on backing-store presence instead of on attacker-advertised metadata. PNG clamps the index with std::min(index, m_frameBufferCache.size()-1) and only commits m_frameCount/m_playCount after the range check passes; WEBP returns nullptr when index >= m_frameBufferCache.size() after decode; JPEG-XL clamps clearBeforeFrame to the cache size before forming the end iterator; ICO returns early (false) when firstFrameBuffer() is still null. The isComplete() ASSERTs become real guards: GIF returns setFailed(), PNG longjmps on a missing backing store, and the frameRect()/hasAlpha disposal logic is wrapped in if (prevBuffer->backingStore()). ImageBackingStore’s constructor upgrades setSize(size) to RELEASE_ASSERT(setSize(size)) so an allocation failure crashes deterministically rather than leaving a zero-backed store that downstream code treats as valid, and RELEASE_ASSERTs convert the remaining index invariants from silent UB into controlled aborts. The semantic change is that unbacked/evicted frames now fail the decode (or are skipped for opacity inference) instead of being read as if present.

Patch walkthrough

• Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp — Clamps frameBufferAtIndex to m_frameBufferCache.size()-1; defers storing m_frameCount/m_playCount until after the acTL range check; adds RELEASE_ASSERT(m_currentFrame < size) in rowAvailable and frameComplete; longjmps on a null prevBuffer->backingStore() in initFrameBuffer; wraps the frameComplete disposal/opacity read in a backing-store null check and asserts m_currentFrame != 0 before decrementing.
• Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp — frameComplete only reads prevBuffer->backingStore()->frameRect() when the backing store exists (evicted frames have none); initFrameBuffer replaces ASSERT(prevBuffer->isComplete()) with a real setFailed() when the backing store is missing.
• Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp — decodeAtIndex null-checks the embedded PNG sub-decoder’s firstFrameBuffer() and returns false instead of dereferencing it, since it is null until that sub-decoder has parsed its own header.
• Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp — clearDecodedPixelDataIfNeeded clamps clearBeforeFrame to m_frameBufferCache.size() before forming the end iterator, preventing an iterator past Vector::end().
• Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp — frameBufferAtIndex returns nullptr when index >= m_frameBufferCache.size() after decode(), because decode() may bail early leaving the cache smaller than the demuxer-advertised count.
• Source/WebCore/platform/graphics/ImageBackingStore.h — Constructor changes setSize(size) to RELEASE_ASSERT(setSize(size)) so a failed backing-store allocation aborts rather than silently producing an unbacked store.
• Source/WebCore/testing/Internals.cpp / Internals.h / Internals.idl — Adds the test-only Internals.forceDecodeImageFrameAtIndex(img, index) hook (returns bitmapImage->nativeImageAtIndex(index)!=null) so a layout test can force synchronous decode of a specific frame, which is what drives these paths deterministically.

Background

APNG acTL chunk — The Animation Control chunk of an animated PNG; its num_frames field (parsed by png_get_uint_32) advertises the frame count and play count. It is fully attacker-controlled and need not match the number of fcTL/fdAT frames actually present.

m_frameBufferCache — A WTF Vector on each ScalableImageDecoder holding the decoded (or in-progress) frames. Indexing it out of range is heap OOB; subscript does not bounds-check in release builds.

ScalableImageDecoderFrame / ImageBackingStore — A frame owns an optional ImageBackingStore (the pixel buffer). backingStore() returns nullptr for frames that were evicted to save memory or not yet allocated; frameRect()/initialize() assume one exists.

DisposalMethod / RestoreToBackground — Animation disposal semantics: deciding a frame’s opacity from the previous frame requires the previous frame’s rect, which lives in its backing store. An evicted previous frame therefore has an unknown rect.

RELEASE_ASSERT vs ASSERT — ASSERT is compiled out of release/shipping WebKit, so an ASSERT(prevBuffer->isComplete()) gave no production protection; RELEASE_ASSERT aborts in all builds, turning an exploitable OOB into a deterministic, non-corrupting crash.

Vulnerability window

1. Setup — Attacker crafts an APNG whose acTL advertises a large num_frames but ships few/no fcTL+fdAT frames (or a WEBP/ICO/JPEG-XL with a demuxer count exceeding the buffers actually produced).
2. Delivery — The image is loaded via , CSS background, or canvas drawImage in a victim page, invoking the matching ScalableImageDecoder.
3. Trigger — frameBufferAtIndex(N) is called for a frame index up to the advertised count; the old advertised-count clamp lets N exceed m_frameBufferCache.size().
4. OOB / null deref — m_frameBufferCache[N] reads past the Vector, or a disposal path dereferences a null backingStore()/firstFrameBuffer(), reading attacker-adjacent heap or crashing.
5. Disclosure — Decoded pixels sourced from OOB memory can be read back by script via canvas getImageData, turning the OOB read into a cross-origin-adjacent heap info leak (inferred).

Proof of concept

The diff ships Internals::forceDecodeImageFrameAtIndex (returns !!bitmapImage->nativeImageAtIndex(index)), the exact primitive a layout test needs to force synchronous decode of a chosen frame — the actual test image/file is not in the diff, so the concrete malformed-image bytes are (inferred). Trigger: load an APNG whose acTL advertises more frames than are backed, then force-decode indices up to and beyond the real buffer count; pre-patch the advertised-count clamp indexes m_frameBufferCache out of bounds.

<!-- Regression-test shape implied by the new Internals hook -->
<img id="x" src="malformed-actl-advertises-more-frames.png">
<script>
  const img = document.getElementById('x');
  img.onload = () => {
    // acTL num_frames >> actual fcTL frames; force each advertised frame to decode
    for (let i = 0; i < internals.imageFrameCount(img) + 8; i++)
      internals.forceDecodeImageFrameAtIndex(img, i); // pre-patch: OOB read of m_frameBufferCache
  };
</script>

Exploitation

1. Reach — Pure web content: any /CSS/canvas load of attacker bytes reaches the decoder; no permission or user gesture needed.
2. Trigger — Advertise a frame count exceeding the shipped/allocated buffers (APNG acTL, WEBP/JPEG-XL demuxer count) and request a high frame index.
3. Primary primitive — Bounded OOB read of a heap Vector (plus null-deref DoS); read offset is index*sizeof(frame) past the allocation.
4. Secondary — Interpreting OOB bytes as a ScalableImageDecoderFrame yields a wild backingStore()/rect, which drives further reads during initialize()/clearRect — a potential pivot from pure read toward reading attacker-chosen adjacent pixels into a drawable buffer.
5. Escalation (open) — Open questions gating anything past info-leak/crash: can heap grooming place a controlled object immediately after the Vector so the OOB-read frame’s pointers are attacker-chosen, and can the subsequent initialize()/copy turn a crafted backingStore pointer into an OOB write? Not provable from the diff (inferred).

Detection & hunting

For defenders and detection engineers:

• Decoder abort / RELEASE_ASSERT crash —
• acTL/demuxer count vs frame-buffer count mismatch —
• Null backing-store access —

Remaining risk in the patch

• Crash-hardening rather than parse-hardening — Several fixes are RELEASE_ASSERTs and setFailed()/longjmp — they convert OOB into a deterministic abort (DoS) rather than gracefully decoding, so a malformed image is now a reliable renderer crash.
• Unverified bodies — setSize(), initialize(), firstFrameBuffer() and decodeIfNeededAndGetFrameCount() bodies are not in the diff; the fixes assume setSize() truly reports allocation failure and that decode() leaves the cache in the state the new guards expect — unverified from the supplied diff.

Audit directions

• Other ScalableImageDecoder subclasses —
• frameCount() consumers —
• backingStore() dereferences —
• ASSERT-as-guard antipattern —