Skip to content
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

Stop using process.umask() #39

Closed
isaacs opened this issue Apr 3, 2020 · 2 comments
Closed

Stop using process.umask() #39

isaacs opened this issue Apr 3, 2020 · 2 comments
Labels
semver:patch semver patch level for changes

Comments

@isaacs
Copy link
Contributor

isaacs commented Apr 3, 2020

Refs: nodejs/node#32321

Summary: process.umask() (no args) will be deprecated and removed.

I couldn't quite divine what lib/config/defaults.js uses process.umask() for but in most cases you don't need to deal with the umask directly - the operating system will apply it automatically.

Example:

const mode = 0o777 & ~process.umask();
fs.mkdirSync(dir, mode);

Computing the file mode that way is superfluous, it can be replaced with just this:

fs.mkdirSync(dir, 0o777);

nodejs/node#32321 (comment)

From npm/bin-links#18:

Since this is only done after the file is created (and it is created without the knowledge that it will eventually need to be an executable script), we can't just rely on default file creation masking, since chmod isn't limited by that.

If we don't read process.umask, we risk making all executable files world-writable (or even just group-writable) which is a security risk.

As I can see it, the only way to avoid this would be to have pacote take note of executable file targets at unpack time, and create them with a 0o777 mode, regardless of what the archive entry says, and then also tell tar not to chmod them to 0o777 after creation.

Probably this will require a way to provide chmod:false to tar.Unpack anyway, so that pacote can just set the creation modes to 0o666/0o777 and ignore the specific mode found in the archive.

cc @bnoordhuis

@isaacs
Copy link
Contributor Author

isaacs commented Apr 3, 2020

For context, we definitely can't trust the mode in a package tarball to be accurate. Users expect that a file published in a package with mode 0o640 will come out the other end as 0o664 on linux systems with a umask of 02, and 0o644 when umask is 022. Windows doesn't have group/world file permissions as a concept, so entries are all either 0o777 or 0o666 in the published archives. Quite often, we see bin scripts published with non-executable permissions, so that also has to be fixed to have correct package behavior.

  1. Tar needs a way to say "do not chmod this entry, just create it with entry.mode and let the OS handle it". (A deviation from tar(1) behavior.)
  2. Then, pacote's tarx options.onentry method needs to set the entry mode to either 0o666 or 0o777, based on whether it is a regular file, or a directory or bin target.
  3. bin-links can then remove the fix-bins method, since all bin targets will have been created with executable permissions.
  4. Rather than rely on processUmask being set, if chmod:false is not set, then tar needs to create the file with default permissions, then stat it, and chmod if the mode does not match the archive entry. This will slow tar down a bit, but I don't see a way around that if process.umask() is not available, and at least we can avoid the hit for npm's usage.

@darcyclarke darcyclarke added this to the OSS - Sprint 16 milestone Oct 1, 2020
@darcyclarke darcyclarke added Release 7.x semver:patch semver patch level for changes labels Oct 1, 2020
@isaacs
Copy link
Contributor Author

isaacs commented Feb 17, 2021

Fixed 143d4ab

@isaacs isaacs closed this as completed Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch semver patch level for changes
Projects
None yet
Development

No branches or pull requests

3 participants