[2] PNGImageDecoder ICC transform overruns the frame buffer

HighWebCore image decodersOOB

The APNG geometry check authorised the compositing loop, not the transform after it.

5b391e6

High. The geometry check that gates APNG sub-frames authorises the compositing loop, not the colour transform that follows it — and the transform's length is one origin term larger. Every input needed is in the image file, so the only gating condition is that the port builds with LCMS.

Image decoders are the classic attacker-controlled parsing surface: the entire geometry of what gets written, and where, comes from untrusted bytes. An animated PNG declares a canvas in IHDR and then a sequence of sub-frames, each carrying an fcTL chunk with xOffset, yOffset, width and height; the decoder composites each sub-frame into one contiguous full-canvas pixel allocation. The expectation the pipeline rests on is that a per-frame geometry check performed at parse time confines every subsequent per-pixel operation on that frame to the allocation.

The angle: a crafted animated PNG with an embedded ICC profile can drive a heap read-modify-write past the end of the decoded-image allocation on GTK/WPE, with attacker-chosen overrun length and values passed through an attacker-supplied colour transform.

Commit message

Out-of-bounds write in PNGImageDecoder::frameComplete() ICC transform

1. destinationRow starts at pixelsStartingAt(rect.x(), y) and the loop above writes rect.width() pixels, but cmsDoTransform gets rect.maxX() as its pixel count.
2. With a TYPE_BGRA_8 transform that is a 4-byte-per-pixel read-modify-write, so once 2*rect.x() + rect.width() passes the canvas width it runs off each row, and on the last row (yOffset + height == height) past the pixel allocation. The only fcTL guard is xOffset + width <= width, which does not stop that; reachable from an animated PNG with an RGB iCCP chunk on ports built with USE(LCMS) (GTK, WPE).

Pass rect.width() so the count matches the row-relative span, like the first-frame path in rowAvailable().

Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp

LayoutTests/fast/images/animated-png-icc-transform-crash.html

LayoutTests/fast/images/resources/animated-png-icc-transform-crash.png

Patch Details

One production line changes, inside the #if USE(LCMS) block of PNGImageDecoder::frameComplete() that applies an embedded ICC profile to a just-composited APNG sub-frame. The per-row loop obtains destinationRow from pixelsStartingAt(rect.x(), y) and writes rect.width() pixels; the in-place cmsDoTransform() call that follows passed rect.maxX() — that is, rect.x() + rect.width() — as the LCMS pixel count. The patch changes that fourth argument to rect.width(), matching the row-relative span actually written and matching the first-frame path in rowAvailable(). The remaining files are collateral: a layout test, its expectation file, and a crafted APNG whose second frame sits at xOffset + width == canvas width with an embedded ICC profile.

Length argument expressed in a different coordinate space than the base pointer it is applied to, so an absolute end-coordinate is used where a relative span length is required.

Background

APNG frame model. An animated PNG declares a canvas size in IHDR and a sequence of sub-frames; each sub-frame carries an fcTL chunk giving xOffset, yOffset, width and height. Sub-frames are composited into a single full-canvas frame buffer, so the decoder writes a height-row band starting at canvas column xOffset.

Frame buffer layout. A decoded frame is a contiguous 4-bytes-per-pixel allocation of canvasWidth * canvasHeight pixels. A helper of the form pixelsStartingAt(x, y) returns a pointer into that allocation at row y, column x, so consecutive rows are adjacent in memory with no per-row padding or guard.

IntRect accessors. rect.width() is the span of the rectangle. rect.x() is its left edge in canvas coordinates. rect.maxX() equals rect.x() + rect.width() — an absolute coordinate, not a length. Both return the same type, which is the whole reason the two are confusable at a call site.

LCMS and cmsDoTransform. Little-CMS is the colour-management library used on some WebKit ports. cmsDoTransform(transform, input, output, pixelCount) converts pixelCount pixels from input to output using a transform built from a source profile — here the image's embedded profile — and a destination profile. Passing the same pointer for input and output makes it an in-place read-modify-write over pixelCount * bytesPerPixel bytes, with the byte width per pixel fixed by the format the transform was created with (a 4-byte BGRA-style format for premultiplied frame buffers). An ICC profile can encode arbitrary curves and lookup tables, so the byte-to-byte mapping the transform applies is determined by data in the image file.

iCCP chunk. The PNG chunk carrying an embedded ICC profile; the decoder builds m_iccTransform from it when colour management is enabled.

Analysis

The bug is a heap out-of-bounds write via an off-by-rect.x()-pixels length, with an accompanying out-of-bounds read because the transform is a read-modify-write over the same span.

  Row in the frame buffer (canvas width W):

  col 0            x = rect.x()        rect.maxX()          W
   |                    |                   |               |
   +--------------------+===================+---------------+
                        |  written by the   |
                        |  loop: width px   |
                        |                   |
                        |<---- cmsDoTransform gets maxX() = x + width ---->|
                                                                   overrun: x px
  Last row (yOffset + height == canvasHeight):
       overrun leaves the allocation entirely  ──►  4 * rect.x() bytes past end

