buffer: add bounds validation to SlowCopy and FastCopy #60264
Conversation
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: nodejs#59985 Signed-off-by: Rabie45 <[email protected]>
nodejs-github-bot
added
buffer
src/node_buffer.cc
Outdated
| } | ||
| // Validate First before call CopyImpl blindly | ||
| size_t src_len = Buffer::Length(source_obj.As<v8::Object>()); | ||
| size_t dst_len = Buffer::Length(target_obj.As<v8::Object>()); |
There was a problem hiding this comment.
Just fyi, Buffer::HasInstance(obj) is an alias for obj->IsArrayBufferView() and Buffer::Length(obj) is an alias for obj.As<ArrayBufferView>()->ByteLength(). I'd suggest just using those here directly
There was a problem hiding this comment.
Thanks for the note you’re absolutely right. I’ve updated the code to use IsArrayBufferView() and ByteLength() directly instead of Buffer::HasInstance() and Buffer::Length() for consistency with the V8 API.
src/node_buffer.cc
Outdated
|
|
||
| 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) |
There was a problem hiding this comment.
This seems to be an unresolved comment here? We don't do fallbacks anymore, but we still need to handle the situation in this case.
Maybe we could return -1 and check for that on the JS side?
There was a problem hiding this comment.
You’re right, returning a sentinel like -1 would be clearer to signal an invalid range. However, since the function currently returns a uint32_t, using -1 would actually wrap around to 4294967295, which could cause unexpected behavior.
If we want to use -1 safely, we should change the return type to int32_t so the error code is properly represented.
…ce() and Buffer::Length()
There was a problem hiding this comment.
The same validation behaviour can be performed in JS, it just requires the use of the immutable TypedArrayPrototypeGetByteLength rather than direct .byteLength property access, which is the reason for the unsafe behaviour reported in that issue.
All calls to the bound C++ Copy callback occur via the _copyActual validation wrapper in JS; it doesn't really make much sense to duplicate the same validation steps in C++, instead of just fixing the existing ones.
|
|
||
| if (source_start > src_len || target_start > dst_len || | ||
| to_copy > src_len - source_start || to_copy > dst_len - target_start) { | ||
| return -1; |
There was a problem hiding this comment.
This still needs to be actually handled on the JS side, right?
4 participants
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: #59985