Normalize unicode internally using NFD

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.

Author: isaacs

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]
 }

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(

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) => {

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＼＼＼＼＼＼
+`

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()
+})