The base pointer is already offset by rect.x(), and the count was canvas-absolute rather than row-relative, so the transform touches canvas columns [rect.x(), 2*rect.x() + rect.width()). Whenever 2*rect.x() + rect.width() > canvasWidth it runs past the end of the row. The only validation applied to the fcTL chunk is xOffset + width <= canvasWidth (the supplied source excerpt is truncated before the geometry-validation code, so the exclusivity of that guard follows the commit message), which is satisfied by e.g. xOffset = W/2, width = W/2 while 2*xOffset + width = 1.5*W overshoots. For interior rows the overrun spills into the following row of the same contiguous allocation — in-bounds corruption of image content. For the final row of a sub-frame whose yOffset + height == canvasHeight, the row being processed is the last row of the allocation, so the same overshoot walks off the end: up to rect.x() pixels, i.e. 4 * rect.x() bytes of heap read-modify-write.

Concrete trigger, following the bundled image's shape:

1. Emit IHDR with canvas width W, height H.
2. Emit an iCCP chunk with an RGB profile so m_iccTransform is non-null in frameComplete().
3. Emit acTL plus a first full-canvas frame.
4. Emit a second fcTL with xOffset = X, width = Wf such that X + Wf <= W (passing the only geometry guard) but 2*X + Wf > W, and with yOffset + height == H so the sub-frame's last row is the allocation's last row.
5. Load the image — a single <img src="evil.png">, a CSS background, or a drawImage into a canvas is enough — so frameComplete() composites the sub-frame.

On the final row, destinationRow points at the last row's column X, the compositing loop writes Wf pixels in bounds, and cmsDoTransform is handed X + Wf as its pixel count, reading and rewriting X pixels beyond the end of the allocation. (The bundled test's expectation text asserts the horizontal condition explicitly; the vertical placement needed for an allocation-end overrun follows the commit message's description of the last-row case.)

The best-case attacker transition, stated conditionally: the overrun length is a direct function of xOffset, which the attacker picks per frame, so if the allocator places attacker-groomable objects immediately after the pixel allocation — plausible for a large malloc-class buffer, though the specific bin behaviour for the frame backing store is not established by the supplied context — then repeatedly decoding sub-frames with different xOffset values could give a tunable-distance overwrite. Because the write is a read-modify-write through an attacker-supplied ICC profile, and a profile can encode near-arbitrary lookup tables, the value written at each overrun position could be substantially attacker-influenced rather than merely a scramble of the pre-existing bytes — which would make the primitive closer to a controlled overwrite than to a blind smash. Realising that would require heap grooming so a security-relevant object (a length field, a vector capacity, a pointer) lands in the overrun range; a profile whose transform maps the pre-existing bytes at that offset to the desired bytes, which in turn requires knowing or forcing those bytes; and surviving the fact that the same overrun corrupts everything else in the span.

The interior-row case — 2*rect.x() + rect.width() > W without the last-row condition — stays inside the allocation and only corrupts the next row's pixels, so it is a rendering artefact rather than a memory-safety issue on its own. It is a convenient oracle, though: script can confirm the miscount by reading back the composited frame through a canvas.

Discovery reads as targeted pattern auditing of the sub-frame compositing path. The tell is a rectangle accessor used as a length next to a pointer already offset by the rectangle's origin, and the commit message frames the fix as restoring consistency with the rowAvailable() first-frame path — which is how a reviewer comparing the two paths side by side would find it. Fuzzing is a less likely route to this exact shape: reaching it needs three conditions to coincide (an iCCP chunk, a sub-frame with non-zero xOffset where 2*xOffset + width > canvasWidth, and yOffset + height == canvasHeight) on an LCMS-configured build, and interior-row overruns are silent, so a crash-only oracle would rarely fire. The hand-constructed test PNG, built to sit exactly on xOffset + width == canvasWidth, reads as an auditor's minimal reproducer rather than a reduced fuzzer artefact.

Image decoding for <img> happens in the WebContent process, so the primitive lands in the renderer sandbox and full compromise would still require a separate escape. The affected code is behind USE(LCMS), which covers the GTK and WPE ports; Apple ports use a different colour-management backend and do not compile this branch.

This vulnerability weakens memory safety inside the WebContent process at the image-decoding boundary, where the entire geometry and the colour transform itself come from untrusted image bytes. The security model assumption being violated is that the fcTL validation is sufficient to confine all per-frame pixel operations to the frame buffer allocation — it is not, because the ICC transform uses a different, larger span than the compositing loop it follows. An attacker who serves a crafted animated PNG with an embedded RGB iCCP chunk could reach a heap read-modify-write past the end of the decoded-image allocation on LCMS-based ports, with attacker-chosen overrun length and attacker-chosen transform semantics; that would be a starting point for heap corruption in the renderer rather than a mere crash.

Insight

