Making files/directories world writable (and readable) does matter #14

Closed
p opened this Issue Dec 19, 2012 · 8 comments

Comments

Projects
None yet
2 participants

p commented Dec 19, 2012

https://github.com/isaacs/rimraf/blob/master/rimraf.js

// make file writable
// user/group/world, doesn't matter at this point
// since it's about to get nuked.

If you make a large directory world writable, it will stay world writable for a significant period of time. At the very least this is an easy DOS. Another user account can create a file in it that the original process will be unable to remove.

Then, you don't just make the directory writable but also world readable. This is an easy information disclosure vulnerability.

Owner

isaacs commented Dec 19, 2012

This is only done if the directory was not writable by the current user/group in the first place. I don't see where it's made world-readable.

I suppose another approach would be to chmod it to 0700, and also chown it to the current uid, but that won't work on windows, and is 2 syscalls instead of 1.

p commented Dec 19, 2012

Indeed I misread the code, readable bit does not appear to be set as you said.

Therefore the scope of vulnerability is reduced to denial of service and code execution (by creating files that do something meaningful, which will not be removable by the original application and therefore survive the recursive removal operation).

There is a reason why rmdir and chmod/chown are distinct operations. You do not know a site's security policy and whether a given permission bit may be turned on on a particular file/directory.

if what you claim to do is "A rm -rf for node.", you should do that and only that.

Owner

isaacs commented Dec 19, 2012

rm -rf removes non-writable files. How do you suggest that I accomplish this without modifying the writable bit?

If you want to call rmdir, you can do that easily, and you don't need rimraf for it. require("fs").rmdir(dir, cb)

Do you have a specific suggestion? Can you express your suggestion in the form of a failing test and a pull request?

p commented Dec 20, 2012

rm -rf removes non-writable files. How do you suggest that I accomplish this without modifying the writable bit?

pie@reactor ~ % mkdir test
pie@reactor ~ % chmod 000 test
pie@reactor ~ % rm -rf test
rm: test: Permission denied

You need correct permissions for rm to work.

If you want to call rmdir, you can do that easily, and you don't need rimraf for it.

Obviously I will not use software that I consider insecure. However there are projects/people who may use rimraf without inspecting its source code, trusting that all it does is the equivalent of rm -rf like its description claims.

Do you have a specific suggestion?

Delete all calls to chmod from the code.

If you want to have a recursive chmod, a better place for it might be https://github.com/jprichardson/node-fs-extra.

Can you express your suggestion in the form of a failing test and a pull request?

I can write the equivalent of the unix rm test I quoted above into a test.

On which note, you need read bit to traverse a directory. As rimraf does not set the read bit it should be unable to delete directories with 000 permissions currently. If you add the read bit to your chmod call the information disclosure vulnerability trivially appears.

Owner

isaacs commented Dec 20, 2012

On which note, you need read bit to traverse a directory.

No, you also need the +x to traverse a directory. (The x bit for dirs is list, not execute. Those POSIX.1 guys really packed the semantics in there.) The readable bit is also needed, but not enough.

$ mkdir foo

$ touch foo/bar

$ ls -laF foo
total 0
drwxr-xr-x   3 isaacs  staff   102 Dec 20 22:28 ./
drwxr-xr-x  64 isaacs  staff  2176 Dec 20 22:28 ../
-rw-r--r--   1 isaacs  staff     0 Dec 20 22:28 bar

$ chmod 0444 foo # r, no x

$ ls -laF foo
ls: .: Permission denied
ls: ..: Permission denied
ls: bar: Permission denied

$ node -pe 'require("fs").readdirSync("foo")'
[ 'bar' ]

$ node -pe 'require("fs").statSync("foo/bar")'

fs.js:524
  return binding.stat(pathModule._makeLong(path));
                 ^
Error: EACCES, permission denied 'foo/bar'
    at Object.fs.statSync (fs.js:524:18)
    at [eval]:1:15
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:449:26)
    at evalScript (node.js:282:25)
    at startup (node.js:76:7)
    at node.js:627:3

$ chmod 0111 foo # x, no r

$ ls -laF foo
ls: foo: Permission denied

$ chmod 0555 foo # rx

$ ls -laF foo # works
total 0
dr-xr-xr-x   3 isaacs  staff   102 Dec 20 22:28 ./
drwxr-xr-x  64 isaacs  staff  2176 Dec 20 22:28 ../
-rw-r--r--   1 isaacs  staff     0 Dec 20 22:28 bar

It looks like indeed rimraf is doing more than rm -rf does, and strictly more than it needs to do. Either the chmod should be removed entirely, or changed to 0555.

I'm tempted to just remove it, because removing syscalls is usually a good idea, and this is only there for some edge case I can't remember and probably don't have to worry about. I'll do that, and if anyone complains then we can make it a configurable option.

isaacs closed this in 839158a Dec 20, 2012

Owner

isaacs commented Dec 20, 2012

Also removed the stats. Published in v2.1.0, pushed to master. Thanks for the nudge.

p commented Dec 20, 2012

Thank you.

this is only there for some edge case I can't remember and probably don't have to worry about

php projects tend to do this due to shared hosting. Their typical security policy is any writable files are supposed to be world writable. php applications will instruct users to make files world writable and code will helpfully make files world writable, out of necessity or "just in case". For non-php applications or anything deployed by competent sysadmins this sort of behavior tends to lie between questionable and unacceptable.

Even in php land the decision to make files world writable is made on application level, not on library level.

Owner

isaacs commented Dec 21, 2012

I'm sure that the edge case most likely involved npm, since that's where rimraf began life, but ownership and modes are controlled much more elegantly now by other parts of the program. When the npm tarball is created, and again when it's unpacked, npm is setting the mode and ownership appropriately, so whatever edge case this was covering is almost certainly not an issue that I care about any more.

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