search

syoyo / tinyexr

Tiny OpenEXR image loader/saver library
710 stars 139 forks source link

memory error in tinyexr::InitSingleResolutionOffsets #191

Closed 0xdd96 closed 1 year ago

0xdd96 commented 1 year ago

Describe the issue

requested allocation size 0x3fffffffe0000008 (0x3fffffffe0001008 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x10000000000

To Reproduce

Environment

poc: poc

Steps to reproduce the behavior:

Expected behavior

user@c3ae4d510abb:$ ./test_tinyexr $POC
terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc
Aborted (core dumped)

Here is the trace reported by gdb:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff7be4859 in __GI_abort () at abort.c:79
#2  0x00007ffff7e6d911 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#3  0x00007ffff7e7938c in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#4  0x00007ffff7e793f7 in std::terminate() () from /lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007ffff7e796a9 in __cxa_throw () from /lib/x86_64-linux-gnu/libstdc++.so.6
#6  0x00007ffff7e6d522 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#7  0x0000555555572831 in __gnu_cxx::new_allocator<unsigned long>::allocate (this=0x5555555ab650, __n=<optimized out>) at /usr/include/c++/9/ext/new_allocator.h:102
#8  std::allocator_traits<std::allocator<unsigned long> >::allocate (__a=..., __n=<optimized out>) at /usr/include/c++/9/bits/alloc_traits.h:443
#9  std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_M_allocate (this=<optimized out>, __n=<optimized out>) at /usr/include/c++/9/bits/stl_vector.h:343
#10 std::vector<unsigned long, std::allocator<unsigned long> >::_M_default_append (this=0x5555555ab650, __n=576460752236314625) at /usr/include/c++/9/bits/vector.tcc:635
#11 0x0000555555563f8b in std::vector<unsigned long, std::allocator<unsigned long> >::resize (__new_size=576460752236314625, this=<optimized out>) at /usr/include/c++/9/bits/stl_construct.h:107
#12 tinyexr::InitSingleResolutionOffsets (offset_data=..., num_blocks=576460752236314625) at tinyexr/tinyexr.h:5517
#13 0x00005555555642b0 in tinyexr::DecodeEXRImage (exr_image=0x7fffffffddf0, exr_header=0x7fffffffdec0, head=0x7ffff7fc7000 "v/1\001\002", marker=0x7ffff7fc7158 "y\001", size=9113, err=0x7fffffffe228) at tinyexr/tinyexr.h:5829
#14 0x00005555555650cf in LoadEXRImageFromMemory (exr_image=<optimized out>, exr_header=<optimized out>, memory=<optimized out>, size=<optimized out>, err=<optimized out>) at tinyexr/tinyexr.h:6776
#15 0x000055555556610f in LoadEXRImageFromFile (exr_image=0x7fffffffddf0, exr_header=0x7fffffffdec0, filename=0x7fffffffe5ff "../../fuzz-test-v0/crashes/id:000000,sig:06,src:000434,op:havoc,rep:64", err=0x7fffffffe228) at tinyexr/tinyexr.h:6753
#16 0x000055555556fb4c in LoadEXRWithLayer (out_rgba=0x7fffffffe230, width=0x7fffffffe220, height=0x7fffffffe224, filename=0x7fffffffe5ff "../../fuzz-test-v0/crashes/id:000000,sig:06,src:000434,op:havoc,rep:64", layername=0x0, err=0x7fffffffe228) at tinyexr/tinyexr.h:6059
#17 0x0000555555558989 in LoadEXR (err=0x7fffffffe228, filename=0x7fffffffe5ff "../../fuzz-test-v0/crashes/id:000000,sig:06,src:000434,op:havoc,rep:64", height=0x7fffffffe224, width=0x7fffffffe220, out_rgba=0x7fffffffe230) at tinyexr/tinyexr.h:6006
#18 test_main (argv=0x0, argc=0) at tinyexr/test_tinyexr.cc:223
#19 main (argc=argc@entry=2, argv=argv@entry=0x7fffffffe358) at tinyexr/test_tinyexr.cc:194
#20 0x00007ffff7be6083 in __libc_start_main (main=0x555555558920 <main(int, char**)>, argc=2, argv=0x7fffffffe358, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe348) at ../csu/libc-start.c:308
#21 0x0000555555558b6e in _start () at tinyexr/test_tinyexr.cc:469

Vulnerability analysis The value of the exr_header->data_window structure is as below, which allows the program to bypass the checks on lines 5760 and 5761 of tinyexr.h, reach line 5766 and cause data_height to be -2147483631. Although line 5775 tries to check the value of data_height to avoid it being too large, this check is a signed comparison, and the current poc can still bypass this check. https://github.com/syoyo/tinyexr/blob/41cc1405bbc7ab05e99bd0d581f72aa6d2c190c7/tinyexr.h#L5760-L5779

pwndbg> p exr_header->data_window
$2 = {
  min_x = 0,
  min_y = -2147483648,
  max_x = 0,
  max_y = 16
}

pwndbg> p data_height
$9 = -2147483631

These two incomplete checks made the num_blocks calculated in line 5822 very large (576460752236314624), which eventually led to a memory error. https://github.com/syoyo/tinyexr/blob/41cc1405bbc7ab05e99bd0d581f72aa6d2c190c7/tinyexr.h#L5822-L5829

pwndbg> p num_blocks
$15 = 576460752236314624
syoyo commented 1 year ago

Thanks! The reason was It triggers integer overflow. I have fixed it by using int64 to calculate (max - min + 1) and add negative value check.

0xdd96 commented 1 year ago

Thank you for your timely response! But the fix for this bug is incomplete, and the original poc can still trigger the vulnerability.

I did a simple analysis, it appears that the compiler optimizes the check for negative numbers (line 5787).

Since the compiler assumes that the program has no undefined behavior (e.g., integer overflow), when exr_header->data_window.max_y>=exr_header->data_window.min_y (bypassing line 5778), the compiler considers exr_header->data_window.max_y - exr_header->data_window.min_y is impossible to be negative, so the negative check is optimized away.

syoyo commented 1 year ago

Oh, confirmed the issue still happens in gcc 9.4(I've been using clang 14 for ASAN testing). It looks gcc does not upcast int32 expression to int64 implicitly.

I did further fix to apply explicit int64 cast to the expression (max - min + 1) here: https://github.com/syoyo/tinyexr/commit/2a4dd61afc62d35348df75fc1cbd7142cf331041

Now test_tinyexr should report

Load EXR err: data height too large.(code -4)

both for gcc(9.4) and clang(14.0)

If you still see the issue, please describe the compiler and its version you are using.