-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Sandbox mode for external scripts #1639
Comments
I don't think this is implementable in a safe way without lots of work and unstandard restrictions. Javascript modules still run in the same global context and can easily modify global variables to steal capabilities from other modules. See #1189 (comment) and #378 (comment) for further explanation. |
Something, that's could go along way her, is to support a kind of CSP... so that e.g. you could restrict (via a whiteless) which domains deno could make network requests to, or where on the filesystem it can access, or which env variables. Rather than these being simply on/off. |
I feel this is still a compelling feature, given the amount of node modules people just install and run in production without ever evaluating the source. See https://hackernoon.com/im-harvesting-credit-card-numbers-and-passwords-from-your-site-here-s-how-9a8cb347c5b5. Reading @Macil comments in previous threads seems this has been considered extensively enough. For various reasons it's hard to implement. |
Yeah, I think that's a must-have. A common way of using dependencies is having your own application code use all privileges while third-party deps may just help in a non-sandboxed way. That's the main issue with node.js dependencies: any dep, no matter how small, could leak your shit to thieves. If we had a way to ensure most third-party code cannot execute any privileged code, that'd be perfect. |
Having a CSP which restricts on domain + filename + env etc. would be a huge win. Ideally you'd be able to configure based on source location, but if that's infeasible (which I am not convinced) doing at the top-level still mitigates a bunch of attacks. |
I don't really see any point in sandboxing if people would still be able to backdoor their packages similar to event-stream issue |
The attack described above would be completely mitigated if you - at the top-level - restricted domains (specifically don't allow POST requests to |
There is a big difference between sandboxing remote modules and limiting the scope of network access. I think the latter is more logical. Treating remote modules differently is going to be really stupidly hard. Where as restricting external connectivity is different. We should treat programmes as a whole, whether the be local or remote modules. Having the ability to support a whitelist and blacklist modes for both network and file access would solve most of the exploits that are possible. If the workload needs a blacklist approach, then the author needs to have better controls in place for all their code, local and remote. |
Exactly what do you mean by better controls? Do you know, how the event-stream trojan was executed? It changed JS prototypes. |
It actually amazes me how this and similar issues have been solved a long time ago in Haskell etc. Very simple: there’s IO monad. If your method returns IO, there would be IO. If it doesn’t, there won’t be. |
btw the idea behind deno_std is to audit a large body of code so that transitive deps largely terminate in the same trusted code base. |
@ry great idea! |
@paulmillr the attack above worked because node allowed network requests to |
@hayd is there a whitelist planned? That could help a lot. |
Better controls, as in far of source and security of their code. External concerns that Deno alone cannot solve. We assume a remote module is less trustworthy than local code. That is really a false assumption because in reality, single developers seldom are the only people with access to local code. Exploits are obfuscated. A bad actor with access to the local code is much more likely to exploit, as there are far more eyes on a distributed exploit (which was how the recent attack was detected).
@paulmillr that is what I was proposing, a whitelist and a blacklist, versus sandboxing. Certain workload means you take certain risks. Deno can only go so far in helping. |
Also whitelist for filesystem and env access (maybe subprocess too?). This would be relatively straightforward by amending the DenoFlag&IsolateState to include regex/strings (?)... not sure if would affect perf / need a cheaper way. I do think additional sandboxing is worth exploring... E.g. I only want a particular lib to access my aws env variables. (Whether there's a solution which can't be sidestepped...) |
Sandboxing is just a mess, and I don't think it would deliver any real world security. There is now way to make it make sense from a sensible default, so that means people wouldn't be bothered. Outside of the whitelist capability for net and file system, the only other attack vector to narrow down would be to whitelist modules that can load the That leads to another possibility, an "audit" API, where all privileged operations are available to be sent somewhere for analysis or threat/exploit detection. |
Having a log of all privileged ops seems like a good idea. 👍 Maybe I'm unclear what precisely "sandboxing" means here... If you can "whitelist modules that can load the There's definitely the problem of an API for something like that, but it sounds like the underlying machinery could be feasible...? |
(Just to be clear, in this post I'm using "sandboxing" to refer to the general concept of running code with fewer permissions.) Any sandboxing controls (including limiting access to the deno module) that apply on a finer grain than a Javascript context/realm (such as on modules) will usually be bypassable with attacks like those in #1189 (comment). Currently, deno does sandboxing on the process level (the whole process has filesystem access allowed or disallowed based on command line flags, etc). There is a step further that deno could go easily: deno could allow Workers to be created that have more restricted permissions. It could look like this: const smallLibWorker = new Worker("https://url.com/some-small-library-worker.ts", {denoPermissions: ["allow-read"]});
smallLibWorker.postMessage(123);
smallLibWorker.onmessage = message => {
console.log('got reply from smallLibWorker', message);
}; That's not exactly sandboxing for arbitrary imports, but it's close to the OP request. |
Just to be clear, what is my mind when we say "sandboxing" is that it runs in its own environment, specifically a different instance of the global scope, with limited or no communication to other environments. The OP asked for it for remote modules. That is impractical, would make a lot of use cases unworkable, and generally not significantly improve security. Constraining privileged APIs, but all modules running in the same environment, I wouldn't consider sandboxing. Web workers inherently are a form of sandboxing. I think the idea of providing Deno security flags on Worker init is a great way for people to explicitly sandbox under a different set of controls! I really like it. We can argue the semantics of the options, but it is a really good idea!!! I checked and excess properties in the options do not cause browsers to though, so code portability should be ok. |
what would this approach be called? |
It is the existing Deno security module, just finer grained... permissions based security. |
"but all modules running in the same environment" does this also mean sharing the same constraints? |
Can you clarify why exactly it won't improve sec? I'm pretty sure sandboxing single modules would improve security a lot. |
How? Outside of breaking lots of code. I am afraid I can't prove a negative. If you can explain how sandboxing each module would increase security. The initial runtime is a sandbox of its own. It is a totally fallacy to trust a local module more than a remote module.
Yes, because I don't think varying the constraints for "this program" is logical... if there is something a local module shouldn't do, then a remote module shouldn't do it... if there is something a local module should do, a remote module should do it... couple that with any realistic implementation of sandboxing would mean breaking pretty much everything and putting on a huge overhead. Essentially every module would need to be a seperate web worker, dealing with all the limitations that come from postMessage. There would be no sharing of references/symbols/etc. across the sandboxes. |
Huh?
If all modules except for 2 don't have any access to outside world - and they SHOULD NOT - the problem would be easily mitigated. That's a real threat today - while "sandboxing an entire app" is useful only for microservices; it adds no value for monoliths. As for the implementation itself - i'm positive proper sandboxing can be achieved without huge performance downgrade. Think creatively. |
Granular permissions address this. A white list of the one valid network destination.
If your app has 400 modules, what is the chances the only permission you need is one narrow slice of privileged access? You are simply going to If you have a security need and you can deal with the limitations of sandboxing, load all your external modules in a web worker. As suggested above, varying the Deno permissions for that web worker is an ideal way. That is real world security.
I guess I can't. Sandboxing 400 modules would break lots of portable code and dramatically impact the memory footprint. |
Actually, another side conversation reminded me... We could/should support realms: https://github.com/tc39/proposal-realms That would be as efficient version of sandboxing without using web workers. It is still arguable how automatic it would be. |
It needn't break any code, this could be completely opt-in. Choosing a backwards compatible API is achievable... Something like: // deps.ts
@trust({env: "^AWS", net: "^[a-z\.]+\.us-west-2.amazonaws.com"}) // requires parent to have this or higher permission
import * as aws from "XXX/aws-sdk/mod.ts";
export aws;
@trust() // label to say "no trust" (perhaps optionally can make this the default)
import * as es from "XXX/event-stream/mod.ts";
export es; The problem isn't the API, it's feasibility... but it seems like this could (eventually!) be done with Realms. 👍 Almost certainly it won't be perfectly secure, but it'd cut off another bunch of attack vectors. |
@kitsonk I don't care whether it's realms, whitelist, or sandbox. That's just a matter of implementation. What I argue here for is any way to protect production systems and developer machines from modules gone rogue. If we can achieve that, and the result is simple in development experience terms, that would be ideal. If it's painful for developers to use, the result would be the same as if there was no such functionality. |
I'm arguing for any sort of sandboxing — whitelisting stuff is also sandboxing. It's just different semantics. |
The syntax is not important, and way less important that the implementation. The semantics above are that each labelled dependency has its own Realm (any dependencies of that module would inherit that Realm). Breaking code is a feature (and you may be right there could be serialisation issues) and that's why it would have to be opt-in...
We want all the things. 😉 |
Realms, workers, or other similar safe sandboxing mechanisms are all incompatible with standard import statements. They each require code to be written tailored for them. They can't be an implementation detail. I think this thread is having some miscommunication and essentially going back and forth between "sandboxing is important, let's sprinkle it on to imports" "that can't work out on imports, we can do something similar" "but we should do sandboxing". |
I think there's two actionable things here:
|
Just submitted #1864 should make option 2 pretty easy to implement. |
Not directly applicable, but a team within Google recently open sourced a sandboxing framework that wraps C libraries:
— https://security.googleblog.com/2019/03/open-sourcing-sandboxed-api.html So IIUC basically what deno does at the process level between the JS code and the OS, this makes happen around a given set of |
If only Deno was not a global, then all of the above would be possible with ease. Imagine that there is no Deno object, but a getDeno() function that works only once. In your own program at the very beginning you would do: const Deno = getDeno(); But the libraries would know that they cannot rely on getDeno() so they need to get the Deno object from the code that use them using simple dependency injection. So, as a library author instead of writing: export function a() {
Deno.x();
}
export function b() {
Deno.y();
} you would write, e.g. let Deno;
export function useDeno(deno: Deno) {
Deno = deno;
}
// ... the rest and use it as: import { a, b, useDeno } from './mod.ts';
useDeno(Deno); or: export function mod(Deno: Deno) {
return {
a() {
Deno.x();
},
b() {
Deno.y();
},
};
} and use it as: import { mod } from './mod.ts';
const { a, b } = mod(Deno); or you could use few other techniques. But the point is that if the modules had no other option to get the Deno object than having it injected by the caller then you would have a good control of which libraries use Deno in the first place, so you would not give it to a regex validation library or something that should not use the system resources. But even better, for any module that needs access to the system, you could give a wrapped version of Deno with only the methods that are needed, possibly with additional parameter validation. Because I think that @paulmillr is arguing that if you have a complex application that stores sensitive files and needs to use some logs then you don't want the logging library to have access to anything else than a single directory where it needs to save logs, and if you have a number formatting library then you don't want it to have access to anything. Or the example by @hayd with not giving AWS credentials in the env vars to every dependency and subdependency is also very reasonable. But I also understand why @kitsonk is saying that adding a new syntax or entirely new sandboxing to all imports would be a mess, and it's true what @Macil is saying about some of the things described in this issue as being incompatible with the normal imports. Not having a Deno global object on the other hand would mean that we would only pass it to something that needs it with no new import syntax, no module sanboxing, no per-module privilege configuration etc. Of course the problem is that Deno is a global variable and because of that everything has access to everything. If there was a command-line switch to turn off Deno (like with noDenoNamespace for workers in #2717) then most of the things described in the comments above would be easy to implement with changes in Deno itself. (Of course it would be possible for one module that gets the Deno object to pass it to another one, but it will be at least possible to control which library gets access to what. Currently everything has full access to everything so any restriction of that could only be an improvement.) A switch like this being optional would not break any existing code. Some security-conscious libraries could In fact the only thing needed for a proof of concept would be a way to remove Deno from the global object. @kevinkassimo do you think that making Deno not deletable/writable as you did in #2153 could be the default but optional, with a command line-switch that disables the freeze? I agree with @ry saying that There is no avoiding the need to audit code you depend on but if every code that we depend on (and the code that this code depends on, etc.) was 100% proved to never make a mistake of writing to a wrong directory then we wouldn't need Having an additional layer of safety by (optionally) not exposing the Deno object to every dependency would be a nice feature that is not hard to implement (just a way to delete the global Deno would be enough). Edit: I think that by not having a global Deno most of the features described by @rivertam in Permissions on a module-level #171 could be done with no changes to the import syntax needed. Of course it might not be as convenient, but it will all be possible (please @rivertam correct me if I'm wrong). |
@rsp Removing the Deno object from the globals alone isn't secure because modules can redefine global functions in order to capture the Deno object. For example, if you had a privileged module use a privileged API in a callback that's passed to a global function, like Removing Deno from globals could make modules easier to manually audit, but it seems like a big change if it doesn't make things secure, and it might give some users a false sense of security. There is a Javascript standard proposal for allowing this pattern to be secure though: https://github.com/tc39/proposal-ses. The idea is that you could make a Frozen Realm where all of the global variables are fully immutable, and then you could execute untrusted code within that realm. Code within the frozen realm wouldn't be able to redefine global variables to try to capture capabilities (the Deno object), and therefore could only use capabilities if passed to it. |
@rsp Haven't fully gone through your reply, sorry if I missed anything. I still believe making |
IMO a pattern I think might be acceptable is to make main worker having access to I would even imagine a library developed to hide all these details: a "polyfill" for Some similar work I have seen before: https://github.com/ampproject/worker-dom this library implements many DOM APIs through messaging to main worker. |
@kevinkassimo, If there was an option like this, with some way to get the
That would be the most secure option but it would mean that using something like lodash running in a different worker would be not as convenient. But it's a nice idea of having a I am not arguing that the pattern that I am proposing is the only or the best way to solve all problems, I am just saying that it is one way with a relatively simple solution (e.g. compared to changing the syntax of imports and convincing TC39 to adopt it etc.) |
@Macil , In other words, the worst case scenario would be to have what we got today, where every module has access to Deno.readFileSync with no need for a coincidence of using a function like that in some other part of the code and having to exploit that without breaking all other code that uses [].map(). By the way, I think that by default all built in prototypes should be frozen. |
@Macil, to answer your edit:
Yes, this is what would be perfect but at least being able to restrict Deno would be a step in the right direction. The frozen realms were discussed in #378 and I agree with what you wrote there. |
@bartlomieju and I talked last night on another topic which seems to relate somewhat to this. We are going to build a worker in Of course as V8 would implement SES, Deno would also integrate it as well. |
Hi team, I was looking to create an issue about this, but this issue seems the right spot to put more use case. isolated-vm You probably already saw that, if I'm not mistaken it's currently used by Cloudflare for their own "lambdas" (workers) and we currently used it in Algolia for our Crawler custom codes. It helps letting the user code a function with anything they want inside and execute it on our servers while not being afraid of leaking info or break our infra. Right now it's a bit cumbersome, the only way to use it correctly when providing a base of code, is to compile your whole script, then compile your untrusted script and then send both to a new instance of Why Deno could be a good fit? I believe it could be part of your roadmap to add more flags to control the execution of scripts even though you are not aiming at full sandboxing. Like Example # Pseudo code example
const myUntrustedCode = `a ts file in a string format`;
const p = Deno.run({
args: [
"deno",
"run",
"--allow-read=false",
"--alow-write=false",
"--allow-net=false",
"--allow-env=false",
"--allow-import=false",
"--allow-modify-builtin=false",
"--cpu-max=10%",
"--ram-max=512",
"--execution-time-max=10s",
"--import=./my-trusted-base-code.ts",
myUntrustedCode,
],
stdout: "piped",
stderr: "piped"
}); Notice that I'm not even talking about sandbox here, having flags is sufficient enough to create a sandbox without having a new api. This would allow:
I believe it can benefit a lot of backend developers, even the ones that are not aware that they have security issues. |
We need this feature to ensure that only a small chunk of code have FFI access. #5566 It is like Otherwise, any part of your code can call from libc and summon dragons if you want to use FFI. |
There has been a lot of good discussion here over the years, but at this point there isn't anything where the upside benefit of increasing security would outweigh the technical or usability challenges of such a solution. We have items like package locks and finer grained controls around dynamic imports and worker scripts and work to vary permissions with workers, which is likely the best "sandbox" approach (see: #4687). Based on this I am closing this for now. |
@ry This is a game changer, don't regret not implementing this |
Sorry if this has been asked or discussed before. Can't seem to find it.
Can I make sure imported scripts are in Sandbox mode even if my own script isn't.
// I'd like to make sure this imported script doesn't have access to disk or network even though my code is networking
import { SomeFeature} from "https://url.com/some-small-library.ts";
The text was updated successfully, but these errors were encountered: