Skip to content

read: add bounds checks during extent merge to prevent buffer overflow#3004

Open
Naveed8951 wants to merge 2 commits intoAOMediaCodec:mainfrom
Naveed8951:fix/extent-merge-bounds-read
Open

read: add bounds checks during extent merge to prevent buffer overflow#3004
Naveed8951 wants to merge 2 commits intoAOMediaCodec:mainfrom
Naveed8951:fix/extent-merge-bounds-read

Conversation

@Naveed8951
Copy link
Contributor

Validate write offsets and per-extent copy sizes before memcpy when merging item extents to avoid out-of-bounds writes triggered by malformed or malicious AVIF metadata.

Reject invalid extents with AVIF_RESULT_BMFF_PARSE_FAILED.

Validate write offsets and per-extent copy sizes before memcpy when
merging item extents to avoid out-of-bounds writes triggered by malformed
or malicious AVIF metadata.

Reject invalid extents with AVIF_RESULT_BMFF_PARSE_FAILED.
src/read.c Outdated
AVIF_ASSERT_OR_RETURN(front);
// Validate that the write will not exceed the allocated buffer
if ((size_t)(front - item->mergedExtents.data) > item->mergedExtents.size ||
bytesToRead > item->mergedExtents.size - (size_t)(front - item->mergedExtents.data)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

At this point we have already validated all the sizes and offsets. These two conditions cannot be true if our code is correct. So this check is an assertion. We can consider adding this assertion because the code is quite complicated, but the error code should be AVIF_RESULT_INTERNAL_ERROR, not AVIF_RESULT_BMFF_PARSE_FAILED.

The expression (size_t)(front - item->mergedExtents.data) occurs three times. I suggest saving the result of this expression in a local variable to simplify the code.

@Naveed8951
Copy link
Contributor Author

Updated to return AVIF_RESULT_INTERNAL_ERROR and stored the write offset in a local variable as suggested.

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