Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permissions whitelist #2129

Merged
merged 20 commits into from May 8, 2019
Merged
Changes from 3 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -14,8 +14,11 @@ pub struct DenoFlags {
pub version: bool,
pub reload: bool,
pub allow_read: bool,
pub read_whitelist: Vec<String>,
pub allow_write: bool,
pub write_whitelist: Vec<String>,
pub allow_net: bool,
pub net_whitelist: Vec<String>,
pub allow_env: bool,
pub allow_run: bool,
pub allow_high_precision: bool,
@@ -45,14 +48,29 @@ pub fn create_cli_app<'a, 'b>() -> App<'a, 'b> {
Arg::with_name("allow-read")
.long("allow-read")
.help("Allow file system read access"),
).arg(
Arg::with_name("read-wl")
.long("read-wl")
.takes_value(true)
.help("Allow file system read access to specific file or directory"),
).arg(
Arg::with_name("allow-write")
.long("allow-write")
.help("Allow file system write access"),
).arg(
Arg::with_name("write-wl")
.long("write-wl")
.takes_value(true)
.help("Allow file system write access to specific file or directory"),
).arg(
Arg::with_name("allow-net")
.long("allow-net")
.help("Allow network access"),
).arg(
Arg::with_name("net-wl")
.long("net-wl")
.takes_value(true)
.help("Allow file system net access to specific domain"),
).arg(
Arg::with_name("allow-env")
.long("allow-env")
@@ -150,12 +168,21 @@ pub fn parse_flags(matches: ArgMatches) -> DenoFlags {
if matches.is_present("allow-read") {
flags.allow_read = true;
}
if let Some(read_wl) = matches.values_of("read-wl") {
flags.read_whitelist = read_wl.map(|s| s.to_string()).collect();
}
if matches.is_present("allow-write") {
flags.allow_write = true;
}
if let Some(write_wl) = matches.values_of("write-wl") {
flags.write_whitelist = write_wl.map(|s| s.to_string()).collect();
}
if matches.is_present("allow-net") {
flags.allow_net = true;
}
if let Some(net_wl) = matches.values_of("net-wl") {
This conversation was marked as resolved by afinch7

This comment has been minimized.

Copy link
@ry

ry Apr 21, 2019

Collaborator

I'm not sure about the actual naming of these flags.

In wasmtime, they have --dir which is close to these whitelist flags. I wonder if we should adopt that.

In any case, I think this is fine for a first pass with the understanding that we'd want to simplify this at some point.

This comment has been minimized.

Copy link
@hayd

hayd Apr 24, 2019

Contributor

Naming wise why not reuse the original, so that --allow-net is equivalent to --allow-net=*.

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Apr 24, 2019

Contributor

I was pondering it for some time now and @hayd's idea seems pretty ergonomic, but... consider that if there's a whitelist, people will probably want a blacklist as well. Surely this would be solvable by adding some special syntax (maybe -?, eg. --allow-net=127.0.0.1,-0.0.0.0,). But then again that would add some unneeded cognitive overhead and magic... Ideas?

CC @rsp might be interested

This comment has been minimized.

Copy link
@hayd

hayd Apr 24, 2019

Contributor

Semantically using - under --allow-net to signify blacklist is a bit of a stretch. Also imo one char difference is too small.

Another flag for --reject-net or something?

This comment has been minimized.

Copy link
@hayd

hayd Apr 25, 2019

Contributor

--no-net could also work for blacklisting (consistent with other flags e.g. --no-prompt)
ie. --no-net=facebook.com and --no-read=/etc/passwd

This comment has been minimized.

Copy link
@afinch7

afinch7 May 2, 2019

Author Contributor

Going to go with @hayd suggestion on this one and use --allow-x=.

flags.net_whitelist = net_wl.map(|s| s.to_string()).collect();
}
if matches.is_present("allow-env") {
flags.allow_env = true;
}
@@ -6,8 +6,10 @@ use crate::flags::DenoFlags;
use ansi_term::Style;
use crate::errors::permission_denied;
use crate::errors::DenoResult;
use std::collections::HashSet;
use std::fmt;
use std::io;
use std::path::Path;
use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
use std::sync::Arc;

@@ -127,8 +129,11 @@ impl Default for PermissionAccessor {
pub struct DenoPermissions {
// Keep in sync with src/permissions.ts
pub allow_read: PermissionAccessor,
pub read_whitelist: Vec<String>,
pub allow_write: PermissionAccessor,
pub write_whitelist: Vec<String>,
pub allow_net: PermissionAccessor,
pub net_whitelist: Arc<HashSet<String>>,
pub allow_env: PermissionAccessor,
pub allow_run: PermissionAccessor,
pub allow_high_precision: PermissionAccessor,
@@ -139,9 +144,12 @@ impl DenoPermissions {
pub fn from_flags(flags: &DenoFlags) -> Self {
Self {
allow_read: PermissionAccessor::from(flags.allow_read),
read_whitelist: flags.read_whitelist.iter().cloned().collect(),
allow_write: PermissionAccessor::from(flags.allow_write),
allow_env: PermissionAccessor::from(flags.allow_env),
write_whitelist: flags.write_whitelist.iter().cloned().collect(),
allow_net: PermissionAccessor::from(flags.allow_net),
net_whitelist: Arc::new(flags.net_whitelist.iter().cloned().collect()),
allow_env: PermissionAccessor::from(flags.allow_env),
allow_run: PermissionAccessor::from(flags.allow_run),
allow_high_precision: PermissionAccessor::from(
flags.allow_high_precision,
@@ -170,51 +178,89 @@ impl DenoPermissions {
pub fn check_read(&self, filename: &str) -> DenoResult<()> {
match self.allow_read.get_state() {
PermissionAccessorState::Allow => Ok(()),
PermissionAccessorState::Ask => match self
.try_permissions_prompt(&format!("read access to \"{}\"", filename))
{
Err(e) => Err(e),
Ok(v) => {
self.allow_read.update_with_prompt_result(&v);
v.check()?;
Ok(())
state => {
let file_path = Path::new(filename);
for path in &self.read_whitelist {
if file_path.starts_with(path) {
return Ok(());
}
}
},
PermissionAccessorState::Deny => Err(permission_denied()),
match state {
// This shouldn't be possible but I guess rust doesn't realize.
PermissionAccessorState::Allow => Ok(()),
PermissionAccessorState::Ask => match self
.try_permissions_prompt(&format!("read access to \"{}\"", filename))
{
Err(e) => Err(e),
Ok(v) => {
self.allow_read.update_with_prompt_result(&v);
v.check()?;
Ok(())
}
},
PermissionAccessorState::Deny => Err(permission_denied()),
}
}
}
}

pub fn check_write(&self, filename: &str) -> DenoResult<()> {
This conversation was marked as resolved by afinch7

This comment has been minimized.

Copy link
@ry

ry Apr 21, 2019

Collaborator

Some new Rust unit tests for check_write, check_read, and check_net would be appropriate

match self.allow_write.get_state() {
PermissionAccessorState::Allow => Ok(()),
PermissionAccessorState::Ask => match self
.try_permissions_prompt(&format!("write access to \"{}\"", filename))
{
Err(e) => Err(e),
Ok(v) => {
self.allow_write.update_with_prompt_result(&v);
v.check()?;
Ok(())
state => {
let file_path = Path::new(filename);
for path in &self.write_whitelist {
if file_path.starts_with(path) {
return Ok(());
}
}
This conversation was marked as resolved by afinch7

This comment has been minimized.

Copy link
@ry

ry Apr 21, 2019

Collaborator

I think this could be combined with the read_whitelist loop into a standalone function fn path_is_allowed(p: &Path, white_list: &HashSet): bool, and ideally it would have some unit tests.

},
PermissionAccessorState::Deny => Err(permission_denied()),
match state {
// This shouldn't be possible but I guess rust doesn't realize.
PermissionAccessorState::Allow => Ok(()),
PermissionAccessorState::Ask => match self.try_permissions_prompt(
&format!("write access to \"{}\"", filename),
) {
Err(e) => Err(e),
Ok(v) => {
self.allow_write.update_with_prompt_result(&v);
v.check()?;
Ok(())
}
},
PermissionAccessorState::Deny => Err(permission_denied()),
}
}
}
}

pub fn check_net(&self, domain_name: &str) -> DenoResult<()> {
match self.allow_net.get_state() {
PermissionAccessorState::Allow => Ok(()),
PermissionAccessorState::Ask => match self.try_permissions_prompt(
&format!("network access to \"{}\"", domain_name),
) {
Err(e) => Err(e),
Ok(v) => {
self.allow_net.update_with_prompt_result(&v);
v.check()?;
Ok(())
state => {
match self.net_whitelist.contains(domain_name) {
true => Ok(()),
false => {
match state {
// This shouldn't be possible but I guess rust doesn't realize.
PermissionAccessorState::Allow => Ok(()),
PermissionAccessorState::Ask => {
match self.try_permissions_prompt(&format!(
"network access to \"{}\"",
domain_name
)) {
Err(e) => Err(e),
Ok(v) => {
self.allow_net.update_with_prompt_result(&v);
v.check()?;
Ok(())
}
}
}
PermissionAccessorState::Deny => Err(permission_denied()),
}
}
}
},
PermissionAccessorState::Deny => Err(permission_denied()),
}
}
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.