From d394dd78de600e89ceea02b3582435e50f249aa4 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 55f5ac40ccc408..abe01e6fed897c 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -95,6 +95,7 @@ void ResetStdio(); // Safe to call more than once and from signal handlers. void SignalExit(int signal, siginfo_t* info, void* ucontext); #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);