92 lines
2.6 KiB
Plaintext
92 lines
2.6 KiB
Plaintext
|
=== How to fix it in Node.js
|
||
|
|
|
||
|
|
:canonicalization_function1: path.join
|
||
|
|
:canonicalization_function2: path.normalize
|
||
|
|
|
||
|
|
include::../../common/fix/code-rationale.adoc[]
|
||
|
|
|
||
|
|
==== Noncompliant code example
|
||
|
|
|
||
|
|
[source,javascript,diff-id=1,diff-type=noncompliant]
|
||
|
|
----
|
||
|
|
const AdmZip = require("adm-zip");
|
||
|
|
const upload = require('multer');
|
||
|
|
|
||
|
|
app.get('/example', upload.single('file'), (req, res) => {
|
||
|
|
const zip = new AdmZip(req.file.buffer);
|
||
|
|
const zipEntries = zip.getEntries();
|
||
|
|
|
||
|
|
zipEntries.forEach(function (zipEntry) {
|
||
|
|
var writer = fs.createWriteStream(zipEntry.entryName); // Noncompliant
|
||
|
|
writer.write(zipEntry.getData().toString("utf8"));
|
||
|
|
});
|
||
|
|
});
|
||
|
|
----
|
||
|
|
|
||
|
|
==== Compliant solution
|
||
|
|
|
||
|
|
[source,javascript,diff-id=1,diff-type=compliant]
|
||
|
|
----
|
||
|
|
const AdmZip = require("adm-zip");
|
||
|
|
const upload = require('multer');
|
||
|
|
|
||
|
|
const unzipTargetDir = "/example/directory/";
|
||
|
|
|
||
|
|
app.get('/example', upload.single('file'), (req, res) => {
|
||
|
|
const zip = new AdmZip(req.file.buffer);
|
||
|
|
const zipEntries = zip.getEntries();
|
||
|
|
|
||
|
|
zipEntries.forEach(function (zipEntry) {
|
||
|
|
const canonicalPath = path.normalize(unzipTargetDir + zipEntry.entryName);
|
||
|
|
if (canonicalPath.startsWith(unzipTargetDir)) {
|
||
|
|
let writer = fs.createWriteStream(canonicalPath);
|
||
|
|
writer.write(zipEntry.getData().toString("utf8"));
|
||
|
|
}
|
||
|
|
});
|
||
|
|
});
|
||
|
|
----
|
||
|
|
|
||
|
|
=== How does this work?
|
||
|
|
|
||
|
|
include::../../common/fix/how-does-this-work.adoc[]
|
||
|
|
|
||
|
|
=== Pitfalls
|
||
|
|
|
||
|
|
include::../../common/pitfalls/partial-path-traversal.adoc[]
|
||
|
|
|
||
|
|
For example, the following code is vulnerable to partial path injection. Note
|
||
|
|
that the string variable `targetDirectory` does not end with a path separator:
|
||
|
|
|
||
|
|
|
||
|
|
[source, javascript]
|
||
|
|
----
|
||
|
|
const AdmZip = require("adm-zip");
|
||
|
|
|
||
|
|
const targetDirectory = "/Users/John";
|
||
|
|
|
||
|
|
app.get('/example', (req, res) => {
|
||
|
|
const canonicalPath = path.normalize(targetDirectory + req.query.filename)
|
||
|
|
|
||
|
|
if (canonicalPath.startsWith(targetDirectory)) {
|
||
|
|
const zip = new AdmZip(canonicalPath);
|
||
|
|
const zipEntries = zip.getEntries();
|
||
|
|
|
||
|
|
zipEntries.forEach(function (zipEntry) {
|
||
|
|
var writer = fs.createWriteStream(zipEntry.entryName);
|
||
|
|
writer.write(zipEntry.getData().toString("utf8"));
|
||
|
|
});
|
||
|
|
}
|
||
|
|
});
|
||
|
|
----
|
||
|
|
|
||
|
|
This check can be bypassed because `"/Users/Johnny".startsWith("/Users/John")`
|
||
|
|
returns `true`. Thus, for validation, `"/Users/John"` should actually be
|
||
|
|
`"/Users/John/"`.
|
||
|
|
|
||
|
|
**Warning**: Some functions remove the terminating path separator in their return
|
||
|
|
value. +
|
||
|
|
The validation code should be tested to ensure that it cannot be impacted by this
|
||
|
|
issue.
|
||
|
|
|
||
|
|
https://github.com/advisories/GHSA-xwg4-93c6-3h42[Here is a real-life example of this vulnerability.]
|