nodejs / security-wg

Node.js Ecosystem Security Working Group
MIT License
490 stars 122 forks source link

Permission Model #791

Closed RafaelGSS closed 1 year ago

RafaelGSS commented 2 years ago

Permission Model initial issue

Hello everybody!

Following up on the Security Model initiative and the Mini summit (Next-10) in April seems and consensus that Node.js aims to have a security permission system, thus, avoiding third-party libraries to access machine resources without user consent.

This system was previously researched by James Snell and Anna Henningsen[1], which resulted in an excellent material as a starting point.

For context, the material is available through those links:

Constraints

This security model is not bulletproof, which means, there are constraints we agree on before implementing this system:

Points to be discussed

This is a big topic that could lead to several discussions topics. In order to avoid unnecessary discussions at this moment, let's use the following 3 topics as boundaries for this first iteration.

1) Should it be process or module scoped?

The effort to make it work with modules is worth it? Example:

{
  "name": "package-name",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "permissions": [
    "fs"
  ]
}

Then the user should provide permission for each module (as you need to do in your smartphone apps)

2) Should the user be able to change the permissions in runtime?

If yes, how does it behave in an Asynchronous Context? Example:

const { setTimeout } = require('timers/promises');

console.log(process.policy.check('net')); // true

process.nextTick(() => {
  process.policy.deny('net');
  setTimeout(1000).then(() => {
    console.log(process.policy.check('net')); // false
  });
});

process.nextTick(() => {
  setTimeout(1000).then(() => {
    console.log(process.policy.check('net')); // ???
  });
});

3) What would be the desired granularity level?

cc/ @jasnell @Qard @mcollina @mhdawson

vdeturckheim commented 2 years ago

Thanks for opening the topic - for questions 1 and 2, I have two concerns on the direction on the answers:

  1. Should it be process or module scoped?

tbh, I am more scared by the performance hit or feasibility of a module-scoped approach. Do we have guidelines on how to reach that?

  1. Should the user be able to change the permissions in runtime?

If so, we need to find a way to make sure potential malicious code does not rewrite policies to its own interests at runtime.

RafaelGSS commented 2 years ago

tbh, I am more scared by the performance hit or feasibility of a module-scoped approach. Do we have guidelines on how to reach that?

Yes, being module-scoped certainly would add such as big complexity. I'm not sure about the performance impacts, though. I think is feasible to look at a possible implementation and see if it's, at least, reasonable to do in the core.

If so, we need to find a way to make sure potential malicious code does not rewrite policies to its own interests at runtime.

Yes, good point.


Maybe we can start with an MVP and then start iterating over it when the need arises. What do you think? I mean, I can start an MVP with:

And then, along this discussion, I can see the viability of the further updates

UlisesGascon commented 2 years ago

Thanks for the thread @RafaelGSS! 🙌

Should it be process or module scoped?

I suggest process model. We keep a simpler scope for the MVP.

There are some complex scenarios if we want to support modules, as seems hard deal with a big dependency tree like:

My projects dependes on package-a and package-b. I want that library-a can modify files but the opposite for package-b, then my settings for modules are:

[
  {
    "name": "package-a",
    "permissions": [ "fs" ]
  },
  {
    "name": "package-b",
    "permissions": null
  }
]

But then package-a and package-b depends on package-c, how does the modules policy works in this scenario ? Because from package-a should be fine to use fs but not from package-b

Should the user be able to change the permissions in runtime?

I agree with @vdeturckheim malicious packages can do modifications. In the other hand we might need that flexibility in some cases, so... maybe we can assume that by default is not possible to modify the policies without restarting the application at least you specifically allow that behaviour (and assume the risk) by using an specific flag like --allow-policy-changes

What would be the desired granularity level?

+1 to Deno model (at least for the first iteration) and then find if we really have a solid feedback to add more complex features.

jasnell commented 2 years ago

For 1, I think the process-scoped is the only viable answer. Module-scoped is going to be far too complex and disruptive to do, and there's really no clear must have use case that would justify it.

For 2, I would argue that a model where permissions can only be restricted at runtime would be appropriate. That is, the process starts with a given set of permissions, within the code, there are options for restricting permissions further (either something like process.permissions.deny('net') that immediately turns it off for the entire process or something like process.permissions.deny('net', () => { ... }) where the callback passed in runs with the more restrictive... with my preference being on the former).

For 3, keeping the model similar to Deno's is great but it's going to be very difficult to change later if we don't make it granular enough. Let's be sure to give this particular bit a good deal of thought.

bengl commented 2 years ago
  1. Should it be process or module scoped?

Multiple threads could run with different policy (with caveats), but that's not terribly different from process-level policies. So yes, process scoped. Module-level policies aren't possible to enforce without non-trivial barriers between them and even at that point you run into logical nightmares as pointed out by @UlisesGascon.

  1. Should the user be able to change the permissions in runtime?

Much like with other well-known and well-used permissions systems, code ought to be able to decide it can drop privileges, but never be able to grant itself any expanded privileges.

If yes, how does it behave in an Asynchronous Context?

It should be applied synchronously, much like other things that modify process-level behaviour do today. For example, changing environment variables or using process.setuid() are synchronous operations that take effect immediately. Attempting to have policies follow through with async context (like AsyncLocalStorage does) means invoking async_hooks and the performance hit from that, and ensuring the correctness of async_hooks, even in weird security scenarios, which is non-trivial at the very least.

If I'm reading the example correctly, both calls to check() happen at least 1000ms after deny() has been called, so under my suggestion, the second check would also return false.

  1. What would be the desired granularity level?

Filters on any given API should eventually give the ability to filter on individual operations and individual resources. For prior art on this, see https://web.archive.org/web/20190821102906/https://intrinsic.com/docs/latest/index.html, which describes a (now-defunct, but previously working) policy system for Node.js.

An MVP blocking entire subsystems is fine for an MVP so long as the configuration format doesn't preclude any further detail in filtering.

mcollina commented 2 years ago

I think we must provide some level of restrictions on the file system or what host/port a process can connect or listen to.

As an example, I might want to limit the folder in which a process write files to the tree of folders from the nearest package.json. This will severely limit the surface attack of a malicious dependency.

vdeturckheim commented 2 years ago

What were actually the points that prevented the previous iterations. Given the current discussion, I am under the impression that at least one prior PR did most of what we are heading to?

jasnell commented 2 years ago

The thing that stopped the prior effort was lack of engagement... We couldn't get anyone else to engage in the conversation enough to move it forward. Hopefully the timing is better this time

RaisinTen commented 2 years ago

What would happen if we start node with --policy-deny=net and the main process spawns a child process? Will the child process be able to access the net?

jasnell commented 2 years ago

@RaisinTen ... see the discussion on that point here:

A core part of Node.js is the ability to spawn child processes and load native addons. It does no good for Node.js to restrict a process’s ability to access the filesystem if the script can just turn around and spawn a separate process that does not have that same restriction! Likewise, given that native addons can directly link to system libraries and execute system calls, they can be used to completely bypass any permissions that have been denied.

The assumption, then, is that explicitly denying any permission should also implicitly deny other permissions that could be used to bypass those restrictions. In the above example, invoking the node binary with --policy-deny=net would also restrict access to loading native addons and spawning child processes. The --policy-grant would be used to explicitly re-enable those implicitly denied permissions if necessary.

In other words, a process that has --policy-deny=net will not be able to spawn a child process by default. If the user explicitly allows it to spawn a child process, then it will be the users responsibility to pass along the correct arguments.

Qard commented 2 years ago

Personally I would be really interested in some sort of module-scoped solution, though I feel like that would likely be very complicated to manage given you'd have to handle permission delegation across the dependency graph. You'd probably need not only permissions to use certain things in a module but then also permission to delegate those permissions to its dependencies, and the config would likely get super complicated.

I feel like process-scoped permissions are too simplistic though. A user might turn on net access because one module needs it but then some other unrelated module does something nefarious and doesn't get blocked because net was allowed. 🤔

Perhaps an import/require-based system where a module needs permission to load another module? So it would just be unable to attempt to load a module it's not supposed to be able to use, throwing a permission error on import/require. It wouldn't be too terribly difficult to specify something like --allow=http:fastify,ws to specify that the http module should be allowed to be used directly by the fastify or ws modules, but no others.

RafaelGSS commented 2 years ago

I feel like process-scoped permissions are too simplistic though. A user might turn on net access because one module needs it but then some other unrelated module does something nefarious and doesn't get blocked because net was allowed. thinking

Perhaps an import/require-based system where a module needs permission to load another module? So it would just be unable to attempt to load a module it's not supposed to be able to use, throwing a permission error on import/require. It wouldn't be too terribly difficult to specify something like --allow=http:fastify,ws to specify that the http module should be allowed to be used directly by the fastify or ws modules, but no others.

As described by others, I'm not sure if the complexity to handle module-scoped permissions is worth it. It seems the user can prevent the above behavior by specifying the allowed net port. For instance, --allow-http=3000, then even if another unrelated module does something nefarious, it wouldn't be allowed due the port is already in use & only 3000 is allowed.

RafaelGSS commented 2 years ago

It seems to have a consensus in a MVP with:

Are we all in agreement on that? In case, I'll create a fresh PR (probably using most of the work @jasnell did in: https://github.com/nodejs/node/pull/33504) and then we can iterate over that.

EDIT: Regarding the nomenclature (policy-deny= or policy-deny-net) we can discuss in the PR.

bengl commented 2 years ago

@qard

Any permission system with support for multiple sets of permissions requires sufficient separation between the permission sets and the units of isolation they apply to. This means the units of isolation cannot communicate with each other except through strictly controlled interfaces (meaning no shared objects, not even globals). Without this, privileges can easily leak between the units of isolation. We simply do not have this between modules, and any sensible approach to having this between modules requires massive changes in the way we both import and call code from other modules.

To illustrate this, consider a module a.mjs and module b.mjs. Assume that through whatever permission system we create, we give a access to net, but deny that to b. A poorly written or malicious a could just assign all the relevant net methods to the global, and then b can use it.

A common suggestion is to track module usage via the stack, but this is not performant at all. Even if it were, there are other ways to share functionality and have the calling module appear to be the one with adequate permissions. You'd have to block transitive access, and then the number of related problems here starts expanding.

Qard commented 2 years ago

Sure, there's likely always going to be ways to get around it. But process-scoped just means rather than needing to do some environment hacking if you aren't the specific module the user intended to have access, you will just have access automatically by inheriting it from the process-wide config needed for something else. So you really aren't protecting much of anything if users are just always turning on net access. It seems to me that to adequately control I/O you need either module-level blocking of use in some way or super granular process-wide control of interfaces like saying http requests can only be made to this specific list of URLs or file system access only allows reading these specific files and writing these other specific files.

I don't think we can escape significant complexity without making any sort of policy system essentially useless.

jasnell commented 2 years ago

To deal with more granular levels like per-module, what we would need is proper sandboxing of isolates and global scopes. We could set it up so that worker_threads can be launched with either the same or more restrictive permissions than the current thread; and if we ever did introduce a properly sandboxed isolate mechanism when we could launch those with their own permissions also.

bengl commented 2 years ago

@Qard

It seems to me that to adequately control I/O you need either module-level blocking of use in some way or super granular process-wide control of interfaces like saying http requests can only be made to this specific list of URLs or file system access only allows reading these specific files and writing these other specific files.

Yep! And I'm saying the latter is the only actually feasible one. The approach currently suggested by @RafaelGSS seems to be not as granular for permissions as the ideal, but keeping room open for it.

@jasnell We could do that, but to then have it be per module we'd have to consider:

  1. The weight of all those isolates.
  2. Providing and securing the interfaces between the isolates (non-trivial).
jasnell commented 2 years ago

Yep, which is not a level of complexity we should take on initially.

Qard commented 2 years ago

Could probably make some changes to the vm module to make contexts more isolated and attach security controls to that. I'm definitely getting a sense though that whatever we do we can't really escape that it's going to be a whole lot of work and require substantial changes to many parts of the platform to reach any level of maturity. For sure we can start with some MVP, but I suspect that won't actually be all that useful short of serving as a proof-of-concept.

MarcinHoppe commented 2 years ago

This is a great discussion and this effort is a great way to raise the the overall security bar in the Node.js ecosystem.

I noticed that this discussion became very detailed very quickly. I'd like to highlight some areas that would be good to address in parallel with the implementation work.

Community outreach and documentation

Introducing fundamental security changes to a mature technology is a delicate task. The new model may be surprising to a lot of Node.js developers. I think that in parallel with implementing the feature we should be thinking about how to explain the changes to developers, how to convince them it is safe to use and valuable for their applications. We should have a good idea about how to guide them when to use the new mechanisms and how to introduce them gradually into existing applications.

Threat model

I wonder if there has been any prior work done in documenting specific threats and attacks this work would prevent? I feel having a well developed threat model would allow us to better communicate the value proposition as well as the limitations of the solution.

Security testing

The threat models should clearly indicate what types of attacks will be prevented when this work has landed in Node.js. I think it would be super valuable if we had a plan on how to engage the security research community during development of these changes to help us find weak spots and identify limitations of the approach and the implementation.

I know these issues are somewhat tangential to the actual implementation work, and I'll be more than happy to start separate issues to discuss them in more depth without disrupting this discussion.

UlisesGascon commented 2 years ago

Great approach @MarcinHoppe! 🙌

We are running a parallel discussion in Slack regarding the documentation of Security Model and Threat models and current state of art. I prepared a proposal in web format to start the discussion in the Security WG (How to structure the documentation, what should content...). It would be fantastic if you can join the discussion and help us validate and evolve the proposal 🙏

MarcinHoppe commented 2 years ago

I'll chime in that discussion on Slack!

RafaelGSS commented 2 years ago

@MarcinHoppe Thank you! This strategy is also documented by https://github.com/nodejs/node/pull/42709.

brianleroux commented 2 years ago

