security-wg icon indicating copy to clipboard operation
security-wg copied to clipboard

Permission Model - Roadmap

Open RafaelGSS opened this issue 1 year ago • 22 comments

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.

  • [ ] Permission Model adoption on package managers (a new issue will be created to discuss it)
  • [x] Support path.resolve in C++ side to avoid possiblyTransformPath https://github.com/nodejs/node/pull/50758
    • [x] Support relative paths https://github.com/nodejs/node/pull/50758
  • [x] ~Read permissions from a config file (policy.json)~ - https://github.com/nodejs/security-wg/issues/1074
  • [ ] Throw access denied asynchronously for async APIs.
    fs.readFile('restricted-file.md', (error) => {
      // error is ERR_ACCESS_DENIED
    })
    
  • [x] Allow Addons https://github.com/nodejs/node/pull/51183
    
    

New permissions

  • [ ] net

cc: @nodejs/tsc for awareness

RafaelGSS avatar Mar 02 '23 02:03 RafaelGSS

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?

BrunoBernardino avatar Mar 02 '23 06:03 BrunoBernardino

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.

RafaelGSS avatar Mar 02 '23 14:03 RafaelGSS

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.

bmeck avatar Mar 02 '23 15:03 bmeck

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.

RafaelGSS avatar Mar 03 '23 12:03 RafaelGSS

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 ;)

aduh95 avatar Mar 03 '23 12:03 aduh95

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: @.***>

bmeck avatar Mar 03 '23 12:03 bmeck

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..

wesbos avatar Apr 18 '23 20:04 wesbos

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

RafaelGSS avatar Apr 18 '23 20:04 RafaelGSS

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.

bmeck avatar Apr 19 '23 02:04 bmeck

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:

  • https://github.com/KennyLindahl/malicious-npm-package

How my program was run

  • Node version: v20.5.0
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:

  • See any possible permission breaches in my application log
  • Possibility to listen to events of permission breaches (so i can send email to my team etc)

Thanks

KennyLindahl avatar Aug 10 '23 13:08 KennyLindahl

  • [ ] Read permissions from a config file (policy.json)

Hi I'll be working on this.

Ceres6 avatar Aug 14 '23 21:08 Ceres6

@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.

RafaelGSS avatar Aug 14 '23 22:08 RafaelGSS

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?

targos avatar Aug 15 '23 06:08 targos

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?

tniessen avatar Aug 15 '23 08:08 tniessen

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).

targos avatar Aug 15 '23 08:08 targos

[ ] Log whenever permission is denied for some resource

Hi!

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

Ceres6 avatar Aug 28 '23 19:08 Ceres6

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?

legrego avatar Dec 11 '23 13:12 legrego

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).

RafaelGSS avatar Dec 11 '23 18:12 RafaelGSS

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).

legrego avatar Dec 11 '23 18:12 legrego

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 avatar Dec 11 '23 19:12 RafaelGSS

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

RafaelGSS avatar Dec 16 '23 16:12 RafaelGSS

Thank you, @RafaelGSS!

legrego avatar Dec 17 '23 18:12 legrego

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

RafaelGSS avatar Apr 29 '24 19:04 RafaelGSS

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

RafaelGSS avatar May 01 '24 15:05 RafaelGSS