From 1f8b1a587c397dd01e058820769580323a0f7330 Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Tue, 13 Aug 2019 14:51:15 -0400 Subject: [PATCH] Dynamic import should respect permissions (#2764) --- cli/ops.rs | 9 +++-- cli/permissions.rs | 4 +-- cli/state.rs | 36 +++++++++++++++++-- core/modules.rs | 31 +++++++++++----- tests/013_dynamic_import.test | 2 +- tests/014_duplicate_import.test | 2 +- tests/015_duplicate_parallel_import.test | 2 +- tests/042_dyn_import_evalcontext.test | 2 +- .../error_014_catch_dynamic_import_error.test | 2 +- tests/error_015_dynamic_import_permissions.js | 3 ++ .../error_015_dynamic_import_permissions.out | 1 + .../error_015_dynamic_import_permissions.test | 4 +++ .../error_016_dynamic_import_permissions2.js | 5 +++ .../error_016_dynamic_import_permissions2.out | 2 ++ ...error_016_dynamic_import_permissions2.test | 5 +++ tests/subdir/evil_remote_import.js | 4 +++ 16 files changed, 95 insertions(+), 19 deletions(-) create mode 100644 tests/error_015_dynamic_import_permissions.js create mode 100644 tests/error_015_dynamic_import_permissions.out create mode 100644 tests/error_015_dynamic_import_permissions.test create mode 100644 tests/error_016_dynamic_import_permissions2.js create mode 100644 tests/error_016_dynamic_import_permissions2.out create mode 100644 tests/error_016_dynamic_import_permissions2.test create mode 100644 tests/subdir/evil_remote_import.js diff --git a/cli/ops.rs b/cli/ops.rs index 166610bac7..7a68dee55d 100644 --- a/cli/ops.rs +++ b/cli/ops.rs @@ -497,7 +497,12 @@ fn op_fetch_source_file( let specifier = inner.specifier().unwrap(); let referrer = inner.referrer().unwrap(); - let resolved_specifier = state.resolve(specifier, referrer, false)?; + // TODO(ry) Maybe a security hole. Only the compiler worker should have access + // to this. Need a test to demonstrate the hole. + let is_dyn_import = false; + + let resolved_specifier = + state.resolve(specifier, referrer, false, is_dyn_import)?; let fut = state .file_fetcher @@ -750,7 +755,7 @@ fn op_fetch( let req = msg_util::deserialize_request(header, body)?; let url_ = url::Url::parse(url).map_err(ErrBox::from)?; - state.check_net_url(url_)?; + state.check_net_url(&url_)?; let client = http_util::get_client(); diff --git a/cli/permissions.rs b/cli/permissions.rs index 9774d92c03..814c3ff942 100644 --- a/cli/permissions.rs +++ b/cli/permissions.rs @@ -274,7 +274,7 @@ impl DenoPermissions { } } - pub fn check_net_url(&self, url: url::Url) -> Result<(), ErrBox> { + pub fn check_net_url(&self, url: &url::Url) -> Result<(), ErrBox> { let msg = &format!("network access to \"{}\"", url); match self.allow_net.get_state() { PermissionAccessorState::Allow => { @@ -627,7 +627,7 @@ mod tests { for (url_str, is_ok) in url_tests.iter() { let u = url::Url::parse(url_str).unwrap(); - assert_eq!(*is_ok, perms.check_net_url(u).is_ok()); + assert_eq!(*is_ok, perms.check_net_url(&u).is_ok()); } for (domain, is_ok) in domain_tests.iter() { diff --git a/cli/state.rs b/cli/state.rs index eb912161d4..0b0f3b1aed 100644 --- a/cli/state.rs +++ b/cli/state.rs @@ -4,6 +4,7 @@ use crate::compilers::JsCompiler; use crate::compilers::JsonCompiler; use crate::compilers::TsCompiler; use crate::deno_dir; +use crate::deno_error::permission_denied; use crate::file_fetcher::SourceFileFetcher; use crate::flags; use crate::global_timer::GlobalTimer; @@ -119,6 +120,7 @@ impl Loader for ThreadSafeState { specifier: &str, referrer: &str, is_main: bool, + is_dyn_import: bool, ) -> Result { if !is_main { if let Some(import_map) = &self.import_map { @@ -128,8 +130,14 @@ impl Loader for ThreadSafeState { } } } + let module_specifier = + ModuleSpecifier::resolve_import(specifier, referrer)?; - ModuleSpecifier::resolve_import(specifier, referrer).map_err(ErrBox::from) + if is_dyn_import { + self.check_dyn_import(&module_specifier)?; + } + + Ok(module_specifier) } /// Given an absolute url, load its source code. @@ -294,7 +302,7 @@ impl ThreadSafeState { } #[inline] - pub fn check_net_url(&self, url: url::Url) -> Result<(), ErrBox> { + pub fn check_net_url(&self, url: &url::Url) -> Result<(), ErrBox> { self.permissions.check_net_url(url) } @@ -303,6 +311,30 @@ impl ThreadSafeState { self.permissions.check_run() } + pub fn check_dyn_import( + self: &Self, + module_specifier: &ModuleSpecifier, + ) -> Result<(), ErrBox> { + let u = module_specifier.as_url(); + match u.scheme() { + "http" | "https" => { + self.check_net_url(u)?; + Ok(()) + } + "file" => { + let filename = u + .to_file_path() + .unwrap() + .into_os_string() + .into_string() + .unwrap(); + self.check_read(&filename)?; + Ok(()) + } + _ => Err(permission_denied()), + } + } + #[cfg(test)] pub fn mock(argv: Vec) -> ThreadSafeState { ThreadSafeState::new( diff --git a/core/modules.rs b/core/modules.rs index 072de4bc93..5956a7317b 100644 --- a/core/modules.rs +++ b/core/modules.rs @@ -40,6 +40,7 @@ pub trait Loader: Send + Sync { specifier: &str, referrer: &str, is_main: bool, + is_dyn_import: bool, ) -> Result; /// Given ModuleSpecifier, load its source code. @@ -125,12 +126,15 @@ impl RecursiveLoad { fn add_root(&mut self) -> Result<(), ErrBox> { let module_specifier = match self.state { - State::ResolveMain(ref specifier) => { - self.loader.resolve(specifier, ".", true)? - } - State::ResolveImport(ref specifier, ref referrer) => { - self.loader.resolve(specifier, referrer, false)? - } + State::ResolveMain(ref specifier) => self.loader.resolve( + specifier, + ".", + true, + self.dyn_import_id().is_some(), + )?, + State::ResolveImport(ref specifier, ref referrer) => self + .loader + .resolve(specifier, referrer, false, self.dyn_import_id().is_some())?, _ => unreachable!(), }; @@ -160,7 +164,12 @@ impl RecursiveLoad { referrer: &str, parent_id: deno_mod, ) -> Result<(), ErrBox> { - let module_specifier = self.loader.resolve(specifier, referrer, false)?; + let module_specifier = self.loader.resolve( + specifier, + referrer, + false, + self.dyn_import_id().is_some(), + )?; let module_name = module_specifier.as_str(); let mut modules = self.modules.lock().unwrap(); @@ -277,7 +286,12 @@ impl ImportStream for RecursiveLoad { |specifier: &str, referrer_id: deno_mod| -> deno_mod { let modules = self.modules.lock().unwrap(); let referrer = modules.get_name(referrer_id).unwrap(); - match self.loader.resolve(specifier, &referrer, is_main) { + match self.loader.resolve( + specifier, + &referrer, + is_main, + self.dyn_import_id().is_some(), + ) { Ok(specifier) => modules.get_id(specifier.as_str()).unwrap_or(0), // We should have already resolved and Ready this module, so // resolve() will not fail this time. @@ -675,6 +689,7 @@ mod tests { specifier: &str, referrer: &str, _is_root: bool, + _is_dyn_import: bool, ) -> Result { let referrer = if referrer == "." { "file:///" diff --git a/tests/013_dynamic_import.test b/tests/013_dynamic_import.test index 8fe463b206..d65139dfa7 100644 --- a/tests/013_dynamic_import.test +++ b/tests/013_dynamic_import.test @@ -1,2 +1,2 @@ -args: tests/013_dynamic_import.ts --reload +args: tests/013_dynamic_import.ts --reload --allow-read output: tests/013_dynamic_import.ts.out diff --git a/tests/014_duplicate_import.test b/tests/014_duplicate_import.test index 57d5b6e8bc..c4811a32c4 100644 --- a/tests/014_duplicate_import.test +++ b/tests/014_duplicate_import.test @@ -1,2 +1,2 @@ -args: tests/014_duplicate_import.ts --reload +args: tests/014_duplicate_import.ts --reload --allow-read output: tests/014_duplicate_import.ts.out diff --git a/tests/015_duplicate_parallel_import.test b/tests/015_duplicate_parallel_import.test index ec8c79b21d..8378277953 100644 --- a/tests/015_duplicate_parallel_import.test +++ b/tests/015_duplicate_parallel_import.test @@ -1,2 +1,2 @@ -args: tests/015_duplicate_parallel_import.js --reload +args: tests/015_duplicate_parallel_import.js --reload --allow-read output: tests/015_duplicate_parallel_import.js.out diff --git a/tests/042_dyn_import_evalcontext.test b/tests/042_dyn_import_evalcontext.test index cfe0df3f2d..ab1efc084d 100644 --- a/tests/042_dyn_import_evalcontext.test +++ b/tests/042_dyn_import_evalcontext.test @@ -1,2 +1,2 @@ -args: run --reload tests/042_dyn_import_evalcontext.ts +args: run --allow-read --reload tests/042_dyn_import_evalcontext.ts output: tests/042_dyn_import_evalcontext.ts.out diff --git a/tests/error_014_catch_dynamic_import_error.test b/tests/error_014_catch_dynamic_import_error.test index 8345ad1c84..13ece48434 100644 --- a/tests/error_014_catch_dynamic_import_error.test +++ b/tests/error_014_catch_dynamic_import_error.test @@ -1,2 +1,2 @@ -args: tests/error_014_catch_dynamic_import_error.js --reload +args: tests/error_014_catch_dynamic_import_error.js --reload --allow-read output: tests/error_014_catch_dynamic_import_error.js.out diff --git a/tests/error_015_dynamic_import_permissions.js b/tests/error_015_dynamic_import_permissions.js new file mode 100644 index 0000000000..3460ca787a --- /dev/null +++ b/tests/error_015_dynamic_import_permissions.js @@ -0,0 +1,3 @@ +(async () => { + await import("http://localhost:4545/tests/subdir/mod4.js"); +})(); diff --git a/tests/error_015_dynamic_import_permissions.out b/tests/error_015_dynamic_import_permissions.out new file mode 100644 index 0000000000..90ccd0d1a3 --- /dev/null +++ b/tests/error_015_dynamic_import_permissions.out @@ -0,0 +1 @@ +error: Uncaught TypeError: permission denied diff --git a/tests/error_015_dynamic_import_permissions.test b/tests/error_015_dynamic_import_permissions.test new file mode 100644 index 0000000000..62f31e6b36 --- /dev/null +++ b/tests/error_015_dynamic_import_permissions.test @@ -0,0 +1,4 @@ +args: --reload --no-prompt tests/error_015_dynamic_import_permissions.js +output: tests/error_015_dynamic_import_permissions.out +check_stderr: true +exit_code: 1 diff --git a/tests/error_016_dynamic_import_permissions2.js b/tests/error_016_dynamic_import_permissions2.js new file mode 100644 index 0000000000..71c70815ce --- /dev/null +++ b/tests/error_016_dynamic_import_permissions2.js @@ -0,0 +1,5 @@ +// If this is executed with --allow-net but not --allow-read the following +// import should cause a permission denied error. +(async () => { + await import("http://localhost:4545/tests/subdir/evil_remote_import.js"); +})(); diff --git a/tests/error_016_dynamic_import_permissions2.out b/tests/error_016_dynamic_import_permissions2.out new file mode 100644 index 0000000000..f52186481f --- /dev/null +++ b/tests/error_016_dynamic_import_permissions2.out @@ -0,0 +1,2 @@ +[WILDCARD] +error: Uncaught TypeError: permission denied diff --git a/tests/error_016_dynamic_import_permissions2.test b/tests/error_016_dynamic_import_permissions2.test new file mode 100644 index 0000000000..0fc514f0f7 --- /dev/null +++ b/tests/error_016_dynamic_import_permissions2.test @@ -0,0 +1,5 @@ +# We have an allow-net flag but not allow-read, it should still result in error. +args: --no-prompt --reload --allow-net tests/error_016_dynamic_import_permissions2.js +output: tests/error_016_dynamic_import_permissions2.out +check_stderr: true +exit_code: 1 diff --git a/tests/subdir/evil_remote_import.js b/tests/subdir/evil_remote_import.js new file mode 100644 index 0000000000..4ff7d1b975 --- /dev/null +++ b/tests/subdir/evil_remote_import.js @@ -0,0 +1,4 @@ +// We expect to get a permission denied error if we dynamically +// import this module without --allow-read. +export * from "file:///c:/etc/passwd"; +console.log("Hello from evil_remote_import.js");