Skip to content

Fix buffer overflow in MD3Loader#5763

Merged
kimkulling merged 2 commits intoassimp:masterfrom
cla7aye15I4nd:fix-5616
Sep 10, 2024
Merged

Fix buffer overflow in MD3Loader#5763
kimkulling merged 2 commits intoassimp:masterfrom
cla7aye15I4nd:fix-5616

Conversation

@cla7aye15I4nd
Copy link
Contributor

@cla7aye15I4nd cla7aye15I4nd commented Sep 8, 2024

Fix buffer overflow in MD3Loader

Fixed Vulnerability

Heap Buffer Overflow (#5616)

Description

This patch adds bounds checking to ensure that the surfaces and tags do not exceed the allocated buffer size, preventing the heap-buffer-overflow error. The patch has been validated and confirmed to resolve the issue without introducing new bugs.

Sanitizer Report

=================================================================
==458124==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x52800000bc50 at pc 0x555555c89c9f bp 0x7fffffffbf10 sp 0x7fffffffbf08
READ of size 4 at 0x52800000bc50 thread T0
    #0 0x555555c89c9e in Assimp::MD3Importer::InternReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, aiScene*, Assimp::IOSystem*) /root/code/AssetLib/MD3/MD3Loader.cpp:1037:53
    #1 0x555556a8900b in Assimp::BaseImporter::ReadFile(Assimp::Importer*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, Assimp::IOSystem*) /root/code/Common/BaseImporter.cpp:131:9
    #2 0x5555558d468f in Assimp::Importer::ReadFile(char const*, unsigned int) /root/code/Common/Importer.cpp:709:30
    #3 0x5555558d1fd9 in Assimp::Importer::ReadFileFromMemory(void const*, unsigned long, unsigned int, char const*) /root/code/Common/Importer.cpp:507:9
    #4 0x555555861fa9 in LLVMFuzzerTestOneInput /root/fuzz/assimp_fuzzer.cc:54:34
    #5 0x555555847714 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/assimp_fuzzer+0x2f3714) (BuildId: a06dc89f3f19005109152a54e99f5a3fdcf9afaa)
    #6 0x555555830846 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/assimp_fuzzer+0x2dc846) (BuildId: a06dc89f3f19005109152a54e99f5a3fdcf9afaa)
    #7 0x5555558362fa in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/assimp_fuzzer+0x2e22fa) (BuildId: a06dc89f3f19005109152a54e99f5a3fdcf9afaa)
    #8 0x555555860ab6 in main (/root/assimp_fuzzer+0x30cab6) (BuildId: a06dc89f3f19005109152a54e99f5a3fdcf9afaa)
    #9 0x7ffff6f1b1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #10 0x7ffff6f1b28a in __libc_start_main csu/../csu/libc-start.c:360:3
    #11 0x55555582b414 in _start (/root/assimp_fuzzer+0x2d7414) (BuildId: a06dc89f3f19005109152a54e99f5a3fdcf9afaa)

0x52800000bc50 is located 14 bytes after 15170-byte region [0x528000008100,0x52800000bc42)
allocated by thread T0 here:
    #0 0x7ffff73117a3 in malloc build-llvm/tools/clang/stage2-bins/runtimes/runtimes-bins/compiler-rt/lib/asan/asan_malloc_linux.cpp:68:3
    #1 0x7ffff7df05db in operator new(unsigned long) /build/gcc-14-OQFzmN/gcc-14-14-20240412/build/x86_64-linux-gnu/libstdc++-v3/libsupc++/../../../../src/libstdc++-v3/libsupc++/new_op.cc:50:22
    #2 0x55555599e813 in std::allocator_traits<std::allocator<unsigned char>>::allocate(std::allocator<unsigned char>&, unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/alloc_traits.h:482:20
    #3 0x55555599e813 in std::_Vector_base<unsigned char, std::allocator<unsigned char>>::_M_allocate(unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_vector.h:381:20
    #4 0x555555bf4c60 in std::_Vector_base<unsigned char, std::allocator<unsigned char>>::_M_create_storage(unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_vector.h:398:33
    #5 0x555555bf4b71 in std::_Vector_base<unsigned char, std::allocator<unsigned char>>::_Vector_base(unsigned long, std::allocator<unsigned char> const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_vector.h:335:9
    #6 0x555555bf3f48 in std::vector<unsigned char, std::allocator<unsigned char>>::vector(unsigned long, std::allocator<unsigned char> const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_vector.h:557:9
    #7 0x555555c856a4 in Assimp::MD3Importer::InternReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, aiScene*, Assimp::IOSystem*) /root/code/AssetLib/MD3/MD3Loader.cpp:724:32
    #8 0x555556a8900b in Assimp::BaseImporter::ReadFile(Assimp::Importer*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, Assimp::IOSystem*) /root/code/Common/BaseImporter.cpp:131:9
    #9 0x5555558d468f in Assimp::Importer::ReadFile(char const*, unsigned int) /root/code/Common/Importer.cpp:709:30
    #10 0x5555558d1fd9 in Assimp::Importer::ReadFileFromMemory(void const*, unsigned long, unsigned int, char const*) /root/code/Common/Importer.cpp:507:9
    #11 0x555555861fa9 in LLVMFuzzerTestOneInput /root/fuzz/assimp_fuzzer.cc:54:34
    #12 0x555555847714 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/assimp_fuzzer+0x2f3714) (BuildId: a06dc89f3f19005109152a54e99f5a3fdcf9afaa)
    #13 0x555555830846 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/assimp_fuzzer+0x2dc846) (BuildId: a06dc89f3f19005109152a54e99f5a3fdcf9afaa)
    #14 0x5555558362fa in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/assimp_fuzzer+0x2e22fa) (BuildId: a06dc89f3f19005109152a54e99f5a3fdcf9afaa)
    #15 0x555555860ab6 in main (/root/assimp_fuzzer+0x30cab6) (BuildId: a06dc89f3f19005109152a54e99f5a3fdcf9afaa)
    #16 0x7ffff6f1b1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #17 0x7ffff6f1b28a in __libc_start_main csu/../csu/libc-start.c:360:3
    #18 0x55555582b414 in _start (/root/assimp_fuzzer+0x2d7414) (BuildId: a06dc89f3f19005109152a54e99f5a3fdcf9afaa)

SUMMARY: AddressSanitizer: heap-buffer-overflow /root/code/AssetLib/MD3/MD3Loader.cpp:1037:53 in Assimp::MD3Importer::InternReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, aiScene*, Assimp::IOSystem*)
Shadow bytes around the buggy address:
  0x52800000b980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x52800000ba00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x52800000ba80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x52800000bb00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x52800000bb80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x52800000bc00: 00 00 00 00 00 00 00 00 02 fa[fa]fa fa fa fa fa
  0x52800000bc80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x52800000bd00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x52800000bd80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x52800000be00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x52800000be80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==458124==ABORTING

@kimkulling kimkulling added Bug Global flag to mark a deviation from expected behaviour Fuzzer Bugs found by a fuzzer labels Sep 10, 2024
kimkulling
kimkulling approved these changes Sep 10, 2024
View reviewed changes
Copy link
Member

@kimkulling kimkulling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine.

@kimkulling kimkulling merged commit 3bd9861 into assimp:master Sep 10, 2024
@kimkulling
Copy link
Member

kimkulling commented Sep 10, 2024

Merged, thanks a lot for your contribution.

@kimkulling kimkulling mentioned this pull request Sep 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kimkulling kimkulling kimkulling approved these changes

Assignees

No one assigned

Labels

Bug Global flag to mark a deviation from expected behaviour Fuzzer Bugs found by a fuzzer

Projects

The Asset-Importer-Lib DevOps Board
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cla7aye15I4nd @kimkulling