-
Notifications
You must be signed in to change notification settings - Fork 122
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
Comments
Sorry if this is intrusive and not productive, I didn't see a rationale for choosing a new file ( |
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. |
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. |
One could argue that it's another reason for using policies, so both can graduate to stable faster ;) |
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
<#898 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABZJI5W6WL3VK4MGJDJBO3W2HOMBANCNFSM6AAAAAAVM2H4QQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
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!
|
The first 2 items are already on the roadmap. The third one:
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 |
I'd just note, per the config problem, it most likely should not be authoritative from a given |
BackgroundThe program i'm running is using a fake malicious NPM package (for security research). See example attack flow:
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 CasesHTTP requestsWhen 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) QuestionSo my question is if there will be HTTP request block + whitelisting of URLs? SpawnMy fake malicious NPM package tried to start a local tunnel via spawn, and this was blocked which is good. 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:
QuestionIs this expected? In a real scenario i would never know what the attacker has tried to do etc, which is not ideal.
Thanks |
Thank you, @RafaelGSS! |
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>
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>
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>
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>
With nodejs/node#52730 I think it's time to discuss it Permission Model usage with package managers. |
PR-URL: #52730 Refs: nodejs/security-wg#898 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: #52730 Refs: nodejs/security-wg#898 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Closing this as completed. A second iteration of Permission Model will be created shortly. |
PR-URL: nodejs#52730 Refs: nodejs/security-wg#898 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: #52730 Refs: nodejs/security-wg#898 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs#52730 Refs: nodejs/security-wg#898 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs#52730 Refs: nodejs/security-wg#898 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
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.
possiblyTransformPath
lib,src,permission: port path.resolve to C++ node#50758Read permissions from a config file (- Load permission settings from config files #1074policy.json
)New permissions
cc: @nodejs/tsc for awareness
The text was updated successfully, but these errors were encountered: