node
node copied to clipboard
nodejs
Remove --experimental-policy
Our docs reports:
The approval of the module integrity in the policies threat model implies they are allowed to muck with and even circumvent security features once loaded, so environmental/runtime hardening is expected.
Therefore, once a module is loaded, they have the keys to the castle.
After reviewing a few security reports about this feature, I don't think it provides much additional protection against our threat model: https://github.com/nodejs/node/blob/main/SECURITY.md#the-nodejs-threat-model.
Note that this was developed before we had a threat model.
I think the main issue with this feature is that we don't have an active collaborator who understands and maintains it.
I believe there is some interest from microsoft folks for windows https://github.com/nodejs/node/pull/51786
The current volume of issues on HackerOne that cannot understand that policies do not protect against RCE suggest me it's a misunderstood feature. They protect against an "offline" threat, which is very unusual compared to our current threat model.
https://github.com/nodejs/node/pull/51786 makes it significant more robust, so at least the integrity hashes could not be tampered with, making the feature solid.
I'm still leaning toward removal, unless @rdw-msft would like to step in a more active role to help with this.
In my opinion, it is a misunderstood feature. When I saw the policy, I thought it was a way to stop 'trusted' code from accessing modules, which, in turn, ignores the whole explanation of what 'trusted' code really is.
As a person who reviewed a bunch of those reports, unfortunately, I agree with its removal. We have tried a few times to make its boundaries clear, and to be honest, it's still quite confusing (See: https://github.com/nodejs-private/node-private/issues/448). I'm also attempting to improve it at https://github.com/nodejs/security-wg/issues/1255#issuecomment-2061591486. But, its scope is too large that I'm afraid we will never be able to handle all those edge cases.
cc/ @bmeck for awarness
This feature never reached a critical minimal adoption, this seems a fine experiment to remove and pursue other security model possibilities; however if import maps do add SRI it likely would be in a similar situation per Add subresource integrity support for ES modules, through importmaps by yoavweiss · Pull Request #10269 · whatwg/html · GitHub https://github.com/whatwg/html/pull/10269/ ; which would also carry a security framing. Not sure what to do about that in the future, but I'm not able to invest time into this and it is just a heads up.
On Thu, Apr 18, 2024 at 8:41 AM Rafael Gonzaga @.***> wrote:
As a person who reviewed a bunch of those reports, unfortunately, I agree with its removal. We have tried a few times to make its boundaries clear, and to be honest, it's still quite confusing (See: nodejs-private/node-private#448 https://github.com/nodejs-private/node-private/issues/448). I'm also attempting to improve it at nodejs/security-wg#1255 (comment) https://github.com/nodejs/security-wg/issues/1255#issuecomment-2061591486. But, its scope is too large that I'm afraid we will never be able to handle all those edge cases.
cc/ @bmeck https://github.com/bmeck for awarness
— Reply to this email directly, view it on GitHub https://github.com/nodejs/node/issues/52575#issuecomment-2063896485, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZJI6AYQK6JFIT6KA6B63Y57EO7AVCNFSM6AAAAABGMYJVU2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRTHA4TMNBYGU . You are receiving this because you were mentioned.Message ID: @.***>
My reports pushed you to make this thinking and this decision, but my reports are invalid as “informative”. That’s how you treat the researcher spent time on your experimental features.
My reports pushed you to make this thinking and this decision, but my reports are invalid as “informative”. That’s how you treat the researcher spent time on your experimental features.
I don't think any one person is responsible, and it's a team effort for any decision to be made. Instead of being upset over it, we should be happy that NodeJS is changing for the better.
My reports pushed you to make this thinking and this decision, but my reports are invalid as “informative”. That’s how you treat the researcher spent time on your experimental features.
I don't think any one person is responsible, and it's a team effort for any decision to be made. Instead of being upset over it, we should be happy that NodeJS is changing for the better.
Thank you for your reply, but the related vulnerabilities are reported through Hackerone. I spent days and nights to discover security issues to earn reputation and bounty,. They decided to remove unsecure feature after reading my reports, but leave me nothing but a "informative" which equeals 0 reputation. Do you believe it's fair for me?
There are no maintainers that are willing to step in for this. More importantly, it’s usefulness is in plain conflict with our threat model. As a result, the best course of action is to remove it.
There are no maintainers that are willing to step in for this. More importantly, it’s usefulness is in plain conflict with our threat model. As a result, the best course of action is to remove it.
Yeah, it's a good idea to remove this module, I never objected to that. The only I want now is to disclose my report please, as it's "informative", no harm. Even more, disclosing it may prevent other researchers to stop wasting life in this feature.
So many similar reports are valid, but mine is "informative" even my reports and our discussion pushed you to fully remove this feature.
@mcollina BTW, don't just remove --experimental-policy, you should remove --experimental-permission too. Because it’s usefulness is in plain conflict with your threat model too! As you fully trust the code with the Permissions feature enabled, why you add path accessing limitation to it?
Updated:
It's my mistake, the exist of --experimental-permission seems not to against threat model, because it's tend to work with input path, not loaded code.
Whether it's 'fair' or not shouldn't be the end goal. As a security researcher who also worked on this feature, I think the whole 'fair' / 'not fair' mentally isn't the best one. Instead of getting upset at the team for something you think is 'unfair,' you should be grateful that you helped make NodeJS a safer community.
I really envy your good mindset.
On Fri, Apr 19, 2024 at 21:55 Aviv Keller @.***> wrote:
Whether it's 'fair' or not shouldn't be the end goal. As a security researcher who also worked on this feature, I think the whole 'fair' / 'not fair' mentally isn't the best one. Instead of getting upset at the team for something you think is 'unfair,' you should be grateful that you helped make NodeJS a safer community.
— Reply to this email directly, view it on GitHub https://github.com/nodejs/node/issues/52575#issuecomment-2066641360, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE54AMMMGGMYYV26H4QK4QDY6EO3LAVCNFSM6AAAAABGMYJVU2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRWGY2DCMZWGA . You are receiving this because you commented.Message ID: @.***>
Let me provide some context @4xpl0r3r. First, I'm sorry you feel this way and I agree that from your perspective it seems we are doing it in bad faith.
The policy mechanism was created before our threat model and it was designed and implemented by Bradley (bmeck). We have been discussing this feature a few times in the past (in private, due to security concerns) and during every security release. I can assure you that the reason for Matteo creating this issue wasn't mainly because of your reports through HackerOne. In fact, it's a general agreement among the security team and TSC that the codebase had become difficult to maintain over time. To support this, you can refer to my pull request that removed policy-related code, which was extensive.
In the past, our team considered other reports acceptable because we had incorrect assumptions about the boundaries and security expectations. However, just because we may have accepted a similar report in the past, it does not mean that your report will automatically be accepted. An error should not open precedence to accepting your report as well.
I hope to have clarified all misunderstandings and I'd like to emphasise that we appreciate the time invested in looking into the potential threats in our codebase.
Let me provide some context @4xpl0r3r. First, I'm sorry you feel this way and I agree that from your perspective it seems we are doing it in bad faith.
The policy mechanism was created before our threat model and it was designed and implemented by Bradley (bmeck). We have been discussing this feature a few times in the past (in private, due to security concerns) and during every security release. I can assure you that the reason for Matteo creating this issue wasn't mainly because of your reports through HackerOne. In fact, it's a general agreement among the security team and TSC that the codebase had become difficult to maintain over time. To support this, you can refer to my pull request that removed policy-related code, which was extensive.
In the past, our team considered other reports acceptable because we had incorrect assumptions about the boundaries and security expectations. However, just because we may have accepted a similar report in the past, it does not mean that your report will automatically be accepted. An error should not open precedence to accepting your report as well.
I hope to have clarified all misunderstandings and I'd like to emphasise that we appreciate the time invested in looking into the potential threats in our codebase.
Hello @RafaelGSS , this really make me feel much better, thank you for your patient explaining. Let's keep working together in the future, I promise I will send you more security reports.
@4xpl0r3r I think you can view it this way - the reports you sent revealed the maintenance issue we have with this feature. Without your reports we may have an undermaintained security feature lingering around that claim to enhance security but fail to deliver due to maintenance issues. I think that is valuable. Having a friendly researcher who reveal this is definitely a lot better than having it revealed by an attacker via real exploits on users who count on this undermaintained feature. Also, if the boundary problem isn't even clear to security researchers, what are the chances that users can understand it and use it correctly? A misunderstood security feature can be a vulnerability in itself.
I think the permission model on the other hand has a smaller maintenance overhead and an easier-to-understand model, and if we manage to keep up with the bug reports, that proves its a valid, sustainable, educatable security feature for Node.js. Otherwise, we can again be pragmatic and remove it instead of having a false advertisement for our uesrs.
@4xpl0r3r I think you can view it this way - the reports you sent revealed the maintenance issue we have with this feature. Without your reports we may have an undermaintained security feature lingering around that claim to enhance security but fail to deliver due to maintenance issues. I think that is valuable. Having a friendly researcher who reveal this is definitely a lot better than having it revealed by an attacker via real exploits on users who count on this undermaintained feature. Also, if the boundary problem isn't even clear to security researchers, what are the chances that users can understand it and use it correctly? A misunderstood security feature can be a vulnerability in itself.
I think the permission model on the other hand has a smaller maintenance overhead and an easier-to-understand model, and if we manage to keep up with the bug reports, that proves its a valid, sustainable, educatable security feature for Node.js. Otherwise, we can again be pragmatic and remove it instead of having a false advertisement for our uesrs.
Thank for admitting the value of my work! But with just 2 invalid reports, it’s nothing to me. If you really believe this value, maybe you could help me persuade Matteo to resolve my reports XD. Your response raised my hope of my reports again XD. Hope it’s not wrong.
While it's great that we are all in a better mindset, I don't believe a public Github issue is the best place to discuss specific security concerns.
Keep in mind, everything sent here is visible to the public.
While it's great that we are all in a better mindset, I don't believe a public Github issue is the best place to discuss specific security concerns.
Keep in mind, everything sent here is visible to the public.
Thank you for reminding, I never leak confidential technical details. Don’t worry. It’s my professional ethics.
Thank for admitting the value of my work! But with just 2 invalid reports, it’s nothing to me. If you really believe this value, maybe you could help me persuade Matteo to resolve my reports XD. Your response raised my hope of my reports again XD. Hope it’s not wrong.
To clarify I wrote:
Without your reports we may have an undermaintained security feature lingering around that claim to enhance security but fail to deliver due to maintenance issues. I think that is valuable.
Which means the value is social - it comes from your reports making us realize that the policy feature is undermaintained - only a few people (or just one person) really knows what the security model really is, and others are confused, but those who really knows how to maintain it are not involved in the project enough to keep it maintained. As a result we had the “one report was accepted by mistake, while others are not”.
I am not a security triager myself, and please be aware that a lot of the security triaging in the project is done by volunteers. As such I don’t think I am in a position to pile work on other volunteers. Whether the reports are valid come down to whether they match the Node.js threat model, which is technical, not social. I'll defer to those who know about security better than I do to make the technical judgement.
You may get compensation from submitting a valid report, but a lot of the work from maintainers spent in triaging reports (no matter they are valid or not) is uncompensated. You might view it from this angle: triaging work is also valuable and therefore it deserves the patience and the discretion from reporters. When you open a report, take extra care about how likely it is going to be accepted per the Node.js threat model, if you think security triaging is also valuable work.
Thanks for your work with the community! It's so amazing that people volunteer to do this amazing work!
FWIW, I do think that policies can be a valuable tool to have additional assurance that an application only loads trusted code (i.e., through integrity checks). If that trusted code then performs malicious actions or enables RCE or somehow bypasses the policy mechanism, then the user shouldn't have trusted it in the first place.
Dependency redirection is another possibly useful (non-security) feature.
FWIW, I do think that policies can be a valuable tool to have additional assurance that an application only loads trusted code (i.e., through integrity checks).
We have discussed the policy integrity checks with Microsoft in the latest security team meeting and we came to a conclusion that it should be a different new experimental feature. So, we have included it in the possible roadmap for the Security team for this year and I might be working on that soon (with Microsoft). https://github.com/nodejs/security-wg/issues/1255#issuecomment-2077338001