The fcTL validation is an allocation-level bound, but each individual pixel operation in the compositing path carries its own implicit bound, and here two operations on the same row disagreed about their coordinate space. That is the recurring hazard in image decoders: a single geometry check at parse time is treated as blanket authorisation for every downstream pointer/length pair, and any operation deriving its length differently from the one the check was written for silently escapes it. The specific tell — maxX() where width() was meant — is easy to write because both are IntRect accessors returning the same type and, for the common x() == 0 case, they are numerically identical, so the bug is invisible to every non-offset test image.

Audit directions

• An absolute end-coordinate used where a relative span length is required, on a pointer already advanced by the origin. This is dangerous precisely because it is a no-op when the origin is zero, so default-shaped test inputs never exercise it. Narrow: grep Source/WebCore/platform/image-decoders/ and Source/WebCore/platform/graphics/ for maxX(), maxY(), bottom(), or right() appearing as a count/length argument — the last argument of a memcpy/memset/cmsDoTransform, or a loop bound — rather than in a comparison; start with the other APNG and GIF sub-frame compositing paths (GIFImageDecoder::haveDecodedRow, frameComplete) and any remaining #if USE(LCMS) blocks in the decoders. Wider: the same class appears anywhere a base pointer is offset by an origin and a size is computed from an unrelated coordinate frame — blit/scroll rect copies, ImageBuffer sub-rect reads, tiled-layer damage-rect updates, and video frame plane copies where stride and width are conflated. Widest: a length argument must be expressed in the same coordinate space as the pointer it is applied to; this holds in any codebase mixing rectangle types with raw buffer APIs — Skia's SkRect/SkPixmap sub-region draws, FFmpeg's plane/linesize handling, Rust's slice-plus-offset patterns where end and len are both usize. Match tell on every rung: find the line that produced the base pointer and check whether the origin term appears in both the pointer and the count; if it does, the count is wrong.

• Divergence between a fast/first-time path and a slow/incremental path that are supposed to perform the same pixel operation. Here rowAvailable()'s first-frame path used rect.width() while frameComplete()'s sub-frame path used rect.maxX(), so the correct code and the buggy code sat in the same file doing the same job. Narrow: in PNGImageDecoder.cpp, GIFImageDecoder.cpp, JPEGImageDecoder.cpp and WEBPImageDecoder.cpp, diff each first-frame/full-image path against its animation/sub-frame counterpart and confirm the pointer, count and stride expressions are literally identical modulo the sub-frame origin. Wider: the same shape recurs wherever an incremental or partial-update path was added later beside an original whole-object path — progressive versus complete decode, partial repaint versus full repaint, incremental style resolution versus full recalc. Widest: two implementations of the same operation must agree on their bounds arithmetic, and adding a partial-update variant is exactly when they stop agreeing; carry this into any codebase with an added incremental path (V8's incremental marking versus full GC, streaming parsers versus whole-buffer parsers). Match tell: the two paths compute the same quantity with syntactically different expressions — that difference is either an intentional origin adjustment or a bug, and there is no third option.

• Chunk validation enforced at parse time rather than at the point of use. The fcTL guard is checked once, far from the loops that rely on it, and nothing re-derives it at the cmsDoTransform call. Narrow: for each geometry field parsed out of PNG/GIF/WebP chunk headers, trace every consumer and check whether the consumer's own arithmetic is covered by the parse-time predicate — in particular any consumer that adds the offset a second time. Wider: the same class covers all header-then-payload formats where a size field is validated once and then used by several downstream consumers with different arithmetic — font tables, ICC profile tag tables, media container box sizes. Widest: a validation predicate authorises exactly the arithmetic it was written against; every consumer with different arithmetic needs its own check or a span-typed API that cannot express the overrun. Match tell: a bounds check and its beneficiary separated by a function boundary, where the beneficiary's index expression is not textually derivable from the checked expression — if you cannot substitute the checked inequality into the consumer's arithmetic and get a proof, the check does not cover it.

• In-place colour-management calls using a count inconsistent with the transform's format. cmsDoTransform takes a pixel count while the surrounding code frequently reasons in bytes, and the transform's bytes-per-pixel is fixed at creation time far from the call site. Narrow: grep for cmsDoTransform and cmsCreateTransform across Source/WebCore/platform/ and, for each call, confirm the count is a pixel span and that the buffer at the destination pointer holds at least count * bytesPerPixel bytes for the format used at creation. Wider: the same unit-mismatch class covers any API whose length parameter is in elements while the caller's surrounding variables are in bytes or vice versa — wmemcpy, png_read_row row sizes, CGBitmapContext stride arithmetic, audio frame versus sample counts in the WebAudio resampler. Widest: the unit of a length parameter is part of its type and must be established at the call site, not assumed from context. Match tell: a length expression whose variable name or derivation is in one unit passed to a parameter documented in another — if the call site has to multiply or divide by a constant elsewhere in the same function, the unmultiplied call is the suspect.