Fix various OOB reads and writes in kimg_tga and kimg_xcf
ClosedPublic

Authored by fvogt on Jan 28 2019, 8:41 AM.

Details

Summary

I had a look at some image loading code in kimageformats and found memory
corruption bugs (there might be more):

  • oobwrite4b.xcf: OOB write in kimg_xcf:

By overflowing the "size = 3 * ncolors + 4;" calculation, it's possible to make
size == 3 or size == 0, which then allows 1 or 4 bytes to be overwritten:
https://cgit.kde.org/kimageformats.git/tree/src/imageformats/xcf.cpp?id=3f2552f21b1cdef063c2a93cc95d42a8cf907fcf#n484
The values aren't arbitrary, so AFAICT DoS only.
Fix is to move the sanity check for size below the assignment.

  • oobread.tga: OOB read in kimg_tga:

By overflowing the "size = tga.width * tga.height * pixel_size" calculation,
it's possible to cause OOB reads later on as the image data array is too small:
https://cgit.kde.org/kimageformats.git/tree/src/imageformats/tga.cpp?id=3f2552f21b1cdef063c2a93cc95d42a8cf907fcf#n192
Fix is to use a 64bit integer instead.

  • oobwrite4b.tga/oobwrite507.tga: OOB write in kimg_tga

If RLE is enabled, any size checks are skipped, so it's possible to write
either 128 repetitions of an arbitrary four byte value (oobwrite4b.tga)
or or 507 arbitrary bytes (oobwrite507.tga) out of bounds.
https://cgit.kde.org/kimageformats.git/tree/src/imageformats/tga.cpp?id=3f2552f21b1cdef063c2a93cc95d42a8cf907fcf#n209
Fix is to check for "num" being negative before reading into the buffer.

Also, bail out early if there is no more data available (reading a 65kx65k px image from 14B data takes ages otherwise)

Test Plan

Stopped crashing and valgrind don't complain anymore.

TGA preview still works for valid files.

Diff Detail

Repository
R287 KImageFormats
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
fvogt created this revision.Jan 28 2019, 8:41 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 28 2019, 8:41 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
fvogt requested review of this revision.Jan 28 2019, 8:41 AM

Can you expand a bit the description? I understand you are fixing problems, but why the problems are there and what you are doing exactly.

fvogt updated this revision to Diff 50416.Jan 28 2019, 9:24 AM

Also bail out early if image data known broken.
(Unrelated to the overflow fixes, but nice to have)

fvogt edited the summary of this revision. (Show Details)Jan 28 2019, 9:29 AM
fvogt edited the test plan for this revision. (Show Details)
aacid accepted this revision.Jan 28 2019, 11:28 AM

Thanks :)

This revision is now accepted and ready to land.Jan 28 2019, 11:28 AM
This revision was automatically updated to reflect the committed changes.