Page MenuHome

Fix T89542: Crash when loading certain .hdr files
ClosedPublic

Authored by Jesse Yurkovich (deadpin) on Jul 17 2021, 9:39 AM.

Details

Summary

The direct cause of the bug in question was passing in the raw memory
buffer to sscanf. According to the standard[1], sscanf should be called
with a null-terminated buffer; which isn't guaranteed when blindly
trusting the file data.

When attempting to fuzz this code path, a variety of other crashes were
discovered and fixed.

[1] https://en.cppreference.com/w/c/io/fscanf
(see the Complexity section of that page for a related glibc issue)

Diff Detail

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

Event Timeline

Jesse Yurkovich (deadpin) requested review of this revision.Jul 17 2021, 9:39 AM
Jesse Yurkovich (deadpin) created this revision.

Small tweaks, no functional changes expected from previous commit

  • Make an earlier check against mem_eof. It was already checked later but it wasn't as obvious.
  • Leave a note about one last problem that remains unsolved

Think it would be nice to have an offending image, so that we can start building a regression test collection. Can you attach a file which showcases the memory access issues?

source/blender/imbuf/intern/radiance_hdr.c
259

Why the length of the string argument got reduced?

@Sergey Sharybin (sergey) The one in the linked report should do it - on Windows at least. Pasted here as well:

I reduced the size for those since they should only be one of "+Y", "-Y", "+X", and "-X".

@Sergey Sharybin (sergey) This patch also addresses a new bug T94572. Is there something that's left to do here?

I haven't had time to figure out the exact cause yet, but the following HDR image still manages to cause an out-of-bounds read (in IMB_flipy()) even with the above patch applied:

#?RADIANCE

-Y 96
-Y 9630838

Output of a small program that runs imb_loadhdr() in isolation:

WARNING! HDR decode error, image may be just truncated, or completely wrong...
UndefinedBehaviorSanitizer:DEADLYSIGNAL
==24868==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x7f7b03ab80e8 (pc 0x7f7f9f540885 bp 0x7ffc14097e90 sp 0x7ffc14097da8 T24868)
==24868==The signal is caused by a READ memory access.
    #0 0x7f7f9f540884  /build/glibc-eX1tMB/glibc-2.31/string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:431
    #1 0x32d163e in IMB_flipy /home/albin/fuzz/blender/blender.git/source/blender/imbuf/intern/rotate.c:78:7
    #2 0x3289374 in imb_loadhdr /home/albin/fuzz/blender/blender.git/source/blender/imbuf/intern/radiance_hdr.c:323:5
    #3 0x324fd5f in fuzz_specific(unsigned char*, unsigned long, unsigned long) /home/albin/fuzz/blender/blender.git/fuzz/load_img.cpp:38:23
    #4 0x325076c in main /home/albin/fuzz/blender/blender.git/fuzz/load_img.cpp:126:13
    #5 0x7f7f9f3d90b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #6 0x322e2dd in _start (/home/albin/fuzz/blender/blender.git/build/bin/load_img+0x322e2dd)

UndefinedBehaviorSanitizer can not provide additional info.
==24868==ABORTING

It's an integer overflow in IMB_flipy at rotate.c:71.

The bottomf pointer wraps around and points before topf, because 4 * ((y - 1) * x) wraps around to a negative offset.

I guess this is a separate issue (doesn't only affect HDR images), and belongs in its own task?

@Albin Eldstål-Ahrens (eldstal) Yes, file a separate issue for that and yes I can confirm the issue too and your analysis. It looks like you're doing a fuzzing exercise here. I can't speak for the project as a whole but I'm not sure what kind of support we can provide for such an endeavor. It might be best to run this by some folks over on https://blender.chat/channel/blender-coders first to see what can be done here.

T94629 has been filed. I am indeed fuzzing the blender codebase, as a hobby exercise. I don't think any particular support is needed - my output is bug reports and possibly patches.

From reading previous discussion in T52924, I gather that open security advisories are welcome in the bug tracker rather than being submitted via private channels. Is this still true?

This revision is now accepted and ready to land.Jan 10 2022, 11:40 AM