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

This package appears to have a TOCTOU bug #14

Closed
hacksk opened this Issue Jul 2, 2017 · 23 comments

Comments

Projects
None yet
@pravi

This comment has been minimized.

Copy link

pravi commented Oct 22, 2017

@isaacs can you clarify? This will help us in updating npm package in debian. cacache depends on this module.

@simevo

This comment has been minimized.

Copy link

simevo commented Jan 10, 2018

@isaacs

This comment has been minimized.

Copy link
Owner

isaacs commented Jan 10, 2018

The *at methods suggested in the Debian bug report are not exposed in Node.js, so that's not a solution.

Patch welcome to address this. However, I suspect that any recursive file modification in Node that intends to not follow symbolic links will suffer from this sort of loophole. The lstat is a best effort attempt to avoid following symbolic links, but yes, a well-timed file system update can cause it to perform in unexpected ways.

simevo pushed a commit to simevo/chownr that referenced this issue Jan 11, 2018

@simevo

This comment has been minimized.

Copy link

simevo commented Jan 11, 2018

Mmmh turns out that fs.lchown is deprecated (why??), besides it is not available at all on linux !

When I tried on debian sid with nodejs v8.9.3 I got:

not ok fs.lchown is not a function

so it seems my proposed fix is not viable ATM.

Apart from that, it would be cool to have @jepler at least review it by inspection !

@jepler

This comment has been minimized.

Copy link

jepler commented Jan 11, 2018

I agree, with the primitives provided in node fs it doesn't seem to be able to safely do operations like these on systems where an adversarial user can create symlinks within the hierarchy being operated on. Besides which, I failed to find any npm packages which provide the *at family of functions, so there may not even be an alternative you can depend on to help you resolve the problem. I am sorry I don't have any positive suggestions to offer.

@simevo

This comment has been minimized.

Copy link

simevo commented Jan 12, 2018

Fine then I propose that we escalate this to the nodejs issue tracker. As it stands now the nodejs platform makes it impossible for module devs to protect users against time of check to time of use bugs.

@simevo

This comment has been minimized.

Copy link

simevo commented Jun 27, 2018

libuv 1.2.1 now has lchown
it should be usable from nodejs master and land in 10.x soon
so the patch I proposed could become viable

@spencern

This comment has been minimized.

Copy link

spencern commented Aug 5, 2018

The snyk.io vulnerability testing tool has started to report this issue for any package that depends on chownr

https://snyk.io/vuln/npm:chownr:20180731

spencern added a commit to reactioncommerce/reaction that referenced this issue Aug 5, 2018

fix: add chownr to snyk ignore file
Chownr has a recently reported issue to snyk, though the issue itself
has been known for over a year. isaacs/chownr#14
This issue has been introduced via sharp via tar and tar-fs npm packages

We'll continue to follow this issue on the chownr repo. And I've added
a comment to the issue thread.
isaacs/chownr#14 (comment)
@EightArmCode

This comment has been minimized.

Copy link

EightArmCode commented Aug 9, 2018

@simevo any news on the status of your patch?

@VPenkov

This comment has been minimized.

Copy link

VPenkov commented Aug 10, 2018

I'm also interested in this, the package is so popular that NPM itself depends on it.
Any suggestions how to remove it from vulnerability lists such as Snyk's would be appreciated.

simevo pushed a commit to simevo/chownr that referenced this issue Aug 10, 2018

Paolo Greppi
use fs.lchown rather than fs.chown and thereby fix isaacs#14
fixes the symlinks problem isaacs#3 while not causing the TOCTOU vulnerability isaacs#14

The [patch in libuv 1.21.0](https://github.com/libuv/libuv/releases/tag/v1.21.0) that undeprecates `fs.lchown` [has been incorporated in nodejs Version 10.6.0](https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V10.md#2018-07-04-version-1060-current-targos).

So I specified the minimum nodejs version in `package.json` with the `engine` key: https://docs.npmjs.com/files/package.json#engines
@morkro

This comment has been minimized.

Copy link

morkro commented Aug 28, 2018

What is the status on this?

@simevo

This comment has been minimized.

Copy link

simevo commented Aug 28, 2018

see my open PR #15 which requires nodejs > 10.6
CI tests don't pass since original tests are failing too

@joelataylor

This comment has been minimized.

Copy link

joelataylor commented Aug 28, 2018

Looking forward to a patch on the vulnerability

@artakvg

This comment has been minimized.

Copy link

artakvg commented Sep 9, 2018

Hope the patch will appear soon

@TimeBomb

This comment has been minimized.

Copy link

TimeBomb commented Sep 14, 2018

We develop software used by enterprises. This issue is a potential showstopper for us from using this module or modules that leverage this component, e.g. pm2.

I appreciate @simevo's efforts and would love to see the project maintainers give some priority to getting a patch for this.

@benwiggins

This comment has been minimized.

Copy link

benwiggins commented Sep 15, 2018

The silence around a year-plus old security issue is pretty disappointing and @simevo's work is greatly appreciated — major kudos for getting lchown into libuv.

But dropping compatibility with Node versions prior to 10.6 is not exactly a small undertaking or even feasible in some cases. Admittedly it's not far away, but 10.x is not even in Active LTS yet and neither 8.x or even 6.x have reached End-of-life.

Even if the patch was merged today, such a breaking compatibility change would necessitate a major semver bump. That isn’t going to magically propagate its way up the dependency chain. And each package maintainer then has to be willing to upgrade and break their compatibility. That’s six major, backwards-incompatible releases to get a patched chownr into pm2.

Maybe a better option in the short term would be to decide at runtime to use fs.lchown wherever possible (Node 10.6+ for sure, but maybe also ignore the deprecation on earlier versions when running under MacOS?) but fallback to fs.chown. But I think for all intents and purposes chownr would probably still be considered a vulnerable package by Snyk etc. in that case.

@simevo

This comment has been minimized.

Copy link

simevo commented Sep 15, 2018

Thanks for the kudos, I'd like to send a fair share of those to the fellow debian people @jepler, @hacksk and @pravi who discovered this and reported it here.

This is a good example of fruitful collaboration between the nodejs and debian opensource ecosystems, notwithstanding the differing approaches to module/package dependencies.

Now back to the point, nodejs 6.x has End-of-Life 2019-04, and 8.x has EoL 2019-12; does it make sense to backport the libuv 1.2.1 fixes on top of 1.16.1 for 6.x and on top of 1.19.2 for 8.x ?

@isaacs

This comment has been minimized.

Copy link
Owner

isaacs commented Sep 16, 2018

I just published 1.1.0, which uses lchown when available, and also reduces the stat calls on node v10.10.0, where stat data can be included in the readdir call.

However, as there is no readdir that will succeed on actual directories, and fail on symlinks to directories, there is still a problem that I don't see a way to address. So the TOCTOU is smaller, but not gone.

The basic flow would be:

  1. Read a directory, get a list of items
  2. One of those items is a directory
  3. After the initial readdir (or readdir+lstat), but before the directory traversal, an attacker moves the directory aside and replaces it with a symbolic link to some other directory.
  4. The script will proceed to change ownership of all items in the symlink target directory.

Unless Node.js gains the ability to call fs.readdir with an option to fail if the directory is a symbolic link, I don't see a way around this. As far as I know, that capacity does not exist in readdir(3), so this is probably not possible.

My advice is to not run chownr on untrusted systems. For the vast majority of cases, this isn't a problem, since it requires an attacker having access to the filesystem where the user script is running chown, and in that case, this TOCTOU is the least of your worries.

@webdevbyjoss

This comment has been minimized.

Copy link

webdevbyjoss commented Jan 2, 2019

Hello from 2019!! I've got the TOCTOU security alert from Snyk with the following packages chain:
react-fullstory-component@2.1.0 › fsevents@1.2.4 › node-pre-gyp@0.10.3 › tar@4.4.8 › chownr@1.1.1

As the "chownr" isn't used directly here. What is the typical action plan in this case except temporary muting the alert with snyk ignore or get rid of the whole vulnerable dependency chain?

@pravi

This comment has been minimized.

Copy link

pravi commented Jan 2, 2019

@webdevbyjoss use nodejs >= 10.10 and chownr >= 1.1.0 as per #14 (comment)

@dr3

This comment has been minimized.

Copy link

dr3 commented Jan 4, 2019

@pravi I am still getting the TOCTOU Snyk alert with nodejs >= 10.10 and chownr >= 1.1.0

✗ Medium severity vulnerability found in chownr
  Description: Time of Check Time of Use (TOCTOU)
  Info: https://snyk.io/vuln/npm:chownr:20180731
  Introduced through: speculate@1.7.4, compression-webpack-plugin@2.0.0, razzle@2.4.0, brotli-webpack-plugin@1.0.0
  From: speculate@1.7.4 > tar-fs@1.16.3 > chownr@1.1.1
  From: compression-webpack-plugin@2.0.0 > cacache@11.3.1 > chownr@1.1.1
  From: razzle@2.4.0 > uglifyjs-webpack-plugin@1.2.4 > cacache@10.0.4 > chownr@1.1.1
  and 8 more...

The vulnerability still appears to be in v1.1.1

@pravi

This comment has been minimized.

Copy link

pravi commented Jan 4, 2019

@dr3 you should report it to Snyk.

@isaacs

This comment has been minimized.

Copy link
Owner

isaacs commented Jan 4, 2019

As I mentioned on 2018-09-15, there is no readdir(3) call that will succeed on a "real" directory, but fail on a symbolic link to a directory. What that means is that there is no atomic way to verify that, at the exact time of readding a directory, it's a real directory and not a symlink to somewhere else.

That being the case there will always be a TOCTOU issue for any recursive filesystem operation that traverses directories making changes at each level. No exceptions. The same TOCTOU issue exists for chown -r, rm -rf, chmod -r, etc.

The additional TOCTOU issue (using chown instead of lchown) is fixed on v1.1.0, and the second TOCTOU issue (relying on additional lstat calls) has been fixed in v1.1.0 when using Node.js 10.10 or higher. But the final one, "readdir follows symlinks" is unavoidable.

Since this particular remaining TOCTOU issue is inherent in any recursive filesystem mutation, and this is a module designed specifically to do a specific recursive filesystem mutation, I'd say you're as safe as you can be. It's not a problem with this module, but inherent in the operation. If you wrote it yourself, you'd have the same problem. If it's an issue in your situation, then don't use chownr, and instead call fs.lchown yourself.

I'd suggest that anyone sent to this issue via snyk please report this to them, or ask them to contact me if they'd like more information. I'm not a customer of theirs, so I don't want to bother their support folks with this issue myself.

Repository owner locked as resolved and limited conversation to collaborators Jan 4, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.