From fb9c084b9701cbd6c804a63bdb931e2869e6dec3 Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Tue, 15 Oct 2024 15:11:37 +0200 Subject: [PATCH 1/2] Avoid double wrapping path --- library/sinks/Path.doubleWrapping.test.ts | 41 ++++++++++++ library/sinks/Path.test.ts | 11 +++- library/sinks/Path.ts | 76 +++++++++++++++++++---- 3 files changed, 116 insertions(+), 12 deletions(-) create mode 100644 library/sinks/Path.doubleWrapping.test.ts diff --git a/library/sinks/Path.doubleWrapping.test.ts b/library/sinks/Path.doubleWrapping.test.ts new file mode 100644 index 000000000..3b846cce4 --- /dev/null +++ b/library/sinks/Path.doubleWrapping.test.ts @@ -0,0 +1,41 @@ +import { join, normalize, resolve } from "path/posix"; +import * as t from "tap"; +import { isWrapped } from "../helpers/wrap"; +import { Path } from "./Path"; +import { createTestAgent } from "../helpers/createTestAgent"; + +t.test("it works", { skip: process.platform === "win32" }, async (t) => { + const agent = createTestAgent(); + + agent.start([new Path()]); + + const { join, resolve, normalize } = require("path/posix"); + + // Path required after path/posix + require("path"); + + const checkForDoubleWrapping = [join, resolve, normalize]; + for (const fn of checkForDoubleWrapping) { + if (isWrapped(fn) && isWrapped(fn.__original)) { + t.fail(`${fn.name} is double wrapped!`); + } + } +}); + +t.test("it works", { skip: process.platform !== "win32" }, async (t) => { + const agent = createTestAgent(); + + agent.start([new Path()]); + + const { join, resolve, normalize } = require("path/win32"); + + // Path required after path/win32 + require("path"); + + const checkForDoubleWrapping = [join, resolve, normalize]; + for (const fn of checkForDoubleWrapping) { + if (isWrapped(fn) && isWrapped(fn.__original)) { + t.fail(`${fn.name} is double wrapped!`); + } + } +}); diff --git a/library/sinks/Path.test.ts b/library/sinks/Path.test.ts index 52b646236..a0c891a4c 100644 --- a/library/sinks/Path.test.ts +++ b/library/sinks/Path.test.ts @@ -1,5 +1,6 @@ import * as t from "tap"; import { Context, runWithContext } from "../agent/Context"; +import { isWrapped } from "../helpers/wrap"; import { Path } from "./Path"; import { createTestAgent } from "../helpers/createTestAgent"; @@ -30,7 +31,8 @@ t.test("it works", async (t) => { agent.start([new Path()]); - const { join, resolve } = require("path"); + // Path required before path/posix and path/win32 + const { join, resolve, normalize } = require("path"); function safeCalls() { t.same(join("test.txt"), "test.txt"); @@ -147,4 +149,11 @@ t.test("it works", async (t) => { "Zen has blocked a path traversal attack: path.normalize(...) originating from body.file.matches" ); }); + + const checkForDoubleWrapping = [join, resolve, normalize]; + for (const fn of checkForDoubleWrapping) { + if (isWrapped(fn) && isWrapped(fn.__original)) { + t.fail(`${fn.name} is double wrapped!`); + } + } }); diff --git a/library/sinks/Path.ts b/library/sinks/Path.ts index 0b971781c..7f51447b6 100644 --- a/library/sinks/Path.ts +++ b/library/sinks/Path.ts @@ -4,8 +4,12 @@ import { wrapExport } from "../agent/hooks/wrapExport"; import { WrapPackageInfo } from "../agent/hooks/WrapPackageInfo"; import { Wrapper } from "../agent/Wrapper"; import { checkContextForPathTraversal } from "../vulnerabilities/path-traversal/checkContextForPathTraversal"; +import type * as path from "path"; export class Path implements Wrapper { + private patchedPosix = false; + private patchedWin32 = false; + private inspectPath(args: unknown[], operation: string) { const context = getContext(); @@ -34,19 +38,69 @@ export class Path implements Wrapper { return undefined; } - wrap(hooks: Hooks): void { + private wrapFunctions(exports: unknown, pkgInfo: WrapPackageInfo) { const functions = ["join", "resolve", "normalize"]; - const onRequire = (exports: any, pkgInfo: WrapPackageInfo) => { - for (const func of functions) { - wrapExport(exports, func, pkgInfo, { - inspectArgs: (args) => this.inspectPath(args, func), - }); - } - }; + for (const func of functions) { + wrapExport(exports, func, pkgInfo, { + inspectArgs: (args) => this.inspectPath(args, func), + }); + } + } + + private isWindows() { + return process.platform === "win32"; + } + + private wrapMainModule(exports: typeof path, pkgInfo: WrapPackageInfo) { + // If `path/win32` or `path/posix` was not required before `path`, we should wrap the functions in `path` + if (!this.patchedWin32 && !this.patchedPosix) { + this.wrapFunctions(exports, pkgInfo); + } + + if (this.isWindows()) { + // `require("path").join` is the same as `require("path/win32").join` + this.patchedWin32 = true; + } else { + // `require("path").join` is the same as `require("path/posix").join` + this.patchedPosix = true; + } + + this.wrapPosix(exports.posix, pkgInfo); + this.wrapWin32(exports.win32, pkgInfo); + } + + private wrapPosix(exports: unknown, pkgInfo: WrapPackageInfo) { + if (this.patchedPosix) { + return; + } + + this.wrapFunctions(exports, pkgInfo); + + this.patchedPosix = true; + } + + private wrapWin32(exports: unknown, pkgInfo: WrapPackageInfo) { + if (this.patchedWin32) { + return; + } + + this.wrapFunctions(exports, pkgInfo); + + this.patchedWin32 = true; + } + + wrap(hooks: Hooks): void { + hooks + .addBuiltinModule("path") + .onRequire((exports, pkgInfo) => this.wrapMainModule(exports, pkgInfo)); + + hooks + .addBuiltinModule("path/posix") + .onRequire((exports, pkgInfo) => this.wrapPosix(exports, pkgInfo)); - hooks.addBuiltinModule("path").onRequire(onRequire); - hooks.addBuiltinModule("path/posix").onRequire(onRequire); - hooks.addBuiltinModule("path/win32").onRequire(onRequire); + hooks + .addBuiltinModule("path/win32") + .onRequire((exports, pkgInfo) => this.wrapWin32(exports, pkgInfo)); } } From f9c1395b6fb452629b9c29f10ddc79a20d6ffad8 Mon Sep 17 00:00:00 2001 From: Hans Ott Date: Wed, 16 Oct 2024 10:44:13 +0200 Subject: [PATCH 2/2] Extract helper for checking if it's windows --- library/helpers/isWindows.ts | 3 ++ library/sinks/Path.doubleWrapping.test.ts | 53 +++++++++++++---------- library/sinks/Path.ts | 7 +-- 3 files changed, 36 insertions(+), 27 deletions(-) create mode 100644 library/helpers/isWindows.ts diff --git a/library/helpers/isWindows.ts b/library/helpers/isWindows.ts new file mode 100644 index 000000000..3cc1885e2 --- /dev/null +++ b/library/helpers/isWindows.ts @@ -0,0 +1,3 @@ +export function isWindows() { + return process.platform === "win32"; +} diff --git a/library/sinks/Path.doubleWrapping.test.ts b/library/sinks/Path.doubleWrapping.test.ts index 3b846cce4..ee9c22325 100644 --- a/library/sinks/Path.doubleWrapping.test.ts +++ b/library/sinks/Path.doubleWrapping.test.ts @@ -1,41 +1,50 @@ import { join, normalize, resolve } from "path/posix"; import * as t from "tap"; +import { isWindows } from "../helpers/isWindows"; import { isWrapped } from "../helpers/wrap"; import { Path } from "./Path"; import { createTestAgent } from "../helpers/createTestAgent"; -t.test("it works", { skip: process.platform === "win32" }, async (t) => { - const agent = createTestAgent(); +t.test( + "it works", + { skip: isWindows() ? "path is not the same as path/posix" : false }, + async (t) => { + const agent = createTestAgent(); - agent.start([new Path()]); + agent.start([new Path()]); - const { join, resolve, normalize } = require("path/posix"); + const { join, resolve, normalize } = require("path/posix"); - // Path required after path/posix - require("path"); + // Path required after path/posix + require("path"); - const checkForDoubleWrapping = [join, resolve, normalize]; - for (const fn of checkForDoubleWrapping) { - if (isWrapped(fn) && isWrapped(fn.__original)) { - t.fail(`${fn.name} is double wrapped!`); + const checkForDoubleWrapping = [join, resolve, normalize]; + for (const fn of checkForDoubleWrapping) { + if (isWrapped(fn) && isWrapped(fn.__original)) { + t.fail(`${fn.name} is double wrapped!`); + } } } -}); +); -t.test("it works", { skip: process.platform !== "win32" }, async (t) => { - const agent = createTestAgent(); +t.test( + "it works", + { skip: !isWindows() ? "path is not the same as path/win32" : false }, + async (t) => { + const agent = createTestAgent(); - agent.start([new Path()]); + agent.start([new Path()]); - const { join, resolve, normalize } = require("path/win32"); + const { join, resolve, normalize } = require("path/win32"); - // Path required after path/win32 - require("path"); + // Path required after path/win32 + require("path"); - const checkForDoubleWrapping = [join, resolve, normalize]; - for (const fn of checkForDoubleWrapping) { - if (isWrapped(fn) && isWrapped(fn.__original)) { - t.fail(`${fn.name} is double wrapped!`); + const checkForDoubleWrapping = [join, resolve, normalize]; + for (const fn of checkForDoubleWrapping) { + if (isWrapped(fn) && isWrapped(fn.__original)) { + t.fail(`${fn.name} is double wrapped!`); + } } } -}); +); diff --git a/library/sinks/Path.ts b/library/sinks/Path.ts index 7f51447b6..a1d19058b 100644 --- a/library/sinks/Path.ts +++ b/library/sinks/Path.ts @@ -3,6 +3,7 @@ import { Hooks } from "../agent/hooks/Hooks"; import { wrapExport } from "../agent/hooks/wrapExport"; import { WrapPackageInfo } from "../agent/hooks/WrapPackageInfo"; import { Wrapper } from "../agent/Wrapper"; +import { isWindows } from "../helpers/isWindows"; import { checkContextForPathTraversal } from "../vulnerabilities/path-traversal/checkContextForPathTraversal"; import type * as path from "path"; @@ -48,17 +49,13 @@ export class Path implements Wrapper { } } - private isWindows() { - return process.platform === "win32"; - } - private wrapMainModule(exports: typeof path, pkgInfo: WrapPackageInfo) { // If `path/win32` or `path/posix` was not required before `path`, we should wrap the functions in `path` if (!this.patchedWin32 && !this.patchedPosix) { this.wrapFunctions(exports, pkgInfo); } - if (this.isWindows()) { + if (isWindows()) { // `require("path").join` is the same as `require("path/win32").join` this.patchedWin32 = true; } else {