From 4b94984ac531c6c6394f167bf996e8c56c5b419f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=BCseyin=20A=C3=A7acak?= <110401522+huseyinacacak-janea@users.noreply.github.com> Date: Wed, 6 Nov 2024 02:21:54 +0300 Subject: [PATCH 1/2] path,win: fix bug in resolve and normalize Fixes: https://github.com/nodejs/node/issues/54025 PR-URL: https://github.com/nodejs/node/pull/55623 Reviewed-By: Antoine du Hamel Reviewed-By: Ruben Bridgewater Reviewed-By: Luigi Pinca --- lib/path.js | 41 ++++++++++++++++++---------- src/path.cc | 13 +++++++-- test/cctest/test_path.cc | 4 +++ test/parallel/test-path-makelong.js | 2 +- test/parallel/test-path-normalize.js | 10 ++++--- test/parallel/test-path-resolve.js | 2 ++ 6 files changed, 49 insertions(+), 23 deletions(-) diff --git a/lib/path.js b/lib/path.js index 722e1ea27cf0a0..c35e4b005e7550 100644 --- a/lib/path.js +++ b/lib/path.js @@ -291,10 +291,16 @@ const win32 = { j++; } if (j === len || j !== last) { - // We matched a UNC root - device = - `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`; - rootEnd = j; + if (firstPart !== '.' && firstPart !== '?') { + // We matched a UNC root + device = + `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`; + rootEnd = j; + } else { + // We matched a device root (e.g. \\\\.\\PHYSICALDRIVE0) + device = `\\\\${firstPart}`; + rootEnd = 4; + } } } } @@ -404,17 +410,22 @@ const win32 = { !isPathSeparator(StringPrototypeCharCodeAt(path, j))) { j++; } - if (j === len) { - // We matched a UNC root only - // Return the normalized version of the UNC root since there - // is nothing left to process - return `\\\\${firstPart}\\${StringPrototypeSlice(path, last)}\\`; - } - if (j !== last) { - // We matched a UNC root with leftovers - device = - `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`; - rootEnd = j; + if (j === len || j !== last) { + if (firstPart === '.' || firstPart === '?') { + // We matched a device root (e.g. \\\\.\\PHYSICALDRIVE0) + device = `\\\\${firstPart}`; + rootEnd = 4; + } else if (j === len) { + // We matched a UNC root only + // Return the normalized version of the UNC root since there + // is nothing left to process + return `\\\\${firstPart}\\${StringPrototypeSlice(path, last)}\\`; + } else { + // We matched a UNC root with leftovers + device = + `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`; + rootEnd = j; + } } } } diff --git a/src/path.cc b/src/path.cc index 5fb745832e492d..1f88e38a857171 100644 --- a/src/path.cc +++ b/src/path.cc @@ -170,9 +170,16 @@ std::string PathResolve(Environment* env, j++; } if (j == len || j != last) { - // We matched a UNC root - device = "\\\\" + firstPart + "\\" + path.substr(last, j - last); - rootEnd = j; + if (firstPart != "." && firstPart != "?") { + // We matched a UNC root + device = + "\\\\" + firstPart + "\\" + path.substr(last, j - last); + rootEnd = j; + } else { + // We matched a device root (e.g. \\\\.\\PHYSICALDRIVE0) + device = "\\\\" + firstPart; + rootEnd = 4; + } } } } diff --git a/test/cctest/test_path.cc b/test/cctest/test_path.cc index 16bd9872f3b035..9e860d02cf77bd 100644 --- a/test/cctest/test_path.cc +++ b/test/cctest/test_path.cc @@ -39,6 +39,10 @@ TEST_F(PathTest, PathResolve) { EXPECT_EQ( PathResolve(*env, {"C:\\foo\\tmp.3\\", "..\\tmp.3\\cycles\\root.js"}), "C:\\foo\\tmp.3\\cycles\\root.js"); + EXPECT_EQ(PathResolve(*env, {"\\\\.\\PHYSICALDRIVE0"}), + "\\\\.\\PHYSICALDRIVE0"); + EXPECT_EQ(PathResolve(*env, {"\\\\?\\PHYSICALDRIVE0"}), + "\\\\?\\PHYSICALDRIVE0"); #else EXPECT_EQ(PathResolve(*env, {"/var/lib", "../", "file/"}), "/var/file"); EXPECT_EQ(PathResolve(*env, {"/var/lib", "/../", "file/"}), "/file"); diff --git a/test/parallel/test-path-makelong.js b/test/parallel/test-path-makelong.js index 7a4783953c8fde..4a6589cb6b1a45 100644 --- a/test/parallel/test-path-makelong.js +++ b/test/parallel/test-path-makelong.js @@ -79,7 +79,7 @@ assert.strictEqual(path.win32.toNamespacedPath('\\\\foo\\bar'), '\\\\?\\UNC\\foo\\bar\\'); assert.strictEqual(path.win32.toNamespacedPath('//foo//bar'), '\\\\?\\UNC\\foo\\bar\\'); -assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo\\'); +assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo'); assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\c:\\Windows/System'), '\\\\?\\c:\\Windows\\System'); assert.strictEqual(path.win32.toNamespacedPath(null), null); assert.strictEqual(path.win32.toNamespacedPath(true), true); diff --git a/test/parallel/test-path-normalize.js b/test/parallel/test-path-normalize.js index 8f6d822241bf14..8b537676dbf45d 100644 --- a/test/parallel/test-path-normalize.js +++ b/test/parallel/test-path-normalize.js @@ -40,6 +40,8 @@ assert.strictEqual( '..\\..\\..\\..\\baz' ); assert.strictEqual(path.win32.normalize('foo/bar\\baz'), 'foo\\bar\\baz'); +assert.strictEqual(path.win32.normalize('\\\\.\\foo'), '\\\\.\\foo'); +assert.strictEqual(path.win32.normalize('\\\\.\\foo\\'), '\\\\.\\foo\\'); // Tests related to CVE-2024-36139. Path traversal should not result in changing // the root directory on Windows. @@ -58,10 +60,10 @@ 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('\\\\?\\test/../?/D:/Test'), '\\\\?\\?\\D:\\Test'); +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'), '\\\\.\\?\\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'), diff --git a/test/parallel/test-path-resolve.js b/test/parallel/test-path-resolve.js index fc704b815764b0..088ed4b3ff183f 100644 --- a/test/parallel/test-path-resolve.js +++ b/test/parallel/test-path-resolve.js @@ -35,6 +35,8 @@ const resolveTests = [ [['c:/', '///some//dir'], 'c:\\some\\dir'], [['C:\\foo\\tmp.3\\', '..\\tmp.3\\cycles\\root.js'], 'C:\\foo\\tmp.3\\cycles\\root.js'], + [['\\\\.\\PHYSICALDRIVE0'], '\\\\.\\PHYSICALDRIVE0'], + [['\\\\?\\PHYSICALDRIVE0'], '\\\\?\\PHYSICALDRIVE0'], ], ], [ path.posix.resolve, From b6033a023945029cc089936b19a67815bd2b0260 Mon Sep 17 00:00:00 2001 From: Rafael Gonzaga Date: Sat, 9 Aug 2025 09:54:53 -0300 Subject: [PATCH 2/2] lib: handle windows reserved device names on UNC We have found that UNC paths weren't covered when .join/.normalize windows reserved device names (COM1, LPT1). PR-URL: https://github.com/nodejs/node/pull/59286 Reviewed-By: Robert Nagy Reviewed-By: Matteo Collina Reviewed-By: James M Snell --- lib/path.js | 37 +++++++++++++++++++ test/parallel/test-path-join.js | 8 ++++ .../test-path-win32-normalize-device-names.js | 15 ++++++++ 3 files changed, 60 insertions(+) diff --git a/lib/path.js b/lib/path.js index c35e4b005e7550..75eb6e6636885e 100644 --- a/lib/path.js +++ b/lib/path.js @@ -415,6 +415,13 @@ const win32 = { // We matched a device root (e.g. \\\\.\\PHYSICALDRIVE0) device = `\\\\${firstPart}`; rootEnd = 4; + const colonIndex = StringPrototypeIndexOf(path, ':'); + // Special case: handle \\?\COM1: or similar reserved device paths + const possibleDevice = StringPrototypeSlice(path, 4, colonIndex + 1); + if (isWindowsReservedName(possibleDevice, possibleDevice.length - 1)) { + device = `\\\\?\\${possibleDevice}`; + rootEnd = 4 + possibleDevice.length; + } } else if (j === len) { // We matched a UNC root only // Return the normalized version of the UNC root since there @@ -573,6 +580,36 @@ const win32 = { joined = `\\${StringPrototypeSlice(joined, slashCount)}`; } + // Skip normalization when reserved device names are present + const parts = []; + let part = ''; + + for (let i = 0; i < joined.length; i++) { + if (joined[i] === '\\') { + if (part) parts.push(part); + part = ''; + // Skip consecutive backslashes + while (i + 1 < joined.length && joined[i + 1] === '\\') i++; + } else { + part += joined[i]; + } + } + // Add the final part if any + if (part) parts.push(part); + + // Check if any part has a Windows reserved name + if (parts.some((p) => { + const colonIndex = StringPrototypeIndexOf(p, ':'); + return colonIndex !== -1 && isWindowsReservedName(p, colonIndex); + })) { + // Replace forward slashes with backslashes + let result = ''; + for (let i = 0; i < joined.length; i++) { + result += joined[i] === '/' ? '\\' : joined[i]; + } + return result; + } + return win32.normalize(joined); }, diff --git a/test/parallel/test-path-join.js b/test/parallel/test-path-join.js index b8d6375989a75e..ec4f5fea7097a5 100644 --- a/test/parallel/test-path-join.js +++ b/test/parallel/test-path-join.js @@ -110,6 +110,14 @@ joinTests.push([ [['c:.', 'file'], 'c:file'], [['c:', '/'], 'c:\\'], [['c:', 'file'], 'c:\\file'], + // UNC path join tests (Windows) + [['\\server\\share', 'file.txt'], '\\server\\share\\file.txt'], + [['\\server\\share', 'folder', 'another.txt'], '\\server\\share\\folder\\another.txt'], + [['\\server\\share', 'COM1:'], '\\server\\share\\COM1:'], + [['\\server\\share', 'path', 'LPT1:'], '\\server\\share\\path\\LPT1:'], + [['\\fileserver\\public\\uploads', 'CON:..\\..\\..\\private\\db.conf'], + '\\fileserver\\public\\uploads\\CON:..\\..\\..\\private\\db.conf'], + // Path traversal in previous versions of Node.js. [['./upload', '/../C:/Windows'], '.\\C:\\Windows'], [['upload', '../', 'C:foo'], '.\\C:foo'], diff --git a/test/parallel/test-path-win32-normalize-device-names.js b/test/parallel/test-path-win32-normalize-device-names.js index 2c6dcf142a2674..b34c9061e56539 100644 --- a/test/parallel/test-path-win32-normalize-device-names.js +++ b/test/parallel/test-path-win32-normalize-device-names.js @@ -9,6 +9,19 @@ if (!common.isWindows) { } const normalizeDeviceNameTests = [ + // UNC paths: \\server\share\... is a Windows UNC path, where 'server' is the network server name and 'share' + // is the shared folder. These are used for network file access and are subject to reserved device name + // checks after the share. + { input: '\\\\server\\share\\COM1:', expected: '\\\\server\\share\\COM1:' }, + { input: '\\\\server\\share\\PRN:', expected: '\\\\server\\share\\PRN:' }, + { input: '\\\\server\\share\\AUX:', expected: '\\\\server\\share\\AUX:' }, + { input: '\\\\server\\share\\LPT1:', expected: '\\\\server\\share\\LPT1:' }, + { input: '\\\\server\\share\\COM1:\\foo\\bar', expected: '\\\\server\\share\\COM1:\\foo\\bar' }, + { input: '\\\\server\\share\\path\\COM1:', expected: '\\\\server\\share\\path\\COM1:' }, + { input: '\\\\server\\share\\COM1:..\\..\\..\\..\\Windows', expected: '\\\\server\\share\\Windows' }, + { input: '\\\\server\\share\\path\\to\\LPT9:..\\..\\..\\..\\..\\..\\..\\..\\..\\file.txt', + expected: '\\\\server\\share\\file.txt' }, + { input: 'CON', expected: 'CON' }, { input: 'con', expected: 'con' }, { input: 'CON:', expected: '.\\CON:.' }, @@ -81,6 +94,8 @@ const normalizeDeviceNameTests = [ // Test cases from original vulnerability reports or similar scenarios { input: 'COM1:.\\..\\..\\foo.js', expected: '.\\COM1:..\\..\\foo.js' }, { input: 'LPT1:.\\..\\..\\another.txt', expected: '.\\LPT1:..\\..\\another.txt' }, + // UNC paths + { input: '\\\\?\\COM1:.\\..\\..\\foo2.js', expected: '\\\\?\\COM1:\\foo2.js' }, // Paths with device names not at the beginning { input: 'C:\\CON', expected: 'C:\\CON' },