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

Multiple heap-based buffer overflows in Ca2mLoader::load() #88

Closed
fcambus opened this issue Aug 6, 2019 · 2 comments
Closed

Multiple heap-based buffer overflows in Ca2mLoader::load() #88

fcambus opened this issue Aug 6, 2019 · 2 comments

Comments

@fcambus
Copy link
Contributor

fcambus commented Aug 6, 2019

Hi,

While fuzzing AdPlug with American Fuzzy Lop, I found multiple heap-based buffer overflows in Ca2mLoader::load(), in src/a2m.cpp L106 and L184.

Attaching reproducers for both issues (gzipped so GitHub accepts them):

=================================================================
==6688==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x63000000fc28 at pc 0x7f43e5614913 bp 0x7ffe7780f470 sp 0x7ffe7780f468
WRITE of size 1 at 0x63000000fc28 thread T0
    #0 0x7f43e5614912 in Ca2mLoader::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CFileProvider const&) /home/fcambus/adplug/src/a2m.cpp:106:37
    #1 0x7f43e56341d5 in CAdPlug::factory(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Copl*, CPlayers const&, CFileProvider const&) /home/fcambus/adplug/src/adplug.cpp:169:10
    #2 0x4fcd62 in play(char const*, Player*, int) /home/fcambus/adplay-unix/src/adplay.cc:309:11
    #3 0x4fbaf4 in main /home/fcambus/adplay-unix/src/adplay.cc:544:5
    #4 0x7f43e4e7209a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #5 0x41f759 in _start (/home/fcambus/reps2/adplay+0x41f759)

0x63000000fc28 is located 0 bytes to the right of 63528-byte region [0x630000000400,0x63000000fc28)
allocated by thread T0 here:
    #0 0x4f6972 in operator new[](unsigned long) (/home/fcambus/reps2/adplay+0x4f6972)
    #1 0x7f43e5614614 in Ca2mLoader::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CFileProvider const&) /home/fcambus/adplug/src/a2m.cpp:99:13
    #2 0x7f43e56341d5 in CAdPlug::factory(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Copl*, CPlayers const&, CFileProvider const&) /home/fcambus/adplug/src/adplug.cpp:169:10
    #3 0x4fcd62 in play(char const*, Player*, int) /home/fcambus/adplay-unix/src/adplay.cc:309:11
    #4 0x4fbaf4 in main /home/fcambus/adplay-unix/src/adplay.cc:544:5
    #5 0x7f43e4e7209a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/fcambus/adplug/src/a2m.cpp:106:37 in Ca2mLoader::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CFileProvider const&)
Shadow bytes around the buggy address:
  0x0c607fff9f30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c607fff9f40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c607fff9f50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c607fff9f60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c607fff9f70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c607fff9f80: 00 00 00 00 00[fa]fa fa fa fa fa fa fa fa fa fa
  0x0c607fff9f90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c607fff9fa0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c607fff9fb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c607fff9fc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c607fff9fd0: 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
  Shadow gap:              cc
==6688==ABORTING
=================================================================
==6686==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61a000000508 at pc 0x7efe60571e7c bp 0x7ffe914318d0 sp 0x7ffe914318c8
WRITE of size 1 at 0x61a000000508 thread T0
    #0 0x7efe60571e7b in Ca2mLoader::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CFileProvider const&) /home/fcambus/adplug/src/a2m.cpp:184:35
    #1 0x7efe6058f1d5 in CAdPlug::factory(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Copl*, CPlayers const&, CFileProvider const&) /home/fcambus/adplug/src/adplug.cpp:169:10
    #2 0x4fcd62 in play(char const*, Player*, int) /home/fcambus/adplay-unix/src/adplay.cc:309:11
    #3 0x4fbaf4 in main /home/fcambus/adplay-unix/src/adplay.cc:544:5
    #4 0x7efe5fdcd09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #5 0x41f759 in _start (/home/fcambus/reps2/adplay+0x41f759)

