Constrain XMP,Exif size in avifJPEGReadInternal() #2045
Conversation
| if (found) { | ||
| // TODO(yguyon): Implement instead of outputting an error. | ||
| fprintf(stderr, "Exif extraction failed: unsupported Exif split into multiple segments or invalid multiple Exif segments\n"); | ||
| goto cleanup; | ||
| } | ||
|
|
||
| if (marker->data_length - AVIF_JPEG_EXIF_HEADER_LENGTH > imageSizeLimit) { |
There was a problem hiding this comment.
I know there is a condition above but in case the code gets moved one day, maybe it is safer (overflow/type change-wise) to write: marker->data_length > AVIF_JPEG_EXIF_HEADER_LENGTH + imageSizeLimit
There was a problem hiding this comment.
imageSizeLimit could be MAX_UINT
There was a problem hiding this comment.
We are using imageSizeLimit for a different purpose: the maximum Exif or XMP metadata size.
Would it be better to impose our own maximum Exif and XMP metadata sizes? For example, we can use a small multiple of 64 KB. See
https://stackoverflow.com/questions/3248946/what-is-the-maximum-size-of-jpeg-metadata
There was a problem hiding this comment.
Renamed imageSizeLimit in #2047
There was a problem hiding this comment.
Would it be better to impose our own maximum Exif and XMP metadata sizes? For example, we can use a small multiple of 64 KB. See
I do not think it is worth it until there is a use case or user complaint for that.
| @@ -1062,8 +1073,13 @@ static avifBool avifJPEGReadInternal(FILE * f, | |||
| // "offset of this portion as a 32-bit unsigned integer" | |||
| const uint32_t extendedXMPOffset = avifJPEGReadUint32BigEndian( | |||
| &marker->data[AVIF_JPEG_EXTENDED_XMP_TAG_LENGTH + AVIF_JPEG_EXTENDED_XMP_GUID_LENGTH + 4]); | |||
| if (((uint64_t)standardXMPSize + totalExtendedXMPSize) > SIZE_MAX) { | |||
| fprintf(stderr, "XMP extraction failed: total XMP size is too large\n"); | |||
| if (((uint64_t)standardXMPSize + totalExtendedXMPSize) > imageSizeLimit) { | |||
There was a problem hiding this comment.
In the original code, this check compares the sum with SIZE_MAX. I believe it was intended to check if it would be safe to cast the sum to size_t in line 1101 below.
In the new code, this check is stricter, so the size_t cast is still safe.
There was a problem hiding this comment.
Optional: It may be good to add an assertion here:
assert(imageSizeLimit <= SIZE_MAX);
There was a problem hiding this comment.
Optional: It may be good to add an assertion here:
assert(imageSizeLimit <= SIZE_MAX);
My IDE CLion shows the following warning:
Result of comparison of constant 18446744073709551615 with expression of type 'uint32_t' (aka 'unsigned int') is always true
There was a problem hiding this comment.
Kept the old condition in #2047 instead
| if (found) { | ||
| // TODO(yguyon): Implement instead of outputting an error. | ||
| fprintf(stderr, "Exif extraction failed: unsupported Exif split into multiple segments or invalid multiple Exif segments\n"); | ||
| goto cleanup; | ||
| } | ||
|
|
||
| if (marker->data_length - AVIF_JPEG_EXIF_HEADER_LENGTH > imageSizeLimit) { |
There was a problem hiding this comment.
We are using imageSizeLimit for a different purpose: the maximum Exif or XMP metadata size.
Would it be better to impose our own maximum Exif and XMP metadata sizes? For example, we can use a small multiple of 64 KB. See
https://stackoverflow.com/questions/3248946/what-is-the-maximum-size-of-jpeg-metadata
BUG=oss-fuzz:67120