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.mts 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.mts 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

[Renegade334]

This doesn't guarantee execution order either; that would require wait/notify.

[giulioAZ]

execution order is guaranteed (originalChdir is a blocking syscall that completes before AtomicsAdd executes). the Atomic provides a memory barrier for cross-thread.
the current fix (lines swapped) already ensures the cwdCounter only signals freshness after the directory actually changes. This s turning the race condition (workers cache stale data as current) into a safe eventual consistency (workers may be conservative but never trust stale data).
the test results confirm 0% error rate across the 832 races of the PoC, what race condition do you see remaining?

[addaleax]

@Renegade334 It's true that applications which rely on process.cwd() being accurate in the presence of multi-threaded chdir() calls do need to do their own synchronization to ensure that accuracy.

The fix here isn't about that; it's about the fact that before this patch, process.cwd() in a worker thread may cache the result at a point where the counter has already been increased but the chdir() syscall has not yet been executed, and then it will use that cached value indefinitely, esp. if process.chdir() is never called again.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.73%. Comparing base (36ca627) to head (7b093bf).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61664      +/-   ##
==========================================
+ Coverage   88.86%   89.73%   +0.87%     
==========================================
  Files         674      674              
  Lines      204394   204394              
  Branches    39188    39286      +98     
==========================================
+ Hits       181634   183417    +1783     
+ Misses      15014    13268    -1746     
+ Partials     7746     7709      -37     

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

... and 103 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.

[addaleax]

@Renegade334 It's true that applications which rely on process.cwd() being accurate in the presence of multi-threaded chdir() calls do need to do their own synchronization to ensure that accuracy.

The fix here isn't about that; it's about the fact that before this patch, process.cwd() in a worker thread may cache the result at a point where the counter has already been increased but the chdir() syscall has not yet been executed, and then it will use that cached value indefinitely, esp. if process.chdir() is never called again.

[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/