Add --allow-read

[ry]

Currently, deno allows disk read access without a flag. The idea behind this is that although programs may be able to access secret info (e.g. your ssh key) without network access or disk write access, there isn't anyway they could exploit that knowledge. It's meant as a connivance.

Environmental variable access is quite similar to disk read access. If you can access the environ without having write or network access - there is no way to exploit that.

This seems to be a double standard. Thus I propose we either remove --allow-env and treat it as read access to the disk. Or we get more constrained and bundle and add --allow-read for disk access.

[hayd]

👍 to --allow-read. It's conceivable you'd want net access without read.

This could later be modified e.g. to specify which dirs have read access.

[kitsonk]

This could later be modified e.g. to specify which dirs have read access.

Isn't this better handled by OS level permissions? Why reinvent the wheel (likely badly)? With every feature, especially one like this, there is a huge cost to usability, and large usability friction will only reduce adoption. If there is no risk with read access, why hassle with it. Ergo, why not remove --allow-env?

Yeah, it is conceivable that you would want net access without read. You might want net access without access to the env too, that does mean it weakens the security module by implicitly allowing it.

[pietvanzoen]

I was experimenting a little to see if it was possible to read environment variables from the filesystem via /proc/<pid>/environ. It seems quite limited vs env(), but it is possible to get some environment variables that way. So a determined person might be able to read the environment despite --allow-env? Maybe?

At the very least if they know where to look for environment variables (.env files, ~/.bashrc) then the protection of --allow-env is a bit limited.

Although protection from writing environment variables might still be useful.

[Jerska]

I have no knowledge of the inner workings on deno. However, the lack of --allow-read was the first hing that surprised me when reading the README.
[Btw, I'm super excited about it! Keep up the great work!]

Your explanation makes perfect sense

... without network access or disk write access, there isn't anyway they could exploit that knowledge.

However, the assumption behind this message is that the sandboxing can't and won't be broken.

Having an --allow-read flag would make sure that even if the net sandboxing gets broken, scripts without --allow-read could still be considered safe from having accessed anything on the filesystem.
This sounds like an easy layer of security thrown away for convenience. You could also imagine having it allowed as a default, but giving the ability of explicitely disabling it.

[kitsonk]

Having an --allow-read flag would make sure that even if the net sandboxing gets broken, scripts without --allow-read could still be considered safe from having accessed anything on the filesystem.

If they can break the net sandboxing, they can break any of the sandboxing. The flag would be meaningless. At the moment, I don't think there is a way to break the sandboxing. If there is an exploit that breaks outside of the V8 isolate, then it affects the whole world. The only thing that Deno "adds" is the communication to Rust via flatbuffers. The way those buffers are handled, there is no arbitrary execution of any code or commands. There are some very narrow ways the messages are interpreted. It would require someone figuring a way to trigger some sort of stack overflow in Rust via a badly formed flatbuffer message that would allow them to change the state the Rust stores around the runtime flags and overwrite that (or to allow the call of an op that they aren't privileged for at the moment), which means if you found a way to break one, you could break them all. I would think even supporting Web Sockets would require transiting via Rust and therefore the same privilege system.

At the moment, while you can't get unfettered net access, you could form a module identity in the runtime that could make a request to an arbitrary server that could send information "readable" from the host. That is covered by #712.

[tjconcept]

I have no knowledge of the inner workings on deno. However, the lack of --allow-read was the first hing that surprised me when reading the README.

I can second that. And reading the reasoning also makes sense, but:

I like the statement on the roadmap: "We want to be secure by default; user should be able to run untrusted code, like the web."
So deno will be compared to a browser. A browser does not allow read access to disk. I guess a browser has almost the reverse reasoning: allow networking and even writes (but only into a sandbox like localStorage) but never leak information into the sandbox.

[ry]

After discussing with several people it seems adding --allow-read is the right way to go.

As @hayd said in the first comment, it's conceivable you'd want net access without read.

