src: add process.cveRevert
#52090
src: add process.cveRevert
#52090
Conversation
Refs: nodejs#52017 Add API to enable CVE reverts for use in environments where the command line option cannot be used. Signed-off-by: Michael Dawson <midawson@redhat.com>
|
Review requested:
|
nodejs-github-bot
added
c++
| } \ | ||
| #define V(code, _, __) \ | ||
| do { \ | ||
| if (IsReverted(SECURITY_REVERT_##code)) { \ |
There was a problem hiding this comment.
This change is from running make format-cpp
| Local<String> revert_error_return = | ||
| String::NewFromUtf8(env->isolate(), | ||
| revert_error.c_str(), | ||
| NewStringType::kNormal, |
There was a problem hiding this comment.
minor nit: might make sense to intern the string, but definitely not critical. Also, these should likely always be one-byte so maybe NewFromOneByte would work to avoid the utf8 transcoding?
|
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 |
|
@nodejs/security-triage FYI to get more input on approach. |
|
Can you provide an example of how it should be used? |
There was a problem hiding this comment.
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.
I'm also curious about how this would function for reversions that are desired prior to or during Node.js startup. Let's consider a scenario where we introduce a --security-revert flag designed to set an HTTP request option to true by default. In this case, ongoing requests will persist with the default value of false, and if there's an instance of HTTPClient, it will maintain false as the default parameter post-initialization.
|
@marco-ippolito the current use case is outline in #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. |
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 |
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 |
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 |
Except it cannot (be used in |
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. |
I think it's worth discussing this again since we didn't have a well-defined threat model back then. |
We are now facing the same issue in Google Cloud Functions as well. |
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 #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 #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 <#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
<#52365>
—
Reply to this email directly, view it on GitHub
<#52090 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANJLIH2SU45MWOUK3RNGHATY4QT6ZAVCNFSM6AAAAABEXGSKMGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBVG44TIMZXHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
@singyantam would it work if it was tied only to the major version ? |
|
I'm generally not a big fan of the microsyntax approach but incorporating both the major and the |
|
One more suggestion to see if we can find an approach most people can live with. How about we require both
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.
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 |
If a malicious package can control env variables it means it already has complete control |
Imagine you need to revert a specific CVE, so you enable it through ENV and then disable it programmatically. However, once the runtime API is enabled, any other CVE can be disabled. Which is different than explicitly passing --security-revert in NODE_OPTIONS IMO |
Ok so the follow on suggestion would be that you need to specify the CVE to be reverted in BOTH the environment variable and the cveRevert call in the code. ie the command line variable sets which CVEs are allowed to be reverted programatically and then they are only reverted if they are included in a cveRevert call. |
|
One of the key issues with a programmatic API approach is that the security fix that may need to be reverted might actually apply before the user code runs... at least in theory. For instance, let's imagine that we need to make a semver major security fix that impacts the Node.js bootstrap and that somehow breaks users code. The user wants to temporarily revert the change when they run, but the change applies before any of the users code runs. In theory they might be able to use a preload module but not all vendor deployments give users that option since preloads are also configured via command line arguments. I don't know if there's a good solution to this general problem. The revert option was always one that has been an imperfect compromise from the beginning. Ideally we wouldn't even offer such a mechanism as it shouldn't be possible for users to roll back security fixes. I'm not going to block changes here but I'm not convinced that any of the changes proposed so far are a good idea and would generally prefer to keep things as they are. |
|
We discussed in the Security WG meeting today and the consensus was that we proceed with the proposal in #52090 (comment) versus nothing at all. It was agreed its imperfect but still better than not doing anything. Since @jasnell mentioned he'd not block although prefering the nothing option I think we can move forward with the proposal in #52090 (comment). I think that is pulling in the changes from this PR along with the changes in #52365 that @tniessen put together with some extension to validate both. I'll wait until next week to see if there are any other objections, otherwise, I'll talk to @tniessen how we pull the PRs together. |
|
A limitation of requiring programmatic |
|
@JoostK understood there are limitations, it also won't be able to handle cases where a change needs to be reverted before user code can run. But it would have helped in the most recent case so seems to be better than nothing new. |
Refs: #52017
Add API to enable CVE reverts for use in environments where the command line option cannot be used.