From 7c4bd4729c5e516f22ef890880ba085ae0d171b7 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 4 Jun 2026 09:34:44 -0700 Subject: [PATCH] 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> --- Core/Node-API/Source/js_native_api_chakra.cc | 8 ++- Tests/UnitTests/Shared/Shared.cpp | 69 ++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/Core/Node-API/Source/js_native_api_chakra.cc b/Core/Node-API/Source/js_native_api_chakra.cc index 2e95ce09..53ef2af9 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/Core/Node-API/Source/js_native_api_chakra.cc @@ -2244,7 +2244,13 @@ napi_status napi_create_dataview(napi_env env, &unused, &bufferLength)); - if (byte_length + byte_offset > bufferLength) { + // bufferLength is 32-bit; byte_offset and byte_length are caller-supplied + // size_t values. Validate each against the buffer size without adding them + // (byte_offset + byte_length could overflow size_t and wrap past the check, + // after which the values would be truncated to 32-bit for JsCreateDataView + // while the original 64-bit values get stored in DataViewInfo and later + // handed back by napi_get_dataview_info, enabling an out-of-bounds access). + if (byte_offset > bufferLength || byte_length > bufferLength - byte_offset) { napi_throw_range_error( env, "ERR_NAPI_INVALID_DATAVIEW_ARGS", diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 9ca01bc9..d2d52b5a 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -276,6 +277,74 @@ TEST(AppRuntime, DestroyDoesNotDeadlock) testThread.join(); } +#if SIZE_MAX > 0xFFFFFFFFu +TEST(NodeApi, CreateDataViewRejectsOverflowingRange) +{ + // Regression: napi_create_dataview must reject a (byte_offset, byte_length) + // pair whose sum overflows size_t. The pre-fix code performed an unchecked + // `byte_offset + byte_length > bufferLength` comparison; with the inputs + // below the 64-bit sum wraps to 8 and slips past it. It then truncated the + // values to 32-bit (offset -> 0, length -> 8) and created a valid 8-byte + // DataView, but stored the ORIGINAL 64-bit offset/length in DataViewInfo, + // which napi_get_dataview_info hands back alongside the small real buffer -- + // an out-of-bounds access primitive. This path is not reachable from JS + // `new DataView`, so it is covered natively here. The scenario requires a + // 64-bit size_t (where the 32-bit truncation diverged from the stored value), + // hence the size_t-width guard. + Babylon::AppRuntime runtime{}; + + std::promise overflowSafe; + std::promise validAccepted; + + runtime.Dispatch([&overflowSafe, &validAccepted](Napi::Env env) { + napi_env nenv{env}; + + Napi::ArrayBuffer arrayBuffer{Napi::ArrayBuffer::New(env, 16)}; + napi_value arrayBufferValue{arrayBuffer}; + + // Low 32 bits are individually valid for the 16-byte buffer (offset 0, + // length 8), but the full 64-bit values are enormous and their sum wraps + // around size_t to 8. + const size_t hugeOffset{0xFFFFFFFF00000000ull}; + const size_t hugeLength{0x0000000100000008ull}; + + napi_value result{nullptr}; + napi_status status{napi_create_dataview(nenv, hugeLength, arrayBufferValue, hugeOffset, &result)}; + + bool safe; + if (status != napi_ok || result == nullptr) + { + // Fixed path: the out-of-range request is rejected outright. + safe = true; + } + else + { + // If creation unexpectedly succeeds, the reported extents must still + // lie within the 16-byte backing buffer (i.e. not the raw 64-bit + // inputs). The pre-fix code reported the huge stored values here. + size_t reportedLength{0}; + size_t reportedOffset{0}; + void* data{nullptr}; + napi_get_dataview_info(nenv, result, &reportedLength, &data, nullptr, &reportedOffset); + safe = reportedOffset <= 16 && reportedLength <= 16 && reportedOffset + reportedLength <= 16; + } + + // Clear any pending range error so it doesn't surface as an unhandled error. + napi_value pendingException{nullptr}; + napi_get_and_clear_last_exception(nenv, &pendingException); + overflowSafe.set_value(safe); + + // A legitimate offset/length pair must still succeed. + napi_value validResult{nullptr}; + napi_status validStatus{napi_create_dataview(nenv, 8, arrayBufferValue, 4, &validResult)}; + validAccepted.set_value(validStatus == napi_ok && validResult != nullptr); + }); + + EXPECT_TRUE(overflowSafe.get_future().get()); + EXPECT_TRUE(validAccepted.get_future().get()); +} +#endif + int RunTests() { testing::InitGoogleTest();