Skip to content

Fix heap buffer overflow in JPEG encoder xsize/ysize handling#213

Open
sharadboni wants to merge 1 commit intogoogle:mainfrom
sharadboni:fix/security-heap-overflow-encoder-exr
Open

Fix heap buffer overflow in JPEG encoder xsize/ysize handling#213
sharadboni wants to merge 1 commit intogoogle:mainfrom
sharadboni:fix/security-heap-overflow-encoder-exr

Conversation

@sharadboni
Copy link
Copy Markdown
Contributor

@sharadboni sharadboni commented Apr 27, 2026

Security fix

Heap buffer overflow in JPEG encoder (xsize/ysize mismatch) (lib/extras/enc/jpegli.cc)

In EncodeJpeg, the pixel-copy loops iterate up to info.ysize/info.xsize, but row_bytes and pixels are sized for image.ysize/image.xsize. When PackedImage dimensions differ from JxlBasicInfo dimensions, the loop reads past the end of the source buffer and writes past the end of row_bytes.

Fixed by clamping the loop bounds to image.ysize/image.xsize.


The original PR also bundled an unrelated EXR decoder fix; per @eustas's review that has been split out into #216.

@sharadboni
Copy link
Copy Markdown
Contributor Author

Hi @eustas — would you be able to review this security fix? It addresses heap buffer overflows in the JPEG encoder (xsize/ysize mismatch) and EXR decoder (negative-count memcpy when display/data windows don't overlap).

@eustas
Copy link
Copy Markdown
Collaborator

eustas commented Apr 30, 2026

Hi. Lets split this to 2 PRs. One of them (about EXR) will be useful for JPEG XL as well.

And for EXR let's do some combing: pull exr_x1 and exr_x2 calculation before exr_y loop.
As well calculate exr_x_span to be max(0, exr_x2 - exr_x1 + 1). Enter exr_y only if exr_x_span is positive.

@sharadboni sharadboni force-pushed the fix/security-heap-overflow-encoder-exr branch from 1098e2b to 337c67e Compare April 30, 2026 15:47
@sharadboni sharadboni changed the title Fix heap buffer overflows in JPEG encoder xsize handling and EXR decoder Fix heap buffer overflow in JPEG encoder xsize/ysize handling Apr 30, 2026
@sharadboni
Copy link
Copy Markdown
Contributor Author

sharadboni commented Apr 30, 2026

Thanks for the review, split done.

@eustas
Copy link
Copy Markdown
Collaborator

eustas commented May 6, 2026

Good. Now let's deal with root cause. Both image and info come from same source: ppf.

Moreover, image is just a first frame.

So, instead of "plumbing" let's do the same thing other encoders: use VerifyPackedImage for that frame.

When a PackedPixelFile is constructed with frame[0].color dimensions
that disagree with info.xsize/ysize, the encoder's pixel-copy loops in
EncodeJpeg iterate up to info.* while pixels and row_bytes are sized
for image.*, causing OOB reads and writes past the source buffer and
the destination row buffer.

VerifyInput already calls Encoder::VerifyImageSize, but that helper
intentionally leaves the xsize/ysize equality check disabled (TODO in
encode.cc) since some encoders allow per-frame sizes to differ from
the basic info. jpegli is not such an encoder: cinfo.image_width/height
and the buffer math both come from info.*, so the assumption is
load-bearing and must be enforced locally.

Adds the dim-equality check to VerifyInput right after the existing
VerifyImageSize call. Also adds a regression test in jpegli_test.cc
covering all three mismatch directions.

The first attempt at this fix (clamping the encoder loops to image.*)
was rejected during review as plumbing rather than a root-cause fix.
@sharadboni sharadboni force-pushed the fix/security-heap-overflow-encoder-exr branch from 337c67e to c38e270 Compare May 7, 2026 17:44
@sharadboni
Copy link
Copy Markdown
Contributor Author

Pushed an updated version that addresses the root cause per your suggestion (force-push, head now c38e270).

Approach. Instead of clamping the encoder loops, VerifyInput now rejects frames whose dimensions don't match ppf.info right after the existing Encoder::VerifyImageSize call:

const PackedImage& image = ppf.frames[0].color;
JPEGLI_RETURN_IF_ERROR(Encoder::VerifyImageSize(image, info));
if (image.xsize != info.xsize || image.ysize != info.ysize) {
  return JPEGLI_FAILURE("Frame size does not match image size.");
}

The encoder's pixel-copy loops are back to info.xsize/info.ysize (the pre-PR original), no plumbing.

Why a local check rather than uncommenting the equality test in Encoder::VerifyImageSize. That helper has a TODO(jon) deliberately leaving the dim-equality check disabled because some encoders allow per-frame sizes to differ from the basic info. Tightening it globally could regress those callers. jpegli is the encoder where the assumption is load-bearing (cinfo.image_width/image_height and the buffer math both come from info.*), so the check belongs in jpegli's input verifier rather than in the shared base. Happy to move it into VerifyImageSize if you'd prefer the global tightening, just let me know.

Regression test. Added JpegliTest.JpegliEncodeRejectsMismatchedFrameSize in lib/extras/jpegli_test.cc covering all three mismatch directions (info larger ysize, larger xsize, smaller xsize). Verified locally:

  • On the patched build (Debug + -fsanitize=address,undefined): test passes; encoder returns JPEGLI_FAILURE: "Frame size does not match image size."
  • On the same test against unpatched code: ASan trips with heap-buffer-overflow at jpegli.cc:514 (memcpy reading past the source buffer), confirming the test actually catches the bug.

Also rebased on current main (the de-jxl-isation rename), so the PR is now mergeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants