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.

Sign up for GitHub

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

Jump to bottom

Heap-based buffer overflow in CmkjPlayer::load() #87

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

Heap-based buffer overflow in CmkjPlayer::load() #87

fcambus opened this issue Aug 6, 2019 · 2 comments
Labels
help wanted

Comments

@fcambus
Copy link
Contributor

fcambus commented Aug 6, 2019

Hi,

While fuzzing AdPlug with American Fuzzy Lop, I found a heap-based buffer overflow in CmkjPlayer::load(), in src/mkj.cpp L51.

Attaching a reproducer (gzipped so GitHub accepts it): test01.mkj.gz

=================================================================
==6090==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x612000000178 at pc 0x7fd68b04325b bp 0x7ffc1bb1fa10 sp 0x7ffc1bb1fa08
WRITE of size 2 at 0x612000000178 thread T0
    #0 0x7fd68b04325a in CmkjPlayer::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CFileProvider const&) /home/fcambus/adplug/src/mkj.cpp:51:21
    #1 0x7fd68af8d1d5 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 0x7fd68a7cb09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #5 0x41f759 in _start (/home/fcambus/reps/adplay+0x41f759)

0x612000000178 is located 0 bytes to the right of 312-byte region [0x612000000040,0x612000000178)
allocated by thread T0 here:
    #0 0x4f67e2 in operator new(unsigned long) (/home/fcambus/reps/adplay+0x4f67e2)
    #1 0x7fd68b042c17 in CmkjPlayer::factory(Copl*) /home/fcambus/adplug/src/mkj.cpp:30:10
    #2 0x7fd68af8d11d 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:168: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 0x7fd68a7cb09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/fcambus/adplug/src/mkj.cpp:51:21 in CmkjPlayer::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CFileProvider const&)
Shadow bytes around the buggy address:
  0x0c247fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c247fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c247fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c247fff8000: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c247fff8010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c247fff8020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00[fa]
  0x0c247fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c247fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c247fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c247fff8060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c247fff8070: 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
==6090==ABORTING
@fcambus
Copy link
Contributor Author

fcambus commented Aug 6, 2019

This issue has been assigned CVE-2019-14692.

@Malvineous
Copy link
Member

Having failing unit tests for all these would be extremely helpful!

miller-alex added a commit to miller-alex/adplug that referenced this issue Apr 3, 2020
@miller-alex @fcambus
Add regression test cases for issues adplug#85 to adplug#91
dc2334a 
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
@miller-alex
Add missing checks when loading and playing .mkj files
186de9f 
Fix the following issues in src/mkj.cpp:
* Check number of channels before loading instruments data.
  This fixes a heap-based buffer overflow in CmkjPlayer::load()
  (issue adplug#87).
* Check number of notes befor calculating size of song data
  to avoid interger overflows as well as out-of-bounds reads
  later in update(). (Size of song data vs. used data is really
  hilarious, but that's the way it is.)
* Fail loading if there was an error while reading file data.
* Also in update(), end the song if invalid data is encountered.
  That avoids integer overflows or out-of-range OPL writes.

This commit fixes CVE-2019-14692.

Fixes: adplug#87
Malvineous pushed a commit that referenced this issue May 11, 2020
@miller-alex @fcambus @Malvineous
Add regression test cases for issues #85 to #91
9ede4fd 
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
Assignees
No one assigned
Labels
help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Malvineous @fcambus