From b0525edd6fb2fa414407ec73c981051d692d1c26 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 17 Sep 2024 00:08:02 +0100 Subject: [PATCH] feat: warn when using `--allow-run` with no allow list (#25215) --- cli/args/mod.rs | 23 +++++++++++++++++++ tests/napi/cleanup_hook_test.js | 4 +--- .../deny_run_binary_absolute_path/main.out | 1 + .../permission/path_not_permitted/main.out | 4 ++-- .../permission/path_not_permitted/main.ts | 7 ++++-- .../permission/path_not_permitted/sub.ts | 6 ++--- .../__test__.jsonc | 8 +++++++ .../run/allow_run_insecure_warnings/main.ts | 0 .../no_allow_list.out | 1 + .../specs/test/captured_output/__test__.jsonc | 2 +- .../run/deny_some_permission_args.out | 1 + 11 files changed, 46 insertions(+), 11 deletions(-) create mode 100644 tests/specs/run/allow_run_insecure_warnings/__test__.jsonc create mode 100644 tests/specs/run/allow_run_insecure_warnings/main.ts create mode 100644 tests/specs/run/allow_run_insecure_warnings/no_allow_list.out diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 0e4004a530..db8cf149e6 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -809,6 +809,8 @@ impl CliOptions { } } + warn_insecure_allow_run_flags(&flags); + let maybe_lockfile = maybe_lockfile.filter(|_| !force_global_cache); let deno_dir_provider = Arc::new(DenoDirProvider::new(flags.cache_path.clone())); @@ -1688,6 +1690,27 @@ impl CliOptions { } } +/// Warns for specific uses of `--allow-run`. This function is not +/// intended to catch every single possible insecure use of `--allow-run`, +/// but is just an attempt to discourage some common pitfalls. +fn warn_insecure_allow_run_flags(flags: &Flags) { + let permissions = &flags.permissions; + if permissions.allow_all { + return; + } + let Some(allow_run_list) = permissions.allow_run.as_ref() else { + return; + }; + + // discourage using --allow-run without an allow list + if allow_run_list.is_empty() { + log::warn!( + "{} --allow-run can be trivially exploited. Prefer specifying an allow list (https://docs.deno.com/runtime/fundamentals/security/#running-subprocesses)", + colors::yellow("Warning") + ); + } +} + /// Resolves the path to use for a local node_modules folder. fn resolve_node_modules_folder( cwd: &Path, diff --git a/tests/napi/cleanup_hook_test.js b/tests/napi/cleanup_hook_test.js index 017674ad49..2c1f73e12b 100644 --- a/tests/napi/cleanup_hook_test.js +++ b/tests/napi/cleanup_hook_test.js @@ -17,9 +17,7 @@ if (import.meta.main) { "--config", Deno.realPathSync("../config/deno.json"), "--no-lock", - "--allow-read", - "--allow-run", - "--allow-ffi", + "-A", "--unstable-ffi", import.meta.url, ], diff --git a/tests/specs/permission/deny_run_binary_absolute_path/main.out b/tests/specs/permission/deny_run_binary_absolute_path/main.out index 45b2283879..7f11e7880d 100644 --- a/tests/specs/permission/deny_run_binary_absolute_path/main.out +++ b/tests/specs/permission/deny_run_binary_absolute_path/main.out @@ -1,3 +1,4 @@ +Warning --allow-run can be trivially exploited. Prefer specifying an allow list (https://docs.deno.com/runtime/fundamentals/security/#running-subprocesses) NotCapable: Requires run access to "deno", run again with the --allow-run flag at [WILDCARD] { name: "NotCapable" diff --git a/tests/specs/permission/path_not_permitted/main.out b/tests/specs/permission/path_not_permitted/main.out index b057d0a63c..02e5b937f9 100644 --- a/tests/specs/permission/path_not_permitted/main.out +++ b/tests/specs/permission/path_not_permitted/main.out @@ -1,10 +1,10 @@ Running... -NotCapable: Requires run access to "deno", run again with the --allow-run flag +NotCapable: Requires run access to "binary", run again with the --allow-run flag [WILDCARD] at file:///[WILDLINE]/sub.ts:15:5 { name: "NotCapable" } -NotCapable: Requires run access to "deno", run again with the --allow-run flag +NotCapable: Requires run access to "binary", run again with the --allow-run flag [WILDCARD] at file:///[WILDLINE]/sub.ts:23:22 { name: "NotCapable" diff --git a/tests/specs/permission/path_not_permitted/main.ts b/tests/specs/permission/path_not_permitted/main.ts index 0cc141e7ac..0587db916b 100644 --- a/tests/specs/permission/path_not_permitted/main.ts +++ b/tests/specs/permission/path_not_permitted/main.ts @@ -1,4 +1,4 @@ -const binaryName = Deno.build.os === "windows" ? "deno.exe" : "deno"; +const binaryName = Deno.build.os === "windows" ? "binary.exe" : "binary"; Deno.copyFileSync(Deno.execPath(), binaryName); console.log("Running..."); @@ -9,9 +9,12 @@ new Deno.Command( "run", "--allow-write", "--allow-read", - `--allow-run=deno`, + `--allow-run=binary`, "sub.ts", ], + env: { + PATH: Deno.cwd(), + }, stderr: "inherit", stdout: "inherit", }, diff --git a/tests/specs/permission/path_not_permitted/sub.ts b/tests/specs/permission/path_not_permitted/sub.ts index ea527a938b..e501b5b7b6 100644 --- a/tests/specs/permission/path_not_permitted/sub.ts +++ b/tests/specs/permission/path_not_permitted/sub.ts @@ -1,4 +1,4 @@ -const binaryName = Deno.build.os === "windows" ? "deno.exe" : "deno"; +const binaryName = Deno.build.os === "windows" ? "binary.exe" : "binary"; const pathSep = Deno.build.os === "windows" ? "\\" : "/"; Deno.mkdirSync("subdir"); @@ -6,7 +6,7 @@ Deno.copyFileSync(binaryName, "subdir/" + binaryName); try { const commandResult = new Deno.Command( - "deno", + "binary", { env: { "PATH": Deno.cwd() + pathSep + "subdir" }, stdout: "inherit", @@ -22,7 +22,7 @@ try { try { const child = Deno.run( { - cmd: ["deno"], + cmd: ["binary"], env: { "PATH": Deno.cwd() + pathSep + "subdir" }, stdout: "inherit", stderr: "inherit", diff --git a/tests/specs/run/allow_run_insecure_warnings/__test__.jsonc b/tests/specs/run/allow_run_insecure_warnings/__test__.jsonc new file mode 100644 index 0000000000..b64146ee95 --- /dev/null +++ b/tests/specs/run/allow_run_insecure_warnings/__test__.jsonc @@ -0,0 +1,8 @@ +{ + "tests": { + "no_allow_list": { + "args": "run --allow-run main.ts", + "output": "no_allow_list.out" + } + } +} diff --git a/tests/specs/run/allow_run_insecure_warnings/main.ts b/tests/specs/run/allow_run_insecure_warnings/main.ts new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/specs/run/allow_run_insecure_warnings/no_allow_list.out b/tests/specs/run/allow_run_insecure_warnings/no_allow_list.out new file mode 100644 index 0000000000..277d0036cb --- /dev/null +++ b/tests/specs/run/allow_run_insecure_warnings/no_allow_list.out @@ -0,0 +1 @@ +Warning --allow-run can be trivially exploited. Prefer specifying an allow list (https://docs.deno.com/runtime/fundamentals/security/#running-subprocesses) diff --git a/tests/specs/test/captured_output/__test__.jsonc b/tests/specs/test/captured_output/__test__.jsonc index d620f61aa3..f56c54555f 100644 --- a/tests/specs/test/captured_output/__test__.jsonc +++ b/tests/specs/test/captured_output/__test__.jsonc @@ -1,5 +1,5 @@ { - "args": "test --allow-run --allow-read captured_output.ts", + "args": "test -A captured_output.ts", "output": "main.out", "envs": { "NO_COLOR": "1" }, "exitCode": 0 diff --git a/tests/testdata/run/deny_some_permission_args.out b/tests/testdata/run/deny_some_permission_args.out index abb5274eed..fe3e57d697 100644 --- a/tests/testdata/run/deny_some_permission_args.out +++ b/tests/testdata/run/deny_some_permission_args.out @@ -1,3 +1,4 @@ +Warning --allow-run can be trivially exploited. Prefer specifying an allow list (https://docs.deno.com/runtime/fundamentals/security/#running-subprocesses) PermissionStatus { state: "granted", onchange: null, partial: true } PermissionStatus { state: "denied", onchange: null } PermissionStatus { state: "granted", onchange: null }