Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix avifParseItemLocationBox() itemReferenceIndex #2080

Merged
merged 2 commits into from
Mar 30, 2024
Merged

Fix avifParseItemLocationBox() itemReferenceIndex #2080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9795108
Fix avifParseItemLocationBox() itemReferenceIndex
y-guyon Mar 29, 2024
0b801b8
Add CHANGELOG.md entries
y-guyon Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ The changes are relative to the previous release, unless the baseline is specifi
(https://crbug.com/oss-fuzz/65657).
* ext/libjpeg.cmd now pulls libjpeg-turbo instead of libjpeg and AVIF_JPEG=LOCAL
now expects the library dependency in ext/libjpeg-turbo/build.libavif.
* Fix 'iloc' box parsing bugs that may have wrongly accepted, rejected or parsed
some files with rare values of offset_size, length_size, base_offset_size and
index_size.

## [1.0.4] - 2024-02-08

Expand Down
30 changes: 18 additions & 12 deletions src/read.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1744,23 +1744,31 @@ static avifResult avifParseItemLocationBox(avifMeta * meta, const uint8_t * raw,
{
BEGIN_STREAM(s, raw, rawLen, diag, "Box[iloc]");

// Section 8.11.3.2 of ISO/IEC 14496-12.
uint8_t version;
AVIF_CHECKERR(avifROStreamReadVersionAndFlags(&s, &version, NULL), AVIF_RESULT_BMFF_PARSE_FAILED);
if (version > 2) {
avifDiagnosticsPrintf(diag, "Box[iloc] has an unsupported version [%u]", version);
return AVIF_RESULT_BMFF_PARSE_FAILED;
}

uint8_t offsetSize, lengthSize, baseOffsetSize;
uint8_t offsetSize, lengthSize, baseOffsetSize, indexSize = 0;
uint32_t reserved;
AVIF_CHECKERR(avifROStreamReadBits8(&s, &offsetSize, /*bitCount=*/4), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(4) offset_size;
AVIF_CHECKERR(avifROStreamReadBits8(&s, &lengthSize, /*bitCount=*/4), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(4) length_size;
AVIF_CHECKERR(avifROStreamReadBits8(&s, &baseOffsetSize, /*bitCount=*/4), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(4) base_offset_size;
if (((version == 1) || (version == 2)) && (baseOffsetSize != 0)) {
avifDiagnosticsPrintf(diag, "Box[iloc] has an unsupported base_offset_size [%u]", baseOffsetSize);
if (version == 1 || version == 2) {
AVIF_CHECKERR(avifROStreamReadBits8(&s, &indexSize, /*bitCount=*/4), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(4) index_size;
} else {
AVIF_CHECKERR(avifROStreamReadBits(&s, &reserved, /*bitCount=*/4), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(4) reserved;
}

// Section 8.11.3.3 of ISO/IEC 14496-12.
if ((offsetSize != 0 && offsetSize != 4 && offsetSize != 8) || (lengthSize != 0 && lengthSize != 4 && lengthSize != 8) ||
(baseOffsetSize != 0 && baseOffsetSize != 4 && baseOffsetSize != 8) || (indexSize != 0 && indexSize != 4 && indexSize != 8)) {
avifDiagnosticsPrintf(diag, "Box[iloc] has an invalid size");
return AVIF_RESULT_BMFF_PARSE_FAILED;
}
uint32_t reserved;
AVIF_CHECKERR(avifROStreamReadBits(&s, &reserved, /*bitCount=*/4), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(4) reserved;

uint16_t tmp16;
uint32_t itemCount;
Expand Down Expand Up @@ -1796,7 +1804,7 @@ static avifResult avifParseItemLocationBox(avifMeta * meta, const uint8_t * raw,
return AVIF_RESULT_BMFF_PARSE_FAILED;
}

if ((version == 1) || (version == 2)) {
if (version == 1 || version == 2) {
AVIF_CHECKERR(avifROStreamReadBits(&s, &reserved, /*bitCount=*/12), AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(12) reserved = 0;
if (reserved) {
avifDiagnosticsPrintf(diag, "Box[iloc] has a non null reserved field [%u]", reserved);
Expand All @@ -1805,8 +1813,8 @@ static avifResult avifParseItemLocationBox(avifMeta * meta, const uint8_t * raw,
uint8_t constructionMethod;
AVIF_CHECKERR(avifROStreamReadBits8(&s, &constructionMethod, /*bitCount=*/4),
AVIF_RESULT_BMFF_PARSE_FAILED); // unsigned int(4) construction_method;
if ((constructionMethod != 0 /* file */) && (constructionMethod != 1 /* idat */)) {
// construction method item(2) unsupported
if (constructionMethod != 0 /* file offset */ && constructionMethod != 1 /* idat offset */) {
// construction method item(2) unsupported (item offset)
wantehchang marked this conversation as resolved.
Show resolved Hide resolved
avifDiagnosticsPrintf(diag, "Box[iloc] has an unsupported construction method [%u]", constructionMethod);
return AVIF_RESULT_BMFF_PARSE_FAILED;
}
Expand All @@ -1830,10 +1838,8 @@ static avifResult avifParseItemLocationBox(avifMeta * meta, const uint8_t * raw,
uint16_t extentCount; // unsigned int(16) extent_count;
AVIF_CHECKERR(avifROStreamReadU16(&s, &extentCount), AVIF_RESULT_BMFF_PARSE_FAILED); //
for (int extentIter = 0; extentIter < extentCount; ++extentIter) {
// If extent_index is ever supported, this spec must be implemented here:
// :: if (((version == 1) || (version == 2)) && (index_size > 0)) {
// :: unsigned int(index_size*8) extent_index;
// :: }
uint64_t itemReferenceIndex; // unsigned int(index_size*8) item_reference_index; (ignored unless construction_method=2)
AVIF_CHECKERR(avifROStreamReadUX8(&s, &itemReferenceIndex, indexSize), AVIF_RESULT_BMFF_PARSE_FAILED);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit/optional: It would be clearer to put this call inside if (indexSize > 0). The corresponding spec is:

    if (((version == 1) || (version == 2)) && (index_size > 0)) {
         unsigned int(index_size*8) item_reference_index;
    }

But perhaps it is that spec that should omit the && (index_size > 0) check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offset_size can be 0 so it makes as much sense as unsigned int(0) extent_offset; to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #2091.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. While reviewing #2091, I discovered a subtle point about item_reference_index -- its value is 1 rather than 0 when item_size is 0. Here is the excerpt from ISO/IEC 14496-12:2022 Section 8.11.3.1, page 82:

The item_reference_index is only used for the method item_offset; it indicates the 1-based index of the item reference with referenceType 'iloc' linked from this item. If index_size is 0, then the value 1 is implied; the value 0 is reserved.

Calling avifROStreamReadUX8(&s, &itemReferenceIndex, indexSize) with indexSize=0 will set itemReferenceIndex to 0. So item_reference_index needs different handling anyway.


uint64_t extentOffset; // unsigned int(offset_size*8) extent_offset;
AVIF_CHECKERR(avifROStreamReadUX8(&s, &extentOffset, offsetSize), AVIF_RESULT_BMFF_PARSE_FAILED);
Expand Down
Loading