fix: eliminate TOCTOU Race Condition (Hackerone report #3407207)

[giulioAZ]

Summary

Fixes a Time-of-Check Time-of-Use race condition in worker thread process.cwd() caching where the counter is incremented before the directory change completes, allowing workers to cache stale directory values.

Description: The main thread's process.chdir() wrapper previously incremented the shared counter before calling the actual chdir(), creating a race window where workers could read the old directory but cache it with the new counter value. This caused subsequent cwd() calls to return incorrect paths until the next chdir().

Proof of Concept: included in the test/parallel/test-worker-cwd-race-condition.js and already reviewed and validated by Node.js security team here (https://hackerone.com/reports/3407207).

Fix: this fix reorders the operations to change the directory first, then increment the counter, ensuring workers are only notified after the directory change completes.

CVSS 3.0: 3.6 (Low) - AV:L/AC:L/PR:L/UI:N/S:U/C:L/I:L/A:N

---

Problem

In lib/internal/worker.js, the main thread's process.chdir() wrapper increments the shared counter before calling the actual chdir():

process.chdir = function(path) {
  AtomicsAdd(cwdCounter, 0, 1);  // Signal workers first
  originalChdir(path);            // Change directory second
};

This creates a race window where:

Counter increments (workers see "cwd changed")
Worker calls cwd() during this window
Worker reads old directory but caches it with new counter
Directory actually changes
Worker's cache is now stale and persists until next chdir()

Race Window:

┌───────
│ Step   │ [Main Thread]          │ [Worker Thread]              │
├────────
│ Step 1 │ AtomicsAdd(counter, 1) │ → counter = 5                │
│ Step 2 │                        │ AtomicsLoad(counter) → 5     │
│ Step 3 │                        │ cachedCwd = cwd() → /old     │
│ Step 4 │ originalChdir("/new")  │ → CWD = /new                 │
├──────
│ Result │ CWD is /new            │ Cached /old with counter=5  (Wrong)│
└───────
Once cached, subsequent cwd() calls return the stale value:
if (currentCounter === lastCounter) // 5 === 5 ✓
  return cachedCwd; // Returns "/old" when CWD is actually "/new"

Solution

Swap the order - change directory first, then increment counter:

process.chdir = function(path) {
  originalChdir(path);            // Change directory first
  AtomicsAdd(cwdCounter, 0, 1);  // Signal workers second
};

This ensures workers are only notified after the directory change completes, transforming the race into safe eventual consistency.

After Fix:

┌───────
│ Step   │ [Main Thread]          │ [Worker Thread]            │
├────────────
│ Step 1 │ originalChdir("/new")  │ → CWD = /new               │
│ Step 2 │                        │ AtomicsLoad(counter) → 5   │
│ Step 3 │                        │ cachedCwd = cwd() → /new   │
│ Step 4 │ AtomicsAdd(counter, 1) │ → counter = 6              │
├─────────
│ Result │ CWD is /new            │ Cached /new (Correct)    │

Impact

• Affected applications: Multi-threaded Node.js applications using process.chdir() with worker threads, including:
  • Build systems processing multiple projects in parallel
  • CI/CD platforms executing concurrent build jobs
  • Multi-tenant services using directory-based isolation
  • Applications without container or filesystem namespace separation
• Confidentiality: Workers may read files from unintended directories
• Integrity: File operations (read/write) may target wrong directory context
• Severity: Low (CVSS 3.6) - requires specific usage patterns by the application and concurrent execution
• Exploitability: Race window is 1-10 microseconds but consistently reproducible in testing

Proof of Concept

Tested with test/parallel/test-worker-cwd-race-condition.js on Node.js v25.0.0:

Before fix:
Test case: real process
cwd in worker thread reflected stale value
errors: 54.28% (311/573 races)
After fix:
Test case: fixed process
cwd in worker thread always had expected value
errors: 0% (0/832 races)

Performance Impact

Negligible: same atomic operations, just reordered:
Cache hit rate: Unchanged
Latency: Workers may cache fresh data slightly longer (safer)
Thread safety: Maintained via atomic operations

Related

Original implementation: commit 1d022e8 (April 14, 2019, PR #27224)
CWE-367: Time-of-Check Time-of-Use (TOCTOU) Race Condition
CVSS 3.0: 3.6 (Low) - AV:L/AC:L/PR:L/UI:N/S:U/C:L/I:L/A:N
HackerOne Report: https://hackerone.com/reports/3407207

Credits

Giulio Comi, Caleb Everett, Utku Yildirim

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.75%. Comparing base (36ca627) to head (180906e).
⚠️ Report is 55 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61664      +/-   ##
==========================================
+ Coverage   88.86%   89.75%   +0.88%     
==========================================
  Files         674      675       +1     
  Lines      204394   204628     +234     
  Branches    39188    39327     +139     
==========================================
+ Hits       181634   183662    +2028     
+ Misses      15014    13239    -1775     
+ Partials     7746     7727      -19     

Files with missing lines	Coverage Δ
lib/internal/worker.js	96.52% <100.00%> (ø)

... and 123 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[addaleax]

LGTM

Please fix the first line of the commit message in line with commit message guidelines, if possible (e.g. worker: eliminate race condition in process.cwd()), and feel free to take as much context from the Problem/Solution sections of the PR description and add it to the commit message body itself.

Calling this a vulnerabillity in Node.js would go a bit far, I think. It's a race condition, but as mentioned in the comments here, if an application relies on the specific relative timing of process.chdir() in the main thread and process.cwd() in the worker thread, it should already have implemented its own synchronization logic for that.

In other words, if this does result in a vulnerability in an application that doesn't explicitly synchronize already, then that application will likely still be vulnerable after this fix.

[giulioAZ]

@addaleax , thanks fixed commit title and integrated context from the PR into commit message.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/71200/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/71201/

[Renegade334]

Thanks for the PR!

Change is good, but the tests do need addressing.

This is a very large test burden for a very small behaviour. For me, this single test is taking around 10s to run, at 130% CPU usage.

As general points:

• There's no advantage to adding test files in TS rather than JS, unless it's Node.js's TS support itself that's being tested. There's no type validation here, it all just gets stripped, and it requires the additional burden of "transpiling" the test script to JS every time it's run.
• Adding mock proofs-of-concept to the test suite isn't useful; they can be illustrative in a PR description, but it's only the actual API behaviour that needs testing.

To test this change, you don't need to pummel workers to try and force race conditions. It's possible to wrap the internal chdir method (the one called as originalChdir() in internal/worker) to directly simulate a worker calling process.cwd() during a slow chdir syscall.

Something along the lines of the following would do the trick:

test/parallel/test-worker-cwd-race-condition.js

// Flags: --expose-internals --no-warnings

'use strict';

const common = require('../common');
const { internalBinding } = require('internal/test/binding');

const assert = require('assert');
const { Worker } = require('worker_threads');

const processBinding = internalBinding('process_methods');
const originalChdir = processBinding.chdir;

const cwdOriginal = process.cwd();
const i32 = new Int32Array(new SharedArrayBuffer(12));

processBinding.chdir = common.mustCall(function chdir(path) {
  // Signal to the worker that we're inside the chdir call
  Atomics.store(i32, 0, 1);
  Atomics.notify(i32, 0);

  // Pause the chdir call while the worker calls process.cwd(),
  // to simulate a race condition
  Atomics.wait(i32, 1, 0);

  return originalChdir(path);
});

const worker = new Worker(`
  const {
    parentPort,
    workerData: { i32 },
  } = require('worker_threads');

  // Wait until the main thread has entered the chdir call
  Atomics.wait(i32, 0, 0);

  const cwdDuringChdir = process.cwd();

  // Signal the main thread to continue the chdir call
  Atomics.store(i32, 1, 1);
  Atomics.notify(i32, 1);

  // Wait until the main thread has left the chdir call
  Atomics.wait(i32, 2, 0);

  const cwdAfterChdir = process.cwd();
  parentPort.postMessage({ cwdDuringChdir, cwdAfterChdir });
`, {
  eval: true,
  workerData: { i32 },
});

worker.on('exit', common.mustCall());
worker.on('error', common.mustNotCall());
worker.on('message', common.mustCall(({ cwdDuringChdir, cwdAfterChdir }) => {
  assert.strictEqual(cwdDuringChdir, cwdOriginal);
  assert.strictEqual(cwdAfterChdir, process.cwd());
}));

process.chdir('..');

// Signal to the worker that the chdir call is completed
Atomics.store(i32, 2, 1);
Atomics.notify(i32, 2);

[giulioAZ]

@Renegade334 , thank you for the feedback on the tests!
i have now committed (180906e) the new .js test file according to your great example

[Renegade334]

Thanks for this!

[Renegade334]

Fails on main as expected, with the post-race cwd() call returning a stale value.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/71283/

[giulioAZ]

i get Merging is blocked - You're not authorized to push to this branch.

@Renegade334 / @addaleax would you mind merging on my behalf, please?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/71293/

[richardlau]

The new test is failing when run as a worker (i.e. tools/test.py --worker parallel/test-worker-cwd-race-condition or out/Release/node --expose-internals --no-warnings tools/run-worker.js test/parallel/test-worker-cwd-race-condition.js):

e.g. https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/45886/console

16:25:20 not ok 4405 parallel/test-worker-cwd-race-condition
16:25:20   ---
16:25:20   duration_ms: 214.34200
16:25:20   severity: fail
16:25:20   exitcode: 1
16:25:20   stack: |-
16:25:20     
16:25:20     node:internal/event_target:1118
16:25:20       process.nextTick(() => { throw err; });
16:25:20                                ^
16:25:20     TypeError [ERR_WORKER_UNSUPPORTED_OPERATION]: process.chdir() is not supported in workers
16:25:20         at process.unavailableInWorker [as chdir] (node:internal/process/worker_thread_only:14:11)
16:25:20         at Object.<anonymous> (/home/iojs/build/workspace/node/test/parallel/test-worker-cwd-race-condition.js:60:9)
16:25:20         at Module._compile (node:internal/modules/cjs/loader:1811:14)
16:25:20         at Object..js (node:internal/modules/cjs/loader:1942:10)
16:25:20         at Module.load (node:internal/modules/cjs/loader:1532:32)
16:25:20         at Module._load (node:internal/modules/cjs/loader:1334:12)
16:25:20         at wrapModuleLoad (node:internal/modules/cjs/loader:255:19)
16:25:20         at Module.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:154:5)
16:25:20         at MessagePort.<anonymous> (node:internal/main/worker_thread:223:26)
16:25:20         at [nodejs.internal.kHybridDispatch] (node:internal/event_target:843:20) {
16:25:20       code: 'ERR_WORKER_UNSUPPORTED_OPERATION'
16:25:20     }
16:25:20     
16:25:20     Node.js v26.0.0-pre
16:25:20   ...

The usual way we handle this is, e.g.

node/test/parallel/test-process-chdir.js

Lines 7 to 11 in 4dc0d20

	const { isMainThread } = require('worker_threads');
	if (!isMainThread) {
	common.skip('process.chdir is not available in Workers');
	}

[Renegade334]

@giulioAZ are you able to add the linked skip logic to the test?