From cf39f4326347915fa9160d165b4395b60a085a58 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 18 Dec 2017 13:43:53 +0100 Subject: [PATCH] timers: make setImmediate() immune to tampering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make setImmediate() immune to `process` global tampering by removing the dependency on the `process._immediateCallback` property. PR-URL: https://github.com/nodejs/node/pull/17736 Fixes: https://github.com/nodejs/node/issues/17681 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Tobias Nießen Reviewed-By: Anatoli Papirovski Reviewed-By: Jeremiah Senkpiel Reviewed-By: James M Snell --- lib/timers.js | 23 ++++++++++------------- src/env.cc | 2 +- src/env.h | 2 +- src/node.cc | 15 --------------- src/timer_wrap.cc | 23 +++++++++++++++++++++++ test/common/index.js | 1 + test/parallel/test-timer-immediate.js | 5 +++++ 7 files changed, 41 insertions(+), 30 deletions(-) create mode 100644 test/parallel/test-timer-immediate.js diff --git a/lib/timers.js b/lib/timers.js index 43d2cbbd07bdb3..de0c2689c5ccc2 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -22,7 +22,10 @@ 'use strict'; const async_wrap = process.binding('async_wrap'); -const TimerWrap = process.binding('timer_wrap').Timer; +const { + Timer: TimerWrap, + setImmediateCallback, +} = process.binding('timer_wrap'); const L = require('internal/linkedlist'); const internalUtil = require('internal/util'); const { createPromise, promiseResolve } = process.binding('util'); @@ -47,12 +50,8 @@ const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants; const async_id_symbol = Symbol('asyncId'); const trigger_async_id_symbol = Symbol('triggerAsyncId'); -/* This is an Uint32Array for easier sharing with C++ land. */ -const scheduledImmediateCount = process._scheduledImmediateCount; -delete process._scheduledImmediateCount; -/* Kick off setImmediate processing */ -const activateImmediateCheck = process._activateImmediateCheck; -delete process._activateImmediateCheck; +const [activateImmediateCheck, scheduledImmediateCountArray] = + setImmediateCallback(processImmediate); // Timeout values > TIMEOUT_MAX are set to 1. const TIMEOUT_MAX = 2 ** 31 - 1; @@ -706,8 +705,6 @@ function processImmediate() { } } -process._immediateCallback = processImmediate; - // An optimization so that the try/finally only de-optimizes (since at least v8 // 4.7) what is in this smaller function. function tryOnImmediate(immediate, oldTail) { @@ -724,7 +721,7 @@ function tryOnImmediate(immediate, oldTail) { if (!immediate._destroyed) { immediate._destroyed = true; - scheduledImmediateCount[0]--; + scheduledImmediateCountArray[0]--; if (async_hook_fields[kDestroy] > 0) { emitDestroy(immediate[async_id_symbol]); @@ -778,9 +775,9 @@ function Immediate(callback, args) { this); } - if (scheduledImmediateCount[0] === 0) + if (scheduledImmediateCountArray[0] === 0) activateImmediateCheck(); - scheduledImmediateCount[0]++; + scheduledImmediateCountArray[0]++; immediateQueue.append(this); } @@ -826,7 +823,7 @@ exports.clearImmediate = function(immediate) { if (!immediate) return; if (!immediate._destroyed) { - scheduledImmediateCount[0]--; + scheduledImmediateCountArray[0]--; immediate._destroyed = true; if (async_hook_fields[kDestroy] > 0) { diff --git a/src/env.cc b/src/env.cc index 902429e18a7e74..67a6f2c3d46fd3 100644 --- a/src/env.cc +++ b/src/env.cc @@ -309,7 +309,7 @@ void Environment::CheckImmediate(uv_check_t* handle) { MakeCallback(env->isolate(), env->process_object(), - env->immediate_callback_string(), + env->immediate_callback_function(), 0, nullptr, {0, 0}).ToLocalChecked(); diff --git a/src/env.h b/src/env.h index dbea629b74bbd3..0fd9c32189f3a0 100644 --- a/src/env.h +++ b/src/env.h @@ -158,7 +158,6 @@ class ModuleWrap; V(homedir_string, "homedir") \ V(hostmaster_string, "hostmaster") \ V(ignore_string, "ignore") \ - V(immediate_callback_string, "_immediateCallback") \ V(infoaccess_string, "infoAccess") \ V(inherit_string, "inherit") \ V(input_string, "input") \ @@ -288,6 +287,7 @@ class ModuleWrap; V(http2ping_constructor_template, v8::ObjectTemplate) \ V(http2stream_constructor_template, v8::ObjectTemplate) \ V(http2settings_constructor_template, v8::ObjectTemplate) \ + V(immediate_callback_function, v8::Function) \ V(inspector_console_api_object, v8::Object) \ V(pbkdf2_constructor_template, v8::ObjectTemplate) \ V(pipe_constructor_template, v8::FunctionTemplate) \ diff --git a/src/node.cc b/src/node.cc index c5b1c6be2af602..6edfb0c9c8a77f 100644 --- a/src/node.cc +++ b/src/node.cc @@ -3169,12 +3169,6 @@ static void DebugEnd(const FunctionCallbackInfo& args); namespace { -void ActivateImmediateCheck(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - env->ActivateImmediateCheck(); -} - - void StartProfilerIdleNotifier(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); env->StartProfilerIdleNotifier(); @@ -3399,12 +3393,6 @@ void SetupProcessObject(Environment* env, FIXED_ONE_BYTE_STRING(env->isolate(), "ppid"), GetParentProcessId).FromJust()); - auto scheduled_immediate_count = - FIXED_ONE_BYTE_STRING(env->isolate(), "_scheduledImmediateCount"); - CHECK(process->Set(env->context(), - scheduled_immediate_count, - env->scheduled_immediate_count().GetJSArray()).FromJust()); - auto should_abort_on_uncaught_toggle = FIXED_ONE_BYTE_STRING(env->isolate(), "_shouldAbortOnUncaughtToggle"); CHECK(process->Set(env->context(), @@ -3536,9 +3524,6 @@ void SetupProcessObject(Environment* env, env->as_external()).FromJust()); // define various internal methods - env->SetMethod(process, - "_activateImmediateCheck", - ActivateImmediateCheck); env->SetMethod(process, "_startProfilerIdleNotifier", StartProfilerIdleNotifier); diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc index 874c80d8d7095b..5c3f499d163005 100644 --- a/src/timer_wrap.cc +++ b/src/timer_wrap.cc @@ -29,7 +29,9 @@ namespace node { namespace { +using v8::Array; using v8::Context; +using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; @@ -67,11 +69,32 @@ class TimerWrap : public HandleWrap { env->SetProtoMethod(constructor, "stop", Stop); target->Set(timerString, constructor->GetFunction()); + + target->Set(env->context(), + FIXED_ONE_BYTE_STRING(env->isolate(), "setImmediateCallback"), + env->NewFunctionTemplate(SetImmediateCallback) + ->GetFunction(env->context()).ToLocalChecked()).FromJust(); } size_t self_size() const override { return sizeof(*this); } private: + static void SetImmediateCallback(const FunctionCallbackInfo& args) { + CHECK(args[0]->IsFunction()); + auto env = Environment::GetCurrent(args); + env->set_immediate_callback_function(args[0].As()); + auto activate_cb = [] (const FunctionCallbackInfo& args) { + Environment::GetCurrent(args)->ActivateImmediateCheck(); + }; + auto activate_function = + env->NewFunctionTemplate(activate_cb)->GetFunction(env->context()) + .ToLocalChecked(); + auto result = Array::New(env->isolate(), 2); + result->Set(0, activate_function); + result->Set(1, env->scheduled_immediate_count().GetJSArray()); + args.GetReturnValue().Set(result); + } + static void New(const FunctionCallbackInfo& args) { // This constructor should not be exposed to public javascript. // Therefore we assert that we are not trying to call this as a diff --git a/test/common/index.js b/test/common/index.js index 30b6aca88e05e3..b5973b1821bb5f 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -21,6 +21,7 @@ /* eslint-disable required-modules, crypto-check */ 'use strict'; +const process = global.process; // Some tests tamper with the process global. const path = require('path'); const fs = require('fs'); const assert = require('assert'); diff --git a/test/parallel/test-timer-immediate.js b/test/parallel/test-timer-immediate.js new file mode 100644 index 00000000000000..385fa4baca4ee5 --- /dev/null +++ b/test/parallel/test-timer-immediate.js @@ -0,0 +1,5 @@ +'use strict'; +const common = require('../common'); +common.globalCheck = false; +global.process = {}; // Boom! +setImmediate(common.mustCall());