-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
policy: add WDAC integration on Windows #51786
Conversation
Review requested:
|
@nodejs/platform-windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your first commit message and some tests seem to be faulty
49495e2
to
5d17818
Compare
Hey @mertcanaltin thank you for taking a look at the PR. Sorry for the long delay, I was sick last week. I'll start addressing your comments today. I rewrote the commit message and it looks like that retriggered a lot of workflows that need approval to run. In doing so, I think I lost what workflows failed and their associated error messages. Do you know if there's a way I can view history of workflow output? |
4b1e674
to
72a1616
Compare
Hey @mhdawson @RafaelGSS and @bmeck , while I work on getting all the workflows green, I would like to discuss whether this approach is reasonable for ensuring the integrity of the policy file. Alternative to passing a launch arg that stores the integrity value of the policy, we use can a PKCS7 detached signature, signed by a key the OS trusts for CI purposes. Please let me know if you have any questions. I'd be happy to answer them here, in a security WG meeting, or whatever forum you prefer. Thanks |
@rdw-msft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know enough about WDAC to have an opinion on the benefits of this PR, but the implementation seems like a huge TOCTOU vulnerability to me. The C++ layer appears to verify the signature long before the JavaScript layer reads the policy file.
@tniessen that's a good callout. Other callers of this function on Windows mitigate this by opening the file without a What I should do here is hold on to the open file handle until after the policy file is parsed. I can make this change soon |
I seem to have botched something when syncing my fork and rebasing. I'm working on fixing this history. |
You can try to do your own code first by using or if you know the hash number of the clean version of your code |
ff7a00e
to
1379c44
Compare
…in CI enforcement mode
1379c44
to
5c40cc2
Compare
Sorry to be the bearer of bad news, but I'm not sure if the experimental policy feature will remain in NodeJS much longer, WDYT @nodejs/security-wg |
I hate to disappoint, but the experimental policy system is being removed in a future version of Node.js. I'm closing this PR, as it will not be added (because of the removal). I encourage you to make an NPM package if you feel passionate about this change. |
Last year, I spoke with @bmeck @mhdawson and @RafaelGSS about extending the chain of trust of node policy files into the OS. Work has been done on the OS side to better support language interpreters with code integrity, and now we're ready to integrate this work with NodeJS.
Changes
Added an alternate option to --policy-integrity so the integrity of the policy can be rooted in the OS.
The alternative option is --policy-signature which is a path to a PKCS7 detached signature of the policy file.
The policy signature is verified using the OS CI policy system. On Windows, this uses WDAC.
On *nix this is stubbed out.
Added a query to WDAC to see if a policy file and signature are mandatory for Node to run.
The –policy-signature option provides an additional layer of defense by limiting what an adversary with file system access can do with NodeJS. Protecting the policy manifest with a digital signature raises the bar for an attacker, as they now need to compromise code signing PKI.
The Windows code integrity subsystem (called WDAC) is commonly used in high-risk environments with fixed or limited workload devices. These devices are allowed to run a constrained set of applications that are digitally signed by keys allowed in the system's WDAC policy. This ensures that only known code is allowed to be run on these devices. WDAC limits the capabilities of an attacker who has access to a system by preventing them from doing things outside what the system typically does.
I have uploaded part of a presentation I gave to the security working group a while back that demonstrates a simplified version of what we’re trying to protect against here https://github.com/rdw-msft/slides/blob/main/NodePolicySignature.pptx.
The deck has two videos. One video demonstrates someone with command line access running a script which modifies a web app to add a web shell, modifies the SRI hash in the manifest file so it matches the modifications made, and then relaunches the app with a modified –policy-integrity value.
The second video demonstrates the same scenario, but with the policy file protected by a digital signature. Node refuses to run with the modified policy manifest because of the signature mismatch.
Windows Code Integrity (WDAC)
Official WDAC docs
WDAC is Window's code integrity enforcement system. Using WDAC, system administrators can define policy rules that dictate which executables can be run on a system where WDAC is enabled. One of the criteria they can define is a set of valid signing keys. EXEs and DLLs have signatures embedded into them (in headers and other "unused" blocks within the file) which are validated when loaded. Since interpreted languages don't use the standard EXE and DLL loading entry points, and the code which they interpret can be various file types, enforcement by the OS is difficult. There needs to be some cooperation between the OS and the language runtime.
Node policies seem like a natural point we can integrate with WDAC. In the current form, however, a Node policy manifest can be altered on disk. This can be mitigated with the --policy-integrity option. However, this can be bypassed if an attacker can modify the command parameters. We're proposing linking the chain of trust into something the OS trusts, rather (or in addition to) a command line parameter. To do this, the manifest is signed with a PKCS7 detached signature. This adds another layer of security, since now a trusted signing key would need to be compromised to use NodeJS on a locked-down system.
This can be done with signtool with
signtool sign /p7 .\ /p7co 1.2.840.113549.1.7.2 /p7ce Pkcs7DetachedSignedData /fd sha256 /f key.pfx node_policy.json
Or OpenSSL with
openssl smime -sign -binary -in policy.json -signer signer.pem -inkey signer.key -outform DER -out policy.json.p7s
This signature can then be passed in along with a Node policy.
node.exe --experimental-policy policy.json --policy-signature policy.json.p7s app.js
If the OS is configured to enforce code integrity for Node, Node will not execute unless both a policy and its signature are provided and pass WDAC's verification. From that point, integrity enforcement is delegated to Node's manifest/policy subsystem.
Some questions that I have:
I have tested this on Windows with a couple VMs on different OS versions. However, I don't know what the best course of action is for adding unit tests, since this is an OS integration feature. I'm open to any suggestions you have.