IMO this is security theatre. There are two audiences for runtimes security. People running workloads on the cloud. And people running local dev tools on their machine. With the cloud we can't rely on runtime security for governance. Locking down Node won't do anything for the overarching infrastructure requirements so, in most cases, the vendor will have a concept called IAM which enables complete granular security surrounding the runtime. Said another way, this feature won't make sense for the cloud. So that leaves devs running local dev tools. This audience is even less likely to use granular permissions to get the job done and will more likely run with --allow-all or equiv.

Overall I'm against more complexity, without much concrete benefit, and especially in the code loading paths which are too slow already compared to other JS runtimes.

RafaelGSS commented 2 years ago

That's a fair point. I think the parallel discussion on Slack will bring relevant points towards this point of vision. I do feel that we can't generalize all the Node.js apps running in a cloud that ensures the application security using IAM, nor that local dev tools would run the application using --allow-all.

The developer environment is a relevant security threat, and Node.js doesn't provide a security mechanism to avoid malicious packages using host sensitive data

mhdawson commented 2 years ago

Threat model

This was discussed in the mini-summit and I think defining/documenting the security model as captured in https://github.com/nodejs/node/blob/master/doc/contributing/security-model-strategy.md#document-the-security-model and then the followon https://github.com/nodejs/node/blob/master/doc/contributing/security-model-strategy.md#document-threat-models-and-current-state-of-the-art are steps that benefits the Node.js project even if we don't enhance functionality but might also be the foundation for what @MarcinHoppe mentions as important for motivating/explaining any enhanced functionality. I think focusing on those to start will deliver value and make future discussion about specific enhancements easier.

RafaelGSS commented 2 years ago

Hey there!

To follow up on this, I've been working on implementing an MVP for the FileSystem access in the past weeks.

