Skip to content

Commit e448fbf

Browse files
bkaradzicCopilot
andcommitted
Fix integer overflow in Chakra napi_create_dataview bounds check
napi_create_dataview validated the requested view with an unchecked �yte_length + byte_offset > bufferLength comparison. byte_length and byte_offset are caller-supplied size_t values, so on 64-bit builds their sum can overflow and wrap past the limit. When that happens the values are then truncated to 32-bit for JsCreateDataView (creating a small, valid view) while the ORIGINAL 64-bit values are stored in DataViewInfo and later returned by napi_get_dataview_info alongside the small backing buffer, giving a calling addon an out-of-bounds read/write primitive. Validate byte_offset and byte_length against the buffer size individually without adding them, so neither overflow nor the subsequent 32-bit truncation can slip an out-of-range view past the check. After the check both values are <= bufferLength (a 32-bit quantity), so the truncation and the stored values are guaranteed in range. The JavaScriptCore backend delegates to the JS DataView constructor and is unaffected. The V8 backend carries the same upstream pattern but is vendored verbatim from Node and left untouched. Add a native regression test (the path is not reachable from JS ew DataView) that crafts an offset/length whose low 32 bits are valid but whose 64-bit sum wraps, and asserts the view is rejected (or at least never reports the raw 64-bit extents). Guarded to the Chakra engine and 64-bit builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent a128e68 commit e448fbf

3 files changed

Lines changed: 82 additions & 1 deletion

File tree

Core/Node-API/Source/js_native_api_chakra.cc

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2244,7 +2244,13 @@ napi_status napi_create_dataview(napi_env env,
22442244
&unused,
22452245
&bufferLength));
22462246

2247-
if (byte_length + byte_offset > bufferLength) {
2247+
// bufferLength is 32-bit; byte_offset and byte_length are caller-supplied
2248+
// size_t values. Validate each against the buffer size without adding them
2249+
// (byte_offset + byte_length could overflow size_t and wrap past the check,
2250+
// after which the values would be truncated to 32-bit for JsCreateDataView
2251+
// while the original 64-bit values get stored in DataViewInfo and later
2252+
// handed back by napi_get_dataview_info, enabling an out-of-bounds access).
2253+
if (byte_offset > bufferLength || byte_length > bufferLength - byte_offset) {
22482254
napi_throw_range_error(
22492255
env,
22502256
"ERR_NAPI_INVALID_DATAVIEW_ARGS",

Tests/UnitTests/CMakeLists.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,10 @@ endif()
4646
add_executable(UnitTests ${SOURCES} ${SCRIPTS} ${TYPE_SCRIPTS} ${ASSETS})
4747
target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}")
4848

49+
if(NAPI_JAVASCRIPT_ENGINE STREQUAL "Chakra")
50+
target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_CHAKRA)
51+
endif()
52+
4953
target_link_libraries(UnitTests
5054
PRIVATE AppRuntime
5155
PRIVATE Console

Tests/UnitTests/Shared/Shared.cpp

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include <arcana/threading/blocking_concurrent_queue.h>
1616
#include <atomic>
1717
#include <chrono>
18+
#include <cstdint>
1819
#include <future>
1920
#include <iostream>
2021
#include <thread>
@@ -274,6 +275,76 @@ TEST(AppRuntime, DestroyDoesNotDeadlock)
274275
testThread.join();
275276
}
276277

278+
#ifdef JSRUNTIMEHOST_NAPI_ENGINE_CHAKRA
279+
#ifdef _WIN64
280+
TEST(NodeApi, CreateDataViewRejectsOverflowingRange)
281+
{
282+
// Regression: napi_create_dataview must reject a (byte_offset, byte_length)
283+
// pair whose sum overflows size_t. The pre-fix code performed an unchecked
284+
// `byte_offset + byte_length > bufferLength` comparison; with the inputs
285+
// below the 64-bit sum wraps to 8 and slips past it. It then truncated the
286+
// values to 32-bit (offset -> 0, length -> 8) and created a valid 8-byte
287+
// DataView, but stored the ORIGINAL 64-bit offset/length in DataViewInfo,
288+
// which napi_get_dataview_info hands back alongside the small real buffer --
289+
// an out-of-bounds access primitive. This path is not reachable from JS
290+
// `new DataView`, so it is covered natively here. The scenario is specific
291+
// to 64-bit size_t (where the 32-bit truncation diverges from the stored
292+
// value), hence the _WIN64 guard.
293+
Babylon::AppRuntime runtime{};
294+
295+
std::promise<bool> overflowSafe;
296+
std::promise<bool> validAccepted;
297+
298+
runtime.Dispatch([&overflowSafe, &validAccepted](Napi::Env env) {
299+
napi_env nenv{env};
300+
301+
Napi::ArrayBuffer arrayBuffer{Napi::ArrayBuffer::New(env, 16)};
302+
napi_value arrayBufferValue{arrayBuffer};
303+
304+
// Low 32 bits are individually valid for the 16-byte buffer (offset 0,
305+
// length 8), but the full 64-bit values are enormous and their sum wraps
306+
// around size_t to 8.
307+
const size_t hugeOffset{0xFFFFFFFF00000000ull};
308+
const size_t hugeLength{0x0000000100000008ull};
309+
310+
napi_value result{nullptr};
311+
napi_status status{napi_create_dataview(nenv, hugeLength, arrayBufferValue, hugeOffset, &result)};
312+
313+
bool safe;
314+
if (status != napi_ok || result == nullptr)
315+
{
316+
// Fixed path: the out-of-range request is rejected outright.
317+
safe = true;
318+
}
319+
else
320+
{
321+
// If creation unexpectedly succeeds, the reported extents must still
322+
// lie within the 16-byte backing buffer (i.e. not the raw 64-bit
323+
// inputs). The pre-fix code reported the huge stored values here.
324+
size_t reportedLength{0};
325+
size_t reportedOffset{0};
326+
void* data{nullptr};
327+
napi_get_dataview_info(nenv, result, &reportedLength, &data, nullptr, &reportedOffset);
328+
safe = reportedOffset <= 16 && reportedLength <= 16 && reportedOffset + reportedLength <= 16;
329+
}
330+
331+
// Clear any pending range error so it doesn't surface as an unhandled error.
332+
napi_value pendingException{nullptr};
333+
napi_get_and_clear_last_exception(nenv, &pendingException);
334+
overflowSafe.set_value(safe);
335+
336+
// A legitimate offset/length pair must still succeed.
337+
napi_value validResult{nullptr};
338+
napi_status validStatus{napi_create_dataview(nenv, 8, arrayBufferValue, 4, &validResult)};
339+
validAccepted.set_value(validStatus == napi_ok && validResult != nullptr);
340+
});
341+
342+
EXPECT_TRUE(overflowSafe.get_future().get());
343+
EXPECT_TRUE(validAccepted.get_future().get());
344+
}
345+
#endif
346+
#endif
347+
277348
int RunTests()
278349
{
279350
testing::InitGoogleTest();

0 commit comments

Comments
 (0)