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

Double free in Cu6mPlayer::~Cu6mPlayer() #91

Open
fcambus opened this issue Aug 9, 2019 · 10 comments
Open

Double free in Cu6mPlayer::~Cu6mPlayer() #91

fcambus opened this issue Aug 9, 2019 · 10 comments
Labels

Comments

@fcambus
Copy link

@fcambus fcambus commented Aug 9, 2019

Hi,

While fuzzing AdPlug with American Fuzzy Lop, I found a double free in in Cu6mPlayer::~Cu6mPlayer(), in src/u6m.h L42.

Attaching a reproducer (gzipped so GitHub accepts it): double-free.gz

=================================================================
==16659==ERROR: AddressSanitizer: attempting double-free on 0x629000000200 in thread T0:
    #0 0x4f75a2 in operator delete[](void*) (/home/fcambus/adplay-asan/adplay+0x4f75a2)
    #1 0x7f96ab794bb7 in Cu6mPlayer::~Cu6mPlayer() /home/fcambus/adplug/./src/u6m.h:42:21
    #2 0x7f96ab794bf8 in Cu6mPlayer::~Cu6mPlayer() /home/fcambus/adplug/./src/u6m.h:41:5
    #3 0x7f96ab63b66a 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:187:2
    #4 0x4fcd62 in play(char const*, Player*, int) /home/fcambus/adplay-unix/src/adplay.cc:309:11
    #5 0x4fbaf4 in main /home/fcambus/adplay-unix/src/adplay.cc:544:5
    #6 0x7f96aae7909a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #7 0x41f759 in _start (/home/fcambus/adplay-asan/adplay+0x41f759)

0x629000000200 is located 0 bytes inside of 16735-byte region [0x629000000200,0x62900000435f)
freed by thread T0 here:
    #0 0x4f75a2 in operator delete[](void*) (/home/fcambus/adplay-asan/adplay+0x4f75a2)
    #1 0x7f96ab78d0a9 in Cu6mPlayer::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CFileProvider const&) /home/fcambus/adplug/src/u6m.cpp:87:7
    #2 0x7f96ab63b5b4 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:182:13
    #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 0x7f96aae7909a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

previously allocated by thread T0 here:
    #0 0x4f6972 in operator new[](unsigned long) (/home/fcambus/adplay-asan/adplay+0x4f6972)
    #1 0x7f96ab78cb8d in Cu6mPlayer::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CFileProvider const&) /home/fcambus/adplug/src/u6m.cpp:69:15
    #2 0x7f96ab63b5b4 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:182:13
    #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 0x7f96aae7909a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

SUMMARY: AddressSanitizer: double-free (/home/fcambus/adplay-asan/adplay+0x4f75a2) in operator delete[](void*)
==16659==ABORTING
@Malvineous

This comment has been minimized.

Copy link
Member

@Malvineous Malvineous commented Aug 9, 2019

@fcambus Any chance you can submit the reproducers as PRs implemented as extra test files, so running the tests fails because of these bugs? (Some notes are in the 'debugging' section of the README). It would help us a lot as we are only a small project and don't have anyone regularly working on it. I'd be happy to merge the commits in and you would become listed as a contributor to the project.

@fcambus

This comment has been minimized.

Copy link
Author

@fcambus fcambus commented Aug 9, 2019

Sure thing! I'll be busy this weekend but will gladly take a look at this sometime next week.

I really like the AdPlug project so I would be very happy to help.

@debrouxl

This comment has been minimized.

Copy link

@debrouxl debrouxl commented Aug 17, 2019

This one can be fixed by something like

diff --git a/src/u6m.cpp b/src/u6m.cpp
index 144e0de..b36a415 100644
--- a/src/u6m.cpp
+++ b/src/u6m.cpp
@@ -84,7 +84,7 @@ bool Cu6mPlayer::load(const std::string &filename, const CFileProvider &fp)
   if (!lzw_decompress(source,destination))
     {
       delete[] compressed_song_data;
-      delete[] song_data;
+      delete[] song_data; song_data = NULL; // nullptr in C++11+
       return(false);
     }

Also, in order to further enforce that no double delete[] occurs even when calling the destructor more than once, which would be a bug in itself, the following change does the job:

diff --git a/src/u6m.h b/src/u6m.h
index 0f25673..7032835 100644
--- a/src/u6m.h
+++ b/src/u6m.h
@@ -39,7 +39,7 @@ class Cu6mPlayer: public CPlayer
 
   ~Cu6mPlayer()
     {
-      if(song_data) delete[] song_data;
+      delete[] song_data; song_data = NULL; // nullptr in C++11+, again.
     };
 
   bool load(const std::string &filename, const CFileProvider &fp);
@Malvineous

This comment has been minimized.

Copy link
Member

@Malvineous Malvineous commented Aug 17, 2019

