`_joinDirectoryName` traversal check bypassable via directory prefix collision

[vnykmshr]

The path traversal check in _joinDirectoryName() compares string prefixes, not filesystem paths. It can be bypassed when uploadDir doesn't end with a path separator (the default).

// Current check (src/Formidable.js, line 598):
const newPath = path.join(this.uploadDir, name);
if (!newPath.startsWith(this.uploadDir)) { ... }

When uploadDir = "/tmp/uploads":

path.join("/tmp/uploads", "../uploads-evil/x")  →  "/tmp/uploads-evil/x"
"/tmp/uploads-evil/x".startsWith("/tmp/uploads")  →  true  ← bypassed

The resolved path escapes the intended directory but passes the check because it shares a string prefix. os.tmpdir() (the default uploadDir) never returns a trailing separator on any platform, so the check is broken for default configurations.

This affects applications using the filename callback that return values derived from part.originalFilename. Combined with createDirsFromUploads: true, the sibling directory gets created automatically.

Introduced in #689 (v2.0.0), present in all v2.x and v3.x releases through v3.5.4 (current).

Fix: append path.sep to the resolved directory before comparison:

_joinDirectoryName(name) {
    const resolvedDir = path.resolve(this.uploadDir);
    const resolvedPath = path.resolve(resolvedDir, name);

    if (resolvedPath === resolvedDir || !resolvedPath.startsWith(resolvedDir + path.sep)) {
      return path.join(this.uploadDir, this.options.defaultInvalidName);
    }

    return resolvedPath;
}

I'll open a PR with this fix and tests.

[GrosSacASac]

This cannot happen in reality ...,

as you can see _joinDirectoryName is a private method, it will not be called directly with user input

    const newFilename = this._getNewName(part);
    const filepath = this._joinDirectoryName(newFilename);

newFilename is a variable that is already sanitized and cannot contain "../uploads-evil/x" or any other made up examples

[vnykmshr]

When options.filename is provided, _getNewName returns whatever the callback returns -- no sanitization (https://github.com/node-formidable/formidable/blob/master/src/Formidable.js#L653):

 return this.options.filename.call(this, name, ext, part, this);

That value goes straight into _joinDirectoryName. The old startsWith check without the separator was the only guard on that path, and it was bypassable. The fix in #1062 closes that.

[GrosSacASac]

It does not return whatever, it returns precisely what the user of the formidable library wants. So it should be sanitized if the function filename is correctly written. And if the filename option is not used, it will return a random hexoid.

Now, do you think this library should not trust the developer using it ?

[vnykmshr]

The traversal check you added in #689 was the right call. The fix in #1062 just closes a gap -- adding the path separator ensures users get what they intended.

[GrosSacASac]

I am not convinced until I see 1 end-to-end example of this vulnerability, without using private methods.

[vnykmshr]

// server with folder uploads (createDirsFromUploads: true)
const form = formidable({
  uploadDir: "/tmp/uploads",
  filename: (name, ext, part) => part.originalFilename,
  createDirsFromUploads: true,
});

Attacker sends:

Content-Disposition: form-data; name="file"; filename="../uploads-evil/payload.txt"

part.originalFilename is "../uploads-evil/payload.txt" -- controlled from the header, createDirsFromUploads creates /tmp/uploads-evil/, and the file lands outside uploadDir.

Before: "/tmp/uploads-evil/payload.txt".startsWith("/tmp/uploads")  -> true  -> goes thru
After : "/tmp/uploads-evil/payload.txt".startsWith("/tmp/uploads/") -> false -> blocked

[GrosSacASac]

filename: (name, ext, part) => part.originalFilename,

just as I thought the only problem is when you provide an insecure filename with 0 verification function on purpose (off by default)
and use createDirsFromUploads (false by default)

Back to the original question:
Now, do you think this library should not trust the developer using it ?

[GrosSacASac]

The problem with answering no, is that you could find an infinite amount of problems if you configure most libraries (on npm and more) with bad options

[vnykmshr]

I get your point, not arguing against trust, allowing users to upload structured files, preserving the filenames is acceptable usecase, the startsWith check was there, adding the path separator closes a gap, happy to leave it here

@tunnckoCore would appreciate your take on this

[tunnckoCore]

@vnykmshr just seeing that.

well, it DOES make sense to have basic sanitization. But it also does to NOT have anything since it's intentional option for customizations.

What's the case? I see #1062 merged and perfectly simple and valid.

@GrosSacASac

[GrosSacASac]

if you can find vulnerable code with default options then prove it and make a test case.

Otherwise this is just bloatware.

[tunnckoCore]

@GrosSacASac why you are focusing on "defaults"? Sure, the defaults are good enough.

Such a basic line edit is actually good for giving a very basic start of protection even if you are dumb enough to bypass the "good defaults", it's not bloat.

[GrosSacASac]

If someone goes out of their way to use a custom option, I assume that persons knows what they are doing. So the extra protection does nothing except add dead code that is not used.

I should say start of bloat, not bloat.

I can imagine a PR that will check if option maxFileSize is a number.

Also "only" 1 line of code.

Never used, if you use default, never used if you are reading the docs and use a number.

And same things can be said, that it is nice and protects and why not bla bla ...

The problem is that the list of this kind of protection is infinite. So we might as well decide a clear boundary.