Deno's prerogative is security over ergonomics. Once we have --allow-read in place, we can get a feel for how annoying it is, and if we need to add some features (like being able to remember a permission setting) reduce the number of console interactions.

I'm renaming this issue to reflect the new goal. Thanks for the helpful comments.

[F001]

I'm wondering whether we should provide an explicit not-allowed option for users. Then the permission flag has 3 status:

1. default. Prompt in console to ask the permission
2. allowed. Allow directly without asking.
3. not-allowed. Reject this request without asking.

[kevinkassimo]

@F001 or adding a 4th one?:
4. always-ask. Ask in console every single time when there is a write/net/etc. request (useful when there are very few writes needed but each write is sensitive)

[F001]

@kevinkassimo Actually, I think int flag might be not enough for fine grained permission control. As discussed in #720, many people want different permission for different paths. Which means, we should provide a map (or even a route system) for permission settings.

"/a/b/c" : "allow-write",
"/a/b/c/x" : "allow-read",
"/a/b/c/y" : "prompt",
...

What's more, if we want to provide a sandbox, this system should allow mounting. For example, we can access "/a/b/c" in typescript, but it actually mapped to "/x/y/z" in operating system.

[ry]

Let's do this piecemeal - it should be easy to add --allow-read - just by adding conditions to the read ops. I'm very open to more fine grained permission structures - in particular access to file system subtrees or restricting network access to a particular host - but this is more complex. We should setup the infrastructure such that the permission check mechanism gets the necessary parameters so it may, in the future, make such fine grained checks. How exactly that will be specified, I would like to punt on.

[eventualbuddha]

I agree that --allow-read is a good first step here. I wonder whether a more fine-grained configuration could be achieved by introducing something akin to a web worker that can intercept and wrap requests to the disk or environment etc. That way you could allow specific paths or even remap them.

[fewf]

Hi all--new Deno'er here. It looks like @sh7dm and I have duplicated some work on this, but not a ton. There are some open questions I've had as I started on this.

What's a read operation?
Both my PR and @sh7dm treat the following Ops as needing Read Access:

• op_open
• op_read_file
• op_copy_file
• op_stat
• op_read_dir
• op_read_link

whereas in mine, I include also:

• op_remove
• op_cwd
• op_rename
• op_symlink

Can anyone give an authoritative/exhaustive list of what Ops should ask for read access?

@sh7dm let me know if you want to merge our branches and split up the remaining work on this.

Thanks!

[kitsonk]

@fewf it was discussed on Gitter that @sh7dm was going to take a look at it.

All, we likely need to add comments to issues if/when we are working on them, as well as discuss the requirements in the issue or in Gitter before people expend time duplicating work.

[fewf]

Thanks @kitsonk this was just a case of bad timing as I started looking into this yesterday just as a way to familiarize myself with Deno, so I'm fine to step off of it.

[ry]

@fewf Thanks for looking into this!

(FYI I asked @fewf to look into this but stupidly also asked @sh7dm to too - many apologies - bad project management!)

op_open, op_read_file, op_copy_file, op_stat, op_read_dir, op_read_link

Agree that these need allow-read

op_cwd

Not sure about this one... I guess it is exposing something about the disk but import.meta.url also might expose that... Not sure.

op_remove, op_rename, op_symlink

I think these should require allow-write but not allow-read. Or am I missing something?

[fewf]

Naw, that makes sense. I was thinking very maximally at first.

(edit to add: for instance, in the case of op_rename granted write-only access, if the user gets back an error that oldpath doesn't exist, that would be revealing information about the fs, but maybe that's too paranoid.)

[fewf]

I've closed my PR in favor of @sh7dm . I did continue to do some more work towards fixing this issue, which I have in a PR into @sh7dm 's branch: dsseng#1 . Tests are passing, but I think there's still room to add new tests to ensure the feature is implemented correctly. Hope this can be used and sorry if I stepped on anyone's toes--very excited to be working on this project!

[dsseng]

@fewf I think we should merge our branches to find the best code and use it