chmod -R #59

Activity

1. boneskull

   added

   tooling-agenda

   on Jan 27, 2020
2. bcoe commented on Jan 27, 2020

   

   bcoe

   on Jan 27, 2020

   More actions

   @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).
3. darcyclarke commented on Apr 17, 2020

   

   darcyclarke

   on Apr 17, 2020

   Last edited by darcyclarke

   Member

   More actions

   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

   ❤️React with ❤️1Reacted by Ruy Adorno
4. darcyclarke commented on May 1, 2020

   

   darcyclarke

   on May 1, 2020

   Member

   More actions

   @boneskull might make sense to remove this from the agenda until there's been some work/update to give.
5. isaacs commented on May 4, 2020

   

   isaacs

   on May 4, 2020

   Last edited by isaacs

   More actions

   @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.

   👍React with 👍2Reacted by Benjamin E. Coe and Sean Allen
6. isaacs commented on May 4, 2020

   

   isaacs

   on May 4, 2020

   More actions

   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 :)

   👍React with 👍1Reacted by Sean Allen
7. boneskull commented on May 4, 2020

   

   boneskull

   on May 4, 2020

   MemberAuthor

   More actions

   (an explanation of TOCTOU: https://deadliestwebattacks.com/2012/12/26/toctou-twins/)
8. bmeck commented on Jun 12, 2020

   

   bmeck

   on Jun 12, 2020

   Member

   More actions

   @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.
9. darcyclarke commented on Jun 12, 2020

   

   darcyclarke

   on Jun 12, 2020

   Last edited by darcyclarke

   Member

   More actions

   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
10. darcyclarke commented on Jan 22, 2021

    

    darcyclarke

    on Jan 22, 2021

    Last edited by darcyclarke

    Member

    More actions

    • 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.
11. iansu

    removed

    tooling-agenda

    on Jan 22, 2021
12. angstyloop commented on Oct 2, 2022

    

    angstyloop

    on Oct 2, 2022

    Last edited by angstyloop

    More actions

    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();
    
    
13. isaacs commented on Apr 7, 2023

    

    isaacs

    on Apr 7, 2023

    More actions

    @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.