This is the roadmap I have in mind(Feel free to edit in case I'm missing something):

This is the diff so far: https://github.com/nodejs/node/compare/master...RafaelGSS:feat/permission-system?expand=1.

Along with this implementation, I have created a document that aims to drive this API design. It's currently discussed in every Security WG Meeting (feel free to join if you are interested in this feature) - This is still under development.

Remember: no discussions to the nomenclature were made, don't pollute the issue with concerns regarding that. The MVP scope is: https://github.com/nodejs/security-wg/issues/791#issuecomment-1106581564

RafaelGSS commented 2 years ago

See: https://github.com/nodejs/node/pull/44004

arhart commented 2 years ago

It’s not a sandbox, we assume the user trusts in the running code

@RafaelGSS if we assume the user trusts in the running code, what problem is the permission model trying to solve?

Also, many of the comments discuss protecting against malicious code. Isn't that out of scope based on the assumption the user trusts in the running code?

Perhaps the user partially trusts the running code, and the feature is to let the user specify what the running code should or should not be trusted with (ex: trust it to read a document to process but not to access ~/.ssh or the network). In which case escape from the restrictions imposed should be impossible, even for malicious code, but granting the ability to launch other processes is granting the ability to escape restrictions. Is that the goal?

RafaelGSS commented 2 years ago

FWIW The @arhart question was answered in the Node.js Security WG.


@GeoffreyBooth I'm moving the concern you raised to this issue. It seems an appropriate place to have this discussion.


Thanks. I would also take a look at https://deno.land/manual/getting_started/permissions, there are a lot of good ideas there. For example, they separate the permissions for filesystem read and filesystem write; that's probably something we should do too. For network access, if it's possible to create separate permissions for full network access versus only responding to incoming requests, that would be a good distinction too. Such a permission would let you spin up a webserver that wouldn't be capable of exfiltrating data, like if one of your dependencies stole your environment variables on startup and posted them somewhere. (It would still be vulnerable to data exfiltration as part of responding to a request, but at least the other attack vector would be denied.)

Before creating this PR I did a long study leveraging other resources as part of the foundation (this is described by this issue, the Deno permission system was also mentioned). Other resources such as networks in will be handled on further pull requests. The way the code was designed, any additional module should be easier to implement.

Honestly, I don’t see the existing feature overlapping with this one. As said above, they might have a similar behaviour, but the purpose is different.

I think while they’re both experimental it’s not urgent to resolve the differences between the two; but I think a coherent user experience for users using both features together needs to be something that we prioritize as you’re developing this. It feels to me like something that should be worked out early on, in a design phase, not after you’ve already landed the first PR; but I won’t block on those grounds.

In general I think it’s bad UX to have two features that achieve the same result, and have similar-seeming intentions, but with completely different methods (a flag versus a config file). I understand that the two features don’t fully overlap, that each one does some things that the other doesn’t, but in a way that makes this even harder to resolve because you can’t just replace one with the other. I want to resolve the conflict of how to handle what overlap they do have as early as possible so that development can continue on both without significant breaking changes or refactoring needed by either team. I’m not sure I can attend the next security meeting; perhaps just open a discussion issue and we can hash it out async?

It's extremely important to mention that those features wouldn't achieve the same result. The policy system acts in the application bootstrap, you can deny access to modules, the permission system instead, allows the user to deny access to resources. Restricting what modules can be loaded is a separate problem from limiting what actions those can take on the operating system. To enhance this discussion I would love to hear @bmeck thoughts too.

IMO this feature (permission system) doesn't overlap with the policy system at all. I see it as a new security mechanism for developers. Both features can be used in a single application each one with its own purpose.

GeoffreyBooth commented 2 years ago

IMO this feature (permission system) doesn’t overlap with the policy system at all.

I understand your point, but from the user’s perspective they seem to overlap. See in https://nodejs.org/api/policy.html#:~:text=The%20following%20example%2C%20would%20allow%20access%20to%20fs%20for%20all%20data%3A%20resources%3A, “The following example, would allow access to fs for all data: resources”—that sounds a lot like you’re restricting access to the file system, and you could do so for not just data: resources but all of them. The end result of this would appear to be the same as --deny-fs. I understand that there are technical reasons why they’re actually not the same and that the permissions flag is more powerful, but that distinction is easily lost on users.

The policies feature seems like it was intended to start with file loading and continue from there, supporting other actions that the user might want to explicitly give permission for (otherwise why would it have been named “policies”). A flag-based system that hooks into bootstrap before module loading is more powerful, and probably a better approach for that; but the current “policies” feature has some useful abilities that we want to preserve, like redirecting specifiers and validating integrity hashes. Perhaps all it needs is a new name that’s clearer that it’s restricted to things related to module loading, though I don’t know what that name might be; though the examples about allowing or denying access to fs (like under https://nodejs.org/api/policy.html#dependency-redirection) strike me as ripe for user confusion. Maybe it’s fine to leave that as it is, and --deny-fs is a systemwide setting while resources.dependencies is a per-resource setting; but it feels like a UX footgun where users might be confused as to which feature they should be using to achieve the “deny access to the file system” desired result.

bmeck commented 2 years ago

Nothing in policies was intended to absolutely limit scope to module loading. Not quite sure how/why current feature set is being seen as the absolute scope or intention. I'm actually of the opposite opinion and concerned about the lack of complexity in the permissions system. Having it done via flags is pretty irrelevant when we can just set it as the defaults that any policies + permissions share but an entirely different scoping/cascade is certainly a bad thing in my mind.

That said. I'm not really going to have time to try and merge the models. Making permissions configuration available in the policy file would be great so things like integrity on permissions granted can be done as well as things like supporting --conditions and scopes in same manner.

I think the main difference between the goals is a desire to have permissions set close to the C++ binding layer but don't see how that inherently makes the features discrete to a user.

On Mon, Aug 8, 2022, 11:05 PM Geoffrey Booth @.***> wrote:

IMO this feature (permission system) doesn’t overlap with the policy system at all.

I understand your point, but from the user’s perspective they seem to overlap. See in https://nodejs.org/api/policy.html#:~:text=The%20following%20example%2C%20would%20allow%20access%20to%20fs%20for%20all%20data%3A%20resources%3A, “The following example, would allow access to fs for all data: resources”—that sounds a lot like you’re restricting access to the file system, and you could do so for not just data: resources but all of them. The end result of this would appear to be the same as --deny-fs. I understand that there are technical reasons why they’re actually not the same and that the permissions flag is more powerful, but that distinction is easily lost on users.

The policies feature seems like it was intended to start with file loading and continue from there, supporting other actions that the user might want to explicitly give permission for (otherwise why would it have been named “policies”). A flag-based system that hooks into bootstrap before module loading is more powerful, and probably a better approach for that; but the current “policies” feature has some useful abilities that we want to preserve, like redirecting specifiers and validating integrity hashes. Perhaps all it needs is a new name that’s clearer that it’s restricted to things related to module loading, though I don’t know what that name might be; though the examples about allowing or denying access to fs (like under https://nodejs.org/api/policy.html#dependency-redirection) strike me as ripe for user confusion. Maybe it’s fine to leave that as it is, and --deny-fs is a systemwide setting while resources.dependencies is a per-resource setting; but it feels like a UX footgun where users might be confused as to which feature they should be using to achieve the “deny access to the file system” desired result.

— Reply to this email directly, view it on GitHub https://github.com/nodejs/security-wg/issues/791#issuecomment-1208885646, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZJI3U3WTTF3VFBHWATSDVYHKJNANCNFSM5T3PDW4Q . You are receiving this because you were mentioned.Message ID: @.***>

arhart commented 2 years ago

Clarification on the goals of both features could be helpful.

Although I've read through both a Google doc and a side deck, I still don't understand the goals behind policy. For example, why is it expected to apply file ownership and permission settings to prevent modification of the policy file but not expected to do the same for the JavaScript source?

Nothing in policies was intended to absolutely limit scope to module loading.

The introduction to policies in the documentation gives that impression.

Node.js contains experimental support for creating policies on loading code.

Policies are a security feature intended to allow guarantees about what code Node.js is able to load. The use of policies assumes safe practices for the policy files such as ensuring that policy files cannot be overwritten by the Node.js application by using file permissions.

If "loading code" means more than modules, the documentation could be clearer. For example several readings of the documentation didn't make it clear to me that the main script's package.json could be covered by policies.

If there is a better place for this discussion it can be moved.

bmeck commented 2 years ago

Although I've read through both a Google doc and a side deck, I still don't understand the goals behind policy. For example, why is it expected to apply file ownership and permission settings to prevent modification of the policy file but not expected to do the same for the JavaScript source?

For this very specific question it is likely outside the actual scope of this dicussion; ideally using --policy-integrity is better, but in the case of exploitation having the policy file completely locked down would still prevent loading untrusted code. Relying entirely on file system permissions is tricky and we could go into a course on how to sandbox a fs but people do weird things in node deployments, from layered fs to multi-user global shared directories, etc. Definitely should lock down everything you can, never says you shouldn't lock down your source code, and that would be inappropriate given the docs are for the specific policy feature.

The introduction to policies in the documentation gives that impression. Node.js contains experimental support for creating policies on loading code.

This describes the feature as is; documentation is not scoping or roadmapping. E.G. esm didn't document intention to load off network until experimental feature landed; cjs still doesn't document intention to eventually use loaders/hooks to replace require.extensions. Don't use documentation for scoping/constraints.

If "loading code" means more than modules, the documentation could be clearer. For example several readings of the documentation didn't make it clear to me that the main script's package.json could be covered by policies.

It is somewhat intentionally just scoped to loading code. package.json is part of loading code and discussed in all documentations on how module loading is performed in algorithmic steps. A call out might be good to add, but I'd be against maintaining a list of all things similarly to how I'd be against a list of all the ways you need to lock down a fs

If there is a better place for this discussion it can be moved.

Unclear where would be better. Policies are mostly on topic just for how they perform integration with this feature / conflict. The potential conflicts are not with implementation but rather with conventions and best practices. Having multiple scoping/cascading conventions would mean needing to cover CWEs for both independently and users to configure both with different skill sets.

RafaelGSS commented 2 years ago

Clarification on the goals of both features could be helpful. - @arhart

The goal for the Permission System is pretty straightforward. Supply Chain Attack is one of the attacks that anybody is vulnerable to nowadays. Today, if you install a package using npm i x, the package can exploit it in several ways. This security feature consists to grant powers to the developer in order to select which resource an application is allowed to execute

It's important to mention that, this feature will not mitigate supply chain attacks, but in theory, if this feature were used correctly, it can help. As I said previously, this is an MVP and once it releases and gets feedback from the community, it will be more accurate.


@GeoffreyBooth Thanks for the clarification. It indeed seems to overlap from a user perspective. However, it's not clear to me what would need to be done from the Permission System side to avoid that bad UX. Is it just naming? Should it accept the overlap and use the policy namespace, including the interoperability with policy.json? I really wouldn't like to see it interchanging with the current policy mechanism because they are quite different (technically speaking), but I'm totally open to discussions.

GeoffreyBooth commented 2 years ago

However, it’s not clear to me what would need to be done from the Permission System side to avoid that bad UX. Is it just naming?

I think naming at the very least. If you go with flags like --allow-* and --deny-*, then all that remains is what to call the page in the docs, and I would suggest that maybe both features are discussed on the same page. Personally I think Permissions is a better overall name than Policies, but I don’t work in the security space so I defer to everyone else’s judgment. I would group the two overall features under subheadings like “Process-based Permissions” and “Resource-based Permissions” as the flags control what the overall Node process can do whereas the file controls what various files/modules can do. I think if this is all on the same page with an introduction that explains the difference between the two general features, that would go a long way toward avoiding user confusion. (And again this is all just a first draft on my part, don’t think it has to be this way; this is just my stab at a starting point for your team to iterate from.) What do you think?

Also there’s something to be said for basing permissions around resources or file paths. I think it would be a very common user desire to lock down third-party dependencies while still allowing full access to the user’s own app code. Like for example network access could be allowed from the root down but denied from ./node_modules down, except for ./node_modules/fastify say; that would let me run a Fastify server but none of my other dependencies could make network requests. If all the flag can do is let me turn network access on or off at a global/process level, it doesn’t let me protect myself against malicious dependencies while still letting the app function.

RafaelGSS commented 2 years ago

I think naming at the very least. If you go with flags like --allow- and --deny-, then all that remains is what to call the page in the docs, and I would suggest that maybe both features are discussed on the same page. Personally I think Permissions is a better overall name than Policies, but I don’t work in the security space so I defer to everyone else’s judgment. I would group the two overall features under subheadings like “Process-based Permissions” and “Resource-based Permissions” as the flags control what the overall Node process can do whereas the file controls what various files/modules can do. I think if this is all on the same page with an introduction that explains the difference between the two general features, that would go a long way toward avoiding user confusion. (And again this is all just a first draft on my part, don’t think it has to be this way; this is just my stab at a starting point for your team to iterate from.) What do you think?

I agree with the namespace --deny-* and I would include the @Qard suggestion:

Is there a reason we can't follow the Deno model but have permissive by default unless there are --allow-* flags set? It should mean no ecosystem breakage. Also gives us a path to gradual adoption and then have the opportunity to flip to locked down by default in the future, if we so choose.

In the second iteration of this feature. Once it should land as experimental, we can design better the API based on its usage. Regarding the documentation namespace, I don't see with good eyes (at least in this moment), grouping the Permission System docs into the policy mechanism. My initial idea is to create a new namespace (https://nodejs.org/api/permissions.html?) including the purpose of the feature and how it can be used together with the policy to improve the security of a Node.js app (avoiding the confusion you mentioned)

Also there’s something to be said for basing permissions around resources or file paths. I think it would be a very common user desire to lock down third-party dependencies while still allowing full access to the user’s own app code. Like for example network access could be allowed from the root down but denied from ./node_modules down, except for ./node_modules/fastify say; that would let me run a Fastify server but none of my other dependencies could make network requests. If all the flag can do is let me turn network access on or off at a global/process level, it doesn’t let me protect myself against malicious dependencies while still letting the app function.

As said previously in this thread, this would add a big complexity for the MVP. Your use-case of fastify would be superseded by just: --allow-net=in:3000. So, if any other dependency tries to listen to a socket in any other port, it wouldn't be allowed (you will need to handle out, though - but let's discuss any other implementation when the feature becomes a bit more mature). When you run any Node.js application all the code that runs is considered user-land code. Making a distinction between which modules shouldn't be able to access specific resources is inefficient (at least, I don't see how it gonna work without adding a huge complexity and compromising the performance). Actually, this is a good example of policy and the Permission System working together. You can deny access to the net module to all the modules except fastify + you can deny access to the entire FileSystemOut using the Permission System.

GeoffreyBooth commented 2 years ago

I feel pretty strongly that both features should be on the same page of the docs, with an introduction explaining how they differ. That seems like the least we can do to avoid user confusion, since then you couldn’t discover one without the other.

# Permissions

<introduction: process-based permissions and resource-based permissions>

## Process-based permissions

<new feature documented>

## Resource-based permissions

<existing policies docs>
RafaelGSS commented 2 years ago

So it would need to change the doc namespace policy to something else, right?

GeoffreyBooth commented 2 years ago

So it would need to change the doc namespace policy to something else, right?

Yes. Either “Resource-based permissions” or “Module-based permissions,” I think; but probably the former as the API uses "resources" as the field in policies.json and users can represent non-module things like all data: URLs.

We could perhaps land these renames first as its own PR, then land the permissions PR that adds a big section to this page. That might be the easiest to follow.

These are just my opinions, though, I’m curious to hear what others think, especially @bmeck.

bmeck commented 2 years ago

Renaming is fine as long as redirecting existing link url works. Having them configured so differently/ not even being able to use 1 configuration mechanism is awkward if they are coupled but out of scope. Still worried about only being able to apply integrity and persistence to the file based configs.

On Thu, Aug 11, 2022, 10:39 PM Geoffrey Booth @.***> wrote:

So it would need to change the doc namespace policy to something else, right?

Yes. Either “Resource-based permissions” or “Module-based permissions,” I think; but probably the former as the API uses "resources" as the field in policies.json and users can represent non-module things like all data: URLs.

We could perhaps land these renames first as its own PR, then land the permissions PR that adds a big section to this page. That might be the easiest to follow.

These are just my opinions, though, I’m curious to hear what others think, especially @bmeck https://github.com/bmeck.

— Reply to this email directly, view it on GitHub https://github.com/nodejs/security-wg/issues/791#issuecomment-1212698971, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZJI6V4GHGRX6TFUNCZRDVYXBPBANCNFSM5T3PDW4Q . You are receiving this because you were mentioned.Message ID: @.***>

RafaelGSS commented 2 years ago

Could somebody review https://github.com/nodejs/node/pull/44222?

github-actions[bot] commented 1 year ago

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

LongTengDao commented 1 year ago

As I said in https://github.com/nodejs/security-wg/issues/467, I think most important thing is that, in 99% case, we want an app's main part run in full feature mode while some dependency (and its dependency chain) running like a pure function, at most with limited fs/net ability:

current

// app
const a = require('a');
a(__dirname+'/tmp/name', 'data');
// a
module.exports=require('b');
// b
module.exports=require('c');

...

// y
module.exports=require('z');
// z
module.exports=(path, data)=>require('fs').writeFileSync(path, data);

proposal (minimal sample)

// app
const a = require.pure('a', {
    net: (address, port) => false,
    fs: (path, mode) => mode==='w' && path.startsWith(__dirname+'/tmp/'),
});
a(__dirname+'/tmp/name', 'data');

then there is only two context: app, and a-z.

RafaelGSS commented 1 year ago

That's under development. I'm currently fixing a few tests, but the unpermissive approach is ready (merging it to nodejs/node#44004 pretty soon).

tniessen commented 1 year ago

The goal for the Permission System is pretty straightforward. Supply Chain Attack is one of the attacks that anybody is vulnerable to nowadays. Today, if you install a package using npm i x, the package can exploit it in several ways. This security feature consists to grant powers to the developer in order to select which resource an application is allowed to execute

It's important to mention that, this feature will not mitigate supply chain attacks, but in theory, if this feature were used correctly, it can help. As I said previously, this is an MVP and once it releases and gets feedback from the community, it will be more accurate.

@RafaelGSS Could you elaborate on this? You say the goal is "pretty straightforward" and then talk about supply chain attacks, yet this seems to somewhat contradict this constraint:

  • It’s not a sandbox, we assume the user trusts in the running code

Is this contraint only true in the experimental prototype, or will it forever be an assumption for the permission model?

RafaelGSS commented 1 year ago

Hi @tniessen! A few things changed before the PR implementation. I'll surely review it.

The permission model should mitigate supply chain attacks if used correctly by npm/yarn/pnpm scripts. Of course, Supply Chain Attack is a broad attack and the permission model will only help in resource access.

It’s not a sandbox, we assume the user trusts in the running code

Basically, my intention with this statement was to avoid users thinking they will be able to run any code when the permission model is enabled. Although the resources must be restricted, there are many other attack vectors. But, I think I should remove this statement since it's somewhat unrelated to the feature itself.

tniessen commented 1 year ago

Thanks for the clarification @RafaelGSS :)

The permission model should mitigate supply chain attacks if used correctly by npm/yarn/pnpm scripts. Of course, Supply Chain Attack is a broad attack and the permission model will only help in resource access.

My understanding is that npm would have to at least disable native add-ons and child processes in order for this security mechanism to have any effect. So would this only be helpful for the subset of npm install executions that don't work with --ignore-scripts but that also don't use native add-ons or child processes? And in those cases, npm would have to fine-tune permissions to prevent, for example, uploading credentials from disk?

Also, it sounds like this mechanism does not have any effect on supply chain attacks that don't become active during installation but rather during normal execution, unless the user can disable subprocesses and native add-ons for the application as well.

RafaelGSS commented 1 year ago

My understanding is that npm would have to at least disable native add-ons and child processes in order for this security mechanism to have any effect.

When using the permission model (--experimental-permission), child processes and worker threads are disabled by default -- we are discussing about native addons, see https://github.com/nodejs/node/pull/44004#discussion_r1056957811).

So would this only be helpful for the subset of npm install executions that don't work with --ignore-scripts but that also don't use native add-ons or child processes? And in those cases, npm would have to fine-tune permissions to prevent, for example, uploading credentials from disk?

So, we haven't thought too much about its interoperability with package managers. My intention is to land the permission model targeting runtime usage and allowing its expansion easily. I'm pretty sure once it gets landed we'll discuss the next steps to implement it by default on package managers. Ideally, each package should provide which install access it should have (in the case of postinstall scripts) and the developer should agree on that.

Also, it sounds like this mechanism does not have any effect on supply chain attacks that don't become active during installation but rather during normal execution, unless the user can disable subprocesses and native add-ons for the application as well.

That's partially correct. Although I agree that it requires work from package managers to specify the right flags for npm install command, the subprocess and native add-ons will be disabled by default (when using the permission model). As mentioned in the PR description, this is the foundation for all further permission logic, I'm pretty sure after releasing it and collecting the feedback from the ecosystem, we will have a better vision.

mcollina commented 1 year ago

I might have lost the reasoning.. why it's not possible to support worker_threads?