22

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?

3 Answers 3

20

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

10
  • 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
    Commented May 22, 2016 at 7:47
  • 1
    A bit late to the party here, but wouldn't this still be vulnerable to path/../../../whatever
    – 3ocene
    Commented Nov 28, 2018 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
    Commented Nov 29, 2018 at 15:59
  • 1
    This does not deal with the case where unsafePrefix = '..'! Perhaps .replace(/^(\.\.[\/\])+/, '').replace(/^(\.\.)$/, '') is safer?
    – Brendon
    Commented Mar 1, 2019 at 7:34
  • 6
    I don't know what the situation was when this question was originally answered, but there is official NodeJS documentation on how to prevent path traversal. Basically you use path.join to create an absolute path and then check if the result starts with your base path. If not: attempted path traversal. Seems simpler and safer to me. Commented Mar 19, 2020 at 9:17
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);
0

Another option: You can just join twice.

path1 = "public/html"                // Hardcoded path.
path2 = req.query.path               // Untrusted user input.

const normalized = path.join('/', path2)
const result = path.join(path1, normalized)

In the first joining, all .., trying to go above the root folder, are ignored because the join method always resolves a parent folder of the root folder is the root folder itself.

So, for example, when path2 is ../../../test1/test2/../test3, normalized becomes /test1/test3 and result will be public/html/test1/test3.

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .