Make image size limit configurable, expose to avifdec #527

joedrago · 2021-02-26T22:00:13Z

This should resolve #263 .

Should we make the default 0 (no limit) and manually set the limit in our fuzzer (and possibly Chrome), or should we keep the constant AVIF_MAX_IMAGE_SIZE (perhaps renamed to have DEFAULT somewhere in it?) and force people to opt into no limit?
Does libaom have a similar size limit we can use? It'd be nice to not have to caveat "dav1d only" in various places.

wantehchang

Joe: I suggest we be secure by default and force people to opt into no limit.

wantehchang · 2021-02-27T23:45:23Z

include/avif/avif.h

@@ -76,6 +76,10 @@ typedef int avifBool;
 #define AVIF_SPEED_SLOWEST 0
 #define AVIF_SPEED_FASTEST 10

+// A maximum image size to avoid out-of-memory errors or integer overflow in


Nit: I think we should call this "The default maximum image size ..." now

wantehchang · 2021-02-27T23:45:38Z

include/avif/avif.h

@@ -701,6 +705,11 @@ typedef struct avifDecoder
    avifBool ignoreExif;
    avifBool ignoreXMP;

+    // This represents the maximum size of a image (in pixel count) that the underlying AV1 decoder


Nit: a image => an image

wantehchang · 2021-02-27T23:50:52Z

include/avif/avif.h

+    // This represents the maximum size of a image (in pixel count) that the underlying AV1 decoder
+    // should attempt to decode. It defaults to AVIF_MAX_IMAGE_SIZE, and can be set to 0 to disable
+    // the limit. Currently supported codecs: dav1d.
+    uint32_t imageSizeLimit;


In src/avif.c, avifImageSetDefaults() needs to initialize imageSizeLimit to the default (AVIF_MAX_IMAGE_SIZE).

The use of AVIF_MAX_IMAGE_SIZE in src/read.c needs to be updated. It may need to replaced with imageSizeLimit, depending on whether you also want imageSizeLimit to apply to grid image size. (The current comment suggests imageSizeLimit does not apply to grid image size.)

We also need to make sure all of our multiplications involving width or height do not overflow the integer type used in the arithmetic. I remember I reviewed that before, but it is best to check it again.

imageSizeLimit is not a member of avifImage, so bullets 1 and 2 don't apply here, right?

Sorry about my confusion. I checked the wrong function. Bullet 1 should read:

In src/read.c, avifDecoderCreate() needs to initialize imageSizeLimit to the default (AVIF_MAX_IMAGE_SIZE).

Bullet 2 is correct. I was referring to the following code in avifParseImageGridBox():

if ((grid->outputWidth == 0) || (grid->outputHeight == 0) || (grid->outputWidth > (AVIF_MAX_IMAGE_SIZE / grid->outputHeight))) { return AVIF_FALSE; }

Ah, I see. That is a good question.

Do we want imageSizeLimit to be something solely enforced by the underlying AV1 decoders, or do we want to explicitly check these limits even at the container level? For example, if the ispe box returned something that exceeds imageSizeLimit, do we bail out immediately? Do we want a new error of AVIF_RESULT_IMAGE_TOO_LARGE?

imageSizeLimit serves two purposes:

To help prevent integer arithmetic overflow in libavif clients' code. For this purpose, we want to explicitly check the image size limits even at the container level. (I believe we are not doing that now.) I think we should add a new error of AVIF_RESULT_IMAGE_TOO_LARGE.

