CWE-195 in ExternalMemoryAccounter::Increase()

**Summary:** V8's `ExternalMemoryAccounter::Increase()` expects an unsigned `size_t` argument, but a signed `ssize_t` which in some cases results in garbage collection to happen during garbage collection. Here's a simplified version of what happens (full backtrace has been attached in the issue): ``` v8::internal::Heap::CollectAllGarbage GC triggered by as negative integer passed to v8::ExternalMemoryAccounter::Increase(v8::Isolate*, unsigned int) called by node::(anonymous namespace)::CompressionStream<node::(anonymous namespace)::ZlibContext>::Close() during destruction of node::(anonymous namespace)::ZlibStream::~ZlibStream during an active GC v8::internal::Heap::CollectGarbage ``` I could not produce a PoC due to limited experience with bug bounty. I believe provided these details, it is possible to do a PoC though. Also this is my first security report, so please feel free to correct me where I am wrong. **Description:** This bug was introduced in the commit https://github.com/nodejs/node/commit/1d5d7b6eedb2274c9ad48b5f378598a10479e4a7 while replacing deprecated calls to `Isolate::AdjustAmountOfExternalAllocatedMemory`. However mistakenly `ExternalMemoryAccounter::Increase()` is being called instead of `ExternalMemoryAccounter::Update()`. During GC, native objects are being destructed by V8. This results in calling destructor for ZlibStream which calls `ExternalMemoryAccounter::Increase()` instead of `ExternalMemoryAccounter::Update()` **Suggested Fix:** The following patch changes calls from `ExternalMemoryAccounter::Increase()` `ExternalMemoryAccounter::Update()` for passing in `ssize_t` (the patch has been prepared for the v24.6.0 tag and may not apply cleanly for other branches/main branch): ``` diff --git a/src/node_mem-inl.h b/src/node_mem-inl.h index 06871d031d3..70d28dd524b 100644 --- a/src/node_mem-inl.h +++ b/src/node_mem-inl.h @@ -59,7 +59,7 @@ void* NgLibMemoryManager<Class, T>::ReallocImpl(void* ptr, // Environment*/Isolate* parameter and call the V8 method transparently. const int64_t new_size = size - previous_size; manager->IncreaseAllocatedSize(new_size); - manager->env()->external_memory_accounter()->Increase( + manager->env()->external_memory_accounter()->Update( manager->env()->isolate(), new_size); *reinterpret_cast<size_t*>(mem) = size; mem += sizeof(size_t); diff --git a/src/node_zlib.cc b/src/node_zlib.cc index c088c547539..b8617093bdf 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -644,7 +644,7 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork { if (report == 0) return; CHECK_IMPLIES(report < 0, zlib_memory_ >= static_cast<size_t>(-report)); zlib_memory_ += report; - AsyncWrap::env()->external_memory_accounter()->Increase( + AsyncWrap::env()->external_memory_accounter()->Update( AsyncWrap::env()->isolate(), report); } ``` `ExternalMemoryAllocator::Update()` and `ExternalMemoryAllocator::Decrease()` properly takes care of not executing GC when memory is being released. The above patch showed no regression in test cases on Linux x64 as well as Android i686 ## Steps To Reproduce: I could only reproduce this issue inside https://github.com/termux/termux-docker for i686 and arm architecture. Although I believe this is a real issue on other architectures as well. This is also reproducible on arm container, but I'll go ahead with i686 as arm container requires qemu-binfmt on x86 hosts or a arm host. 1. Create the container for reproduction: `docker run -v -it --privileged termux/termux-docker:i686` `docker container start <container_name>` 2. Workaround for bionic on Android not supporting pid >= 2**16 on 32-bit architectures (note that privileged docker containers are required for this workaround) `docker exec --user root -it "$container_id" bash -c "echo 32767 > /proc/sys/kernel/pid_max" ` 3. Start the shell `docker exec --user system -it "$container_id" bash` 4. Install nodejs ```sh pkg update pkg install nodejs ``` I've also attached debug builds of nodejs for Android i686 before and after the suggested patch. The debs can be installed using `apt install ./path/to/nodejs.deb` after copying them to the container The packages are built using `./scripts/run-docker.sh ./build-package -I -a i686 -d nodejs` with patches being applied from `packages/nodejs/` directory ## Impact As far as I can tell GC inside GC is not an intended behaviour in the V8 engine. In the source tree I can see checks in place to ensure that it does not happen. But at the same time I could not find any high level documentation regarding internals of V8. Known affected platforms: Android x86 and Android arm32. I could not reproduce this bug on other architectures/platforms, although I believe this does also affect other platforms. ## Supporting Material/References Issue originally reported to downstream Termux project packaging nodejs in https://github.com/termux/termux-packages/issues/25455