0x61a000000508 is located 0 bytes to the right of 1160-byte region [0x61a000000080,0x61a000000508)
allocated by thread T0 here:
    #0 0x4f6972 in operator new[](unsigned long) (/home/fcambus/reps2/adplay+0x4f6972)
    #1 0x7efe60570f6f in Ca2mLoader::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CFileProvider const&) /home/fcambus/adplug/src/a2m.cpp:152:13
    #2 0x7efe6058f1d5 in CAdPlug::factory(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Copl*, CPlayers const&, CFileProvider const&) /home/fcambus/adplug/src/adplug.cpp:169:10
    #3 0x4fcd62 in play(char const*, Player*, int) /home/fcambus/adplay-unix/src/adplay.cc:309:11
    #4 0x4fbaf4 in main /home/fcambus/adplay-unix/src/adplay.cc:544:5
    #5 0x7efe5fdcd09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/fcambus/adplug/src/a2m.cpp:184:35 in Ca2mLoader::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CFileProvider const&)
Shadow bytes around the buggy address:
  0x0c347fff8050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c347fff8060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c347fff8070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c347fff8080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c347fff8090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c347fff80a0: 00[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c347fff80b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c347fff80c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c347fff80d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c347fff80e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c347fff80f0: 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
  Shadow gap:              cc
==6686==ABORTING
@Malvineous
Copy link
Member

Could someone do a PR to have these included in the unit tests?

@fcambus
Copy link
Contributor Author

fcambus commented Aug 7, 2019

This issue has been assigned CVE-2019-14732.

miller-alex added a commit to miller-alex/adplug that referenced this issue Apr 3, 2020
While fuzzing AdPlug with American Fuzzy Lop, Frederic Cambus
found several memory issues and reported them on github. Hook
up the reproducers he provided as test cases in stresstest.cpp.
This includes tests for the following github issues:

* Issue adplug#85 ("Heap-based buffer overflow in
  CxadbmfPlayer::__bmf_convert_stream()")
* Issue adplug#86 ("Heap-based buffer overflow in CdtmLoader::load()")
* Issue adplug#87 ("Heap-based buffer overflow in CmkjPlayer::load()")
* Issue adplug#88 ("Multiple heap-based buffer overflows in Ca2mLoader::load()")
* Issue adplug#89 ("Multiple heap-based buffer overflows in CradLoader::load()")
* Issue adplug#90 ("Multiple heap-based buffer overflows in CmtkLoader::load()")
* Issue adplug#91 ("Double free in Cu6mPlayer::~Cu6mPlayer()")

Co-authored-by: Frederic Cambus <fred@statdns.com>
Bug: adplug#85
Bug: adplug#86
Bug: adplug#87
Bug: adplug#88
Bug: adplug#89
Bug: adplug#90
Bug: adplug#91
miller-alex added a commit to miller-alex/adplug that referenced this issue Apr 3, 2020
Missing checks and wrong calculations in src/a2m.cpp cause
multiple heap-based buffer overflows and out-of-bounds reads
in heap, stack, and static data.

Bugs addressed in this commit:
* Check the number of patterns. Too big values can cause reads
  past the end of the len array.
* Reading a not packed data block with odd length will allocate
  a buffer which is one byte too small and write past the end
  of it (issue adplug#88). Change the allocation/deallocation code
  to fix that in both places.
* Check that data blocks (afer unpacking if applicable) are big
  enough for the expected data before accessing the memory.
* Ensure that the length byte for author, song name, and instrument
  names doesn't exceed the maximum size available.
* Also change the accessor functions for these strings to call
  the proper std::string constructors for char arrays.
* Avoid reads past the end of convfx/newconvfx arrays while
  converting track data.

This commit fixes CVE-2019-14732.

Fixes: adplug#88
Malvineous pushed a commit that referenced this issue May 11, 2020
While fuzzing AdPlug with American Fuzzy Lop, Frederic Cambus
found several memory issues and reported them on github. Hook
up the reproducers he provided as test cases in stresstest.cpp.
This includes tests for the following github issues:

* Issue #85 ("Heap-based buffer overflow in
  CxadbmfPlayer::__bmf_convert_stream()")
* Issue #86 ("Heap-based buffer overflow in CdtmLoader::load()")
* Issue #87 ("Heap-based buffer overflow in CmkjPlayer::load()")
* Issue #88 ("Multiple heap-based buffer overflows in Ca2mLoader::load()")
* Issue #89 ("Multiple heap-based buffer overflows in CradLoader::load()")
* Issue #90 ("Multiple heap-based buffer overflows in CmtkLoader::load()")
* Issue #91 ("Double free in Cu6mPlayer::~Cu6mPlayer()")

Co-authored-by: Frederic Cambus <fred@statdns.com>
Bug: #85
Bug: #86
Bug: #87
Bug: #88
Bug: #89
Bug: #90
Bug: #91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants