Fix unsafe path start on Windows

AikidoSec · Sep 26, 2024 · 84ea85b · 84ea85b
1 parent 0a755f0
commit 84ea85b
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 24 deletions.
diff --git a/library/sinks/FileSystem.test.ts b/library/sinks/FileSystem.test.ts
@@ -100,7 +100,7 @@ t.test("it works", async (t) => {
     if (!isWindows) {
       rename(new URL("file:///test123.txt"), "test2.txt", (err) => {});
     } else {
-      rename(new URL("file:///c:/test123.txt"), "test2.txt", (err) => {});
+      rename(new URL("file:///x:/test123.txt"), "test2.txt", (err) => {});
     }
     rename(Buffer.from("./test123.txt"), "test2.txt", (err) => {});
   };
@@ -184,7 +184,7 @@ t.test("it works", async (t) => {
       throws(
         () =>
           rename(
-            new URL("file:///C:/../test.txt"),
+            new URL("file:///x:/../test.txt"),
             "../test2.txt",
             (err) => {}
           ),
@@ -194,7 +194,7 @@ t.test("it works", async (t) => {
       throws(
         () =>
           rename(
-            new URL("file:///C:/./../test.txt"),
+            new URL("file:///x:/./../test.txt"),
             "../test2.txt",
             (err) => {}
           ),
@@ -204,7 +204,7 @@ t.test("it works", async (t) => {
       throws(
         () =>
           rename(
-            new URL("file:///C:/../../test.txt"),
+            new URL("file:///X:/../../test.txt"),
             "../test2.txt",
             (err) => {}
           ),
@@ -244,7 +244,7 @@ t.test("it works", async (t) => {
         rename(new URL("file:///../../test.txt"), "../test2.txt", (err) => {});
       } else {
         rename(
-          new URL("file:/C://../../test.txt"),
+          new URL("file:/x://../../test.txt"),
           "../test2.txt",
           (err) => {}
         );

diff --git a/library/sinks/Path.test.ts b/library/sinks/Path.test.ts
@@ -47,7 +47,7 @@ t.test("it works", async (t) => {
     if (!isWindows) {
       t.same(join("/app", "/etc/data"), resolve("/app/etc/data"));
     } else {
-      t.same(join("c:/app", "/etc/data"), resolve("c:/app/etc/data"));
+      t.same(join("x:/app", "/etc/data"), resolve("x:/app/etc/data"));
     }
   }
 
@@ -98,8 +98,8 @@ t.test("it works", async (t) => {
     safeCalls();
   });
 
-  runWithContext(unsafeAbsoluteContext, () => {
-    if (!isWindows) {
+  if (!isWindows) {
+    runWithContext(unsafeAbsoluteContext, () => {
       t.throws(
         () => join("/etc/", "test.txt"),
         "Zen has blocked a Path traversal: fs.join(...) originating from body.file.matches"
@@ -109,11 +109,21 @@ t.test("it works", async (t) => {
         () => resolve("/etc/some_directory", "test.txt"),
         "Zen has blocked a Path traversal: fs.resolve(...) originating from body.file.matches"
       );
-    } else {
-      t.throws(
-        () => join("C:/etc/", "test.txt"),
-        "Zen has blocked a Path traversal: fs.join(...) originating from body.file.matches"
-      );
-    }
-  });
+    });
+  } else {
+    runWithContext(
+      {
+        ...unsafeAbsoluteContext,
+        ...{
+          body: { file: { matches: "X:/etc/" } },
+        },
+      },
+      () => {
+        t.throws(
+          () => resolve("X:/etc/", "test.txt"),
+          "Zen has blocked a Path traversal: fs.join(...) originating from body.file.matches"
+        );
+      }
+    );
+  }
 });
diff --git a/library/vulnerabilities/path-traversal/detectPathTraversal.test.ts b/library/vulnerabilities/path-traversal/detectPathTraversal.test.ts
@@ -126,6 +126,6 @@ t.test(
   "windows drive letter",
   { skip: process.platform !== "win32" ? "Windows only" : false },
   async () => {
-    t.same(detectPathTraversal("C:\\file.txt", "C:\\"), true);
+    t.same(detectPathTraversal("X:\\file.txt", "X:\\"), true);
   }
 );
diff --git a/library/vulnerabilities/path-traversal/unsafePathStart.ts b/library/vulnerabilities/path-traversal/unsafePathStart.ts
@@ -22,7 +22,6 @@ const linuxRootFolders = [
   "/usr/",
   "/var/",
 ];
-const dangerousPathStarts = [...linuxRootFolders, "c:/", "c:\\"];
 
 export function startsWithUnsafePath(filePath: string, userInput: string) {
   // Check if path is relative (not absolute or drive letter path)
@@ -38,13 +37,28 @@ export function startsWithUnsafePath(filePath: string, userInput: string) {
 
   const normalizedPath = origResolve(filePath).toLowerCase();
   const normalizedUserInput = origResolve(userInput).toLowerCase();
-  for (const dangerousStart of dangerousPathStarts) {
-    if (
-      normalizedPath.startsWith(dangerousStart) &&
-      normalizedPath.startsWith(normalizedUserInput)
-    ) {
-      return true;
-    }
+
+  return isDangerous(normalizedPath, normalizedUserInput);
+}
+
+function isDangerous(normalizedPath: string, normalizedUserInput: string) {
+  // Check if normalizedPath starts with normalizedUserInput
+  if (!normalizedPath.startsWith(normalizedUserInput)) {
+    return false;
+  }
+
+  const foundLinuxRoot = linuxRootFolders.find((folder) =>
+    normalizedPath.startsWith(folder)
+  );
+
+  if (foundLinuxRoot) {
+    return true;
   }
+
+  // Check for windows drive letter
+  if (/^[a-z]:(\\|\/)/i.test(normalizedPath)) {
+    return true;
+  }
+
   return false;
 }