nodejs / security-wg

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

Permission Model - Roadmap #898

Closed RafaelGSS closed 5 months ago

RafaelGSS commented 1 year ago

As discussed earlier in nodejs/node#44004 and #791, the purpose of this issue is to establish a comprehensive roadmap for the Permission Model. I will be proceeding with the following list of action items, and in cases where further discussion is necessary, I will initiate a separate issue for each item.

New permissions

cc: @nodejs/tsc for awareness

BrunoBernardino commented 1 year ago

Sorry if this is intrusive and not productive, I didn't see a rationale for choosing a new file (permissions.json) vs an existing file with a new property (package.json:permissions), is there one available somewhere for me to read?

RafaelGSS commented 1 year ago

This is a good discussion, @BrunoBernardino. We'll certainly evaluate that when working on this implementation. Likely, the outcome will be from the discussion with package managers.

bmeck commented 1 year ago

Is there any reason to separate policy integrity/redirects from permissions, would be ideal if they could share the same file. Particularly if eventual possibility of more granular per resource (URL) configuration.

RafaelGSS commented 1 year ago

Is there any reason to separate policy integrity/redirects from permissions, would be ideal if they could share the same file. Particularly if eventual possibility of more granular per resource (URL) configuration.

My only concern is the fact policies are experimental, so the permissions will be blocked to get out of the experimental until policies are stable. Other than that, sounds good to me.

aduh95 commented 1 year ago

My only concern is the fact policies are experimental, so the permissions will be blocked to get out of the experimental until policies are stable. Other than that, sounds good to me.

One could argue that it's another reason for using policies, so both can graduate to stable faster ;)

bmeck commented 1 year ago

Using the same file isn't really related to blocking advancement also name of the file isn't important ( can already use policies with a file called permissions.json)

On Fri, Mar 3, 2023, 6:17 AM Antoine du Hamel @.***> wrote:

My only concern is the fact policies are experimental, so the permissions will be blocked to get out of the experimental until policies are stable. Other than that, sounds good to me.

One could argue that it's another reason for using policies, so both can graduate to stable faster ;)

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

wesbos commented 1 year ago

Just gave this a run through, very excited to see this going through. Just a few notes from an outsider / end user. I'm not up to speed on everything so, please take with a grain of salt!

  1. a separate file for permissions will be groaned at. Everyone has config fatigue - putting it in package.json is ideal for me.
  2. relative paths via the CLI - I'm not sure if there is a technical hurdle here, but this would be really nice, especially for all the random npx scripts people have you running.
  3. Prompting for access would be great - similar to how Deno does it. this way you can run a script, and be prompted to see which files need to be read/written etc..
RafaelGSS commented 1 year ago

The first 2 items are already on the roadmap. The third one:

Prompting for access would be great - similar to how Deno does it. this way you can run a script, and be prompted to see which files need to be read/written etc.

Is tricky. It can open many vectors of attack and I'm not sure about how useful is it for production apps. I feel like I rather prefer to receive an exception, close my app and adjust the permissions according to the expected paths than not have it explicitly and do it by a prompt. We can definitely discuss it at Security WG. cc: @nodejs/security-wg

bmeck commented 1 year ago

I'd just note, per the config problem, it most likely should not be authoritative from a given package.json but likely could be generated from an aggregation of multiple package.json files automatically into a different file is the current thoughts. This gets very complex however, as things like npm run lint likely has different permissions desired than npm run start and would heavily clutter package.json unless making everything run at same permissions level. Certainly open to discuss but would be good to understand the desires of keeping it inline, if it is meant to be configuration or authoritative.

KennyLindahl commented 1 year ago

Background

The program i'm running is using a fake malicious NPM package (for security research).

