From 57d1d73b47ca6a0b3e629a3b1c3b824b96a062d5 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 4 Feb 2020 15:52:39 +0100 Subject: [PATCH] src: fix OOB reads in process.title getter The getter passed a stack-allocated, fixed-size buffer to uv_get_process_title() but neglected to check the return value. When the total length of the command line arguments exceeds the size of the buffer, libuv returns UV_ENOBUFS and doesn't modify the contents of the buffer. The getter then proceeded to return whatever garbage was on the stack at the time of the call, quite possibly reading beyond the end of the buffer. Add a GetProcessTitle() helper that reads the process title into a dynamically allocated buffer that is resized when necessary. Fixes: https://github.com/nodejs/node/issues/31631 PR-URL: https://github.com/nodejs/node/pull/31633 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Gus Caplan Reviewed-By: David Carlier Reviewed-By: Rich Trott --- src/node_internals.h | 1 + src/node_process_object.cc | 8 ++++---- src/node_v8_platform-inl.h | 6 +++--- src/util.cc | 28 +++++++++++++++++++++++++--- test/parallel/test-process-title.js | 22 ++++++++++++++++++++++ 5 files changed, 55 insertions(+), 10 deletions(-) create mode 100644 test/parallel/test-process-title.js diff --git a/src/node_internals.h b/src/node_internals.h index 68b0852e131e95..05e9473771e43c 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -98,6 +98,7 @@ void RegisterSignalHandler(int signal, bool reset_handler = false); #endif +std::string GetProcessTitle(const char* default_title); std::string GetHumanReadableProcessName(); void GetHumanReadableProcessName(char (*name)[1024]); diff --git a/src/node_process_object.cc b/src/node_process_object.cc index a1bf90c8d69fc0..7cf8c125009606 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -32,11 +32,11 @@ using v8::Value; static void ProcessTitleGetter(Local property, const PropertyCallbackInfo& info) { - char buffer[512]; - uv_get_process_title(buffer, sizeof(buffer)); + std::string title = GetProcessTitle("node"); info.GetReturnValue().Set( - String::NewFromUtf8(info.GetIsolate(), buffer, NewStringType::kNormal) - .ToLocalChecked()); + String::NewFromUtf8(info.GetIsolate(), title.data(), + NewStringType::kNormal, title.size()) + .ToLocalChecked()); } static void ProcessTitleSetter(Local property, diff --git a/src/node_v8_platform-inl.h b/src/node_v8_platform-inl.h index ecd8f21d1a2653..0f4a98a98551ba 100644 --- a/src/node_v8_platform-inl.h +++ b/src/node_v8_platform-inl.h @@ -21,12 +21,12 @@ class NodeTraceStateObserver : public v8::TracingController::TraceStateObserver { public: inline void OnTraceEnabled() override { - char name_buffer[512]; - if (uv_get_process_title(name_buffer, sizeof(name_buffer)) == 0) { + std::string title = GetProcessTitle(""); + if (!title.empty()) { // Only emit the metadata event if the title can be retrieved // successfully. Ignore it otherwise. TRACE_EVENT_METADATA1( - "__metadata", "process_name", "name", TRACE_STR_COPY(name_buffer)); + "__metadata", "process_name", "name", TRACE_STR_COPY(title.c_str())); } TRACE_EVENT_METADATA1("__metadata", "version", diff --git a/src/util.cc b/src/util.cc index 26dbfe844995eb..2a594d81ecab8e 100644 --- a/src/util.cc +++ b/src/util.cc @@ -22,6 +22,7 @@ #include "util.h" // NOLINT(build/include_inline) #include "util-inl.h" +#include "debug_utils-inl.h" #include "env-inl.h" #include "node_buffer.h" #include "node_errors.h" @@ -45,6 +46,7 @@ #include #include +#include #include #include @@ -133,10 +135,30 @@ void LowMemoryNotification() { } } +std::string GetProcessTitle(const char* default_title) { + std::string buf(16, '\0'); + + for (;;) { + const int rc = uv_get_process_title(&buf[0], buf.size()); + + if (rc == 0) + break; + + if (rc != UV_ENOBUFS) + return default_title; + + buf.resize(2 * buf.size()); + } + + // Strip excess trailing nul bytes. Using strlen() here is safe, + // uv_get_process_title() always zero-terminates the result. + buf.resize(strlen(&buf[0])); + + return buf; +} + std::string GetHumanReadableProcessName() { - char name[1024]; - GetHumanReadableProcessName(&name); - return name; + return SPrintF("%s[%d]", GetProcessTitle("Node.js"), uv_os_getpid()); } void GetHumanReadableProcessName(char (*name)[1024]) { diff --git a/test/parallel/test-process-title.js b/test/parallel/test-process-title.js new file mode 100644 index 00000000000000..13a030decf887c --- /dev/null +++ b/test/parallel/test-process-title.js @@ -0,0 +1,22 @@ +'use strict'; +const common = require('../common'); +const { spawnSync } = require('child_process'); +const { strictEqual } = require('assert'); + +// FIXME add sunos support +if (common.isSunOS) + common.skip(`Unsupported platform [${process.platform}]`); +// FIXME add IBMi support +if (common.isIBMi) + common.skip('Unsupported platform IBMi'); + +// Explicitly assigning to process.title before starting the child process +// is necessary otherwise *its* process.title is whatever the last +// SetConsoleTitle() call in our process tree set it to. +// Can be removed when https://github.com/libuv/libuv/issues/2667 is fixed. +if (common.isWindows) + process.title = process.execPath; + +const xs = 'x'.repeat(1024); +const proc = spawnSync(process.execPath, ['-p', 'process.title', xs]); +strictEqual(proc.stdout.toString().trim(), process.execPath);