From 926da57e3484caa7242e8ad89fcb628709954f42 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 7 May 2021 23:25:59 -0400 Subject: [PATCH] Minify: Refine `sanitizePathname` to avoid pathname traversal --- src/node/utils/Minify.js | 49 +++++++--------- src/tests/backend/specs/Minify.js | 98 +++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 28 deletions(-) create mode 100644 src/tests/backend/specs/Minify.js diff --git a/src/node/utils/Minify.js b/src/node/utils/Minify.js index 83c4be55..b4d5c851 100644 --- a/src/node/utils/Minify.js +++ b/src/node/utils/Minify.js @@ -21,7 +21,6 @@ * limitations under the License. */ -const assert = require('assert').strict; const settings = require('./Settings'); const fs = require('fs').promises; const path = require('path'); @@ -105,33 +104,23 @@ const requestURIs = (locations, method, headers, callback) => { }); }; -const sanitizePathname = (p) => { - // Replace all backslashes with forward slashes to support Windows. This MUST be done BEFORE path - // normalization, otherwise an attacker will be able to read arbitrary files anywhere on the - // filesystem. See https://nvd.nist.gov/vuln/detail/CVE-2015-3297. Node.js treats both the - // backlash and the forward slash characters as pathname component separators on Windows so this - // does not change the meaning of the pathname. - p = p.replace(/\\/g, '/'); - // The Node.js documentation says that path.join() normalizes, and the documentation for - // path.normalize() says that it resolves '..' and '.' components. The word "resolve" implies that - // it examines the filesystem to resolve symbolic links, so 'a/../b' might not be the same thing - // as 'b'. Most path normalization functions from other libraries (e.g. Python's - // os.path.normpath()) clearly state that they do not examine the filesystem -- they are simple - // string manipulations. Node.js's path.normalize() probably also does a simple string - // manipulation, but if not it must be given a real pathname. Join with ROOT_DIR here just in - // case. ROOT_DIR will be removed later. - p = path.join(ROOT_DIR, p); - // Prevent attempts to read outside of ROOT_DIR via extra '..' components. ROOT_DIR is assumed to - // be normalized. - assert(ROOT_DIR.endsWith(path.sep)); - if (!p.startsWith(ROOT_DIR)) throw new Error(`attempt to read outside ROOT_DIR (${ROOT_DIR})`); - // Convert back to a relative pathname. - p = p.slice(ROOT_DIR.length); - // On Windows, path.normalize replaces forward slashes with backslashes. Convert back to forward - // slashes. THIS IS DANGEROUS UNLESS BACKSLASHES ARE REPLACED WITH FORWARD SLASHES BEFORE PATH - // NORMALIZATION, otherwise on POSIXish systems '..\\' in the input pathname would not be - // normalized away before being converted to '../'. - p = p.replace(/\\/g, '/'); +// Normalizes p and ensures that it is a relative path that does not reach outside. See +// https://nvd.nist.gov/vuln/detail/CVE-2015-3297 for additional context. +const sanitizePathname = (p, pathApi = path) => { + // The documentation for path.normalize() says that it resolves '..' and '.' segments. The word + // "resolve" implies that it examines the filesystem to resolve symbolic links, so 'a/../b' might + // not be the same thing as 'b'. Most path normalization functions from other libraries (e.g., + // Python's os.path.normpath()) clearly state that they do not examine the filesystem. Here we + // assume Node.js's path.normalize() does the same; that it is only a simple string manipulation. + p = pathApi.normalize(p); + if (pathApi.isAbsolute(p)) throw new Error(`absolute paths are forbidden: ${p}`); + if (p.split(pathApi.sep)[0] === '..') throw new Error(`directory traversal: ${p}`); + // On Windows, path normalization replaces forwardslashes with backslashes. Convert them back to + // forwardslashes. Node.js treats both the backlash and the forwardslash characters as pathname + // component separators on Windows so this does not change the meaning of the pathname on Windows. + // THIS CONVERSION MUST ONLY BE DONE ON WINDOWS, otherwise on POSIXish systems '..\\' in the input + // pathname would not be normalized away before being converted to '../'. + if (pathApi.sep === '\\') p = p.replace(/\\/g, '/'); return p; }; @@ -351,3 +340,7 @@ exports.requestURIs = requestURIs; exports.shutdown = async (hookName, context) => { await threadsPool.terminate(); }; + +exports.exportedForTestingOnly = { + sanitizePathname, +}; diff --git a/src/tests/backend/specs/Minify.js b/src/tests/backend/specs/Minify.js new file mode 100644 index 00000000..5834bb94 --- /dev/null +++ b/src/tests/backend/specs/Minify.js @@ -0,0 +1,98 @@ +'use strict'; + +const Minify = require('../../../node/utils/Minify'); +const assert = require('assert').strict; +const path = require('path'); + +const {sanitizePathname} = Minify.exportedForTestingOnly; + +describe(__filename, function () { + describe('absolute paths rejected', function () { + const testCases = [ + ['posix', '/'], + ['posix', '/foo'], + ['win32', '/'], + ['win32', '\\'], + ['win32', 'C:/foo'], + ['win32', 'C:\\foo'], + ['win32', 'c:/foo'], + ['win32', 'c:\\foo'], + ['win32', '/foo'], + ['win32', '\\foo'], + ]; + for (const [platform, p] of testCases) { + it(`${platform} ${p}`, async function () { + assert.throws(() => sanitizePathname(p, path[platform]), {message: /absolute path/}); + }); + } + }); + describe('directory traversal rejected', function () { + const testCases = [ + ['posix', '..'], + ['posix', '../'], + ['posix', '../foo'], + ['posix', 'foo/../..'], + ['win32', '..'], + ['win32', '../'], + ['win32', '..\\'], + ['win32', '../foo'], + ['win32', '..\\foo'], + ['win32', 'foo/../..'], + ['win32', 'foo\\..\\..'], + ]; + for (const [platform, p] of testCases) { + it(`${platform} ${p}`, async function () { + assert.throws(() => sanitizePathname(p, path[platform]), {message: /travers/}); + }); + } + }); + + describe('accepted paths', function () { + const testCases = [ + ['posix', '', '.'], + ['posix', '.'], + ['posix', './'], + ['posix', 'foo'], + ['posix', 'foo/'], + ['posix', 'foo/bar/..', 'foo'], + ['posix', 'foo/bar/../', 'foo/'], + ['posix', './foo', 'foo'], + ['posix', 'foo/bar'], + ['posix', 'foo\\bar'], + ['posix', '\\foo'], + ['posix', '..\\foo'], + ['posix', 'foo/../bar', 'bar'], + ['posix', 'C:/foo'], + ['posix', 'C:\\foo'], + ['win32', '', '.'], + ['win32', '.'], + ['win32', './'], + ['win32', '.\\', './'], + ['win32', 'foo'], + ['win32', 'foo/'], + ['win32', 'foo\\', 'foo/'], + ['win32', 'foo/bar/..', 'foo'], + ['win32', 'foo\\bar\\..', 'foo'], + ['win32', 'foo/bar/../', 'foo/'], + ['win32', 'foo\\bar\\..\\', 'foo/'], + ['win32', './foo', 'foo'], + ['win32', '.\\foo', 'foo'], + ['win32', 'foo/bar'], + ['win32', 'foo\\bar', 'foo/bar'], + ['win32', 'foo/../bar', 'bar'], + ['win32', 'foo\\..\\bar', 'bar'], + ['win32', 'foo/..\\bar', 'bar'], + ['win32', 'foo\\../bar', 'bar'], + ]; + for (const [platform, p, tcWant] of testCases) { + const want = tcWant == null ? p : tcWant; + it(`${platform} ${p || ''} -> ${want}`, async function () { + assert.equal(sanitizePathname(p, path[platform]), want); + }); + } + }); + + it('default path API', async function () { + assert.equal(sanitizePathname('foo'), 'foo'); + }); +});