Skip to content

heif/avif: validate container image sizes before decoding - #498

Open
alepacheco wants to merge 3 commits into
google:mainfrom
alepacheco:fix/heif-plane-size
Open

alepacheco wants to merge 3 commits into
google:mainfrom
alepacheco:fix/heif-plane-size

Conversation

@alepacheco

Copy link
Copy Markdown

decodeHeicUltraHdr() and decodeAvifUltraHdr() take the size of the base image and of the gain map from heif_image_handle_get_width/height(), which libheif derives from the ispe and clap properties of the file, and use it without checking it against anything. Two heap overflows follow, both reachable from uhdr_decode() on an untrusted HEIC/AVIF file:

  1. copy_img_plane() copies that many rows and bytes per row out of the decoded image. An ispe larger than the coded size, or a clap declaring a larger crop than the coded image can provide, makes the copy read past the end of the decoded plane.
  2. A gain map item without an ispe property has a size of 0x0. It is allocated as an empty image and passed to applyGainMap(), which divides by the gain map width and samples it with coordinates clamped to w - 1 and h - 1; with w == 0 the clamps wrap around and sampleMap*() reads past the end of the gain map buffer.

This PR makes the container size something the decoders check before they use it:

  • heif/avif: reject unsupported image sizes before decoding: the decoders read the image size only through get_image_size(), which rejects sizes that are zero or above kMaxWidth x kMaxHeight before anything is decoded or allocated, the same limits the JPEG path enforces. uhdr_dec_probe() applies the same check (is_supported_heif_dimensions(), now available without the item API) to the base image and the gain map, so probe and decode agree on what is decodable.
  • heif/avif: never copy more than the decoded plane holds: copy_img_plane() fails the decode when the decoded plane is smaller than the size being copied, or missing. All base image and gain map copies in both backends go through it. Files whose coded image is larger than declared keep decoding as before.

Tests: UltraHdrApiTest.HeifRejectsImageSizeLargerThanDecodedImage and UltraHdrApiTest.HeifRejectsUnsupportedImageSizes edit the ispe/clap properties and their associations in the existing gainmap_hevc_16x16.heic fixture in memory. Without the fixes AddressSanitizer reports a heap-buffer-overflow in copy_img_plane() and in sampleMap3Channel() respectively; with them, the full unit test suite passes.

decodeHeicUltraHdr() and decodeAvifUltraHdr() size the output images from
heif_image_handle_get_width/height(), which libheif derives from the 'ispe'
and 'clap' properties, and copy_img_plane() then copies that many rows and
bytes per row out of the decoded image. Nothing ties those properties to the
coded image: an 'ispe' larger than the coded size, or a 'clap' declaring a
larger crop than the coded image can provide, makes the copy read past the
end of the decoded plane (heap-buffer-overflow).

copy_img_plane() now fails the decode when the decoded plane is smaller than
the size being copied (or missing). All base image and gain map copies in both
backends go through it. Files whose coded image is larger than declared keep
decoding as before.
The HEIF and AVIF decoders take the size of the base image and of the gain
map from heif_image_handle_get_width/height() without checking it. That
size comes from the container: an image item without an 'ispe' property
has a size of 0x0, and 'ispe' can declare a size far above what the
library supports.

A 0x0 gain map is accepted, allocated as an empty image and passed to
applyGainMap(), which divides the base image width by the gain map width
and then samples the gain map with coordinates clamped to w - 1 and
h - 1. With w == 0 those clamps wrap around and sampleMap*() reads past the
end of the gain map buffer (heap-buffer-overflow). A huge 'ispe' is
allocated as is.

The decoders now read the image size only through get_image_size(), which
rejects sizes that are zero or above kMaxWidth x kMaxHeight before
anything is decoded or allocated, the same limits the JPEG path enforces.
uhdr_dec_probe() applies the same check to the base image and the gain
map, so probe and decode agree on what is decodable; like
uhdr_is_supported_gainmap_image(), it now rejects a file whose gain map
has an unsupported size even when only SDR output is requested.
@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@gregbenz

Copy link
Copy Markdown
Contributor

Small test-portability issue: with -DUHDR_MAX_DIMENSION=16384, these dimensions are within the supported limit, so probing correctly succeeds and this assertion fails. Could the oversized case use the configured kMaxWidth/kMaxHeight, with a conditional skip if exceeding those would also exceed libheif’s parser limits?

The oversized 'ispe' case hardcoded 16384x16384, which is within the
supported size when the library is built with -DUHDR_MAX_DIMENSION=16384.
Use kMaxWidth + 1 and kMaxHeight + 1 instead, one dimension at a time so
the pixel count stays small, and skip when such a size would already be
rejected by libheif's own image size limits.
@alepacheco

Copy link
Copy Markdown
Author

@gregbenz Thanks, good catch. Done in d3a5c76: the oversized case now uses kMaxWidth + 1 and kMaxHeight + 1, one dimension at a time with the other kept at 64 so the pixel count stays small, and it is skipped (GTEST_SKIP) when that size would already exceed libheif's max_image_size_pixels or INT_MAX. The unit tests pass with the default limit and with -DUHDR_MAX_DIMENSION=16384.

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