Skip to content

/ node

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

worker: file creation races with process.umask() #32321

Closed
bnoordhuis opened this issue Mar 17, 2020 · 10 comments
Closed

worker: file creation races with process.umask() #32321

bnoordhuis opened this issue Mar 17, 2020 · 10 comments
Labels
fs process worker

Comments

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Mar 17, 2020

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
Copy link
Member

@addaleax addaleax commented Mar 20, 2020

Fwiw, this seems problematic even without worker threads, because it also affects fs operations running on the libuv thread pool?
@bnoordhuis
Copy link
Member Author

@bnoordhuis bnoordhuis commented Mar 20, 2020

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.)
@cjihrig cjihrig mentioned this issue Mar 26, 2020
Merged
3 of 3 tasks complete
@cjihrig cjihrig closed this in 75ee5b2 Apr 1, 2020
@bnoordhuis
Copy link
Member Author

@bnoordhuis bnoordhuis commented Apr 1, 2020

I'm reopening this. It's deprecated now but the goal is to remove it entirely.
@bnoordhuis bnoordhuis reopened this Apr 1, 2020
@sam-github sam-github closed this Apr 1, 2020
@addaleax addaleax reopened this Apr 1, 2020
@addaleax
Copy link
Member

@addaleax addaleax commented Apr 2, 2020

Fwiw, at least:

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.
@addaleax addaleax added the process label Apr 2, 2020
This was referenced Apr 3, 2020
Closed
Closed
Open
@bnoordhuis
Copy link
Member Author

@bnoordhuis bnoordhuis commented Apr 3, 2020

I filed issues with those projects.
@isaacs
Copy link
Contributor

@isaacs isaacs commented Apr 3, 2020

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 added a commit to isaacs/node-mkdirp that referenced this issue Apr 3, 2020
@isaacs
Remove unnecessary process.umask()
2d42170 
Fix #22
Re nodejs/node#32321
isaacs added a commit to isaacs/node-mkdirp that referenced this issue Apr 3, 2020
@isaacs
Remove unnecessary umask calls
bea6382 
Legacy fix for #22
Re nodejs/node#32321
This was referenced Apr 3, 2020
Open
Open
Open
@isaacs
Copy link
Contributor

@isaacs isaacs commented Apr 3, 2020

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.
bnoordhuis added a commit to bnoordhuis/v8-compile-cache that referenced this issue Apr 10, 2020
@bnoordhuis
Stop using process.umask()
db22964 
It's deprecated in the latest Node.js due to being insecure and
it's not necessary to use it in the first place because the umask
is automatically applied when creating the directory.

Refs: nodejs/node#32321
Refs: isaacs/node-mkdirp#22
@bnoordhuis bnoordhuis mentioned this issue Apr 10, 2020
Merged
coreyfarrell added a commit to coreyfarrell/fs-mkdirp-stream that referenced this issue Apr 22, 2020
@coreyfarrell
Stop using `process.umask()`
Verified
This commit was signed with a verified signature.
coreyfarrell Corey Farrell
GPG key ID: D1B6E98EB07F7F6C Learn about signing commits
52aba53 
Ref nodejs/node#32321
@coreyfarrell coreyfarrell mentioned this issue Apr 22, 2020
Merged
@aduh95 aduh95 mentioned this issue Apr 23, 2020
Closed
coreyfarrell added a commit to coreyfarrell/fs-mkdirp-stream that referenced this issue May 4, 2020
@coreyfarrell
Stop using `process.umask()`
Verified
This commit was signed with a verified signature.
coreyfarrell Corey Farrell
GPG key ID: D1B6E98EB07F7F6C Learn about signing commits
eb41013 
Ref nodejs/node#32321
coreyfarrell added a commit to coreyfarrell/fs-mkdirp-stream that referenced this issue May 4, 2020
@coreyfarrell
Stop using `process.umask()`
Verified
This commit was signed with a verified signature.
coreyfarrell Corey Farrell
GPG key ID: D1B6E98EB07F7F6C Learn about signing commits
f345cf3 
Ref nodejs/node#32321
@BridgeAR BridgeAR pinned this issue May 9, 2020
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented May 9, 2020

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.
phated added a commit to gulpjs/fs-mkdirp-stream that referenced this issue May 10, 2020
@coreyfarrell @phated
Breaking: Stop using `process.umask()` & fallback to node default mode (
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
d4eeaae 
#6)

ref nodejs/node#32321

Co-authored-by: Blaine Bublitz <blaine.bublitz@gmail.com>
zertosh pushed a commit to zertosh/v8-compile-cache that referenced this issue May 30, 2020
@bnoordhuis
Stop using process.umask() (#28)
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a3a45cd 
It's deprecated in the latest Node.js due to being insecure and
it's not necessary to use it in the first place because the umask
is automatically applied when creating the directory.

Refs: nodejs/node#32321
Refs: isaacs/node-mkdirp#22
@AhmedMostafa16
Copy link

@AhmedMostafa16 AhmedMostafa16 commented Jun 12, 2020

Concurrency Sanitizer may spot race conditions.
@bnoordhuis
Copy link
Member Author

@bnoordhuis bnoordhuis commented Jun 14, 2020

@AhmedMostafa16 Not this kind of race condition.
aduh95 added a commit to aduh95/node that referenced this issue Aug 1, 2020
@cjihrig @aduh95
doc: deprecate process.umask() with no arguments
c21875b 
This commit introduces a documentation deprecation for calling
process.umask() with no arguments.

PR-URL: nodejs#32499
Fixes: nodejs#32321
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95 aduh95 mentioned this issue Aug 1, 2020
Closed
3 of 3 tasks complete
MylesBorins added a commit that referenced this issue Aug 18, 2020
@cjihrig @MylesBorins
doc: deprecate process.umask() with no arguments
Verified
This commit was signed with a verified signature.
MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
b7fb94d 
This commit introduces a documentation deprecation for calling
process.umask() with no arguments.

Backport-PR-URL: #34591
PR-URL: #32499
Fixes: #32321
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Sep 29, 2020
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
fs process worker
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

deprecate process.umask() with no arguments
5 participants
@isaacs @bnoordhuis @addaleax @BridgeAR @AhmedMostafa16
You can’t perform that action at this time.