11

I have a node.js webapp in which I need to concatenate two paths in a safe way. The first one (leftmost) is a constant, and the second one (rightmost) is relative to the first one and comes from untrusted user input. The resulting path should be something that is below the first path. So the situation is this:

path1 = "public/html";                // Hardcoded path.
path2 = req.query.path;               // Untrusted user input.
result = safePathJoin(path1, path2);  // Result can be e.g. public/html/index.htm,
                                      // but never private/config.xml

What I need is the function safePathJoin() that is safe against directory traversal attacks. My first naive approach is this:

safePathJoin = function(path1, path2) {
    path1 = path.normalize(path1);
    var result = path.join(path1, path2);
    return result.startsWith(path1) ? result : undefined;
}

Is this good enough? Is there a standard way to do this? Any suggestions?

12

Here's is one approach I've used in this situation:

  1. path.normalize() handles all . and .., so you can be sure that if either one is present, it will be at the front of the path.
  2. Remove any ../../ from the front of your path.

So:

var safeSuffix = path.normalize(unsafeSuffix).replace(/^(\.\.(\/|\\|$))+/, '');
var safeJoin = path.join(basePath, safeSuffix);

About your approach: checking the prefix seems like a pretty good idea to me. There are a couple of problems I see with your implementation:

  • You've checked for a prefix without a trailing slash: ../html-other will resolve to public/html-other, which I guess is not what you want.
  • You would run into trouble on Windows systems, where .normalize() would convert / to \, meaning no paths would work.

When I've done prefix-checking (for slightly different situations), here's what I ended up with:

function checkPrefix(prefix, candidate) {
    // .resolve() removes trailing slashes
    var absPrefix = path.resolve(prefix) + path.sep;
    var absCandidate = path.resolve(candidate) + path.sep;
    return absCandidate.substring(0, absPrefix.length) === absPrefix;
}

(Yes, I added path.sep to both, so that the prefix dir itself passes the test.)

  • Great answer! Thanks! Concerning your last example, is there any benefit to using path.resolve() to get the absolute path, as opposed to use path.normalize() and stay with relative paths? – Anders May 19 '16 at 15:36
  • 1
    path.normalize() keeps trailing slashes, so you'd need some extra logic for prefix directories of both public/html and public/html/ to work correctly. – cloudfeet May 22 '16 at 7:47
  • A bit late to the party here, but wouldn't this still be vulnerable to path/../../../whatever – 3ocene Nov 28 '18 at 17:52
  • 1
    @3ocene: That's the reason we use path.normalise() as step 1. It cancels any internal ../, so your example would be converted to ../../whatever before step 2. :) – cloudfeet Nov 29 '18 at 15:59
  • 1
    This does not deal with the case where unsafePrefix = '..'! Perhaps .replace(/^(\.\.[\/\])+/, '').replace(/^(\.\.)$/, '') is safer? – Brendon Mar 1 at 7:34
0

Given the requirement to receive user input path that is a sub-directory of a defined root directory and allow user to access (in any way) combined root + path directory, to me the safest option would be to allow only absolute paths as an input and check that sanitized of any special directory names (., ..) input path equals the original input:

// Input

const path1 = "public/html";
const path2 = req.query.path;

// Checkout

const isNotSpecialDirName = part => !(['', '.', '..'].includes(part));

const path2Clean = path2.split(path.sep).filter(isNotSpecialDirName).join(path.sep);

// Output

if (path2Clean !== path2) {
  // Obfuscated with Not Found
  throw new Error('Not Found');
}

const result = path.join(path1, path2Clean);

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.