See example attack flow:

  1. Attacker produce the package
    • This package is made to look like an Express server util
  2. Attacker publishes the package to npmjs.com
  3. The victim downloads & installs the package in their NodeJS application (unknowing of the malicious aspect)
  4. The package is used in the victims application code (since they need the legitimate part of the package)
  5. The victim starts their application locally
  6. The attacker gets reports sent from the victims computer with the follwing info:
    • Local tunnel URL (so the attacker can reach a web ui where they can send commands to the victims computer)
    • Victims public ip (used to work around security for Local tunnel, as it's required - similar to a password)
  7. The attacker can now run any command that is allowed by Node (usually root access, as proven by tests)
    • See examples below what can be run (for example)

The intention is to see how Node permission system can protect a potential victim against these types of attacks 3rd party package attacks. Without the permission system in place, a Node program running this dependency WILL be compromised (tested on multiple computers an deployed environment etc).

For code reference see here:

How my program was run

node --experimental-permission --allow-fs-read=$(realpath ./index.js),$(realpath ./node_modules) index.js

Cases

HTTP requests

When run, the malicious package could still get the victims IP and send it to the attacker (this is expected since there is no URL whitelist)

Question

So my question is if there will be HTTP request block + whitelisting of URLs?

Spawn

My fake malicious NPM package tried to start a local tunnel via spawn, and this was blocked which is good. However this was never logged in in the Node program, it was only captured and delivered as text to the attackers computer (see code below).

This is what the code looked like in the fake malicious NPM package

function spawnHelper(command, args, onData) {
  return new Promise((resolve, reject) => {
    try {
      const commandResponse = spawn(command, args);

      commandResponse.stdout.on("data", (data) => {
        onData(data.toString());
      });

      commandResponse.stderr.on("data", (data) => {
        onData(data.toString());
      });

      commandResponse.on("error", (error) => {
        onData(error.toString());
        reject(error);
      });
    } catch (error) {
      onData(error.toString());
    }
  });
}

See reference here: https://github.com/KennyLindahl/malicious-npm-package/blob/master/malicious.js#L82

And executed via:

await spawnHelper(localTunnelBinPath, ["--port", PORT], report);

Question

Is this expected? In a real scenario i would never know what the attacker has tried to do etc, which is not ideal. Ideally i should be able to:

Thanks

Ceres6 commented 1 year ago
  • [ ] Read permissions from a config file (policy.json)

Hi I'll be working on this.

RafaelGSS commented 1 year ago

@KennyLindahl

So my question is if there will be HTTP request block + whitelisting of URLs?

Currently, the Permission Model does not enforce NET permissions (HTTP, TCP, UDP and so on). This issue is about the roadmap of this feature, net certainly is on the road (we haven't designed how it gonna work, but we should be looking into it -- note: it doesn't guarantee we'll add it, but we'll, for sure, try)

However this was never logged in in the Node program, it was only captured and delivered as text to the attackers computer (see code below).

That's expected, but we can improve it for sure. We might want to add diagnostic_channel whenever permission is denied. I'll add it to the roadmap so we can discuss it further.

targos commented 1 year ago

What about this idea: add a CLI flag to make the process abort (so it cannot be caught by the caller) whenever permission is denied?

tniessen commented 1 year ago

If doing so, what exactly is the goal? If this is about security auditability, what should happen if the application uses process.permission.has() to first check whether a subsequent call would result in a permission error?

targos commented 1 year ago

It's about auditability. I didn't think of process.permission.has(). Maybe diagnostics channels are enough for this use case (assuming a malicious actor cannot alter its behavior to hide their activity).

Ceres6 commented 1 year ago

[ ] Log whenever permission is denied for some resource

Hi!

@RafaelGSS and I are investigating on how to support this.

legrego commented 10 months ago

I'm very excited for this feature, and appreciative of the team's continued efforts. I see in the docs for this feature that "Native modules are restricted by default when using the Permission Model". My app requires the use of native modules at runtime, which makes my PoC infeasible at the moment.

The "by default" implies that there's a way to change this behavior, but I can't find a way to allow native modules to load when --experimental-permission is configured. Is this currently possible? If not, are there plans to allow native modules to be loaded in the future, regardless of whether or not they can respect the configured policy?

RafaelGSS commented 10 months ago

We can add an --allow-addons flag @legrego. I'll be working on that, but once you enable it addons will bypass any permission imposed by the permission model (the same applies to --allow-worker and --allow-child-process).

legrego commented 10 months ago

We can add an --allow-addons flag @legrego. I'll be working on that, but once you enable it addons will bypass any permission imposed by the permission model (the same applies to --allow-worker and --allow-child-process).

--allow-addons would be perfect, and having addons not respect the permission model is a reasonable restriction in my mind. Thanks, @RafaelGSS. Forgive me, I'm not sure what the backport policy is, but it would be ideal for me if this were to be included in v20 (LTS).

RafaelGSS commented 10 months ago

Yes, ideally it will be included on Node.js 20. It might take time since our backporting policy to LTS release lines requires a commit to be on main for at least, 2 weeks.

RafaelGSS commented 9 months ago

See https://github.com/nodejs/node/pull/51183 @legrego

legrego commented 9 months ago

Thank you, @RafaelGSS!

RafaelGSS commented 5 months ago

With nodejs/node#52730 I think it's time to discuss it Permission Model usage with package managers.

RafaelGSS commented 5 months ago

Closing this as completed. A second iteration of Permission Model will be created shortly.