From 01d499771bf4d380582b31a7f0504b1515e9ff89 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 28 Aug 2024 12:44:53 -0400 Subject: [PATCH 1/5] fix(permissions): disallow any ld_ prefixed env var without full --allow-run permissions --- runtime/ops/process.rs | 10 +++++----- tests/specs/run/ld_preload/env_arg.out | 2 +- tests/specs/run/ld_preload/set_with_allow_env.out | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/runtime/ops/process.rs b/runtime/ops/process.rs index 564092454f9b52..f824d290e0777d 100644 --- a/runtime/ops/process.rs +++ b/runtime/ops/process.rs @@ -234,15 +234,15 @@ fn create_command( permissions.check_run(&args.cmd, api_name)?; // error the same on all platforms if permissions.check_run_all(api_name).is_err() - && (args.env.iter().any(|(k, _)| k.trim() == "LD_PRELOAD") + && (args.env.iter().any(|(k, _)| k.trim().starts_with("LD_")) || !args.clear_env - && std::env::vars().any(|(k, _)| k.trim() == "LD_PRELOAD")) + && std::env::vars().any(|(k, _)| k.trim().starts_with("LD_"))) { - // we don't allow users to launch subprocesses with the LD_PRELOAD - // env var set because this allows executing any code + // we don't allow users to launch subprocesses with any LD_ env var set + // because this allows executing code (ex. LD_PRELOAD) return Err(deno_core::error::custom_error( "PermissionDenied", - "Requires --allow-all permissions to spawn subprocess with LD_PRELOAD environment variable." + "Requires --allow-all permissions to spawn subprocess with any LD_* environment variable." )); } } diff --git a/tests/specs/run/ld_preload/env_arg.out b/tests/specs/run/ld_preload/env_arg.out index fbf37014ae78b4..0dcc2f36fb6ea9 100644 --- a/tests/specs/run/ld_preload/env_arg.out +++ b/tests/specs/run/ld_preload/env_arg.out @@ -1,4 +1,4 @@ -error: Uncaught (in promise) PermissionDenied: Requires --allow-all permissions to spawn subprocess with LD_PRELOAD environment variable. +error: Uncaught (in promise) PermissionDenied: Requires --allow-all permissions to spawn subprocess with any LD_* environment variable. }).spawn(); ^ at [WILDCARD] diff --git a/tests/specs/run/ld_preload/set_with_allow_env.out b/tests/specs/run/ld_preload/set_with_allow_env.out index 2e92763ddac227..0f9af383c873f0 100644 --- a/tests/specs/run/ld_preload/set_with_allow_env.out +++ b/tests/specs/run/ld_preload/set_with_allow_env.out @@ -1,4 +1,4 @@ -error: Uncaught (in promise) PermissionDenied: Requires --allow-all permissions to spawn subprocess with LD_PRELOAD environment variable. +error: Uncaught (in promise) PermissionDenied: Requires --allow-all permissions to spawn subprocess with any LD_* environment variable. const output = new Deno.Command("echo").spawn(); ^ at [WILDCARD] From 45872018f2efcbc8cfe0daaaae33bfe21d85934f Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 28 Aug 2024 13:36:32 -0400 Subject: [PATCH 2/5] Deny for DYLD_*, surface env var name that was set, and allow unsetting when spawning the command --- runtime/ops/process.rs | 58 +++++++++++++++---- tests/integration/run_tests.rs | 1 + tests/specs/run/ld_preload/__test__.jsonc | 4 ++ tests/specs/run/ld_preload/env_arg.out | 2 +- .../run/ld_preload/set_with_allow_env.out | 2 +- tests/unit/os_test.ts | 1 + 6 files changed, 56 insertions(+), 12 deletions(-) diff --git a/runtime/ops/process.rs b/runtime/ops/process.rs index f824d290e0777d..25edfe662e4753 100644 --- a/runtime/ops/process.rs +++ b/runtime/ops/process.rs @@ -229,21 +229,59 @@ fn create_command( mut args: SpawnArgs, api_name: &str, ) -> Result { + fn get_requires_allow_all_env_var(args: &SpawnArgs) -> Option> { + fn requires_allow_all(key: &str) -> bool { + let key = key.trim(); + // we could be more targted here, but there are quite a lot of + // LD_* and DYLD_* env variables + key.starts_with("LD_") || key.starts_with("DYLD_") + } + + /// Checks if the user set this env var to an empty + /// string in order to clear it. + fn args_has_empty_env_value(args: &SpawnArgs, key_name: &str) -> bool { + args + .env + .iter() + .find(|(k, _)| k == key_name) + .map(|(_, v)| v.trim().is_empty()) + .unwrap_or(false) + } + + if let Some((key, _)) = args + .env + .iter() + .find(|(k, v)| requires_allow_all(k) && !v.trim().is_empty()) + { + return Some(key.into()); + } + + if !args.clear_env { + if let Some((key, _)) = std::env::vars().find(|(k, v)| { + requires_allow_all(k) + && !v.trim().is_empty() + && !args_has_empty_env_value(args, k) + }) { + return Some(key.into()); + } + } + + None + } + { let permissions = state.borrow_mut::(); permissions.check_run(&args.cmd, api_name)?; - // error the same on all platforms - if permissions.check_run_all(api_name).is_err() - && (args.env.iter().any(|(k, _)| k.trim().starts_with("LD_")) - || !args.clear_env - && std::env::vars().any(|(k, _)| k.trim().starts_with("LD_"))) - { - // we don't allow users to launch subprocesses with any LD_ env var set - // because this allows executing code (ex. LD_PRELOAD) - return Err(deno_core::error::custom_error( + if permissions.check_run_all(api_name).is_err() { + // error the same on all platforms + if let Some(name) = get_requires_allow_all_env_var(&args) { + // we don't allow users to launch subprocesses with any LD_ or DYLD_* + // env vars set because this allows executing code (ex. LD_PRELOAD) + return Err(deno_core::error::custom_error( "PermissionDenied", - "Requires --allow-all permissions to spawn subprocess with any LD_* environment variable." + format!("Requires --allow-all permissions to spawn subprocess with {} environment variable.", name) )); + } } } diff --git a/tests/integration/run_tests.rs b/tests/integration/run_tests.rs index ad20b77e1367f8..40b9e15a4888e1 100644 --- a/tests/integration/run_tests.rs +++ b/tests/integration/run_tests.rs @@ -501,6 +501,7 @@ itest!(_088_dynamic_import_already_evaluating { // TODO(bartlomieju): remove --unstable once Deno.Command is stabilized itest!(_089_run_allow_list { args: "run --unstable --allow-run=curl run/089_run_allow_list.ts", + envs: vec![("LD_LIBRARY_PATH".to_string(), "".to_string())], output: "run/089_run_allow_list.ts.out", }); diff --git a/tests/specs/run/ld_preload/__test__.jsonc b/tests/specs/run/ld_preload/__test__.jsonc index f3a9b26bca8918..a778873509fd50 100644 --- a/tests/specs/run/ld_preload/__test__.jsonc +++ b/tests/specs/run/ld_preload/__test__.jsonc @@ -1,4 +1,8 @@ { + "envs": { + "LD_LIBRARY_PATH": "", + "LD_PRELOAD": "" + }, "tests": { "env_arg": { "args": "run --allow-run=echo env_arg.ts", diff --git a/tests/specs/run/ld_preload/env_arg.out b/tests/specs/run/ld_preload/env_arg.out index 0dcc2f36fb6ea9..fbf37014ae78b4 100644 --- a/tests/specs/run/ld_preload/env_arg.out +++ b/tests/specs/run/ld_preload/env_arg.out @@ -1,4 +1,4 @@ -error: Uncaught (in promise) PermissionDenied: Requires --allow-all permissions to spawn subprocess with any LD_* environment variable. +error: Uncaught (in promise) PermissionDenied: Requires --allow-all permissions to spawn subprocess with LD_PRELOAD environment variable. }).spawn(); ^ at [WILDCARD] diff --git a/tests/specs/run/ld_preload/set_with_allow_env.out b/tests/specs/run/ld_preload/set_with_allow_env.out index 0f9af383c873f0..2e92763ddac227 100644 --- a/tests/specs/run/ld_preload/set_with_allow_env.out +++ b/tests/specs/run/ld_preload/set_with_allow_env.out @@ -1,4 +1,4 @@ -error: Uncaught (in promise) PermissionDenied: Requires --allow-all permissions to spawn subprocess with any LD_* environment variable. +error: Uncaught (in promise) PermissionDenied: Requires --allow-all permissions to spawn subprocess with LD_PRELOAD environment variable. const output = new Deno.Command("echo").spawn(); ^ at [WILDCARD] diff --git a/tests/unit/os_test.ts b/tests/unit/os_test.ts index 42b598511128da..d2677e4384ad4d 100644 --- a/tests/unit/os_test.ts +++ b/tests/unit/os_test.ts @@ -239,6 +239,7 @@ Deno.test( async function hostnameWithoutOtherNetworkUsages() { const { stdout } = await new Deno.Command(Deno.execPath(), { args: ["eval", "-p", "Deno.hostname()"], + env: { LD_PRELOAD: "", LD_LIBRARY_PATH: "" }, }).output(); const hostname = new TextDecoder().decode(stdout).trim(); assert(hostname.length > 0); From 6652d418ebb80af09bc818786fb32262b82fc53e Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 28 Aug 2024 13:51:58 -0400 Subject: [PATCH 3/5] maybe fix linux --- tests/integration/run_tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/run_tests.rs b/tests/integration/run_tests.rs index 40b9e15a4888e1..baedea4c8bb2ba 100644 --- a/tests/integration/run_tests.rs +++ b/tests/integration/run_tests.rs @@ -3739,6 +3739,7 @@ itest!(test_and_bench_are_noops_in_run { #[cfg(not(target_os = "windows"))] itest!(spawn_kill_permissions { args: "run --quiet --allow-run=cat spawn_kill_permissions.ts", + envs: vec![("LD_LIBRARY_PATH".to_string(), "".to_string())], output_str: Some(""), }); From 072b26095aa191c5a9eb22eeaa77808d90928996 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 28 Aug 2024 14:22:27 -0400 Subject: [PATCH 4/5] maybe fix on mac --- tests/integration/run_tests.rs | 10 ++++++++-- tests/unit/os_test.ts | 6 +++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/integration/run_tests.rs b/tests/integration/run_tests.rs index baedea4c8bb2ba..05e80e7b3a1c0c 100644 --- a/tests/integration/run_tests.rs +++ b/tests/integration/run_tests.rs @@ -501,7 +501,10 @@ itest!(_088_dynamic_import_already_evaluating { // TODO(bartlomieju): remove --unstable once Deno.Command is stabilized itest!(_089_run_allow_list { args: "run --unstable --allow-run=curl run/089_run_allow_list.ts", - envs: vec![("LD_LIBRARY_PATH".to_string(), "".to_string())], + envs: vec![ + ("LD_LIBRARY_PATH".to_string(), "".to_string()), + ("DYLD_FALLBACK_LIBRARY_PATH".to_string(), "".to_string()) + ], output: "run/089_run_allow_list.ts.out", }); @@ -3739,7 +3742,10 @@ itest!(test_and_bench_are_noops_in_run { #[cfg(not(target_os = "windows"))] itest!(spawn_kill_permissions { args: "run --quiet --allow-run=cat spawn_kill_permissions.ts", - envs: vec![("LD_LIBRARY_PATH".to_string(), "".to_string())], + envs: vec![ + ("LD_LIBRARY_PATH".to_string(), "".to_string()), + ("DYLD_FALLBACK_LIBRARY_PATH".to_string(), "".to_string()) + ], output_str: Some(""), }); diff --git a/tests/unit/os_test.ts b/tests/unit/os_test.ts index d2677e4384ad4d..9503f75d12e03b 100644 --- a/tests/unit/os_test.ts +++ b/tests/unit/os_test.ts @@ -239,7 +239,11 @@ Deno.test( async function hostnameWithoutOtherNetworkUsages() { const { stdout } = await new Deno.Command(Deno.execPath(), { args: ["eval", "-p", "Deno.hostname()"], - env: { LD_PRELOAD: "", LD_LIBRARY_PATH: "" }, + env: { + LD_PRELOAD: "", + LD_LIBRARY_PATH: "", + DYLD_FALLBACK_LIBRARY_PATH: "", + }, }).output(); const hostname = new TextDecoder().decode(stdout).trim(); assert(hostname.length > 0); From a3818d84a06b917f791a51b8ad011fb6c01ccc15 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 28 Aug 2024 15:47:49 -0400 Subject: [PATCH 5/5] now probably fix mac --- tests/specs/run/ld_preload/__test__.jsonc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/specs/run/ld_preload/__test__.jsonc b/tests/specs/run/ld_preload/__test__.jsonc index a778873509fd50..767e423d063055 100644 --- a/tests/specs/run/ld_preload/__test__.jsonc +++ b/tests/specs/run/ld_preload/__test__.jsonc @@ -1,7 +1,8 @@ { "envs": { "LD_LIBRARY_PATH": "", - "LD_PRELOAD": "" + "LD_PRELOAD": "", + "DYLD_FALLBACK_LIBRARY_PATH": "" }, "tests": { "env_arg": {