Backport several OpenEXRCore security fixes#4030
Backport several OpenEXRCore security fixes#4030musicinmybrain wants to merge 6 commits intoPixarAnimationStudios:devfrom
Conversation
…2307) Signed-off-by: Christoph Gohlke <cgohlke@cgohlke.com>
…ixarAnimationStudios#2321) Pointer arithmetic involving `srcbuffer` in the `unpack_*` functions can overflow for very wide images. Multiplying `int w` by a bytes-per-element constant (2, 4, 6, or 8) in `srcbuffer +=` expressions causes signed integer overflow when the image width is large. Promote the leading operand to `int64_t` at every affected multiplication so pointer arithmetic is performed in 64-bit signed arithmetic throughout, eliminating the undefined behavior without imposing any image size limit. Affected sites: `generic_unpack`, `unpack_16bit`, `unpack_32bit`, and all 3- and 4-channel interleave/planar specializations. Made-with: Cursor Signed-off-by: Cary Phillips <cary@ilm.com>
…ecoder) (PixarAnimationStudios#2323) In the `EXR_PIXEL_FLOAT` branch of `undo_pxr24_impl()`, the expressions (uint64_t)(w * 3) compute the signed 32-bit product `w * 3` before the cast to `uint64_t`. When `w` is large this is undefined behavior under the C standard; on two's-complement builds without sanitizers the result wraps to a small positive value, which can cause the bounds check if (nDec + (uint64_t)(w * 3) > outSize) to pass incorrectly. If the check is bypassed the decode loop proceeds to write `4*w` bytes through `dout`, potentially far beyond the allocated output buffer. Fix: cast `w` to `uint64_t` before multiplying so that both the bounds check and the counter update are performed entirely in 64-bit unsigned arithmetic: (uint64_t)w * 3 (cast before multiply, not after) The `EXR_PIXEL_UINT` and `EXR_PIXEL_HALF` decode branches are unaffected: they reuse the pre-computed `nBytes` variable, which is already formed as `(uint64_t)(w) * (uint64_t)(bytes_per_element)`. Also fix the symmetric issue in `apply_pxr24_impl()` (the encoder): lastIn += w * 4 advances a pointer by a signed 32-bit product; corrected to lastIn += (uint64_t)w * 4 Made-with: Cursor Signed-off-by: Cary Phillips <cary@ilm.com>
…Studios#2328) Three classes of signed integer overflow in the PIZ codec path, all reachable from corrupt `dataWindow` dimensions in the EXR file header. **`wav_2D_encode` / `wav_2D_decode` — wavelet loop pointer arithmetic** `oy` is passed as `int` (value `wcount * nx`, at most ~INT32_MAX after the guard below). Inside the hierarchical wavelet loop the expressions ey = in + oy * (ny - p2) // pointer end-of-row sentinel oy1 = oy * p // row stride at level p oy2 = oy * p2 // row stride at level p2 multiply two values that can each approach INT32_MAX, producing a signed 32-bit product that wraps to a small or negative value. The wrapped value is used as a pointer offset, causing reads and writes through `px` / `py` to land outside the allocated wavelet buffer. Fix: widen by introducing `int64_t oy64 = oy` and using it for all three expressions; `oy1` and `oy2` are also declared `int64_t`. **`wavbuf += nx * ny * wcount` — per-channel buffer advance** `nx`, `ny`, and `wcount` are all `int`. Their triple product overflows int32 for moderately large images, causing subsequent channels to be processed at an incorrect (too-small) offset into the wavelet buffer, corrupting both encode and decode output. Fix: cast to `(uint64_t)` before multiplying. **`wcount * nx` — call-site argument overflow** The fifth argument to `wav_2D_encode` / `wav_2D_decode` is `wcount * nx` (`oy` = y-stride = elements per row). `wcount` is 1 or 2 (`bytes_per_element / 2`); for `wcount = 2` the product overflows int32 when `nx > INT32_MAX / 2`. Fix: add an early bounds check `if (wcount > 0 && nx > INT_MAX / wcount)` that rejects such input as `EXR_ERR_CORRUPT_CHUNK` before any arithmetic is performed. This also keeps `wcount * nx` within int32 range at the call site, ensuring `oy` arrives in the wavelet functions with a valid non-overflowed value. Made-with: Cursor Signed-off-by: Cary Phillips <cary@ilm.com>
…ithmetic (PixarAnimationStudios#2329) `numBlocksX` and `numBlocksY` are declared as `int`. Two pointer-offset expressions in `LossyDctDecoder_execute()` multiplied them as signed 32-bit integers before using the result as a pointer offset: rowBlock[comp] = rowBlock[comp - 1] + numBlocksX * 64; currDcComp[comp] = currDcComp[comp - 1] + numBlocksX * numBlocksY; `dataWindow.max.x` is a signed 32-bit value in the EXR file format, so `numBlocksX` can reach `(INT32_MAX + 7) / 8 = 268,435,456`. At that point `numBlocksX * 64 = 17,179,869,184` overflows `int32`, and `numBlocksX * numBlocksY` overflows even sooner. The wraparound produces a small or negative pointer offset, causing `rowBlock[comp]` and `currDcComp[comp]` to point into already-used or pre-buffer memory rather than the intended component stride. Fix: cast `numBlocksX` to `size_t` before multiplying so the arithmetic is performed in pointer-sized unsigned arithmetic: rowBlock[comp] = rowBlock[comp - 1] + (size_t) numBlocksX * 64; currDcComp[comp] = currDcComp[comp - 1] + (size_t) numBlocksX * numBlocksY; This is consistent with the allocation on the line above, which already uses `(size_t) numComp * (size_t) numBlocksX * 64 * sizeof(uint16_t)`, and with the packed-DC count check, which uses explicit `uint64_t` casts. Made-with: Cursor Signed-off-by: Cary Phillips <cary@ilm.com>
…expansion (PixarAnimationStudios#2324) * Fix misaligned memory access in `LossyDctDecoder_execute` HALF→FLOAT expansion After DCT decoding, `LossyDctDecoder_execute()` expands FLOAT-type channels from their intermediate HALF (16-bit) XDR representation back to FLOAT (32-bit) XDR in place. The expansion was done by casting `_rows[y]` (a `uint8_t *`) directly to `float *` and `uint16_t *`, then reading and writing through those typed pointers. Because row buffers are assigned by advancing a byte pointer with no alignment padding (`outBufferEnd += chan->width * chan->bytes_per_element` in `internal_dwa_compressor.h`), a FLOAT channel that follows a HALF channel of odd width receives a `_rows[y]` pointer that is 2-byte aligned but not 4-byte aligned. Dereferencing a `float *` cast from such a pointer is undefined behavior under the C standard: - On ARM, RISC-V, and MIPS (strict alignment) this crashes immediately. - On x86 it is silently tolerated at the hardware level but remains UB: auto-vectorizing compilers (SSE/AVX) may assume aligned access and generate incorrect code. - UBSan reports: `store to misaligned address ... for type 'float', which requires 4 byte alignment` at `internal_dwa_decoder.h:749`. Fix: replace the cast-and-dereference pattern with the `unaligned_load16` / `memcpy` / `unaligned_store32` helpers already used throughout the rest of OpenEXRCore (`internal_xdr.h`, `unpack.c`, `pack.c`, `internal_pxr24.c`). These helpers use `memcpy` internally, which the C standard guarantees is safe for unaligned addresses and which compilers compile to a single load/store instruction on architectures that support it. The byte-order handling is preserved correctly: - `unaligned_load16` reads 2 bytes via `memcpy` and applies `one_to_native16` (XDR → native), returning a native-endian HALF value. - `half_to_float` converts native HALF → native float. - `memcpy(&bits, &f, 4)` reinterprets the float's bit pattern as `uint32_t` without numeric conversion (the correct type-pun idiom in C). - `unaligned_store32` applies `one_from_native32` (native → XDR) and writes 4 bytes via `memcpy`, storing the result in XDR float format. Made-with: Cursor Signed-off-by: Cary Phillips <cary@ilm.com> * add TODO comment Signed-off-by: Cary Phillips <cary@ilm.com> --------- Signed-off-by: Cary Phillips <cary@ilm.com>
|
Filed as internal issue #USD-12069 (This is an automated message. See here for more information.) |
|
Hey @musicinmybrain, just wondering -- you mentioned that latest OpenEXR has a bunch more security fixes. Is there a particular reason you selected these specific fixes to backport? Were they more critical fixes, or just simpler to apply to our version, etc? |
It’s a good question. I cannot say that it was particularly systematic. Most of these were assigned CVE’s, and then were brought to my attention when bugs (like https://bugzilla.redhat.com/show_bug.cgi?id=2455497) were filed against the It is entirely possible that slightly older commits related to memory safety may have contained fixes that were equally or more important, but which were never associated with a CVE. It’s also possible that some of the older CVE’s at https://github.com/AcademySoftwareFoundation/openexr/security may still be unfixed in OpenUSD. I did not attempt to audit the full list, and I won’t have time to do so. |
|
We (the OpenEXR team) have had quite a few CVEs roll into OpenEXR due to recent frameworks making it very easy to poke at things with LLMs. There are a number still incoming, and the team is working through them as fast as we can. As a maintainer of OpenEXR you can rest assured that I too care that the fixes we are making to OpenEXR make it to OpenUSD. My recommendation for OpenUSD integration is to wait until the OpenEXR team has had a change to process the whole list (not all of which is publicly listed yet!) and then, instead of backpatching piecemeal (which will become, in my opinion, overwhelming given the current reporting rate), we should properly rev the interned OpenEXR all at once. The pace of reporting slowed dramatically over the last few days so I believe the LLMs have found what they are going to find, to a large degree. There are performance and other improvements in OpenEXR since the last bump. So I'd rather that we get the latest improvements to OpenEXR all at once after an OpenEXR point release that incorporates all the in process work and work to date. |
|
@meshula, thank you for chiming in! I am looking forward to seeing what comes of your suggestion. |
5 participants
Description of Change(s)
Similar to #4028, this backports several recent security-related commits from OpenEXR upstream.
This is not a substitute for a full rebase on the latest upstream, which contains many other fixes (see also discussion of the actual bundled version in #2935), nor does it include fixes for all potentially-applicable CVE’s on OpenEXR. See https://github.com/AcademySoftwareFoundation/openexr/security for an overview of recent CVE’s.
In particular, I didn’t attempt to backport AcademySoftwareFoundation/openexr@c48f23c for https://www.cve.org/CVERecord?id=CVE-2026-34543 due to changes between OpenEXR 3.2.0 and 3.4.0, #2935 (comment).
As with #4028, all I did here was use
git amto apply commits fromOpenEXR, retaining their commit messages, and in some cases manually adapt the commits due to changes in the surrounding context. Some or all of these commits were LLM-assisted upstream in OpenEXR, but I did not use an LLM to backport them or to write this PR text.Link to proposal (if applicable)
N/A
Fixes Issue(s)
N/A
Fixes:
Checklist
I have created this PR based on the dev branch
I have followed the coding conventions
I have added unit tests that exercise this functionality (Reference:
testing guidelines)
N/A; no new functionality
N/A; it is hard for me to run these locally
Contributor License Agreement instructions)