Merge pull request #581 from AikidoSec/fix-url-path-traversal-bypass

Fix url path traversal bypass

Author: hansott

library/sinks/FileSystem.test.ts

@@ -201,6 +201,82 @@ t.test("it works", async (t) => {
     );
   });
 
+  runWithContext(
+    {
+      remoteAddress: "::1",
+      method: "POST",
+      url: "http://localhost:4000",
+      query: {
+        q: ".\t./etc/passwd",
+      },
+      headers: {},
+      body: {},
+      cookies: {},
+      routeParams: {},
+      source: "express",
+      route: "/posts/:id",
+    },
+    () => {
+      throws(
+        () =>
+          rename(
+            new URL("file:///.\t./etc/passwd"),
+            "../test123.txt",
+            () => {}
+          ),
+        "Zen has blocked a path traversal attack: fs.rename(...) originating from query.q"
+      );
+    }
+  );
+
+  runWithContext(
+    {
+      remoteAddress: "::1",
+      method: "POST",
+      url: "http://localhost:4000",
+      query: {
+        q: "test/test.txt",
+      },
+      headers: {},
+      body: {},
+      cookies: {},
+      routeParams: {},
+      source: "express",
+      route: "/posts/:id",
+    },
+    () => {
+      rename(new URL("file:///test/test.txt"), "../test123.txt", () => {});
+    }
+  );
+
+  runWithContext(
+    {
+      remoteAddress: "::1",
+      method: "POST",
+      url: "http://localhost:4000",
+      query: {
+        q: ".\t\t./etc/passwd",
+      },
+      headers: {},
+      body: {},
+      cookies: {},
+      routeParams: {},
+      source: "express",
+      route: "/posts/:id",
+    },
+    () => {
+      throws(
+        () =>
+          rename(
+            new URL("file:///.\t\t./etc/passwd"),
+            "../test123.txt",
+            () => {}
+          ),
+        "Zen has blocked a path traversal attack: fs.rename(...) originating from query.q"
+      );
+    }
+  );
+
   // Ignores malformed URLs
   runWithContext(
     { ...unsafeContext, body: { file: { matches: "../%" } } },

library/vulnerabilities/path-traversal/checkContextForPathTraversal.test.ts

@@ -138,3 +138,40 @@ t.test("it ignores invalid filename type", async () => {
     undefined
   );
 });
+
+t.test("it works with control characters removed by new URL", async (t) => {
+  const queries = [".\t./etc/passwd", ".\n./etc/passwd", ".\r./etc/passwd"];
+
+  for (const query of queries) {
+    t.same(
+      checkContextForPathTraversal({
+        filename: new URL(`file:///test/${query}`),
+        operation: "operation",
+        context: {
+          cookies: {},
+          headers: {},
+          remoteAddress: "ip",
+          method: "POST",
+          url: "url",
+          query: {
+            q: query,
+          },
+          body: {},
+          routeParams: {},
+          source: "express",
+          route: undefined,
+        },
+      }),
+      {
+        operation: "operation",
+        kind: "path_traversal",
+        source: "query",
+        pathsToPayload: [".q"],
+        metadata: {
+          filename: "/etc/passwd",
+        },
+        payload: query,
+      }
+    );
+  }
+});

library/vulnerabilities/path-traversal/containsUnsafePathParts.test.ts

@@ -0,0 +1,58 @@
+import * as t from "tap";
+import {
+  containsUnsafePathParts,
+  containsUnsafePathPartsUrl,
+} from "./containsUnsafePathParts";
+import { fileURLToPath } from "url";
+
+t.test("not a dangerous path", async () => {
+  t.same(containsUnsafePathParts("test.txt"), false);
+  t.same(containsUnsafePathParts("/var/www/test.txt"), false);
+  t.same(containsUnsafePathParts("./test.txt"), false);
+  t.same(containsUnsafePathParts("test.txt/"), false);
+});
+
+t.test("it detects dangerous path parts", async () => {
+  t.same(containsUnsafePathParts("../test.txt"), true);
+  t.same(containsUnsafePathParts("..\\test.txt"), true);
+  t.same(containsUnsafePathParts("../../test.txt"), true);
+  t.same(containsUnsafePathParts("..\\..\\test.txt"), true);
+  t.same(containsUnsafePathParts("../../../../test.txt"), true);
+  t.same(containsUnsafePathParts("..\\..\\..\\..\\test.txt"), true);
+  t.same(containsUnsafePathParts("/test/../test.txt"), true);
+  t.same(containsUnsafePathParts("/test/..\\test.txt"), true);
+});
+
+t.test(
+  "containsUnsafePathParts does not detect with control characters",
+  async () => {
+    t.same(containsUnsafePathParts("file:///.\t./test.txt"), false);
+    t.same(containsUnsafePathParts("file:///.\n./test.txt"), false);
+    t.same(containsUnsafePathParts("file:///.\r./test.txt"), false);
+  }
+);
+
+t.test("it detects dangerous path parts for URLs", async () => {
+  t.same(containsUnsafePathPartsUrl("/../test.txt"), true);
+  t.same(containsUnsafePathPartsUrl("/..\\test.txt"), true);
+  t.same(containsUnsafePathPartsUrl("file:///../test.txt"), true);
+  t.same(containsUnsafePathPartsUrl("file://..\\test.txt"), true);
+  t.same(containsUnsafePathPartsUrl("file:///.\t./test.txt"), true);
+  t.same(containsUnsafePathPartsUrl("file://.\n./test.txt"), true);
+  t.same(containsUnsafePathPartsUrl("file://.\r./test.txt"), true);
+  t.same(containsUnsafePathPartsUrl("file:///.\t\t./test.txt"), true);
+  t.same(containsUnsafePathPartsUrl("file:///.\t\n./test.txt"), true);
+});
+
+t.test("it only removes some chars from the URL", async () => {
+  t.same(fileURLToPath("file:///.\t./test.txt"), "/test.txt");
+  t.same(fileURLToPath("file:///.\n./test.txt"), "/test.txt");
+  t.same(fileURLToPath("file:///.\r./test.txt"), "/test.txt");
+  t.same(fileURLToPath("file:///.\0./test.txt"), "/.\0./test.txt");
+  t.same(fileURLToPath("file:///.\u0000./test.txt"), "/.\u0000./test.txt");
+  t.same(fileURLToPath("file:///.\v./test.txt"), "/.\v./test.txt");
+  t.same(fileURLToPath("file:///.\f./test.txt"), "/.\f./test.txt");
+  t.same(fileURLToPath("file:///.\b./test.txt"), "/.\b./test.txt");
+  t.same(fileURLToPath("file:///.\t\t./test.txt"), "/test.txt");
+  t.same(fileURLToPath("file:///.\t\n./test.txt"), "/test.txt");
+});

library/vulnerabilities/path-traversal/containsUnsafePathParts.ts

@@ -9,3 +9,17 @@ export function containsUnsafePathParts(filePath: string) {
 
   return false;
 }
+
+/**
+ * This function is used for urls, because they can contain a TAB, carriage return or line feed that is silently removed by the URL constructor.
+ *
+ * The WHATWG URL spec defines the following:
+ * - Remove all ASCII tab or newline from input.
+ * - An ASCII tab or newline is U+0009 TAB, U+000A LF, or U+000D CR.
+ *
+ * See https://url.spec.whatwg.org/#url-parsing
+ */
+export function containsUnsafePathPartsUrl(filePath: string) {
+  const normalized = filePath.replace(/[\t\n\r]/g, "");
+  return containsUnsafePathParts(normalized);
+}

library/vulnerabilities/path-traversal/detectPathTraversal.ts

@@ -1,4 +1,7 @@
-import { containsUnsafePathParts } from "./containsUnsafePathParts";
+import {
+  containsUnsafePathParts,
+  containsUnsafePathPartsUrl,
+} from "./containsUnsafePathParts";
 import { startsWithUnsafePath } from "./unsafePathStart";
 import { fileURLToPath } from "url";
 
@@ -18,7 +21,8 @@ export function detectPathTraversal(
   // The normal check for relative path traversal will fail in this case, because transformed path does not contain ../.
   // For absolute path traversal, we dont need to check the transformed path, because it will always start with /.
   // Also /./ is checked by normal absolute path traversal check (if #219 is merged)
-  if (isUrl && containsUnsafePathParts(userInput)) {
+  // Use containsUnsafePathPartsUrl, because urls can contain a TAB, carriage return or line feed that is silently removed by the URL constructor.
+  if (isUrl && containsUnsafePathPartsUrl(userInput)) {
     const filePathFromUrl = parseAsFileUrl(userInput);
     if (filePathFromUrl && filePath.includes(filePathFromUrl)) {
       return true;