11 ms·
How a double-free bug in WhatsApp turns to remote code execution
- viraptor 7y ago> In Android, a double-free of a memory with size N leads to two subsequent memory-allocation of size N returning the same address. I get that freelists are fast and great for small chunk reuse, (I'm guessing that's the implementation that allows double return) but is there no way to protect against double-free here? I wish it crashed instead. Edit: it does indeed look like a form of a freelist - https://blog.nsogroup.com/a-tale-of-two-mallocs-on-android-libc-allocators-part-1-dlmalloc/ https://blog.nsogroup.com/a-tale-of-two-mallocs-on-android-l...
- rkangel 7y agoAt what level are you looking to cause the crash? Some options: Compiler - insert code and memory usage to show area as free/not-free and check on malloc/free. Data has to be recorded at runtime because the type system does not provide enough information to be sure. Static analysis can only guess with C. OS - Every malloc would need to be in a separate page Libc - could do something similar to compiler They all come with overhead in performance and/or memory usage, unless you change the technology stack in some way. Garbage collection is one approach. Adding extra type information to make it clear from static analysis (so you get something like Rust) is another. Or, some hardware support that gives more granular permissions.
- viraptor 7y agoI guess the naive solution would be to walk the freelist/tree on free() to avoid duplicates. I have no idea what the real world impact would be though. Probably very workload-dependent. At least checking for duplicate would be free on insert in a tree, but in a list that's both a long walk and lots of thrashing.
- gpderetta 7y agoIt would completely defeat the purpose of using a free list.
- josefx 7y agoA call to free could write a gravestone into the freed memory, each free call could check for it and only check the free list if it sees that value. This would make a check of the free list rare, especially if malloc overrides the value again.
- anon4242 7y agoHow would you know it's a gravestone? What if what you consider a gravestone is a perfectly legit byte sequence for a certain program?
- josefx 7y agoThat is the reason you check the freelist in the unlikely case that you see it, the cost would average to near nothing except for the memory accesses to set, check and clear the value. You could also xor the memory location into the marker to avoid multiple collisions for applications that allocate a large amount of similar objects.
- laughinghan 7y agoBesides, what's the minimum chunk size? Can't be less than the 64-bit word size can it? The chances of a random 8-byte gravestone appearing in legit memory is practically nil. If free() took only 1ns to run and you called it 2^64 times, that would take >500 years.
- viraptor 7y agoThats a really cool idea!
- rkangel 7y ago
- DoofusOfDeath 7y agoOne solution is to check each release candidate using extra instrumentation and/or enabled expensive checks.
- pjmlp 7y agoIt won't help now, but because of issues like this, memory tagging is going to be a requirement for native code in Android. https://security.googleblog.com/2019/08/adopting-arm-memory-tagging-extension.html https://security.googleblog.com/2019/08/adopting-arm-memory-...
- Diggsey 7y agoUsing Rust may be the easiest option because there are existing crates for decoding GIF files (and other image formats) for example: https://crates.io/crates/image https://crates.io/crates/image It should be straightforward to swap out the vulnerable C library with a thin wrapper around the above crate.
- pjmlp 7y agoThe safest option would have been to use JNI to call Android hardened codecs instead of using their own decoding library in process.
- pcwalton 7y agoBut it was pointed out upthread that animated GIFs aren't fully supported by the native Android libraries. So Rust would actually be a reasonable solution here. It's true that there are costs associated with using Rust in the build process and so forth, but WhatsApp is trying to be a secure messenger and is FAANG-backed. The challenges can be overcome.
- pjmlp 7y agoIndeed, however Android does support other image formats with animation, which could have been another solution, much cheaper than introducing yet another language not directly supported by the OS SDK tooling.
- bluejekyll 7y ago> not directly supported by the OS SDK tooling. If that's an issue, I bet the Rust community is interested in making it easier and better supported.
- pjmlp 7y agoThink about the Android development experience, using Android Studio, Gradle + CMake/ndk-build, mixed mode debugging across Java/Kotlin and native, Android Studio project templates for native code, packaging Android libraries, Bundles and Instant Apps, NDK APIs and Google libraries for Android intended for NDK consumption like Oboe. This are the expectations that any external language should meet versus what platform languages offer out of the box.
- mixedbit 7y agoPerhaps malloc() could allocate 1 byte extra at the start of the allocated block with some magic number written to it and free() could assert that the number is correct and change the number to mark the block as freed.
- deleted 7y ago[deleted]
- vardump 7y agoProblems: double free might still happen, except now some innocent memory block can be corrupted as well, which just happened to have the magic value in that address. All memory allocations are now misaligned, decreasing performance. On some platforms misaligned access causes a bus error exception. The marker would generally need to be at least 8 bytes. This would alleviate the problems, but also cause pretty significant overhead.
- saagarjha 7y agoI feel like the allocation header might already have a flags bit field where this might go.
- laughinghan 7y agoOn the other hand, the chances of an innocent memory block having a random 8-byte magic value is vanishingly small, so that would work even better. If free() took only 1ns to run and you called it 2^64 times, that would take >500 years.
- bluecalm 7y agoWhy not just always set the pointer to NULL after free is called in it? It would be fantastic if the operation is atomic as well but even if it isn't I like doing it. Freed address is not something you ever want to just again anyway.
- mixedbit 7y agoThere can be many pointers pointing to the same allocated memory. For example, if you have a cyclic list with two elements head.next and head.prev pointers will both point to the second element and free(head.next); free(head.prev); would be double-free.
- nroets 7y agofree() could be wrapped in a macro, but it would not catch the case where multiple copies of the pointer is stored. #define my_free(x) { free(x); (x) = NULL; } Note that the extra assignment will often be optimized away because (a) the pointer is assigned to a malloc() after the call to my_free() OR (b) the pointer goes out of scope e.g. is popped off the stack.
- simias 7y agoI guess it's better than nothing but that won't work if x is some temporary local variable instead of the long term storage location. It also won't help much if there are several pointers to the resource. As such I wonder if a macro is better that just explicitly nulling the pointer after free, at least there's no obfuscation of what's going on. Generally I agree with you though, setting pointers to NULL after free is good practice and probably worth enforcing in the coding style.
- bluecalm 7y agoWell, if there are several pointers to the same memory you need some kind of logic to handle the number anyway. I don't think using free is a good idea in such cases. You need a wrapper which checks for the counter and then decreases it and frees on zero.
- amalcon 7y agoAddress sanitizer can detect this sort of thing, so yes, you can protect against it. It comes at a performance cost, though.
- Zitrax 7y agoYou wouldn't release with that on, and for testing you would have needed to craft a relevant input to detect it or fuzzed it.
- pjmlp 7y agoSure you would. This kind of attacks keep forcing Google security team to increasingly lock down Android native code, including shipping some sanitizers enabled in production devices. https://security.googleblog.com/2019/05/queue-hardening-enhancements.html?m=1 https://security.googleblog.com/2019/05/queue-hardening-enha...
- kccqzy 7y agoWhy wouldn't you release with sanitizers on? The performance impact really is negligible depending on the workload. It's basically like that tiny slowdown when you go from C++ to Java. If you have benchmarks showing the slowdown is acceptable, just ship with it enabled. Saves lots of headaches.
- archi42 7y agoThis is actually a good point. I suspect using asan on this lib as it used by WA would have had a negligble performance impact on the overall UX. (That is, assuming asan would have really captured this specific instance).
- gpderetta 7y agoIs it though? I suspect that the cost of the sanitizer is significantly higher than the overhead of switching, say, to Java. Not that I don't appreciate sanitizers, I just used asan to find the cause of a memory corruption bug just a couple of hours ago and it was great. In the past I would have used valgrind which is significantly slower.
- debatem1 7y agoSeems like you could use an invertible bloom filter or similar, but I'm not sure what the performance hit would look like.
- pjmlp 7y ago> WhatsApp parses it with a native library called libpl_droidsonroids_gif.so to generate the preview of the GIF file. Another victim of Android not exposing the image codecs to the NDK. I guess, like many, they decided to use libpl_droidsonroids_gif instead of having to deal with JNI to call BitmapFactory or ImageDecoder. Because of these kind of exploits codecs have their own hardness processes, https://source.android.com/devices/media/framework-hardening https://source.android.com/devices/media/framework-hardening, but naturally it doesn't help when applications bring their own native alternatives along instead.
- izacus 7y agoImageDecoder didn't support animated gif for all APIs they support so you're again looking for reasoning in the wrong bush - exposing codecs wouldn't help.
- pjmlp 7y agoSo I should look into careless programming, lack of fuzzing and proper testing practices bush instead? Because that is what that tone reminds me of.
- mentat 7y agoYour proposed solution wouldn't work. You appear to have been unaware and instead of saying "thanks, I didn't realize the limitation existed, that's unfortunate" you instead attack the messenger. All file parsers should be throughly fuzzed, yes. Not sure what tone has to do with it.
- pjmlp 7y agoThat would have been indeed my answer if the comment didn't look like a snarky attack. "so you're again looking for reasoning in the wrong bush"
- thenewnewguy 7y ago
- pratio 7y agoThe post is impressive with the demo and explanations
- wil421 7y agoCan someone explain the RCE part? I understand the double free bug but not the exploit part.
- codezero 7y agoBecause one can overwrite the struct with arbitrary content, and that after the function that corrupts the struct is called, it executed a function that lives inside that struct, so the attacker can redefine the function to call out to the system (after a few other tricks - I think)
- aloknnikhil 7y agoYea. They hand-waved over the key part of the RCE. I know they exploited the function call in the struct, but how exactly did they re-write the program counter to point to the address of the system call? They mention that's another vulnerability not part of this article. Also, in the demo, they read a WhatsApp DB file as if it was plain-text from the terminal. I thought WhatsApp DB files were encrypted? How was 'more' able to just read the file?
- icebraining 7y ago> how exactly did they re-write the program counter to point to the address of the system call My understanding is: with the double-free, they managed to overwrite "info", including the location of the function "rewindFunction". When the parsing process calls that function, it will therefore actually call whatever function the attacker has pointed "rewindFunction" to.
- aloknnikhil 7y agoI see now. That's really clever. The fact that there's a double free AND that the WhatsApp gallery parses a GIF twice for some reason is a great catch.
- l33tman 7y agoYes, it's not something you typically think about when writing code even if you're security-conscious. IMO it's not good that the double-free bug has these consequences.
- mruts 7y agoHow does one go about finding stuff like this? Also how much time did it take to research and develop the full exploit? Sounds like a lot of work, especially if you’re not getting paid.
- archi42 7y agoI think the author can answer this better, but I'd guess it's mixture of hobby, curiosity and luck (or rather good intuition where $stuff breaks). [edit] Can be anything from a Saturday if it is the first thing you poke into (and easily grasp the call/stack structure) to a few weekends trying different vectors (or figuring out how to layout your fake memory).[/edit] At least from my experience breaking stuff for security lectures/CTFs. This is probably also a good way to make a name in the security industry (RCE against WA on CV should look pretty neat). [edit]So not getting paid is relative (and again, if happens out of curiosity - not a huge difference spending a weekend binging some anime or breaking stuff).[/edit]
- sebastianconcpt 7y agoAffected versions The exploit works well until WhatsApp version 2.19.230. The vulnerability is official patched in WhatsApp version 2.19.244 The exploit works well for Android 8.1 and 9.0, but does not work for Android 8.0 and below. In the older Android versions, double-free could still be triggered. However, because of the malloc calls by the system after the double-free, the app just crashes before reaching to the point that we could control the PC register.
- captn3m0 7y agoSomewhat related question: Does anyone know when the fixes for CVE-2019-11927 will be released for iOS? The advisory[0] says: >This issue affects WhatsApp for iOS before version v2.19.100 and WhatsApp for Android before version 2.19.243. But the latest version I see on the App Store is 2.19.92 (iPhone S3, iOS 13). The AppStore website says the same[1]. The Android version[2] seems updated (2.19.271) [0]: https://www.facebook.com/security/advisories/cve-2019-11927 https://www.facebook.com/security/advisories/cve-2019-11927 [1]: https://apps.apple.com/in/app/whatsapp-messenger/id310633997 https://apps.apple.com/in/app/whatsapp-messenger/id310633997 [2]: https://www.whatsapp.com/android/ https://www.whatsapp.com/android/
- jannes 7y agoWhenever Apple finishes the app review, I suppose. It's a bit of a shame that people are running vulnerable versions of WhatsApp because Apple is taking its time.
- tinus_hn 7y agoThere is an expedited review process for this kind of thing. It’s impossible to tell who is to blame here. It could be that they abused the expedited review process before and are excluded, or they never requested it, or perhaps they were late in sending in the new version. Or Apple found a problem in the new version, or they were slow.
- danso 7y agoEven as someone who hasn't had to deal with memory allocation since college CS classes many years ago, I found this explainer to be easy to follow and enlightening. Well done! tangent: Your "About" link goes to a 404: https://awakened1712.github.io/about/ https://awakened1712.github.io/about/
- jor-el 7y agoThe author of the blog post is my friend, I will ask him to fix it. The funny thing is, he don't even knows about HN at all and he has no idea that his post is trending. :P
- Dumbdo 7y agoYeah I thought the same, looked like he decided to delete the about page but forgot to delete the references: https://github.com/awakened1712/awakened1712.github.io/commit/80cb662e6e6d9d9f991c28b603e2b80990017919 https://github.com/awakened1712/awakened1712.github.io/commi...
- mgrviper 7y agoAt first i was wondering how one can get RCE out of double-free and then author proceed to drop a bomb - android would reliably return same adress to the next two allocations of same size as freed memory. Android behaviour here is simply unacceptable. One would expect (yeah) memory managment bugs from user space applications, but return same memory from a default allocator twice because of double-free is a terrible peculiarity, undefined behavour or not.
- umanwizard 7y agoHow do other malloc implementations avoid this? It seems natural if what “free” does involves adding the pointer to some free list. Obviously you wouldn’t want to scan the whole free list every time looking for duplicates - is there another way to avoid this behavior?
- tedunangst 7y agoBitmaps don't require scanning.
- saagarjha 7y agoThey do, you'd just scanning a smaller thing.
- tedunangst 7y agoWhat are you scanning the bitmap looking for? Why not just index into it and look at the relevant bit?
- shaklee3 7y agoThis has happened to me in ubuntu 18.04 frequently. Do you have something showing that this is really that rare? If anything, it might help you track down bugs quicker.
- lubesGordi 7y agoYou say this is a peculiarity, but then don't say what it should do instead. Is there some other widely used implementation that doesn't do this? Like others say, scanning the free list for dupes seems inefficient.
- big_chungus 7y agoThe demo clip has been disabled by google drive due to too many plays. Does anyone have a copy/backup he'd be willing to share?
- daenz 7y agoThat was surprisingly easy to follow. I always thought these exploits were quite a bit more magical, but it seems pretty straightforward. Scary.
- notzuck 7y agoAssuming version numbers of acceptable, he should have sold this to Zerodium.
- gok 7y agoIs there a legitimate reason for the Android sandbox to allow calls to `system()`?
- archi42 7y agoI suppose in case you ship some prebuilt executable to do stuff? E.g. ffmpeg. Just a guess, though.
- asveikau 7y agoIMO execve with its already-parsed-argv is a much safer way to invoke another program. You don't need the shell to interpret the boundaries of command line arguments, it invites trouble.
- archi42 7y agoDoesn't execve() end in the same syscall as system()? Not as comfortable for exploitation, but maybe still feasible? (Embedded systems I take a look at usually don't have an OS with either of the two, so I honestly don't know how the implementations look like).
- caleb-allen 7y agoIs issue related to what you're talking about? https://issuetracker.google.com/issues/128554619 https://issuetracker.google.com/issues/128554619 As of Android Q you can no longer execute native binaries that aren't shipped in the .apk, is that related? (not a systems dev, hence the question)
- fourier_mode 7y ago> 24 minute read A simple (number_of_words)/(200WPM) isn't a great metric when words from code snippets are also counted.
- hk__2 7y agoWhat would you suggest instead? We still read words from code snippets.
- fourier_mode 7y agoAccording to that metric you spend 1 minute to read this -- notroot@osboxes:~/Desktop/gif$ ./exploit buffer = 0x7ffc586cd8b0 size = 266 47 49 46 38 39 61 18 00 0A 00 F2 00 00 66 CC CC FF FF FF 00 00 00 33 99 66 99 FF CC 00 00 00 00 00 00 00 00 00 2C 00 00 00 00 08 00 15 00 00 08 9C 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 84 9C 09 B0 C5 07 00 00 00 74 DE E4 11 F3 06 0F 08 37 63 40 C4 C8 21 C3 45 0C 1B 38 5C C8 70 71 43 06 08 1A 34 68 D0 00 C1 07 C4 1C 34 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 54 12 7C C0 C5 07 00 00 00 EE FF FF 2C 00 00 00 00 1C 0F 00 00 00 00 2C 00 00 00 00 1C 0F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 2C 00 00 00 00 18 00 0A 00 0F 00 01 00 00 3B
- billpg 7y agoExactly how terrified should I be right now?
- tzs 7y agoint_fast32_t widthOverflow = gifFilePtr->Image.Width - info->originalWidth; int_fast32_t heightOverflow = gifFilePtr->Image.Height - info->originalHeight; const uint_fast32_t newRasterSize = gifFilePtr->Image.Width * gifFilePtr->Image.Height; if (newRasterSize > info->rasterSize || widthOverflow > 0 || heightOverflow > 0) { ... info->rasterSize = newRasterSize; OT, but isn't that test redundant? For rasterSize = height x width to increase, at least one of height or width must increase, and so anytime the first term of the || is true, at least one of the other terms will also be true, so the first term is redundant. It seems it could be simply this: int_fast32_t widthOverflow = gifFilePtr->Image.Width - info->originalWidth; int_fast32_t heightOverflow = gifFilePtr->Image.Height - info->originalHeight; if (widthOverflow > 0 || heightOverflow > 0) {
- Someone 7y agoIf I understand this correctly, the underlying problem is in GIFLib, which calls reallocatearray, a wrapper around realloc that guards against overflows when computing the size of the memory buffer to reallocate from the number of items and item size. However, reading https://github.com/aseprite/giflib/blob/master/lib/openbsd-reallocarray.c https://github.com/aseprite/giflib/blob/master/lib/openbsd-r..., I don’t see how that could lead to a double-free, unless realloc double-frees, or unless a different reallocatearray gets linked in. Also, that comment on how realloc isn’t portable feels scary. I can see that introduce subtle bugs in libraries used on a different platform from where it is developed. Hence, I think one should forbid the use of raw ‘realloc’ in portable code.
- totony 7y agoMost likely this is getting linked https://code.woboq.org/userspace/glibc/malloc/reallocarray.c.html https://code.woboq.org/userspace/glibc/malloc/reallocarray.c... realloc on linux frees the ptr https://linux.die.net/man/3/realloc https://linux.die.net/man/3/realloc
- deleted 7y ago[deleted]
- jwilk 7y agoandroid-gif-drawable has an older copy of this lib: https://github.com/koral--/android-gif-drawable/blob/dev/android-gif-drawable/src/main/c/giflib/openbsd-reallocarray.c https://github.com/koral--/android-gif-drawable/blob/dev/and... Or perhaps the function from the libc was being used?
- nielsbot 7y agoThis is a very easy to follow explanation, even for those not familiar with these types of attacks. (me)
- swagonomixxx 7y agoDoes anyone know if such behavior is possible on iOS as well? And maybe if Signal has a similar issue? I'm not sure if they use the same GIF decoding library on Android? And iOS? If someone doesn't know off the top of their head maybe they can point me to some docs.
- userbinator 7y agoThe GIF format is over 30 years old. One would think that decoding them was a problem solved long ago, and that decoding libraries would have all the bugs found and fixed by now. As someone who has also written a GIF decoder, more for learning purposes than anything, I checked what mine would do with that GIF: it does reallocate twice, but since the first time already nulls the buffer pointer, it doesn't actually double-free (since free()'ing a NULL pointer is defined by the standard to have no effect.)
- jimbo1qaz 7y agoI recall someone once designed an image format which stored (image width - 1), and added 1 to the image dimensions while decoding. That eliminates an edge case, but no clue what would happen with MAX_INT image size...
- gosuri 7y agoTo prevent falling victim to this attack, it is highly recommended that all WhatsApp users update the app to the latest version. Do I have to do this myself? Doesn't update itself?
- GrayShade 7y agoIt usually does, but you can disable updates in Play Store. > The exploit works well until WhatsApp version 2.19.230. The vulnerability is official patched in WhatsApp version 2.19.244