Skip to content
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

Permission Model - Roadmap #898

Closed
5 of 7 tasks
RafaelGSS opened this issue Mar 2, 2023 · 24 comments
Closed
5 of 7 tasks

Permission Model - Roadmap #898

RafaelGSS opened this issue Mar 2, 2023 · 24 comments

Comments

@RafaelGSS
Copy link
Member

RafaelGSS commented Mar 2, 2023

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

  • net

cc: @nodejs/tsc for awareness

@BrunoBernardino
Copy link

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
Copy link
Member Author

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
Copy link
Member

bmeck commented Mar 2, 2023

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
Copy link
Member Author

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
Copy link

aduh95 commented Mar 3, 2023

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
Copy link
Member

bmeck commented Mar 3, 2023 via email

@wesbos
Copy link

wesbos commented Apr 18, 2023

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
Copy link
Member Author

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
Copy link
Member

bmeck commented Apr 19, 2023

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
Copy link

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

@RafaelGSS
Copy link
Member Author

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
Copy link

legrego commented Dec 11, 2023

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
Copy link
Member Author

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
Copy link
Member Author

RafaelGSS commented Dec 16, 2023

See nodejs/node#51183 @legrego

@legrego
Copy link

legrego commented Dec 17, 2023

Thank you, @RafaelGSS!

nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Dec 30, 2023
Co-Authored-By: Carlos Espa <cespatorres@gmail.com>
PR-URL: #50758
Refs: nodejs/security-wg#898
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
RafaelGSS added a commit to nodejs/node that referenced this issue Jan 2, 2024
Co-Authored-By: Carlos Espa <cespatorres@gmail.com>
PR-URL: #50758
Refs: nodejs/security-wg#898
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
richardlau pushed a commit to nodejs/node that referenced this issue Mar 25, 2024
Co-Authored-By: Carlos Espa <cespatorres@gmail.com>
PR-URL: #50758
Refs: nodejs/security-wg#898
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
richardlau pushed a commit to nodejs/node that referenced this issue Mar 25, 2024
Co-Authored-By: Carlos Espa <cespatorres@gmail.com>
PR-URL: #50758
Refs: nodejs/security-wg#898
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
@RafaelGSS
Copy link
Member Author

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

nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Apr 30, 2024
PR-URL: #52730
Refs: nodejs/security-wg#898
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit to nodejs/node that referenced this issue Apr 30, 2024
PR-URL: #52730
Refs: nodejs/security-wg#898
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@RafaelGSS
Copy link
Member Author

RafaelGSS commented May 1, 2024

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

Ch3nYuY pushed a commit to Ch3nYuY/node that referenced this issue May 8, 2024
PR-URL: nodejs#52730
Refs: nodejs/security-wg#898
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants