Reject item-location boxes that over-allocate memory - #3372
matejsmycka wants to merge 1 commit into
Conversation
| AVIF_CHECKERR(avifROStreamReadUX8(&s, &baseOffset, baseOffsetSize), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(base_offset_size*8) base_offset; | ||
| uint16_t extentCount; | ||
| AVIF_CHECKERR(avifROStreamReadU16(&s, &extentCount), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(16) extent_count; | ||
| const uint64_t bytesPerExtent = (uint64_t)(((version == 1 || version == 2) ? indexSize : 0)) + offsetSize + lengthSize; |
There was a problem hiding this comment.
Nit: since indexSize is initialized to 0, we can omit the (version == 1 || version == 2) check.
| AVIF_CHECKERR(avifROStreamReadUX8(&s, &baseOffset, baseOffsetSize), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(base_offset_size*8) base_offset; | ||
| uint16_t extentCount; | ||
| AVIF_CHECKERR(avifROStreamReadU16(&s, &extentCount), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(16) extent_count; | ||
| const uint64_t bytesPerExtent = (uint64_t)(((version == 1 || version == 2) ? indexSize : 0)) + offsetSize + lengthSize; |
There was a problem hiding this comment.
It is not necessary to use the uint64_t type for the sum of indexSize, offsetSize, and lengthSize. All three variables are <= 8, so their sum is at most 24.
| return AVIF_RESULT_BMFF_PARSE_FAILED; | ||
| } | ||
| #endif | ||
| extent->size = (size_t)extentLength; |
There was a problem hiding this comment.
I found that we don't handle a zero extentLength correctly. ISO BMFF says that if the value of exteng_length is 0, then the length of the extent is the length of the entire referenced container.
| uint16_t extentCount; | ||
| AVIF_CHECKERR(avifROStreamReadU16(&s, &extentCount), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(16) extent_count; | ||
| const uint64_t bytesPerExtent = (uint64_t)(((version == 1 || version == 2) ? indexSize : 0)) + offsetSize + lengthSize; | ||
| AVIF_CHECKERR(extentCount == 0 || bytesPerExtent > 0, AVIF_RESULT_BMFF_PARSE_FAILED); |
There was a problem hiding this comment.
It seems that we should also allow bytesPerExtent to be zero when extentCount == 1. Please see my comment at line 2194.
| AVIF_CHECKERR(avifROStreamReadU16(&s, &extentCount), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(16) extent_count; | ||
| const uint64_t bytesPerExtent = (uint64_t)(((version == 1 || version == 2) ? indexSize : 0)) + offsetSize + lengthSize; | ||
| AVIF_CHECKERR(extentCount == 0 || bytesPerExtent > 0, AVIF_RESULT_BMFF_PARSE_FAILED); | ||
| AVIF_CHECKERR((uint64_t)extentCount * bytesPerExtent <= avifROStreamRemainingBytes(&s), AVIF_RESULT_BMFF_PARSE_FAILED); |
There was a problem hiding this comment.
Since bytesPerExtent is at most 24, it is not necessary to cast extentCount to uint64_t because the usual integer promotion to int is large enough for the product. Or we can cast to uint32_t or size_t (the return type of avifROStreamRemainingBytes().
Also, from a security point of view, this check is less important than the bytesPerExtent > 0 check, because it is the zero bytesPerExtent case that causes us to allocate a disproportionate amount of memory relative to the input size. So we could omit this check.
A crafted file can mint many storage extents from just a few bytes when the extent size fields are all zero, forcing large allocations before any image is decoded. Reject an item that declares more than one extent when each extent consumes no input.
64d21c8 to
3aa171c
Compare
|
Thanks for the quick review Wan-Teh; feedback added |
2 participants
Problem
avifParseItemLocationBox()reads a 16-bitextent_countper item and pushes that manyavifExtentrecords without checking the count against the bytes remaining in the box. Theilocsize fields may be zero, andavifROStreamReadUX8()consumes no input for a zero size factor, so each ~6-byte item can allocate ~1 MiB. This runs insideavifDecoderParse()with no AV1 payload and no codec linked, and none of the decoder limits gate it. The cost is paid while the file is ultimately rejected.Fix
Before the extent loop, compute the per-extent wire size, reject a non-zero
extent_countwhose extents would consume no input, and reject anyextent_countwhose total wire size exceeds the bytes left in the box.Testing
Built parse-only from a clean checkout, patched vs unpatched, with a harness calling
avifDecoderParse()and reporting peak RSS:Valid AVIFs, including files with genuine multi-byte extents, still parse unchanged.
This is availability-only (no memory corruption, no information disclosure).