CWE-195 in ExternalMemoryAccounter::Increase()

How it was found

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

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