Page MenuHome

Fix T71960: Malformed .bmp files lead to crash
Needs ReviewPublic

Authored by Jesse Y (deadpin) on Jun 6 2020, 3:21 AM.

Details

Summary

Fix T71960
Adds appropriate checks/guards around all the untrusted parameters which are used for reading from memory.
Stride calculation from MSDN[1]

Validation:

  • All the crashing files within the bug have been checked to not causes crashes any longer
  • A handful of correct .bmp were validated: 3 different files at each of 1, 4, 8, 24, 32 bpp depth along with a random variety of other 24 bpp files (around 20 in total).
  • ~280 million iterations of fuzzing using AFL were completed with 0 crashes. The old code experienced several dozen crashes in first minutes of running[2]

Before committing:

  • It would be good to have another set of folks try some .bmps they have lying around
  • Unfortunately, I could not find software that would write out _uncompressed_ 16 bpp images (565 format)

[1] https://docs.microsoft.com/en-us/windows/win32/api/wingdi/ns-wingdi-bitmapinfoheader
[2]

Diff Detail

Repository
rB Blender
Branch
fixT71960 (branched from master)
Build Status
Buildable 8447
Build 8447: arc lint + arc unit

Event Timeline

Jesse Y (deadpin) requested review of this revision.Jun 6 2020, 3:21 AM
Jesse Y (deadpin) created this revision.
Jesse Y (deadpin) edited the summary of this revision. (Show Details)Jun 6 2020, 3:22 AM
Ray molenkamp (LazyDodo) requested changes to this revision.Tue, Jun 9, 4:02 AM

Guess that's a valuable lesson on using fuzzers :)

Could be because it's nmapped and that tends to fill the rest of the page with 0's (at least on windows) but 17+ hours of fuzzing and it missed these :

Still accesses memory beyond what size indicate it should look at

Still accesses memory beyond what size indicate it should look at

Still crashes

Just by looking at the code you can pick these out, it's like shooting fish in a barrel!

source/blender/imbuf/intern/bmp.c
155

if size < preamble_bytes data_bytes will be huge and escape the test below :) see

This revision now requires changes to proceed.Tue, Jun 9, 4:02 AM

Do not depend on higher layers having ensured file is of a sufficient size already

Jesse Y (deadpin) marked an inline comment as done.Tue, Jun 9, 5:21 AM

Alright, tested with your new files in both Image Viewer and Shading tabs with thumbnails enabled and when set in an Image texture and looks good (again)

source/blender/imbuf/intern/bmp.c
155

Sigh, this was straight up user error on my part. Disappointed that fuzzing didn't crash either even though it did yield files that were small in size.