Avoid out-of-bounds palette write in wxIFFDecoder::ConvertToImage (PR #26440)
MarkLee131
Fix #26437:
The transparent colour index from the IFF BMHD chunk is a 16-bit value stored unclamped in m_image->transparent. Using it as a palette index without checking it against the CMAP-derived colour count writes 3 bytes at an attacker-controlled offset past the palette buffer.
Validate the index against the actual palette size before applying the magenta mask.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26440
Commit Summary
- 19486f3 Avoid out-of-bounds palette write in wxIFFDecoder::ConvertToImage
File Changes
(2 files)
- M src/common/imagiff.cpp (2)
- M tests/image/image.cpp (25)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26440.patch
- https://github.com/wxWidgets/wxWidgets/pull/26440.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26440)Thanks for the PR (and the detailed description of the bug in the corresponding issue)!
I'm not sure how does the test checking that loading an invalid image doesn't succeed works currently: don't we just ignore the invalid palette indices? I.e. could it be that loading fails but for unrelated reasons?
I don't know if it's better to ignore invalid values as this PR does or refuse to load the file using them outright. The best decision probably depends on the application, so maybe in the ideal case it should be a wxImage option for this handler, with the default being to reject invalid images but also a way of enabling it if necessary. However I'm saying this just in case you'd like to improve this further, I'm happy with just not writing beyond the end of the buffer and the only real question is about the test above.
Thanks again!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26440)In fact, the test does fail in some wxMSW builds (not sure why only some and not all), so there is definitely something wrong here. Should we apply
diff --git a/src/common/imagiff.cpp b/src/common/imagiff.cpp index e7e8df8a80..fcc84b0c95 100644 --- a/src/common/imagiff.cpp +++ b/src/common/imagiff.cpp @@ -148,6 +148,9 @@ bool wxIFFDecoder::ConvertToImage(wxImage *image) const // set transparent colour mask if (transparent != -1) { + if (transparent < 0 || transparent >= colors) + return false; + for (i = 0; i < colors; i++) { if ((pal[3 * i + 0] == 255) &&
to fix this instead?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.