oss-fuzz tests fail if we allocate too much memory from the heap. (oss-fuzz doesn't care whether we handle out-of-memory errors properly.) For this purpose, we don't need to check the image size limits at the container level, but we do need to check the grid image sizes (because we will call avifImageAllocatePlanes() in avifDecoderDataFillImageGrid()), and we also need the underlying AV1 decoders to enforce the same size limits.

wantehchang · 2021-02-27T23:52:04Z

src/codec_dav1d.c

@@ -55,6 +55,7 @@ static avifBool dav1dCodecOpen(avifCodec * codec, avifDecoder * decoder)
        // Give all available threads to decode a single frame as fast as possible
        codec->internal->dav1dSettings.n_frame_threads = 1;
        codec->internal->dav1dSettings.n_tile_threads = AVIF_CLAMP(decoder->maxThreads, 1, DAV1D_MAX_TILE_THREADS);
+        codec->internal->dav1dSettings.frame_size_limit = decoder->imageSizeLimit;


It would be good to add a comment to note that dav1d uses the same convention that 0 = unlimited, so this simple assignment is safe.

Note: I found that dav1d/tools/dav1d_cli_parse.c disallows its --sizelimit argument to be exactly UINT_MAX:

case ARG_SIZE_LIMIT: { char *arg = optarg, *end; uint64_t res = strtoul(arg, &end, 0); if (*end == 'x') // NxM res *= strtoul((arg = end + 1), &end, 0); if (*end || end == arg || res >= UINT_MAX) error(argv[0], optarg, ARG_SIZE_LIMIT, "an integer or dimension"); lib_settings->frame_size_limit = (unsigned) res; break; }

I don't know whether that is intentional or an off-by-one bug.

wantehchang · 2021-02-28T00:04:41Z

include/avif/avif.h

@@ -701,6 +705,11 @@ typedef struct avifDecoder
    avifBool ignoreExif;
    avifBool ignoreXMP;

+    // This represents the maximum size of a image (in pixel count) that the underlying AV1 decoder
+    // should attempt to decode. It defaults to AVIF_MAX_IMAGE_SIZE, and can be set to 0 to disable
+    // the limit. Currently supported codecs: dav1d.


It would be good to note that other AV1 codecs may have a non-configurable frame size limit. That's the case for libgav1 and libaom.

For libgav1, the frame size limit is hardcoded in the source code, currently INT32_MAX:

if (frame_header_.upscaled_width > INT32_MAX / frame_header_.height) { LIBGAV1_DLOG(ERROR, "Frame dimensions too big: width=%d height=%d.", frame_header_.width, frame_header_.height); return false; }

For libaom, the frame size limit is a build opton, consisting of three cmake flags. Here is how libaom's source code uses the three corresponding C-preprocessor macros:

static AOM_INLINE void resize_context_buffers(AV1_COMMON *cm, int width, int height) { #if CONFIG_SIZE_LIMIT if (width > DECODE_WIDTH_LIMIT || height > DECODE_HEIGHT_LIMIT) aom_internal_error(&cm->error, AOM_CODEC_CORRUPT_FRAME, "Dimensions of %dx%d beyond allowed size of %dx%d.", width, height, DECODE_WIDTH_LIMIT, DECODE_HEIGHT_LIMIT); #endif

Note that the clients of libaom cannot change the size limit at run time.

Clarify whether imageSizeLimit applies to grid image size.

wantehchang · 2021-02-28T00:12:39Z

include/avif/avif.h

@@ -76,6 +76,10 @@ typedef int avifBool;
 #define AVIF_SPEED_SLOWEST 0
 #define AVIF_SPEED_FASTEST 10

+// A maximum image size to avoid out-of-memory errors or integer overflow in
+// (32-bit) int or unsigned int arithmetic operations.
+#define AVIF_MAX_IMAGE_SIZE (16384 * 16384)


Your suggestion of adding DEFAULT to the macro's name is good.

…ts in avifDecoder, promote some indexing math vars to uint32_t, update some comments

joedrago · 2021-03-08T20:02:46Z

Alright, I've renamed AVIF_MAX_IMAGE_SIZE to AVIF_DEFAULT_MAX_IMAGE_SIZE, set it in avifDecoder, added AVIF_RESULT_IMAGE_TOO_LARGE and honor it in both grid and ispe cases, did an audit pass on reformat.c and alpha.c to ensure we were using uint32_t for our pixel indexing math local variables, and tweaked some comments. When you get a chance, please re-audit the PR fresh.

joedrago · 2021-03-08T20:04:33Z

Hm, I realized right as I typed that that uint32_t is insufficient for indexing if ispe really allows for uint32_t width and heights. Yuck.

wantehchang · 2021-03-08T23:41:31Z

src/read.c

+    if ((grid->outputWidth == 0) || (grid->outputHeight == 0) || (avifROStreamRemainingBytes(&s) != 0)) {
+        return AVIF_RESULT_INVALID_IMAGE_GRID;
+    }
+    if (imageSizeLimit && (grid->outputWidth > (imageSizeLimit / grid->outputHeight))) {


If imageSizeLimit is equal to 0, we still need to make sure grid->outputWidth * grid->outputHeight does not overflow size_t. We can either check that here or when we allocate the plane buffers.

wantehchang · 2021-03-08T23:49:20Z

src/reformat.c

+                const uint32_t chromaShiftX = 1;
+                const uint32_t chromaShiftY = 1;
+                uint32_t uvI = outerI >> chromaShiftX;
+                uint32_t uvJ = outerJ >> chromaShiftY;


Here we need to make sure the multiplication uvJ * yuvRowBytes[AVIF_CHAN_U] at line 315 below is done using the size_t type and also make sure that multiplication does not overflow size_t.

It is best to check for potential overflow on entry to this function.

By now it should be clear why we (and Chrome) set AVIF_MAX_IMAGE_SIZE to 2^28, so that we can perform these arithmetic operations using the 32-bit 'int' or 'uint32_t' type freely.

Adapted from Joe Drago's pull request AOMediaCodec#527, with the limitation that decoder->imageSizeLimit must be less than or equal to the default value and must not be set to 0 (reserved for future use). This way we don't need to audit our code for integer overflows due to a large image width or height. Set decoder->imageSizeLimit to 11 * 1024 * 10 * 1024 in avif_decode_fuzzer.cc to keep its memory consumption under 2560 MB.

Adapted from Joe Drago's pull request AOMediaCodec#527, with the limitation that decoder->imageSizeLimit must be less than or equal to the default value and must not be set to 0 (reserved for future use). This way we don't need to audit our code for integer overflows due to a large image width or height. Set decoder->imageSizeLimit to 11 * 1024 * 10 * 1024 in avif_decode_fuzzer.cc to keep its memory consumption under 2560 MB. AOMediaCodec#263

Adapted from Joe Drago's pull request #527, with the limitation that decoder->imageSizeLimit must be less than or equal to the default value and must not be set to 0 (reserved for future use). This way we don't need to audit our code for integer overflows due to a large image width or height. Set decoder->imageSizeLimit to 11 * 1024 * 10 * 1024 in avif_decode_fuzzer.cc to keep its memory consumption under 2560 MB. #263

Make image size limit configurable, expose to avifdec

8a6506d

joedrago requested a review from wantehchang February 26, 2021 22:00

wantehchang requested changes Feb 28, 2021

View reviewed changes

Rename AVIF_MAX_IMAGE_SIZE to AVIF_DEFAULT_MAX_IMAGE_SIZE, set defaul…

21167c9

…ts in avifDecoder, promote some indexing math vars to uint32_t, update some comments

wantehchang requested changes Mar 8, 2021

View reviewed changes

wantehchang mentioned this pull request Aug 4, 2021

Make image size limit configurable #729

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make image size limit configurable, expose to avifdec #527

Make image size limit configurable, expose to avifdec #527

joedrago commented Feb 26, 2021

wantehchang left a comment

wantehchang Feb 27, 2021

wantehchang Feb 27, 2021

wantehchang Feb 27, 2021

joedrago Mar 5, 2021

wantehchang Mar 5, 2021

joedrago Mar 5, 2021

wantehchang Mar 5, 2021 •

edited

Loading

wantehchang Feb 27, 2021

wantehchang Feb 28, 2021

wantehchang Feb 28, 2021

joedrago commented Mar 8, 2021

joedrago commented Mar 8, 2021

wantehchang Mar 8, 2021

wantehchang Mar 8, 2021

Make image size limit configurable, expose to avifdec #527

Are you sure you want to change the base?

Make image size limit configurable, expose to avifdec #527

Conversation

joedrago commented Feb 26, 2021

wantehchang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wantehchang Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joedrago commented Mar 8, 2021

joedrago commented Mar 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wantehchang Mar 5, 2021 •

edited

Loading