Validate property allocation size in avifImageCopyProperties()#3226
Validate property allocation size in avifImageCopyProperties()#3226jmestwa-coder wants to merge 1 commit into
Conversation
| AVIF_RESULT_OK); | ||
|
|
||
| src->numProperties = | ||
| std::numeric_limits<size_t>::max() / sizeof(avifImageItemProperty) + 1; |
There was a problem hiding this comment.
This changes src to an invalid avifImage. (This is why the test has to change src->numProperties back to 1 at line 69 before the destructor of src is called.) This violates the API contract that src points to a valid avifImage.
There was a problem hiding this comment.
To drive home the necessity of this kind of API contract (whether explicit or implicit), suppose we change this line to the following instead:
src->numProperties = 2;
It will also make src an invalid avifImage, but the avifImageCopy() function cannot possibly detect this kind of invalid input. So at some point a function has to require that the caller pass a valid input.
|
|
||
| if (srcImage->numProperties != 0) { | ||
| dstImage->properties = (avifImageItemProperty *)avifAlloc(srcImage->numProperties * sizeof(srcImage->properties[0])); | ||
| AVIF_CHECKERR(srcImage->numProperties <= SIZE_MAX / sizeof(srcImage->properties[0]), AVIF_RESULT_INVALID_ARGUMENT); |
There was a problem hiding this comment.
This check is not necessary. The API contract requires that srcImage points to a valid avifImage.
Summary
Add overflow validation before computing the property allocation size in
avifImageCopyProperties().The copy path previously multiplied
numPropertiesby the property size without validating forsize_toverflow. Extremely large values could wrap the allocation size and produce an undersized allocation.This change:
AVIF_RESULT_INVALID_ARGUMENTfor oversized values.Test
Add a regression test covering oversized
numPropertiesvalues.The test:
numProperties,avifImageCopy()rejects the malformed state withAVIF_RESULT_INVALID_ARGUMENT.