validate rowBytes and width overflow in avifImageRGBToYUV#3146
Conversation
|
I've opened a fix PR for this in #3291. Root cause confirmed under ASan: rgb->pixels[ offsetBytesR + (i * rgbPixelBytes) + (j * rgbRowBytes) ]reads 1 byte past the end of the heap allocation at column Fix: 4-line early-return guard in each entry point + a regression test. See #3291. |
…oops The rowBytes guard added in the previous commit (issue AOMediaCodec#3146) only covered avifImageRGBToYUV and avifImageYUVToRGB. The same missing validation is present in three more public API functions whose manual C pixel loops access pixels at offset (i * pixelSize + j * rowBytes) without first verifying that rowBytes >= width * pixelSize: * avifRGBImagePremultiplyAlpha (src/alpha.c) * avifRGBImageUnpremultiplyAlpha (src/alpha.c) * avifRGBImageApplyGainMap (src/gainmap.c) — baseImage * avifRGBImageComputeGainMap (src/gainmap.c) — baseRgbImage + altRgbImage Both alpha functions are confirmed under ASan: heap-buffer-overflow READ at alpha.c:261 when rowBytes = pixelSize and width = 100. Add early-return guards returning AVIF_RESULT_REFORMAT_FAILED / AVIF_RESULT_INVALID_ARGUMENT, and extend the regression test suite with TEST(AlphaTest, UndersizedRowBytesFailsPremultiply).
| // pixel format. The slow-path conversion loop indexes pixels as: | ||
| // rgb->pixels[offsetBytes + (i * pixelBytes) + (j * rowBytes)] | ||
| // A rowBytes that is smaller than (width * pixelBytes) would cause the | ||
| // per-row offset to under-stride, producing out-of-bounds reads on the |
There was a problem hiding this comment.
Please add a test showing out-of-bounds reads with reasonable API use.
There was a problem hiding this comment.
Went to write this and couldn't get an out-of-bounds read with reasonable use. The slow path reads rgb->pixels at the caller-supplied rgb->rowBytes, and avif.h already documents that rowBytes must be at least width * avifRGBImagePixelSize(). Every in-tree caller goes through avifRGBImageAllocatePixels(), which sets rowBytes to exactly that, so none of them can trip it.
The only way to reach the over-read is to hand-set rowBytes below that documented minimum. The ASan repro in #3291 does the same thing (rowBytes = pixelSize with width = 100), so it's that contract violation rather than reasonable use.
So this is really defense-in-depth against documented misuse, not a reachable bug. I'm fine closing it, or keeping the guard as a cheap INVALID_ARGUMENT return instead of an OOB read. Your call.
3 participants
Added validation in avifImageRGBToYUV to make sure rowBytes and width are safe before processing.
What was the issue
The function was using rowBytes directly without checking if it was actually large enough for the given width and pixel size.
If rowBytes is smaller than expected, it can read past the buffer.
Also, width * pixelSize could overflow in some cases.
What I changed
Impact
This prevents out-of-bounds reads when invalid RGB input is passed.