Skip to content

Commit 8474f86

Browse files
bnoordhuisMylesBorins
authored andcommitted
timers: make setImmediate() immune to tampering
Make setImmediate() immune to `process` global tampering by removing the dependency on the `process._immediateCallback` property. Backport-PR-URL: #19006 PR-URL: #17736 Fixes: #17681 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 4acea14 commit 8474f86

File tree

7 files changed

+41
-30
lines changed

7 files changed

+41
-30
lines changed

lib/timers.js

+10-13
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@
2222
'use strict';
2323

2424
const async_wrap = process.binding('async_wrap');
25-
const TimerWrap = process.binding('timer_wrap').Timer;
25+
const {
26+
Timer: TimerWrap,
27+
setImmediateCallback,
28+
} = process.binding('timer_wrap');
2629
const L = require('internal/linkedlist');
2730
const internalUtil = require('internal/util');
2831
const { createPromise, promiseResolve } = process.binding('util');
@@ -47,12 +50,8 @@ const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants;
4750
const async_id_symbol = Symbol('asyncId');
4851
const trigger_async_id_symbol = Symbol('triggerAsyncId');
4952

50-
/* This is an Uint32Array for easier sharing with C++ land. */
51-
const scheduledImmediateCount = process._scheduledImmediateCount;
52-
delete process._scheduledImmediateCount;
53-
/* Kick off setImmediate processing */
54-
const activateImmediateCheck = process._activateImmediateCheck;
55-
delete process._activateImmediateCheck;
53+
const [activateImmediateCheck, scheduledImmediateCountArray] =
54+
setImmediateCallback(processImmediate);
5655

5756
// Timeout values > TIMEOUT_MAX are set to 1.
5857
const TIMEOUT_MAX = 2 ** 31 - 1;
@@ -706,8 +705,6 @@ function processImmediate() {
706705
}
707706
}
708707

