From e7682b1f46756440bd5ca4ea76c37c2284c5a92d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87?= Date: Thu, 11 Jun 2026 17:38:13 -0700 Subject: [PATCH] Fix integer underflow in napi_get_value_string_* zero-bufsize handling napi_get_value_string_utf16 computed bufsize - 1 as the destination capacity and stored the null terminator at buf[bufsize - 1]. With a non-null buffer and bufsize == 0, bufsize - 1 underflows to SIZE_MAX, so the entire JS string is copied into the zero-length buffer and the terminator is written at buf[SIZE_MAX] -- an attacker-sized out-of-bounds write reachable from any native caller passing (buf != nullptr, bufsize == 0) (CWE-191 -> CWE-787). This affected both the Chakra and JavaScriptCore backends: - Chakra (js_native_api_chakra.cc): gate the copy on bufsize != 0 (mirroring the upstream Node implementation). The latin1/utf8 siblings shared a milder bug -- with bufsize == 0 they took the slow path (a needless allocation) and stored the terminator at buf[0], one byte past a zero-length buffer; both now short-circuit bufsize == 0 too. - JavaScriptCore (js_native_api_javascriptcore.cc): the utf16/latin1/utf8 getters delegate to JSString::CopyTo*, which underflow bufsize - 1 and always write buf[size] = 0. Short-circuit bufsize == 0 at the getter before calling the copy helper. V8 already handles bufsize == 0 correctly and is unchanged. Add a native regression test (this path is not reachable from JS) that calls napi_get_value_string_utf16 with a sentinel buffer and bufsize == 0, asserting nothing is written and the reported length is zero, while a normally-sized buffer still copies and null-terminates. Runs on Chakra, V8 and JavaScriptCore; guarded off the V8JSI backend, which does not expose napi_get_value_string_utf16. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Core/Node-API/Source/js_native_api_chakra.cc | 24 ++++++++- .../Source/js_native_api_javascriptcore.cc | 18 +++++++ Tests/UnitTests/Shared/Shared.cpp | 52 +++++++++++++++++++ 3 files changed, 93 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 53ef2af9..297be2aa 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/Core/Node-API/Source/js_native_api_chakra.cc @@ -1405,6 +1405,14 @@ napi_status napi_get_value_string_latin1(napi_env env, CHECK_JSRT_EXPECTED(env, JsCopyString(jsValue, nullptr, 0, result, CP_LATIN1), napi_string_expected); + } else if (bufsize == 0) { + // A non-null buffer with bufsize == 0 has no room for the null terminator; + // report zero and write nothing. The slow path below would otherwise run a + // needless allocation and store the terminator at buf[0], one byte past the + // zero-length buffer. + if (result != nullptr) { + *result = 0; + } } else { size_t count = 0; CHECK_JSRT_EXPECTED(env, @@ -1489,6 +1497,14 @@ napi_status napi_get_value_string_utf8(napi_env env, CHECK_JSRT_EXPECTED(env, JsCopyString(jsValue, nullptr, 0, result), napi_string_expected); + } else if (bufsize == 0) { + // A non-null buffer with bufsize == 0 has no room for the null terminator; + // report zero and write nothing. The slow path below would otherwise run a + // needless allocation and store the terminator at buf[0], one byte past the + // zero-length buffer. + if (result != nullptr) { + *result = 0; + } } else { size_t count = 0; CHECK_JSRT_EXPECTED(env, @@ -1574,7 +1590,7 @@ napi_status napi_get_value_string_utf16(napi_env env, CHECK_JSRT_EXPECTED(env, JsCopyStringUtf16(jsValue, nullptr, 0, result), napi_string_expected); - } else { + } else if (bufsize != 0) { size_t copied = 0; CHECK_JSRT_EXPECTED(env, JsCopyStringUtf16( @@ -1593,6 +1609,12 @@ napi_status napi_get_value_string_utf16(napi_env env, if (result != nullptr) { *result = copied; } + } else if (result != nullptr) { + // A non-null buffer with bufsize == 0 has no room for any character or the + // null terminator. Report zero copied and write nothing, instead of letting + // bufsize - 1 underflow to SIZE_MAX (which would copy the whole string into + // the zero-length buffer and store the terminator at buf[SIZE_MAX]). + *result = 0; } return napi_ok; diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 86f52b17..2c9e8e80 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -1815,6 +1815,12 @@ napi_status napi_get_value_string_latin1(napi_env env, if (buf == nullptr) { *result = string.LengthLatin1(); + } else if (bufsize == 0) { + // No room for any character or the null terminator; report zero and write + // nothing rather than letting the copy helper underflow bufsize - 1. + if (result != nullptr) { + *result = 0; + } } else { string.CopyToLatin1(buf, bufsize, result); } @@ -1844,6 +1850,12 @@ napi_status napi_get_value_string_utf8(napi_env env, if (buf == nullptr) { *result = string.LengthUTF8(); + } else if (bufsize == 0) { + // No room for any byte or the null terminator; report zero and write + // nothing rather than letting the copy helper underflow bufsize - 1. + if (result != nullptr) { + *result = 0; + } } else { string.CopyToUTF8(buf, bufsize, result); } @@ -1873,6 +1885,12 @@ napi_status napi_get_value_string_utf16(napi_env env, if (buf == nullptr) { *result = string.Length(); + } else if (bufsize == 0) { + // No room for any code unit or the null terminator; report zero and write + // nothing rather than letting the copy helper underflow bufsize - 1. + if (result != nullptr) { + *result = 0; + } } else { static_assert(sizeof(char16_t) == sizeof(JSChar)); string.CopyTo(reinterpret_cast(buf), bufsize, result); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index f4fa3c52..a920fa1f 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -351,6 +351,58 @@ TEST(NodeApi, CreateDataViewRejectsOverflowingRange) } #endif +// The V8JSI Node-API shim does not expose napi_get_value_string_utf16, so this +// native test only builds on the Chakra, V8, and JavaScriptCore backends. +#if !defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI) +TEST(NodeApi, GetValueStringUtf16HandlesZeroBufsize) +{ + // Regression: napi_get_value_string_utf16 with a non-null buffer and + // bufsize == 0 must not evaluate bufsize - 1. On the Chakra backend the + // pre-fix code forwarded bufsize - 1 (== SIZE_MAX) to JsCopyStringUtf16 as + // the destination capacity, copying the entire JS string into the + // zero-length buffer, and then stored the terminator at buf[bufsize - 1] + // (== buf[SIZE_MAX]). The call must instead write nothing and report zero. + Babylon::AppRuntime runtime{}; + + std::promise zeroSafe; + std::promise normalWorks; + + runtime.Dispatch([&zeroSafe, &normalWorks](Napi::Env env) { + napi_env nenv{env}; + + napi_value strValue{Napi::String::New(env, "hello world")}; + + // Sentinel-filled buffer. With bufsize == 0 nothing may be written, so + // every element must survive unchanged (a SIZE_MAX-capacity copy would + // clobber it / overflow). + char16_t guard[8]; + for (auto& c : guard) + { + c = static_cast(0x7FFF); + } + + size_t copied{0xDEAD}; + napi_status status{napi_get_value_string_utf16(nenv, strValue, guard, 0, &copied)}; + + bool safe{status == napi_ok && copied == 0}; + for (auto c : guard) + { + safe = safe && (c == static_cast(0x7FFF)); + } + zeroSafe.set_value(safe); + + // A sufficiently-sized buffer must still copy and null-terminate. + char16_t buf[32]; + size_t copied2{0}; + napi_status status2{napi_get_value_string_utf16(nenv, strValue, buf, 32, &copied2)}; + normalWorks.set_value(status2 == napi_ok && copied2 == 11 && buf[copied2] == 0); + }); + + EXPECT_TRUE(zeroSafe.get_future().get()); + EXPECT_TRUE(normalWorks.get_future().get()); +} +#endif + int RunTests() { testing::InitGoogleTest();