This kind of addresses the symptom rather than the problem though. You fix the crash sure, but not the bug that caused the double-free in the first place. That bug is still there, you just don't hear about it any more because two of the detection points are now silenced.

@debrouxl

This comment has been minimized.

Copy link

@debrouxl debrouxl commented Aug 17, 2019

Sure, I could simply have removed the

delete[] song_data;

line from Cu6mPlayer::load(), which allocates song_data, and relied on the delete[] song_data; from ~Cu6mPlayer().

However, I decided to use a more robust option. The combination of my two hunks:

  • fixes the crash caused by the direct double delete[] pointed by afl-fuzz;
  • avoids a leak if the caller misuses the library by not (indirectly) calling the destructor;
  • avoids a double delete[] caused by (indirectly) calling the destructor twice, which would be another misuse of the library, or another bug in the library itself.

It's a fact that my constructs remove two detection points for some bugs... but they do eliminate potential vulnerabilities :)

@Malvineous

This comment has been minimized.

Copy link
Member

@Malvineous Malvineous commented Aug 17, 2019

I see your point, but I'm not sure it's a wise idea to cater for people misusing the library. Are we going to go through every single format player and put checks like this in so that they can have the destructor called multiple times safely? Calling a destructor multiple times is in violation of the C++ standard, so it doesn't seem right to me to include code for handling a situation that can never exist.

Consider this situation - the destructor is called the first time, which deallocates song_data and with your patch, song_data is set to NULL. After the destructor returns, the memory used to store the object itself (where the new NULL value is located) is returned to the heap and then used to allocate another, different object by the library's user. Then somehow this destructor is called a second time. Now, the memory previously used to store the value of song_data has since been overwritten by one of the variables in the new object, so song_data no longer appears to be null. This means the delete[] operator gets called anyway and the code crashes because it tried to deallocate some random piece of memory.

Essentially, once the destructor returns, you should consider all variables in the object to be assigned random values. From here you can see that assigning song_data to NULL in the destructor is a (tiny) waste of effort, because the moment the function returns the value will be lost anyway.

You say your patch also avoids leaks if the caller fails to call the destructor, but I'm not sure it would. If you call load() multiple times with valid files, you will end up with multiple song_data allocations and leaks as load() does not delete song_data if it is called multiple times with valid files. It only releases memory if the decompression failed, which I think is where the true bug is.

To my mind there are two solutions:

  1. If load() is allowed to be called multiple times, then song_data should be deleted at the start of load() and in the destructor, but nowhere else.
  2. If load() is not allowed to be called multiple times then the delete[] song_data that happens on decompression failure should be removed, leaving the bad data in memory to be released by the destructor as normal (as you suggested as a "simple" fix).

Unfortunately I don't know the library well enough to know whether load() is allowed to be called more than once!

@debrouxl

This comment has been minimized.

Copy link

@debrouxl debrouxl commented Aug 18, 2019

Right, my hunks avoid a leak if the caller fails to call the destructor after calling load() with invalid input, but do nothing against calling load()multiple times with valid input...
In several other classes as well, load() methods don't try to handle being called more than once, so I'd think that it's probably not a generally intended use case ?

In any case, even the simple fix of removing delete[] song_data from Cu6mPlayer::load() is an improvement over the current situation, so it should be done. Aborts caused by a DF or a nullptr deref can mask, or at least make harder to detect, other memory unsafety errors.

CrolPlayer::~CrolPlayer() also sets a pointer to NULL after deleting it, but it only executes both statements if the pointer is not NULL, which is redundant because delete checks for that. I see no redefinition of operator delete in adplug.

There are fun, non-destructive ways to pinpoint things which should never have been / no longer be done, such as printing an error message, then sleeping for a large amount of time. That's what Google did for deprecating STLport for Android: https://softwareengineering.stackexchange.com/questions/381763/how-long-to-wait-before-deleting-a-deprecated-method . But it makes little sense to perform the work sprinkling these throughout the code base if there's no consensus that doing so is a good idea :)

@fcambus

This comment has been minimized.

Copy link
Author

@fcambus fcambus commented Aug 22, 2019

This issue has been assigned CVE-2019-15151.

@fcambus

This comment has been minimized.

Copy link
Author

@fcambus fcambus commented Aug 22, 2019

@Malvineous: sent a pull request (#103) to add the test case triggering this issue. Let me know if it's what you add in mind. If so, I can prepare another PR for the other issues.

@mywave82

This comment has been minimized.

Copy link

@mywave82 mywave82 commented Jan 27, 2020

Clearing the pointer after delete is the correct thing to do. Why would you keep the pointer after you call delete one it? The pointer is no longer valid, and there is no reason to keep it, other than causing crashes.

The double free crash detected with tools like valgrind proves that clearing the pointer was not present.

"what if the destructor is called twice" - what kind of call-path would cause that? That is the same as a double-free, only with an class object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.