nodejs
src: add `process.cveRevert`
Refs: https://github.com/nodejs/node/issues/52017
Add API to enable CVE reverts for use in environments where the command line option cannot be used.
Seems fine to me. Obviously needs tests and docs. Please ping again when you're ready for a final review :-)
I have a rather obvious question, what's stopping a random imported module or its dependencies from reverting these security patches without ones knowledge?
I think the opt-in only flag was the right decision at the time.
@panva the related question would be why would not some random imported module or its dependencies from do X? The Node.js threat model (https://github.com/nodejs/node/blob/main/SECURITY.md#the-nodejs-threat-model) is that we trust code that the application asks Node.js to run. I see what you are getting out but if the code can revert a CVE it can also do a lot of other things (X in the question above) which are just as big a risk.
@marco-ippolito the current use case is outline in https://github.com/nodejs/node/issues/52017. For the last security release some people are not able to use the command line to revert.
@RafaelGSS it is understood that it only takes affect for things that are created after the call is made. Are you suggesting that it might not be possible to make the call in the application early enough? I could see that in some cases, but at least for the current use case if the app can be modified it should be done before any calls are made to the crypto APIs and it will work ok.
Unless we enable it through a flag or even through the permission model, I believe this would expose vulnerabilities considering the usage of malicious packages.
Enabling through a flag would make it not useful as the whole point is because people cannot use the --security-revert command line flag. I could see it being disabled by the permission model by default.
Unless we enable it through a flag or even through the permission model, I believe this would expose vulnerabilities considering the usage of malicious packages.
Enabling through a flag would make it not useful as the whole point is because people cannot use the --security-revert command line flag. I could see it being disabled by the permission model by default.
Do you have some scenarios where users won't be able to startup the application with --security-revert?
Unless we enable it through a flag or even through the permission model, I believe this would expose vulnerabilities considering the usage of malicious packages.
Enabling through a flag would make it not useful as the whole point is because people cannot use the --security-revert command line flag. I could see it being disabled by the permission model by default.
Do you have some scenarios where users won't be able to startup the application with
--security-revert?
I think it was mentioned that it was need for aws lambda
Right, sorry. I didn't read the reference issue. IMO this will be a security problem. Any code will be able to revert security patches. I don't think we should address that request. If vendors want to, we could add a compile-time flag to enable this API.
@RafaelGSS - Amazon is the Vendor and I do not believe they will provide any specific Nodejs runtime to address Lambda that needs to decrypt Legacy data which uses the vulnerable padding mechanism.
IMO - providing mechanism to address security risk is super good, but ideally should also allow user to address/relax it. Understood --security-revert is there already in place, but restricting it to only command line option presents challenge to users who have no access/control of the command line option easily.
Blocking all possible malicious package is noble, but should really be responsibility of users
IMO - providing mechanism to address security risk is super good, but ideally should also allow user to address/relax it. Understood --security-revert is there already in place, but restricting it to only command line option presents challenge to users who have no access/control of the command line option easily.
It's important to note that not only users will be able to perform the action in question. Any package and its dependencies that you install will also be able to perform the action, even if you don't intend for it to happen. This poses a risk for regular users. I believe that this action should only be taken when explicitly requested before the app starts up. That's why we have the --security-revert flag ~(which can also be passed through the NODE_OPTIONS environment variable)~
That's why we have the
--security-revertflag (which can also be passed through theNODE_OPTIONSenvironment variable).
Except it cannot (be used in NODE_OPTIONS). That was the original reason https://github.com/nodejs/node/issues/52017 was opened.
That's why we have the
--security-revertflag (which can also be passed through theNODE_OPTIONSenvironment variable).Except it cannot (be used in
NODE_OPTIONS). That was the original reason #52017 was opened.
Hmm, so maybe that's a viable option instead of a runtime change.
Hmm, so maybe that's a viable option instead of a runtime change.
@RafaelGSS I had thought of that to start, but looking at the history there was concern with that (from @jasnell and others) so I talked to James and what is in this PR is what we came up with as an alternative.
Allowing --security-revert in NODE_OPTIONS is a one line change if people believe that is preferable to what's in this PR but we'd need to pull in those with the original concerns to discuss again.
Allowing --security-revert in NODE_OPTIONS is a one line change if people believe that is preferable to what's in this PR but we'd need to pull in those with the original concerns to discuss again.
I think it's worth discussing this again since we didn't have a well-defined threat model back then.
Unless we enable it through a flag or even through the permission model, I believe this would expose vulnerabilities considering the usage of malicious packages.
Enabling through a flag would make it not useful as the whole point is because people cannot use the --security-revert command line flag. I could see it being disabled by the permission model by default.
Do you have some scenarios where users won't be able to startup the application with
--security-revert?I think it was mentioned that it was need for aws lambda
We are now facing the same issue in Google Cloud Functions as well.
Would appreciate it if you add it as a NODE_OPTIONS configuration.
I think supporting it inside NODE_OPTIONS would be a safer alternative, but maybe I'm missing something.
I'd be ok with that but I think we'd want to pull @jasnell back into the conversaion on that.
My challenge with allowing it in NODE_OPTIONS is that it's too easy to set and forget and impact every instance of node that is running. Imagine, for instance someone with multiple electron apps running that pick up the flag from NODE_OPTIONS without the user actually knowing, which could put users at risk. Also consider that the reverts are specific to individual node.js major lines and a user may be running multiple Node.js major versions across multiple applications. Given it's nature, the flag needs to be explicitly opt in per-process instance and setting it as an env var circumvents that and puts users at risk.
@jasnell an idea had last night was what if we limit the revert to a specific Node.js version. For example considering https://github.com/nodejs/node/pull/52365 which adds a new environment variable we also add a +version. I think that would address a few of your concerns around "too easy to set and forget and impact every instance of node that is running."
It would mean that the environment variable would only apply to a specific version of Node.js. If you have multiple versions running it would only affect the specific version you targeted. It would also mean that every time you upgrade your version you would need to update the environment variable and hopefully check if you still need it or not.
It might look something like NODE_SECURITY_REVERT=CVE-1234-456+v18.5.1+sticky
In that case it would only apply if the Node.js runtime being started is v18.5.1.
@tniessen not sure what you think about that in terms of building on what you had in https://github.com/nodejs/node/pull/52365
Michael, tying the env. variable to specific version would be difficult for companies like ours that leverage AWS's automatic runtime update. We specify nodejs18.x - we can not specify the exact revision used -> so never knew what is the right value to set.
On Tue, Apr 9, 2024 at 1:59 PM Michael Dawson @.***> wrote:
@jasnell https://github.com/jasnell an idea had last night was what if we limit the revert to a specific Node.js version. For example considering #52365 https://github.com/nodejs/node/pull/52365 which adds a new environment variable we also add a +version. I think that would address a few of your concerns around "too easy to set and forget and impact every instance of node that is running."
It would mean that the environment variable would only apply to a specific version of Node.js. If you have multiple versions running it would only affect the specific version you targeted. It would also mean that every time you upgrade your version you would need to update the environment variable and hopefully check if you still need it or not.
It might look something like NODE_SECURITY_REVERT=CVE-1234-456+v18.5.1+sticky
In that case it would only apply if the Node.js runtime being started is v18.5.1.
@tniessen https://github.com/tniessen not sure what you think about that in terms of building on what you had in #52365 https://github.com/nodejs/node/pull/52365
— Reply to this email directly, view it on GitHub https://github.com/nodejs/node/pull/52090#issuecomment-2045794379, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANJLIH2SU45MWOUK3RNGHATY4QT6ZAVCNFSM6AAAAABEXGSKMGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBVG44TIMZXHE . You are receiving this because you commented.Message ID: @.***>
I'm generally not a big fan of the microsyntax approach but incorporating both the major and the +stick into the value does help alleviate at least some of the concern. Unfortunately it doesn't address all of it. Say, for instance, that I am running Node.js locally and have two electron apps also running locally that are running the same major version of Node.js. And I set NODE_SECURITY_REVERT=CVS-1234-456+v20 in my environment intending to just revert the fix for my Node.js application and not realizing that it also reverted it in the two electron apps as well. Now imagine that one of those apps is being used to handle sensitive work or personal data and the unintended revert just put me at risk.
@tniessen an FYI just so you see @jasnell last comment in terms of work on - https://github.com/nodejs/node/pull/52365
@mhdawson I don't think there's a good solution for this problem. I'm not pushing for #52365 by any means — please feel free to leave it open/request changes/close it.
One more suggestion to see if we can find an approach most people can live with.
How about we require both
- a programatic request to revert a CVE as is supported in this PR
- AND, a command line option which can be used in NODE_OPTIONS with the sticky option as outlined by @tniessen which instead of specifying a list of CVEs, enables the the programatic cveRevert.
If people are ok with the programatic option OR if people are ok with a new command line option that can be used in NODE_OPTIONS, then I'm thinking that needing to do BOTH should be acceptable to both groups as it is tighter than either on its own.
@jasnell, @RafaelGSS, @tniessen, @marco-ippolito what do you think?
One more suggestion to see if we can find an approach most people can live with.
How about we require both
- a programatic request to revert a CVE as is supported in this PR
- AND, a command line option which can be used in NODE_OPTIONS with the sticky option as outlined by @tniessen which instead of specifying a list of CVEs, enables the the programatic cveRevert.
If people are ok with the programatic option OR if people are ok with a new command line option that can be used in NODE_OPTIONS, then I'm thinking that needing to do BOTH should be acceptable to both groups as it is tighter than either on its own.
@jasnell, @RafaelGSS, @tniessen, @marco-ippolito what do you think?
I'm ok with having both, I think NODE_OPTIONS is required for users on serverless, and programmatic api for the remaining part
If we look at a different perspective (as the one James is concerned) I can agree with him and it does seem a people outside of our scope. Vendors should allow command line options and that's all.
If NODE_OPTIONS will open a new threat vector for us, I think we should be careful to include it.
AND, a command line option which can be used in NODE_OPTIONS with the sticky option as outlined by @tniessen which instead of specifying a list of CVEs, enables the the programatic cveRevert.
I think it won't solve the problem but open a new attack vector, as any package will be able to revert security patches (even the most dangerous ones). I feel this is more risky than just allowing --security-revert on NODE_OPTIONS.
If we look at a different perspective (as the one James is concerned) I can agree with him and it does seem a people outside of our scope. Vendors should allow command line options and that's all.
If NODE_OPTIONS will open a new threat vector for us, I think we should be careful to include it.
AND, a command line option which can be used in NODE_OPTIONS with the sticky option as outlined by @tniessen which instead of specifying a list of CVEs, enables the the programatic cveRevert.
I think it won't solve the problem but open a new attack vector, as any package will be able to revert security patches (even the most dangerous ones). I feel this is more risky than just allowing
--security-revertonNODE_OPTIONS.
If a malicious package can control env variables it means it already has complete control