chmod -R

[boneskull]

This came out of a slack discussion, but adding a recursive mode to fs.chmod (a la chmodr would be super.

cc @isaacs @coreyfarrell

[bcoe]

@boneskull I wonder if we could use the mkdir recursive logic more easily for chmodr, than rimraf (which seemed to have quite a few caveats).

[darcyclarke]

Just updating this thread based on the past two WG calls; I'm willing to get involved here to try and tackle the work - outside of my existing day-to-day responsibilities.

Follow-up Actions

• Set up an initial call w/ @bcoe to download his context & scope the work
• Break-earth on an implementation

[darcyclarke]

@boneskull might make sense to remove this from the agenda until there's been some work/update to give.

[isaacs]

@bcoe chomdr is definitely much closer to rimraf than it is to mkdirp. Ie, it's doing some operation on a tree that exists, rather than creating a linear path when it doesn't exist, so you run into the same parallelization/threading performance concerns that led to doing rimraf in js rather than at the C++ layer. Also, may as well do chownr as well, since it's basically the same.

It's worth noting that chmodr and chownr suffer from some unavoidable TOCTOU issues. Basically, there's no way to say "readdir(path) but don't follow symlink if path is a link to a dir". Chmodr/chownr should not descend into symlinks by default, so you have to lstat a thing to see if it's a real directory or a symlink, and only descend if it's not a symlink. An attacker can replace a directory with a symlink between the lstat and readdir, resulting in changing the mode/ownership of arbitrary files on disk.

This is less of a concern for a userland module, since we can just say "this isn't secure for X purposes", but it becomes a bigger hazard in node core, especially since mode/ownership are such important security features. Rimraf and mkdirp are less of a concern as well, since the damage that a TOCTOU attack causes is more apparent. (And, in rimraf's case at least, there isn't a TOCTOU opening, because there is a way to say "only remove this thing if it is/isn't an actual directory".) A mkdirp exploit can create some garbage, and a rimraf exploit can trick you into deleting /etc/passwd which might ruin your day, but chownr/chmodr can make it world editable, letting an attacker silently own your system forever.

[isaacs]

Also worth noting that the TOCTOU issue exists in chmod -R and chown -R, so really, just don't do ever recursive mode/ownership changes in running production apps :)

[boneskull]

(an explanation of TOCTOU: https://deadliestwebattacks.com/2012/12/26/toctou-twins/)

[bmeck]

@isaacs to my understanding, if we used the following pseudocode to keep the same FD rather than string would this be solved?

fd = open(dirname);
if (isSymlink(fstat(fd))) {
  dir = fdopendir(fd);
}

It seems like the file on represented by fd disk could still be replaced but the symlink check would still be checking the same thing as it is using to iterate the directory.

[darcyclarke]

Quick update: @bcoe & I paired a bit (he drove & got the initial work together here: https://github.com/bcoe/node-1/tree/chmodr) - I'll be picking up where we left off...

Action Items

• Finish off initial work & cut a PR
  • sync up w/ @isaacs to confirm vendoring/licensing of chmodr is onside (also, get a better understanding of concerns)
  • promises
  • synchronous
  • write some tests/coverage
• Bring up discussion about libuv potentially exposing APIs to resolve issues/concerns around fs.lstat & fs.readdir
• Pose the question/get feedback by the @nodejs/security group on the concerns around recursive file system operations

[darcyclarke]

• Note from call today, we can remove this issue/work from the agenda & backlog this. If someone else wants to champion it, please reach out or pick this up.

[angstyloop]

Here's a Node.js script that uses the built-in module fs to recursively chmod a directory. It's light but has 0 armor.

Source code is here too.

But please note the concerns raised by @isaacs about the vulnerability to TOCTOU attacks.

You can use the same kind of approach to walk the FS tree and do things. The approach is the same in C (because the built-ins are the same). All these suffer from TOCTOU vulnerability, but like @isaacs said, some built-ins can be leveraged to get root, while others are just annoying.

USAGE: ./chmodRecursive <DIRECTORY> <PERMISSIONS>

PERMISSIONS is a three character string, like 777.

#! /usr/bin/env node

const { readdirSync, lstatSync, isDirectory, chmodSync } = require('fs');
const { join, posix: { basename } } = require('path');

/** List subdirectories of target directory @d. NOT recursive.
 * @param dir - (string) Target directory.
 * @return (string[]) Subdirectories of @d.
 */
function listDirs(dir) {
    return readdirSync(dir)
            .map(it => join(dir, it))
            .filter(it => lstatSync(it).isDirectory());
}

/** List files (and subdirectories) in target directory @dir. NOT recursive.
 * @param dir - (string) Full or relative path of target directory.
 * @return (string[]) Files of @dir.
 */
function listChildren(dir) {
    return readdirSync(dir).map(it => join(dir, it));
}

/** chmod all files (and subdirectories) in a directory @dir, giving them new
 * permissions @perms. NOT recursive.
 *
 * @param dir - (string) Full or relative path of target directory.
 * @perms - (string) Permissions for chmod, as a string (e.g., '644')
 */
function chmodChildren(dir, perms) {
    listChildren(dir).map(it => chmodSync(it, perms));
}

/* Starting in the directory named @root, recursively find and chmod all
 * all files and subdirectories. The directory @root is also chmod'd.
 * Implements BFS with a FIFO queue.
 *
 * @root - Full or relative path of directory to start in.
 * @perms - (string) Permissions for chmod, as a string (e.g., '644')
 *
 */
/*module.exports = */function chmodRecursive(root, perms) {
    let dir = root;
    chmodSync(root, perms);
    chmodChildren(root, perms);
    const dirs = listDirs(dir);
    while (dirs.length) {
        dir = dirs.shift();
        chmodChildren(dir, perms);
        dirs.push(...listDirs(dir));
    }
}

const EXIT_SUCCESS = 0;
const EXIT_FAILURE = 1;

/** Driver code.
 */
function main() {
    // "node" and the name of the script are always the first two arguments.
    // Your command line arguments start at index=2 (the third argument -
    // totally not confusing, right?)
    if (process.argv.length !== 4) {
        console.log('USAGE: ./chmodRecursive <DIRECTORY> <PERMISSIONS>');
        console.log('PERMISSIONS is a three character string, like 777.');
        process.exit(EXIT_FAILURE);
    }
    chmodRecursive(...process.argv.slice(2));
    process.exit(EXIT_SUCCESS);
}

main();

[isaacs]

@isaacs to my understanding, if we used the following pseudocode to keep the same FD rather than string would this be solved?

fd = open(dirname);

if (isSymlink(fstat(fd))) {

  dir = fdopendir(fd);

}

It seems like the file on represented by fd disk could still be replaced but the symlink check would still be checking the same thing as it is using to iterate the directory.

(Sorry for the many years late reply)

Using fstat in this way would be much worse actually, because O_SYMLINK is not in the posix standard and is only supported on BSD systems, afaik. So any Linux machine would have no way to not follow all symlinks. So if someone swapped the child dir for a symlink to /etc between the readdir({ withFileTypes:true }) and fs.open, you'd have no good way to detect it, since Dirent includes type, but not dev+ino (and it's even worse on windows, of course.)

But yes, on macOS and other BSDs, this would be a solution. You could open the child entries with O_SYMLINK, fstat would report that it's a symlink, and you'd know not to recurse.