From 186211e33cd2e41df9ca9013385a04371df9f612 Mon Sep 17 00:00:00 2001 From: Rabie45 Date: Wed, 15 Oct 2025 14:39:46 +0300 Subject: [PATCH 1/2] buffer: add bounds validation to SlowCopy and FastCopy Fixes an out-of-bounds memory access vulnerability caused by unvalidated copy ranges in Buffer::SlowCopy() and Buffer::FastCopy(). The patch adds proper instance checks and range validation before calling CopyImpl, preventing potential segfaults or memory corruption. Refs: https://github.com/nodejs/node/issues/59985 Signed-off-by: Rabie45 --- src/node_buffer.cc | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 0e4d437c1ea501..3034b7d3275c43 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -578,9 +578,28 @@ void CopyImpl(Local source_obj, void SlowCopy(const FunctionCallbackInfo& args) { Local source_obj = args[0]; Local target_obj = args[1]; + Environment* env = Environment::GetCurrent(args); + Isolate* isolate = env->isolate(); + + // Add validation before call CopyImpl blindly + if (!Buffer::HasInstance(source_obj) || !Buffer::HasInstance(target_obj)) { + isolate->ThrowException(v8::Exception::TypeError(String::NewFromUtf8Literal( + isolate, "Arguments must be Buffer instances"))); + return; + } const uint32_t target_start = args[2].As()->Value(); const uint32_t source_start = args[3].As()->Value(); const uint32_t to_copy = args[4].As()->Value(); + size_t source_len = Buffer::Length(source_obj.As()); + size_t target_len = Buffer::Length(target_obj.As()); + + if (source_start > source_len || target_start > target_len || + to_copy > source_len - source_start || + to_copy > target_len - target_start) { + isolate->ThrowException(v8::Exception::RangeError( + String::NewFromUtf8Literal(isolate, "Buffer copy out of range"))); + return; + } CopyImpl(source_obj, target_obj, target_start, source_start, to_copy); @@ -597,7 +616,21 @@ uint32_t FastCopy(Local receiver, // NOLINTNEXTLINE(runtime/references) FastApiCallbackOptions& options) { HandleScope scope(options.isolate); + Isolate* isolate = options.isolate; + + if (!node::Buffer::HasInstance(source_obj) || + !node::Buffer::HasInstance(target_obj)) { + return 0; + } + // Validate First before call CopyImpl blindly + size_t src_len = Buffer::Length(source_obj.As()); + size_t dst_len = Buffer::Length(target_obj.As()); + if (source_start > src_len || target_start > dst_len || + to_copy > src_len - source_start || to_copy > dst_len - target_start) { + // options.fallback = true; // Let JS slow path handle it (safe fallback) + return 0; + } CopyImpl(source_obj, target_obj, target_start, source_start, to_copy); return to_copy; From 75cfdfb23f90cf6fe89cb0d5f78e29ab49d0ba4c Mon Sep 17 00:00:00 2001 From: Rabie45 Date: Wed, 15 Oct 2025 15:55:19 +0300 Subject: [PATCH 2/2] Use IsArrayBufferView() and ByteLength() instead of Buffer::HasInstance() and Buffer::Length() --- src/node_buffer.cc | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 3034b7d3275c43..967f1964573c88 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -578,20 +578,19 @@ void CopyImpl(Local source_obj, void SlowCopy(const FunctionCallbackInfo& args) { Local source_obj = args[0]; Local target_obj = args[1]; - Environment* env = Environment::GetCurrent(args); - Isolate* isolate = env->isolate(); + Isolate* isolate = args.GetIsolate(); // Add validation before call CopyImpl blindly - if (!Buffer::HasInstance(source_obj) || !Buffer::HasInstance(target_obj)) { - isolate->ThrowException(v8::Exception::TypeError(String::NewFromUtf8Literal( - isolate, "Arguments must be Buffer instances"))); + if (!source_obj->IsArrayBufferView() || !target_obj->IsArrayBufferView()) { + isolate->ThrowException(v8::Exception::TypeError( + String::NewFromUtf8Literal(isolate, "Arguments must be ArrayBufferViews"))); return; } const uint32_t target_start = args[2].As()->Value(); const uint32_t source_start = args[3].As()->Value(); const uint32_t to_copy = args[4].As()->Value(); - size_t source_len = Buffer::Length(source_obj.As()); - size_t target_len = Buffer::Length(target_obj.As()); + size_t source_len = source_obj.As()->ByteLength(); + size_t target_len = target_obj.As()->ByteLength(); if (source_start > source_len || target_start > target_len || to_copy > source_len - source_start || @@ -607,7 +606,7 @@ void SlowCopy(const FunctionCallbackInfo& args) { } // Assume caller has properly validated args. -uint32_t FastCopy(Local receiver, +int32_t FastCopy(Local receiver, Local source_obj, Local target_obj, uint32_t target_start, @@ -618,18 +617,18 @@ uint32_t FastCopy(Local receiver, HandleScope scope(options.isolate); Isolate* isolate = options.isolate; - if (!node::Buffer::HasInstance(source_obj) || - !node::Buffer::HasInstance(target_obj)) { + if (!source_obj->IsArrayBufferView() || !target_obj->IsArrayBufferView()) { + isolate->ThrowException(v8::Exception::TypeError( + String::NewFromUtf8Literal(isolate, "Arguments must be ArrayBufferViews"))); return 0; } // Validate First before call CopyImpl blindly - size_t src_len = Buffer::Length(source_obj.As()); - size_t dst_len = Buffer::Length(target_obj.As()); + size_t src_len = source_obj.As()->ByteLength(); + size_t dst_len = target_obj.As()->ByteLength(); if (source_start > src_len || target_start > dst_len || to_copy > src_len - source_start || to_copy > dst_len - target_start) { - // options.fallback = true; // Let JS slow path handle it (safe fallback) - return 0; + return -1; } CopyImpl(source_obj, target_obj, target_start, source_start, to_copy);