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.

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

reduce size limit for scanline files; prevent large chunkoffset allocations #824

Merged
merged 6 commits into from
Aug 28, 2020

Conversation

peterhillman
Copy link
Contributor

This change introduces a new function into the API
Specially crafted OpenEXR files can cause large amounts of memory to be allocated by the library when reading code, even if the file is very small. This PR proposes two ways to mitigate this:

  • regular (non-deep) scanline files are now limited to less than 2GB of uncompressed data per chunk of scanlines, Previously the maximum size was 2GB of compressed data per scanline chunk, and 2GB uncompressed data per scanline
  • if the computed chunkoffsettable size in scanline or multipart images is very large, a seek followed by temporary read of the last element in the chunk table is attempted before allocating memory. This may cause slightly slower initialization of OpenEXR files containing huge images. This helps to prevent very small OpenEXR files being used to cause a target machine to allocate large amounts of memory, which may form part of a (very obscure) denial of service attack. With this change, memory is only allocated if the file really does contain enough data to store the declared size of chunk table, which means large memory allocations for the table would require an even larger file to be transferred to the target machine

It is not known if any legitimate EXRs exist that have more than 2GB of uncompressed data per scanline chunk. If so, they will rely on effective compression to prevent the compressed data size exceeding 2GB. Such files will have many channels (in which case using multiple parts would be more appropriate) or very wide scanlines (in which case tiled images would be more appropriate). Uncompressed files, and zip-single compressed files, are not affected by this change.

This PR moves int numLinesInBuffer(Compression comp) into ImfCompressor.h and makes it part of the API. It previously was in an anonymous namespace.

Addresses the following oss-fuzz issues:

Signed-off-by: Peter Hillman peterh@wetafx.co.nz

…et allocations

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

lgtm

@peterhillman
Copy link
Contributor Author

Moving the test earlier in the function should also address https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=25156

// return the number of scanlines in each chunk of a scanlineimage for the given scheme
//
IMF_EXPORT int
numLinesInBuffer(Compression comp);
Copy link
Member

Choose a reason for hiding this comment

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

Since this is declared in ImfCompressor.h, does it need to be declared here, too?

//
// avoid allocating excessive memory.
// If the chunktablesize claims to be large,
// check the file is big enough to contain the file before allocating memory
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean "file is big enough to contain the file"? And is the trick here that the read with throw an exception if the size is off? If so, it would be good to state that's the expectation.

Same comment in ImfScanLineInputFile below, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catches: Dumb typing errors. It should make more sense now

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

LGTM

@cary-ilm cary-ilm merged commit 0963ff1 into AcademySoftwareFoundation:master Aug 28, 2020
@peterhillman peterhillman deleted the memorylimit branch August 30, 2020 22:25
cary-ilm pushed a commit to cary-ilm/openexr that referenced this pull request May 12, 2021
…ations (AcademySoftwareFoundation#824)

* reduce size limit for scanline files; protect against large chunkoffset allocations

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>

* bugfix for memory limit changes

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>

* rearrange chunkoffset test to protect bytesperline table too

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>

* remove extraneous function declaration; tidy comments

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm pushed a commit to cary-ilm/openexr that referenced this pull request May 12, 2021
…ations (AcademySoftwareFoundation#824)

* reduce size limit for scanline files; protect against large chunkoffset allocations

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>

* bugfix for memory limit changes

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>

* rearrange chunkoffset test to protect bytesperline table too

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>

* remove extraneous function declaration; tidy comments

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm added a commit that referenced this pull request May 16, 2021
* double-check unpackedBuffer created in DWA uncompress

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Signed-off-by: Cary Phillips <cary@ilm.com>

* compute Huf codelengths using 64 bit to prevent shift overflow

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Signed-off-by: Cary Phillips <cary@ilm.com>

* Avoid overflow in calculateNumTiles when size=MAX_INT (#825)

* Avoid overflow in calculateNumTiles when size=MAX_INT

Signed-off-by: Cary Phillips <cary@ilm.com>

* Compute level size with 64 bits to avoid overflow

Signed-off-by: Cary Phillips <cary@ilm.com>

* More efficient handling of filled channels reading tiles with scanline API (#830)

* refactor channel filling in InputFile API with tiled source

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>

* handle edge-case of empty framebuffer

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Signed-off-by: Cary Phillips <cary@ilm.com>

* fix undefined behavior: ignore unused bits in B44 mode detection (#832)


Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Signed-off-by: Cary Phillips <cary@ilm.com>

* Fix overflow computing deeptile sample table size (#861)


Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Signed-off-by: Cary Phillips <cary@ilm.com>

* sanity check ScanlineInput bytesPerLine instead of lineOffset size (#863)

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>

Co-authored-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Cary Phillips <cary@ilm.com>

* Release notes for v2.4.3

Signed-off-by: Cary Phillips <cary@ilm.com>

* Bump version for v2.4.3

Signed-off-by: Cary Phillips <cary@ilm.com>

* reduce size limit for scanline files; prevent large chunkoffset allocations (#824)

* reduce size limit for scanline files; protect against large chunkoffset allocations

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>

* bugfix for memory limit changes

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>

* rearrange chunkoffset test to protect bytesperline table too

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>

* remove extraneous function declaration; tidy comments

Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
Signed-off-by: Cary Phillips <cary@ilm.com>

* Change v2.4.3 release date to May 17, and clean up urls

Signed-off-by: Cary Phillips <cary@ilm.com>

Co-authored-by: Peter Hillman <peterh@wetafx.co.nz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants