google / traceur-compiler

Traceur is a JavaScript.next-to-JavaScript-of-today compiler
Apache License 2.0
8.17k stars 580 forks source link

Path Manipulation in writeFile #2144

Open dbohannon opened 6 years ago

dbohannon commented 6 years ago

The filename argument passed to the writeFile() function in file-util.js may be manipulated to write arbitrary directories on the filesystem. The writeFile() function attempts to sanitize the untrusted filename, but calls the mkdirRecursive() function on the untrusted filename before the filename is sanitized (https://github.com/google/traceur-compiler/blob/master/src/node/file-util.js#L56)

For example, passing the filename "../test/target.txt" on a *nix system will create the directory "test" in the parent directory. An attacker can use the dot-dot-slash path manipulation technique to create directories anywhere on the filesystem.

function writeFile(filename, contents) {
  // Compute the output path
  var outputdir = fs.realpathSync(process.cwd());
  mkdirRecursive(path.dirname(filename));
  var filedir = fs.realpathSync(path.dirname(filename));
  filedir = removeCommonPrefix(outputdir, filedir);
  outputdir = path.join(outputdir, filedir);

  mkdirRecursive(outputdir);
  var outputfile = path.join(outputdir, path.basename(filename));
  fs.writeFileSync(outputfile, contents, 'utf8');
}
arv commented 6 years ago

Not sure what you are suggesting here?

I think it is important to allow paths starting with .. (or / for that matter). If you are accepting paths from an untrusted source you might want to consider checking the path before handing it to Traceur.

dbohannon commented 6 years ago

Maybe I should clarify. If I submit the filename "../test/target.txt" and my current directory is "/tmp/traceur", the file is written to "/tmp/traceur/test/target.txt". However, the writeFile() function ALSO creates the empty directory "/tmp/test". This appears to be a bug that allows the user to write this "duplicate" directory structure anywhere on the filesystem, when the function is attempting to only write to the current directory. Regardless, this "duplicate" directory is unintended, is it not?

arv commented 6 years ago

I see. Yeah. That seems like a bug. The intent is to create the directory where the output files/dir are going.