709-
process._immediateCallback = processImmediate;
710-
711708
// An optimization so that the try/finally only de-optimizes (since at least v8
712709
// 4.7) what is in this smaller function.
713710
function tryOnImmediate(immediate, oldTail) {
@@ -724,7 +721,7 @@ function tryOnImmediate(immediate, oldTail) {
724721

725722
if (!immediate._destroyed) {
726723
immediate._destroyed = true;
727-
scheduledImmediateCount[0]--;
724+
scheduledImmediateCountArray[0]--;
728725

729726
if (async_hook_fields[kDestroy] > 0) {
730727
emitDestroy(immediate[async_id_symbol]);
@@ -778,9 +775,9 @@ function Immediate(callback, args) {
778775
this);
779776
}
780777

781-
if (scheduledImmediateCount[0] === 0)
778+
if (scheduledImmediateCountArray[0] === 0)
782779
activateImmediateCheck();
783-
scheduledImmediateCount[0]++;
780+
scheduledImmediateCountArray[0]++;
784781

785782
immediateQueue.append(this);
786783
}
@@ -826,7 +823,7 @@ exports.clearImmediate = function(immediate) {
826823
if (!immediate) return;
827824

828825
if (!immediate._destroyed) {
829-
scheduledImmediateCount[0]--;
826+
scheduledImmediateCountArray[0]--;
830827
immediate._destroyed = true;
831828

832829
if (async_hook_fields[kDestroy] > 0) {

src/env.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,7 @@ void Environment::CheckImmediate(uv_check_t* handle) {
309309

310310
MakeCallback(env->isolate(),
311311
env->process_object(),
312-
env->immediate_callback_string(),
312+
env->immediate_callback_function(),
313313
0,
314314
nullptr,
315315
{0, 0}).ToLocalChecked();

src/env.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,6 @@ class ModuleWrap;
158158
V(homedir_string, "homedir") \
159159
V(hostmaster_string, "hostmaster") \
160160
V(ignore_string, "ignore") \
161-
V(immediate_callback_string, "_immediateCallback") \
162161
V(infoaccess_string, "infoAccess") \
163162
V(inherit_string, "inherit") \
164163
V(input_string, "input") \
@@ -289,6 +288,7 @@ class ModuleWrap;
289288
V(http2ping_constructor_template, v8::ObjectTemplate) \
290289
V(http2stream_constructor_template, v8::ObjectTemplate) \
291290
V(http2settings_constructor_template, v8::ObjectTemplate) \
291+
V(immediate_callback_function, v8::Function) \
292292
V(inspector_console_api_object, v8::Object) \
293293
V(pbkdf2_constructor_template, v8::ObjectTemplate) \
294294
V(pipe_constructor_template, v8::FunctionTemplate) \

src/node.cc

-15
Original file line numberDiff line numberDiff line change
@@ -3169,12 +3169,6 @@ static void DebugEnd(const FunctionCallbackInfo<Value>& args);
31693169

31703170
namespace {
31713171

3172-
void ActivateImmediateCheck(const FunctionCallbackInfo<Value>& args) {
3173-
Environment* env = Environment::GetCurrent(args);
3174-
env->ActivateImmediateCheck();
3175-
}
3176-
3177-
31783172
void StartProfilerIdleNotifier(const FunctionCallbackInfo<Value>& args) {
31793173
Environment* env = Environment::GetCurrent(args);
31803174
env->StartProfilerIdleNotifier();
@@ -3399,12 +3393,6 @@ void SetupProcessObject(Environment* env,
33993393
FIXED_ONE_BYTE_STRING(env->isolate(), "ppid"),
34003394
GetParentProcessId).FromJust());
34013395

3402-
auto scheduled_immediate_count =
3403-
FIXED_ONE_BYTE_STRING(env->isolate(), "_scheduledImmediateCount");
3404-
CHECK(process->Set(env->context(),
3405-
scheduled_immediate_count,
3406-
env->scheduled_immediate_count().GetJSArray()).FromJust());
3407-
34083396
auto should_abort_on_uncaught_toggle =
34093397
FIXED_ONE_BYTE_STRING(env->isolate(), "_shouldAbortOnUncaughtToggle");
34103398
CHECK(process->Set(env->context(),
@@ -3536,9 +3524,6 @@ void SetupProcessObject(Environment* env,
35363524
env->as_external()).FromJust());
35373525

35383526
// define various internal methods
3539-
env->SetMethod(process,
3540-
"_activateImmediateCheck",
3541-
ActivateImmediateCheck);
35423527
env->SetMethod(process,
35433528
"_startProfilerIdleNotifier",
35443529
StartProfilerIdleNotifier);

src/timer_wrap.cc

+23
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,9 @@
2929
namespace node {
3030
namespace {
3131

32+
using v8::Array;
3233
using v8::Context;
34+
using v8::Function;
3335
using v8::FunctionCallbackInfo;
3436
using v8::FunctionTemplate;
3537
using v8::HandleScope;
@@ -67,11 +69,32 @@ class TimerWrap : public HandleWrap {
6769
env->SetProtoMethod(constructor, "stop", Stop);
6870

6971
target->Set(timerString, constructor->GetFunction());
72+
73+
target->Set(env->context(),
74+
FIXED_ONE_BYTE_STRING(env->isolate(), "setImmediateCallback"),
75+
env->NewFunctionTemplate(SetImmediateCallback)
76+
->GetFunction(env->context()).ToLocalChecked()).FromJust();
7077
}
7178

7279
size_t self_size() const override { return sizeof(*this); }
7380

7481
private:
82+
static void SetImmediateCallback(const FunctionCallbackInfo<Value>& args) {
83+
CHECK(args[0]->IsFunction());
84+
auto env = Environment::GetCurrent(args);
85+
env->set_immediate_callback_function(args[0].As<Function>());
86+
auto activate_cb = [] (const FunctionCallbackInfo<Value>& args) {
87+
Environment::GetCurrent(args)->ActivateImmediateCheck();
88+
};
89+
auto activate_function =
90+
env->NewFunctionTemplate(activate_cb)->GetFunction(env->context())
91+
.ToLocalChecked();
92+
auto result = Array::New(env->isolate(), 2);
93+
result->Set(0, activate_function);
94+
result->Set(1, env->scheduled_immediate_count().GetJSArray());
95+
args.GetReturnValue().Set(result);
96+
}
97+
7598
static void New(const FunctionCallbackInfo<Value>& args) {
7699
// This constructor should not be exposed to public javascript.
77100
// Therefore we assert that we are not trying to call this as a

test/common/index.js

+1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
/* eslint-disable required-modules, crypto-check */
2323
'use strict';
24+
const process = global.process; // Some tests tamper with the process global.
2425
const path = require('path');
2526
const fs = require('fs');
2627
const assert = require('assert');

test/parallel/test-timer-immediate.js

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
'use strict';
2+
const common = require('../common');
3+
common.globalCheck = false;
4+
global.process = {}; // Boom!
5+
setImmediate(common.mustCall());

0 commit comments

Comments
 (0)