From 4501bdbe59fb56dbc0de6e7e220340aaaef9394d Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 16 May 2023 10:33:22 -0700 Subject: [PATCH] Normalize unicode internally using NFD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, the path reservation system, which defends against unicode path name collisions (the subject of a handful of past CVE issues), was using NFKD normalization internally to determine of two paths would be likely to reference the same file on disk. This has the weird effect of normalizing things like `℀` into simple decomposed character strings, for example `a/c`. These can contain slashes and double-dot sections, which means that the path reservations may end up reserving more (or different) paths than intended. Thankfully, tar was already *extracting* properly, even if the path reservations collided, and these collisions resulted in tar being *more* aggressive than it should be in restricting parallel extraction, rather than less. That's a good direction to err in, for security, but also, made tar less efficient than it could be in some edge cases. Using NFD normalization, unicode characters are not decomposed in compatibility mode, but still result in matching path reservation keys as intended. This does not cause any change in observed behavior, other than allowing some files to be extracted in parallel where it is provably safe to do so. Credit: discovered by @Sim4n6. This did not result in a juicy security vulnerability, but it sure looked like one at first. They were extremely patient, thorough, and persistent in trying to pin this down to a POC and CVE. There is very little reward or visibility when a security researcher finds a bug that doesn't result in a security disclosure, but the attempt often results in improvements to the project. --- lib/normalize-unicode.js | 2 +- lib/path-reservations.js | 2 +- lib/unpack.js | 2 +- .../test/normalize-unicode.js.test.cjs | 30 +++++++++++++++++++ test/normalize-unicode.js | 27 +++++++++++++++++ 5 files changed, 60 insertions(+), 3 deletions(-) create mode 100644 tap-snapshots/test/normalize-unicode.js.test.cjs diff --git a/lib/normalize-unicode.js b/lib/normalize-unicode.js index 43dc406e..79e285ab 100644 --- a/lib/normalize-unicode.js +++ b/lib/normalize-unicode.js @@ -6,7 +6,7 @@ const normalizeCache = Object.create(null) const { hasOwnProperty } = Object.prototype module.exports = s => { if (!hasOwnProperty.call(normalizeCache, s)) { - normalizeCache[s] = s.normalize('NFKD') + normalizeCache[s] = s.normalize('NFD') } return normalizeCache[s] } diff --git a/lib/path-reservations.js b/lib/path-reservations.js index ef380cab..8d349d58 100644 --- a/lib/path-reservations.js +++ b/lib/path-reservations.js @@ -123,7 +123,7 @@ module.exports = () => { // effectively removing all parallelization on windows. paths = isWindows ? ['win32 parallelization disabled'] : paths.map(p => { // don't need normPath, because we skip this entirely for windows - return normalize(stripSlashes(join(p))).toLowerCase() + return stripSlashes(join(normalize(p))).toLowerCase() }) const dirs = new Set( diff --git a/lib/unpack.js b/lib/unpack.js index e341ad0c..fa46611c 100644 --- a/lib/unpack.js +++ b/lib/unpack.js @@ -105,7 +105,7 @@ const uint32 = (a, b, c) => // Note that on windows, we always drop the entire cache whenever a // symbolic link is encountered, because 8.3 filenames are impossible // to reason about, and collisions are hazards rather than just failures. -const cacheKeyNormalize = path => normalize(stripSlash(normPath(path))) +const cacheKeyNormalize = path => stripSlash(normPath(normalize(path))) .toLowerCase() const pruneCache = (cache, abs) => { diff --git a/tap-snapshots/test/normalize-unicode.js.test.cjs b/tap-snapshots/test/normalize-unicode.js.test.cjs new file mode 100644 index 00000000..3163313d --- /dev/null +++ b/tap-snapshots/test/normalize-unicode.js.test.cjs @@ -0,0 +1,30 @@ +/* IMPORTANT + * This snapshot file is auto-generated, but designed for humans. + * It should be checked into source control and tracked carefully. + * Re-generate by setting TAP_SNAPSHOT=1 and running tests. + * Make sure to inspect the output below. Do not ignore changes! + */ +'use strict' +exports[`test/normalize-unicode.js TAP normalize with strip slashes "1/4foo.txt" > normalized 1`] = ` +1/4foo.txt +` + +exports[`test/normalize-unicode.js TAP normalize with strip slashes "\\\\a\\\\b\\\\c\\\\d\\\\" > normalized 1`] = ` +/a/b/c/d +` + +exports[`test/normalize-unicode.js TAP normalize with strip slashes "¼foo.txt" > normalized 1`] = ` +¼foo.txt +` + +exports[`test/normalize-unicode.js TAP normalize with strip slashes "﹨aaaa﹨dddd﹨" > normalized 1`] = ` +﹨aaaa﹨dddd﹨ +` + +exports[`test/normalize-unicode.js TAP normalize with strip slashes "＼bbb＼eee＼" > normalized 1`] = ` +＼bbb＼eee＼ +` + +exports[`test/normalize-unicode.js TAP normalize with strip slashes "＼＼＼＼＼eee＼＼＼＼＼＼" > normalized 1`] = ` +＼＼＼＼＼eee＼＼＼＼＼＼ +` diff --git a/test/normalize-unicode.js b/test/normalize-unicode.js index 485d087f..0d34f38c 100644 --- a/test/normalize-unicode.js +++ b/test/normalize-unicode.js @@ -1,5 +1,8 @@ +process.env.TESTING_TAR_FAKE_PLATFORM = 'win32' const t = require('tap') const normalize = require('../lib/normalize-unicode.js') +const stripSlash = require('../lib/strip-trailing-slashes.js') +const normPath = require('../lib/normalize-windows-path.js') // café const cafe1 = Buffer.from([0x63, 0x61, 0x66, 0xc3, 0xa9]).toString() @@ -10,3 +13,27 @@ const cafe2 = Buffer.from([0x63, 0x61, 0x66, 0x65, 0xcc, 0x81]).toString() t.equal(normalize(cafe1), normalize(cafe2), 'matching unicodes') t.equal(normalize(cafe1), normalize(cafe2), 'cached') t.equal(normalize('foo'), 'foo', 'non-unicode string') + +t.test('normalize with strip slashes', t => { + const paths = [ + '\\a\\b\\c\\d\\', + '﹨aaaa﹨dddd﹨', + '＼bbb＼eee＼', + '＼＼＼＼＼eee＼＼＼＼＼＼', + '¼foo.txt', + '1/4foo.txt', + ] + + t.plan(paths.length) + + for (const path of paths) { + t.test(JSON.stringify(path), t => { + const a = normalize(stripSlash(normPath(path))) + const b = stripSlash(normPath(normalize(path))) + t.matchSnapshot(a, 'normalized') + t.equal(a, b, 'order should not matter') + t.end() + }) + } + t.end() +})