worker: file creation races with process.umask()

[bnoordhuis]

This code is unsafe when worker threads are active:

node/src/node_process_methods.cc

Lines 248 to 249 in 40b559a

	old = umask(0);
	umask(static_cast<mode_t>(old));

The umask(0) call temporarily changes the process-wide umask and races with fs operations from other threads.

Test case:

'use strict';
const { Worker, isMainThread } = require('worker_threads');
const { statSync, writeFileSync, unlinkSync } = require('fs');

function pummel() {
  for (let i = 0; i < 1e4; i++) process.umask();
  setImmediate(pummel);
}

if (isMainThread) {
  process.umask(0o22);
  new Worker(__filename);
  pummel();
} else {
  const file = 'x.txt';
  for (;;) {
    writeFileSync(file, 'ok', { mode: 0o666 });
    const s = statSync(file);
    s.mode &= 0o777;
    if (0o644 !== s.mode) throw 'unexpected mode: ' + s.mode.toString(8);
    unlinkSync(file);
  }
}

Fails within a few iterations with unexpected mode: 666

process.umask() (no arg) is allowed in workers so this test case works both ways.

This bug is potentially a security issue.

[addaleax]

Fwiw, this seems problematic even without worker threads, because it also affects fs operations running on the libuv thread pool?

[bnoordhuis]

Yes, that's right. I could swear there was a warning about that in the documentation at one point (at least about changing the umask) but not anymore.

IMO, process.umask() (no arg) is fundamentally broken and insecure without any way of making it safe. I think it's best to deprecate and remove it posthaste.

process.umask(mode) is less irredeemably broken but still hard to use securely the moment more than one thread is running (which is basically always.)

[bnoordhuis]

I'm reopening this. It's deprecated now but the goal is to remove it entirely.

[addaleax]

Fwiw, at least:

• https://github.com/sindresorhus/make-dir
• https://github.com/isaacs/node-mkdirp
• https://github.com/npm/cli/blob/latest/lib/config/defaults.js

use this function without arguments (I didn’t even try to find anything systematically). I think part of that usage is unnecessary, but we should probably make sure that ecosystem breakage is minimal before #32499 is released, and take a lot of time before removing this entirely.

[bnoordhuis]

I filed issues with those projects.

[isaacs]

Hmm... this is a little bit tricky in the case of node-tar. I think there's a workaround, but wanted to float this here just to see what y'all think.

Tar archive entries can contain a mode field. If a file entry has a mode of 0o664 in the archive, but the process umask is 0o022, then it will be created with a mode of 0o644. In order to follow the behavior of tar(1), it needs to chmod files that are created to the mode in the archive. For efficiency, only does so if it expects that the file will not already be created with the correct mode. (Ie, if mode & umask is not zero.)

So, we check the umask at the start of the unpack process to know whether a chmod will be necessary, to avoid an extra stat call during the unpack process. Of course, this is not 100% bulletproof, since the umask can change along the way, but archive unpacking is never going to be safely atomic anyway.

If process.umask() is going away, then it looks like I'll have to check the mode of every file and folder post-creation, which adds a lot of extra stat calls. Or, just always chmod everything, which is potentially even worse (since the chmod is usually unnecessary, and more expensive than a stat).

Whichever version of Node loses process.umask() entirely, will cease to support all current versions of npm (as of today). It would be wise to have updates for npm in place well ahead of time, if this is landing any time in the next year or so.

[isaacs]

Created issues for the npm deps relying on process.umask(). I think we can avoid the perf hit in most of npm's use cases, but it is going to be a bit of careful fiddling to make sure we're unpacking correctly without creating world-writable executables.

I strongly recommend holding off on this until the linked issues above are resolved.

[BridgeAR]

I pinned this issue to keep track of modules that use process.umask() that should be fixed.

A first step would be to create a list of known commonly used modules with CITGM and Gzemnid.

[AhmedMostafa16]

Concurrency Sanitizer may spot race conditions.

[bnoordhuis]

@AhmedMostafa16 Not this kind of race condition.

[miyaokamarina]

I’m working on a programmatic version of the chmod command with symbolic notation (like in ShellJS, but asynchronous and without its limitations), and it requires the process.umask() with no arguments or another way to read a umask.

When a symbolic mode argument without scope is provided, the function should apply a umask to the requested value. For example we have the 022 umask and the +w mode; it will result in something like:

let requested = 0o222; // +w
let masked = requested & ~umask;
let final = existing | masked;

Without umask reading the implementation will be incomplete and may lead to unexpected results to anyone who expects it’s completely aligned with the chmod command and didn’t read the LIMITATIONS section in docs carefully.

Deprecating process.umask() with no arguments both at the docs and runtime level still seems reasonable, but it would be great if Node will provide another method for cases where umask reading is critical; for example process.unsafeGetUmask().

Another point is process.umask() or process.unsafeGetUmask() with no arguments may use the most common umask 022 instead of 0 as its temporary value. It doesn’t solve the problem, but narrows vulnerability surface to the cases when a process has a umask other than 022.

[kgryte]

I agree with @miyaokamarina, having a way to get the current umask is useful, especially when wanting to provide APIs for setting the umask via symbolic notation.

Aliasing process.umask() under an "unsafe" API similar to how Node.js handled deprecating particular uses of the Buffer constructor would be reasonable.