fix(dds): corruption protection: validate resolution + overflow care#5131
fix(dds): corruption protection: validate resolution + overflow care#5131lgritz merged 3 commits intoAcademySoftwareFoundation:mainfrom
Conversation
|
The person who reported the overflow issue to me confirms that this patch addresses the problem. Would love somebody to give the code a once-over and approve. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the DDS ImageInput plugin against corrupted/malicious headers by adding resolution validation and reducing integer-overflow risk in internal size/offset calculations.
Changes:
- Switched several internal dimension/offset variables and function signatures from
unsigned int/inttosize_t. - Added
check_open(...)resolution/channel validation duringseek_subimage. - Updated call sites and loops to use the new
size_t-based interfaces (including casting back tointforDecompressImage).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check validity of resolutions. | ||
| if (!check_open(m_spec, { 0, 32768, 0, 32768, 0, 16384, 0, 4 })) |
There was a problem hiding this comment.
The new check_open(m_spec, { 0, 32768, 0, 32768, ... }) validation uses m_spec.width/height, but for DDS cubemaps this plugin intentionally sets m_spec to an artificial 1x6 (default) or 3x2 layout (e.g. height = face_h * 6). This makes the effective per-face limit much smaller (in 1x6 layout, face_h must be <= 32768/6), rejecting valid cubemap files.
Consider validating the per-face dimensions instead (e.g. full_width/full_height or tile_width/tile_height), or adjust the allowed range based on the chosen cubemap layout so that a face size limit doesn't unintentionally become a layout size limit.
| // Check validity of resolutions. | |
| if (!check_open(m_spec, { 0, 32768, 0, 32768, 0, 16384, 0, 4 })) | |
| // Check validity of resolutions. Cubemaps are represented using an | |
| // artificial packed layout (default 1x6, optionally 3x2), so validate | |
| // the per-face dimensions rather than the packed layout dimensions. | |
| ImageSpec validate_spec = m_spec; | |
| if (m_dds.caps.flags2 & DDS_CAPS2_CUBEMAP) { | |
| #ifdef DDS_3X2_CUBE_MAP_LAYOUT | |
| validate_spec.width /= 3; | |
| validate_spec.height /= 2; | |
| #else | |
| validate_spec.height /= 6; | |
| #endif | |
| } | |
| if (!check_open(validate_spec, { 0, 32768, 0, 32768, 0, 16384, 0, 4 })) |
There was a problem hiding this comment.
Good point, Copilot!
|
Glad I requested that copilot review. I've amended the PR to apply slightly different (and I believe appropriate) resolution limit tests for 2D, cube map, and volume DDS files. |
|
Any comments? |
jessey-git
left a comment
There was a problem hiding this comment.
There's one more expression overflow. In read_native_scanline the calculation used in the memcpy can overflow, it's enough to just cast the first variable there (z) to size_t to pull the rest of the expression along for the ride.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Fixed and pushed, thanks. |
No description provided.