Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix buffer overflow in MD5Parser::SkipSpacesAndLineEnd #5921

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

tyler92
Copy link
Contributor

@tyler92 tyler92 commented Dec 11, 2024

Fixes #5860

Added additional check before dereferencing buffer pointer: buffer < bufferEnd. ASAN report for reference:

=================================================================
==1114==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x5020000001c0 at pc 0x55a54b9156b6 bp 0x7ffd02555350 sp 0x7ffd02555348
READ of size 1 at 0x5020000001c0 thread T0
    #0 0x55a54b9156b5 in SkipSpacesAndLineEnd /src/assimp/code/AssetLib/MD5/MD5Parser.h:428:13
    #1 0x55a54b9156b5 in SkipSpacesAndLineEnd /src/assimp/code/AssetLib/MD5/MD5Parser.h:450:12
    #2 0x55a54b9156b5 in Assimp::MD5::MD5Parser::ParseHeader() /src/assimp/code/AssetLib/MD5/MD5Parser.cpp:128:5
    #3 0x55a54b9146e1 in Assimp::MD5::MD5Parser::MD5Parser(char*, unsigned int) /src/assimp/code/AssetLib/MD5/MD5Parser.cpp:69:5
    #4 0x55a54b9087f8 in Assimp::MD5Importer::LoadMD5MeshFile() /src/assimp/code/AssetLib/MD5/MD5Loader.cpp:338:20
    #5 0x55a54b902da0 in Assimp::MD5Importer::InternReadFile(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, aiScene*, Assimp::IOSystem*) /src/assimp/code/AssetLib/MD5/MD5Loader.cpp:141:13
    #6 0x55a54c66e968 in Assimp::BaseImporter::ReadFile(Assimp::Importer*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, Assimp::IOSystem*) /src/assimp/code/Common/BaseImporter.cpp:131:9
    #7 0x55a54b5cfead in Assimp::Importer::ReadFile(char const*, unsigned int) /src/assimp/code/Common/Importer.cpp:709:30
    #8 0x55a54b5cd490 in Assimp::Importer::ReadFileFromMemory(void const*, unsigned long, unsigned int, char const*) /src/assimp/code/Common/Importer.cpp:507:9
    #9 0x55a54b586d47 in LLVMFuzzerTestOneInput /src/assimp/fuzz/assimp_fuzzer.cc:54:34
    #10 0x55a54b43b7a0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
    #11 0x55a54b426a15 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
    #12 0x55a54b42c4af in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
    #13 0x55a54b457752 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #14 0x7f95de669082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 0702430aef5fa3dda43986563e9ffcc47efbd75e)
    #15 0x55a54b41ebfd in _start (/out/assimp_fuzzer+0x8b4bfd)

0x5020000001c0 is located 0 bytes after 16-byte region [0x5020000001b0,0x5020000001c0)
allocated by thread T0 here:
    #0 0x55a54b584a4d in operator new[](unsigned long) /src/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:89:3
    #1 0x55a54b90c880 in Assimp::MD5Importer::LoadFileIntoMemory(Assimp::IOStream*) /src/assimp/code/AssetLib/MD5/MD5Loader.cpp:178:15
    #2 0x55a54b9087a0 in Assimp::MD5Importer::LoadMD5MeshFile() /src/assimp/code/AssetLib/MD5/MD5Loader.cpp:335:5
    #3 0x55a54b902da0 in Assimp::MD5Importer::InternReadFile(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, aiScene*, Assimp::IOSystem*) /src/assimp/code/AssetLib/MD5/MD5Loader.cpp:141:13
    #4 0x55a54c66e968 in Assimp::BaseImporter::ReadFile(Assimp::Importer*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, Assimp::IOSystem*) /src/assimp/code/Common/BaseImporter.cpp:131:9
    #5 0x55a54b5cfead in Assimp::Importer::ReadFile(char const*, unsigned int) /src/assimp/code/Common/Importer.cpp:709:30
    #6 0x55a54b5cd490 in Assimp::Importer::ReadFileFromMemory(void const*, unsigned long, unsigned int, char const*) /src/assimp/code/Common/Importer.cpp:507:9
    #7 0x55a54b586d47 in LLVMFuzzerTestOneInput /src/assimp/fuzz/assimp_fuzzer.cc:54:34
    #8 0x55a54b43b7a0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
    #9 0x55a54b426a15 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
    #10 0x55a54b42c4af in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
    #11 0x55a54b457752 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #12 0x7f95de669082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 0702430aef5fa3dda43986563e9ffcc47efbd75e)

SUMMARY: AddressSanitizer: heap-buffer-overflow /src/assimp/code/AssetLib/MD5/MD5Parser.h:428:13 in SkipSpacesAndLineEnd
Shadow bytes around the buggy address:
  0x501fffffff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x501fffffff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x502000000000: fa fa 00 00 fa fa 00 fa fa fa 00 fa fa fa 00 fa
  0x502000000080: fa fa 00 07 fa fa 00 07 fa fa 00 00 fa fa 00 00
  0x502000000100: fa fa 00 fa fa fa 00 fa fa fa fd fa fa fa fd fd
=>0x502000000180: fa fa 00 00 fa fa 00 00[fa]fa fa fa fa fa fa fa
  0x502000000200: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x502000000280: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x502000000300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x502000000380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x502000000400: 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
==1114==ABORTING

@tyler92 tyler92 force-pushed the fix-buffer-overflow-md5parser branch from 37f081c to ff197f5 Compare December 11, 2024 21:44
@tyler92
Copy link
Contributor Author

tyler92 commented Dec 11, 2024

Updated the patch as Sonar suggested

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 added Fuzzer Bugs found by a fuzzer Bug Global flag to mark a deviation from expected behaviour labels Dec 17, 2024
@kimkulling kimkulling merged commit ecc8a1c into assimp:master Dec 17, 2024
12 checks passed
@kimkulling
Copy link
Member

Merged, thanks a lot for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Global flag to mark a deviation from expected behaviour Fuzzer Bugs found by a fuzzer
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Bug: Heap-based Buffer Overflow in SkipSpacesAndLineEnd
2 participants