path: fix path traversal in normalize() on Windows

tniessen · RafaelGSS · commit 99f217369f58 · 2025-01-15T18:09:20.000-03:00
Without this patch, on Windows, normalizing a relative path might result in a path that Windows considers absolute. In rare cases, this might lead to path traversal vulnerabilities in user code. We attempt to detect those cases and return a relative path instead. PR-URL: nodejs-private/node-private#555 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> CVE-ID: CVE-2025-23084
diff --git a/lib/path.js b/lib/path.js
@@ -26,6 +26,7 @@ const {
   ArrayPrototypeSlice,
   FunctionPrototypeBind,
   StringPrototypeCharCodeAt,
+  StringPrototypeIncludes,
   StringPrototypeIndexOf,
   StringPrototypeLastIndexOf,
   StringPrototypeRepeat,
@@ -420,6 +421,23 @@ const win32 = {
     if (tail.length > 0 &&
         isPathSeparator(StringPrototypeCharCodeAt(path, len - 1)))
       tail += '\\';
+    if (!isAbsolute && device === undefined && StringPrototypeIncludes(path, ':')) {
+      // If the original path was not absolute and if we have not been able to
+      // resolve it relative to a particular device, we need to ensure that the
+      // `tail` has not become something that Windows might interpret as an
+      // absolute path. See CVE-2024-36139.
+      if (tail.length >= 2 &&
+          isWindowsDeviceRoot(StringPrototypeCharCodeAt(tail, 0)) &&
+          StringPrototypeCharCodeAt(tail, 1) === CHAR_COLON) {
+        return `.\\${tail}`;
+      }
+      let index = StringPrototypeIndexOf(path, ':');
+      do {
+        if (index === len - 1 || isPathSeparator(StringPrototypeCharCodeAt(path, index + 1))) {
+          return `.\\${tail}`;
+        }
+      } while ((index = StringPrototypeIndexOf(path, ':', index + 1)) !== -1);
+    }
     if (device === undefined) {
       return isAbsolute ? `\\${tail}` : tail;
     }
diff --git a/test/parallel/test-path-join.js b/test/parallel/test-path-join.js
@@ -110,6 +110,13 @@ joinTests.push([
       [['c:.', 'file'], 'c:file'],
       [['c:', '/'], 'c:\\'],
       [['c:', 'file'], 'c:\\file'],
+      // Path traversal in previous versions of Node.js.
+      [['./upload', '/../C:/Windows'], '.\\C:\\Windows'],
+      [['upload', '../', 'C:foo'], '.\\C:foo'],
+      [['test/..', '??/D:/Test'], '.\\??\\D:\\Test'],
+      [['test', '..', 'D:'], '.\\D:'],
+      [['test', '..', 'D:\\'], '.\\D:\\'],
+      [['test', '..', 'D:foo'], '.\\D:foo'],
     ]
   ),
 ]);
diff --git a/test/parallel/test-path-normalize.js b/test/parallel/test-path-normalize.js
@@ -41,6 +41,32 @@ assert.strictEqual(
 );
 assert.strictEqual(path.win32.normalize('foo/bar\\baz'), 'foo\\bar\\baz');
 
+// Tests related to CVE-2024-36139. Path traversal should not result in changing
+// the root directory on Windows.
+assert.strictEqual(path.win32.normalize('test/../C:/Windows'), '.\\C:\\Windows');
+assert.strictEqual(path.win32.normalize('test/../C:Windows'), '.\\C:Windows');
+assert.strictEqual(path.win32.normalize('./upload/../C:/Windows'), '.\\C:\\Windows');
+assert.strictEqual(path.win32.normalize('./upload/../C:x'), '.\\C:x');
+assert.strictEqual(path.win32.normalize('test/../??/D:/Test'), '.\\??\\D:\\Test');
+assert.strictEqual(path.win32.normalize('test/C:/../../F:'), '.\\F:');
+assert.strictEqual(path.win32.normalize('test/C:foo/../../F:'), '.\\F:');
+assert.strictEqual(path.win32.normalize('test/C:/../../F:\\'), '.\\F:\\');
+assert.strictEqual(path.win32.normalize('test/C:foo/../../F:\\'), '.\\F:\\');
+assert.strictEqual(path.win32.normalize('test/C:/../../F:x'), '.\\F:x');
+assert.strictEqual(path.win32.normalize('test/C:foo/../../F:x'), '.\\F:x');
+assert.strictEqual(path.win32.normalize('/test/../??/D:/Test'), '\\??\\D:\\Test');
+assert.strictEqual(path.win32.normalize('/test/../?/D:/Test'), '\\?\\D:\\Test');
+assert.strictEqual(path.win32.normalize('//test/../??/D:/Test'), '\\\\test\\..\\??\\D:\\Test');
+assert.strictEqual(path.win32.normalize('//test/../?/D:/Test'), '\\\\test\\..\\?\\D:\\Test');
+assert.strictEqual(path.win32.normalize('\\\\?\\test/../?/D:/Test'), '\\\\?\\test\\?\\D:\\Test');
+assert.strictEqual(path.win32.normalize('\\\\?\\test/../../?/D:/Test'), '\\\\?\\test\\?\\D:\\Test');
+assert.strictEqual(path.win32.normalize('\\\\.\\test/../?/D:/Test'), '\\\\.\\test\\?\\D:\\Test');
+assert.strictEqual(path.win32.normalize('\\\\.\\test/../../?/D:/Test'), '\\\\.\\test\\?\\D:\\Test');
+assert.strictEqual(path.win32.normalize('//server/share/dir/../../../?/D:/file'),
+                   '\\\\server\\share\\?\\D:\\file');
+assert.strictEqual(path.win32.normalize('//server/goodshare/../badshare/file'),
+                   '\\\\server\\goodshare\\badshare\\file');
+
 assert.strictEqual(path.posix.normalize('./fixtures///b/../b/c.js'),
                    'fixtures/b/c.js');
 assert.strictEqual(path.posix.normalize('/foo/../../../bar'), '/bar');
