-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
src: add WDAC integration (Windows) #54364
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
Can you run the formatter? |
I also want to add - this is meant to facilitate discussion regarding the general implementation. I'm not very familiar with the various module loaders and ways which code can be read off disk and executed. I assume there are gaps in my implementation and I would appreciate any expertise in the area to close them. Thanks! |
@nodejs/platform-windows |
lib/internal/modules/cjs/loader.js
Outdated
@@ -198,6 +199,8 @@ const onRequire = getLazy(() => tracingChannel('module.require')); | |||
|
|||
const relativeResolveCache = { __proto__: null }; | |||
|
|||
const ci = require('code_integrity'); |
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 would put this into an internal module right now.
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.
Just to make sure - this just involves moving it into the lib/internal/ folder? Let me know if I need to do anything else!
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.
Yes @rdw-msft. So it won't be exposed as a module to users (unless they pass --expose-internals)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54364 +/- ##
========================================
Coverage 89.11% 89.11%
========================================
Files 665 666 +1
Lines 193193 193304 +111
Branches 37212 37230 +18
========================================
+ Hits 172158 172260 +102
+ Misses 13775 13769 -6
- Partials 7260 7275 +15
|
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.
We've discussed on the security-wg and the next steps are:
- Creating documentation
- Improving the test coverage
Then we can go the a proper code review. Meanwhile, I'm tagging @nodejs/tsc for awareness.
This is a Windows specific feature that will enabled by a node key in Windows system dictionary. So, it shouldn't be considered a semver-major or a specific Node.js API.
Currently, Windows should be the only environment that makes those checks available via API. We are still evaluating the status of OSX and Linux support for that. However, it's fair to assume this PR will only implement such guarantees for Windows users.
I recommend watching our meeting as we've discussed the usage and how this is turned-on in Windows systems (https://www.youtube.com/watch?v=w4zzH-otKNI)
FYI, we are working on a similar feature for Linux. The goal is the same but the implementation and API are different. Linux already provides access control systems and this new feature (previously open+O_MAYEXEC, now execveat+AT_CHECK) is the missing piece to control script execution the same way other kind of execution can already be controlled. This article gives a good overview: https://lwn.net/Articles/982085/ I plan to send a new patch series in a few weeks. Please let me know what you think. |
7cfafd5
to
e8e5e7a
Compare
e8e5e7a
to
18b9645
Compare
Hey @RafaelGSS , I've added more documentation about code integrity mode. Please let me know if it looks adequate, or if you'd like me to explain more. I also added doc/api/wdac-manifest.xml. This is the manifest we spoke about in the last meeting I attended. It declares the WDAC Application settings that NodeJS will query for. It should be hosted in an accessible place so when system admins author their Windows Code Integrity policy they know what settings are available to them, and it also adds some type-checking for the policy writer. I'll plan on attending the next security WG meeting to discuss in more detail if needed. Thanks |
doc/api/wdac-manifest.xml
Outdated
<AppManifest Id="NodeJS" xmlns="urn:schemas-microsoft-com:windows-defender-application-control"> | ||
<SettingDefinition Name="EnforceCodeIntegrity" Type="Boolean" IgnoreAuditPolicies="false"/> | ||
<SettingDefinition Name="DisableInterpretiveMode" Type="Boolean" IgnoreAuditPolicies="false"/> | ||
</AppManifest> |
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.
</AppManifest> | |
</AppManifest> | |
src/node_code_integrity.cc
Outdated
static void IsFileTrustedBySystemCodeIntegrityPolicy( | ||
const FunctionCallbackInfo<Value>& args) { | ||
args.GetReturnValue().Set(true); | ||
} |
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.
These could probably just be implemented in JavaScript so avoid the extraneous JS->C++ boundary call.
src/node_code_integrity.h
Outdated
|
||
#include <Windows.h> | ||
|
||
// {0xb5367df1,0xcbac,0x11cf,{0x95,0xca,0x00,0x80,0x5f,0x48,0xa1,0x92}} |
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.
It's not clear what this comment is.
src/node_code_integrity.h
Outdated
#ifndef SRC_NODE_CODE_INTEGRITY_H_ | ||
#define SRC_NODE_CODE_INTEGRITY_H_ | ||
|
||
#ifdef _WIN32 |
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.
Given that this is Windows only, I would generally prefer that this entire internal binding only be compiled on windows, with the non-functional stubs implemented solely in javascript on the other platforms.
8765dc6
to
7560a46
Compare
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.
- It's missing some tests, for instance, eval (The check you placed inside internal/main/eval_string)
- This feature should be opt-in, it can be either behind a flag or behind a compile time flag. To be honest, I'd first add it as a compile time flag, then we promote it to runtime flag.
If you find a potential security vulnerability, please refer to our | ||
[Security Policy][]. | ||
|
||
## Code Integrity on 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.
## Code Integrity on Windows | |
## Code Integrity on Windows | |
<!-- YAML | |
added: REPLACEME | |
--> |
|
||
> Stability: 1.1 - Active development | ||
|
||
Code integrity refers to the assurance that software code has not been |
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 think it should be opted-in for - behind a flag - to users. This documentation should first describe what they should do in the terminal such as starting nodejs with --code-integrity then describing nuances of the code integrity.
lib/code_integrity.js
Outdated
isSystemEnforcingCodeIntegrity: () => false, | ||
}; | ||
// Load the actual binding if on Windows | ||
if (isWindows) { |
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.
Should we also check the Windows version or do all supported Windows platforms have it?
lib/internal/modules/cjs/loader.js
Outdated
@@ -198,6 +199,8 @@ const onRequire = getLazy(() => tracingChannel('module.require')); | |||
|
|||
const relativeResolveCache = { __proto__: null }; | |||
|
|||
const ci = require('code_integrity'); |
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.
Yes @rdw-msft. So it won't be exposed as a module to users (unless they pass --expose-internals)
lib/internal/modules/cjs/loader.js
Outdated
@@ -1909,6 +1918,12 @@ Module._extensions['.js'] = function(module, filename) { | |||
* @param {string} filename The file path of the module | |||
*/ | |||
Module._extensions['.json'] = function(module, filename) { | |||
|
|||
const isAllowedToExecute = ci.isAllowedToExecuteFile(filename); |
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.
As I said earlier. It would be better placed inside Module._load so you don't need to replicate it to every extension.
7560a46
to
d4b6249
Compare
Add calls to Windows Defender Application Control to enforce integrity of .js, .json, .node files.
d4b6249
to
ecb293b
Compare
Add calls to Windows Defender Application Control to enforce integrity of .js, .json, .node files.
Motivation
In the past I've spoken to @RafaelGSS and the Node security work group about code integrity. We feel like it's an important defense in depth feature and the removal of the --experimental-policy feature gives us room to use OS-level code integrity features. This is a first pass at a CI implementation that cooperates with the OS.
These additions add an additional layer of defense against malicious code execution on a system that is enforcing code integrity. Code Integrity enforcement mitigates the risk of malicious code modifications after signing-time. For example, they can prevent an arbitrary file-write vulnerability from turning into an RCE. These additional checks are only made if the OS in code integrity enforcement mode and has explicitly set a configuration value in their code integrity policy to have Node enforce CI.
Windows Defender Application Control (WDAC)
Official documentation: https://learn.microsoft.com/en-us/windows/security/application-security/application-control/windows-defender-application-control/wdac-and-applocker-overview
WDAC is the Windows code integrity subsystem. It enforces code integrity using digital signatures. This is straightforward for DLLs and EXEs, since they're loaded using well known entry points in the OS. However, dynamic runtimes (interpreters like Node.js or Python) make it difficult to determine whether a file being read will be executed. In order to assist dynamic runtimes, WDAC provides an interface for runtimes to call with the code they intent to execute, and WDAC will determine if the runtime is allowed to execute the code based off signature information. WDAC (wldp.dll) exposes various methods for runtimes to ask the OS questions about the code integrity policy on the system. These changes leverage three interfaces
WldpCanExecuteFile
,WldpGetApplicationSettingBoolean
, andWldpQuerySecurityPolicy
.WldpCanExecuteFile
- given a file handle, determines if the file is allowed to be executed from WDAC policy.WldpGetApplicationSettingBoolean
- Queries WDAC policy for an application-defined setting with a boolean value. This can be thought of as a more secure setting store.WldpQuerySecurityPolicy
- WdlpGetApplicationSettingBoolean is not available on all versions of Windows, so this method provides fallback behavior to query WDAC policy settings.High-level implementation
At startup, Node will query WDAC policy to see if it should enter integrity enforcement mode. If the policy provider
Node.js
has set the policy settingEnforceCodeIntegrity
to TRUE, then Node will callWldpCanExecuteFile
when a .js, .json, or .node file is loaded usingrequire
. If this setting is not enabled, the call to WldpCanExecuteFile will not be made and execution will continue as normal.Additionally, if
EnforceCodeIntegrity
is set to TRUE, we disable features that allow arbitrary code from being executed by Node (e.g. the "-e" command line option and the REPL) when the system is enforcing code integrity.Signing files
Signatures are stored in catalog file,
.cat
. This catalog file can be thought of as a collection of filenames and their associated hashes. A Node application author can generate a catalog using the Powershell commandNew-FileCatalog
documentation.PS> New-FileCatalog -CatalogFilePath ./MyApplicationCatalog.cat -Path ./MyApplicationRelease/
The catalog then can be signed with a certificate trusted by WDAC policy using Powershell with
Set-AuthenticodeSignature
documentation orsigntool.exe
The signed catalog can then be installed on the system MyApplication is being deployed to.
Other Questions
What about Linux?
At the moment, there is no unified code integrity subsystem that provides similar cooperative interfaces for interpreters on Linux. There are proposals in-flight and we're tracking this work and hope to keep the implementation as similar as possible across OSs.