Skip to content

Grant AppContainer access to NUL device for runtime stdio initialization - #23

Closed
Scott Hanselman (shanselman) wants to merge 1 commit into
microsoft:mainfrom
shanselman:fix/grant-nul-device-access
Closed

Grant AppContainer access to NUL device for runtime stdio initialization#23
Scott Hanselman (shanselman) wants to merge 1 commit into
microsoft:mainfrom
shanselman:fix/grant-nul-device-access

Conversation

@shanselman

Copy link
Copy Markdown
Member

Grant AppContainer access to NUL device for runtime stdio initialization

Summary

Many runtimes (Node.js v19+, Electron v29+, Python) unconditionally open \\.\NUL during stdio initialization. AppContainers block device path access by default, causing fatal crashes before user code executes — even though WXC provides perfectly valid stdio pipes.

This PR grants the AppContainer SID temporary read/write access to the NUL device DACL before CreateProcessW, and restores the original DACL after the child exits.

What changed

wxc_common/AppContainerScriptRunner.cpp — 1 file, ~100 lines added:

  1. GrantNulDeviceAccess() — Opens \\.\NUL with WRITE_DAC, adds an ACE granting the AppContainer SID GENERIC_READ | GENERIC_WRITE, saves the original DACL for restoration
  2. RestoreNulDeviceSecurity() — Restores the original DACL after the child process exits
  3. Two call sites in RunInternal() — grant before CreateProcessW, restore after WaitForMultipleObjects

Security considerations

ConcernAssessment
What is NUL?A kernel data sink — discards all writes, returns EOF on reads
Information disclosure?No — NUL contains no data
Privilege escalation?No — NUL provides no capabilities
System-wide impact?Temporary — original DACL is restored after child exits
Failure behavior?Graceful — all errors logged as warnings, execution continues

Testing

With an Electron v29+ app in the AppContainer:

Before this fix:

[PID:FATAL:electron\shell\common\node_bindings.cc:683]
Unable to open nul device needed for initialization, aborting startup.
=== Exit Code: -2147483645 ===

After this fix (expected --debug output):

Granted AppContainer access to NUL device
Process created successfully (PID: XXXXX)
...
Restored NUL device security
=== Exit Code: 0 ===

Related

Many runtimes (Node.js v19+, Electron v29+, Python, etc.) unconditionally
open \\.\NUL during stdio initialization. AppContainers block device path
access by default, causing these runtimes to crash with a fatal error before
they can use the stdio pipes that WXC provides.
This change temporarily grants the AppContainer SID read/write access to the
NUL device's DACL before creating the child process, and restores the original
DACL after the child exits. Since NUL is a data sink (discards writes, returns
EOF on reads), this carries no security risk.
Fixes the following crash when running Electron apps in WXC:
FATAL:electron\shell\common\node_bindings.cc:683
Unable to open nul device needed for initialization
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR mitigates AppContainer crashes in runtimes that open \\.\NUL during stdio initialization by temporarily granting the AppContainer SID access to the NUL device DACL before process creation and restoring the original DACL after the child exits.

Changes:

  • Add helper functions to grant and restore NUL device DACL access for a given AppContainer SID
  • Invoke the grant before CreateProcessW and restore after the child process exits
  • Add logging around the permission change lifecycle

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +289 to +293
// Grant AppContainer access to the NUL device for runtime stdio initialization.
// This must happen before CreateProcessW so the child can open \\.\NUL on startup.
PACL pOriginalNulDacl = nullptr;
PSECURITY_DESCRIPTOR pNulSD = nullptr;
auto hNulDevice = GrantNulDeviceAccess(_appContainerSid.get(), pOriginalNulDacl, pNulSD, logger);

CopilotAIMar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restoration is only shown on the normal post-exit path. If RunInternal() returns early after the grant (e.g., process creation failure or other error paths), the original DACL may not be restored and pNulSD may leak. Consider using an RAII scope guard (or a small stack helper) that always calls RestoreNulDeviceSecurity(...) during stack unwinding, and only performs the restore when the grant actually succeeded.

Copilot uses AI. Check for mistakes.
Comment on lines +377 to +378
// Restore the NUL device's original security now that the child has exited
RestoreNulDeviceSecurity(hNulDevice, pOriginalNulDacl, pNulSD, logger);

CopilotAIMar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restoration is only shown on the normal post-exit path. If RunInternal() returns early after the grant (e.g., process creation failure or other error paths), the original DACL may not be restored and pNulSD may leak. Consider using an RAII scope guard (or a small stack helper) that always calls RestoreNulDeviceSecurity(...) during stack unwinding, and only performs the restore when the grant actually succeeded.

Copilot uses AI. Check for mistakes.
Comment on lines +109 to +111
::SetSecurityInfo(hNul.get(), SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION,
nullptr, nullptr, pOriginalDacl, nullptr);
logger << L"Restored NUL device security\n";

CopilotAIMar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The restore path logs success unconditionally, even if SetSecurityInfo fails. Capture and check the return value and log a warning with the error code on failure (and only log the success message when it succeeds) to avoid misleading diagnostics.

Suggested change
::SetSecurityInfo(hNul.get(), SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION,
nullptr, nullptr, pOriginalDacl, nullptr);
logger << L"Restored NUL device security\n";
DWORD restoreResult = ::SetSecurityInfo(hNul.get(), SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION,
nullptr, nullptr, pOriginalDacl, nullptr);
if (restoreResult == ERROR_SUCCESS)
{
logger << L"Restored NUL device security\n";
}
else
{
logger << L"Warning: Failed to restore NUL device security (error " << restoreResult << L")\n";
}

Copilot uses AI. Check for mistakes.

EXPLICIT_ACCESS_W ea = {};
ea.grfAccessPermissions = GENERIC_READ | GENERIC_WRITE;
ea.grfAccessMode = SET_ACCESS;

CopilotAIMar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SET_ACCESS can replace existing permissions for the same trustee rather than strictly adding permissions, which increases the risk of unintentionally changing previously-granted rights. Prefer GRANT_ACCESS (or an approach that appends a new ACE) so this change is additive and minimizes impact while the temporary DACL modification is in effect.

Suggested change
ea.grfAccessMode = SET_ACCESS;
ea.grfAccessMode = GRANT_ACCESS;

Copilot uses AI. Check for mistakes.
ea.grfAccessMode = SET_ACCESS;
ea.grfInheritance = NO_INHERITANCE;
ea.Trustee.TrusteeForm = TRUSTEE_IS_SID;
ea.Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;

CopilotAIMar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AppContainer SID here is not a 'well-known group'. Using TRUSTEE_IS_WELL_KNOWN_GROUP is inaccurate and may affect how the trustee is interpreted. Use TRUSTEE_IS_USER or TRUSTEE_IS_UNKNOWN for an AppContainer SID.

Suggested change
ea.Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;
ea.Trustee.TrusteeType = TRUSTEE_IS_USER;

Copilot uses AI. Check for mistakes.
Comment on lines +64 to +69
if (::GetSecurityInfo(hNul.get(), SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION,
nullptr, nullptr, &pOriginalDacl, nullptr, &pSD) != ERROR_SUCCESS)
{
logger << L"Warning: Could not read NUL device security descriptor\n";
return WXC::UniqueHandle();
}

CopilotAIMar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On failure after GetSecurityInfo succeeds, pOriginalDacl can remain non-null while pSD is freed and nulled, leaving pOriginalDacl as a dangling pointer (even if it’s not used today due to the empty handle). To prevent accidental future misuse, explicitly set pOriginalDacl = nullptr whenever pSD is freed/invalidated, and consider logging the relevant error codes (from GetSecurityInfo/SetEntriesInAclW/SetSecurityInfo) to improve diagnosability.

Copilot uses AI. Check for mistakes.
Comment on lines +80 to +96
if (::SetEntriesInAclW(1, &ea, pOriginalDacl, &pNewDacl) != ERROR_SUCCESS)
{
logger << L"Warning: Could not create new ACL for NUL device\n";
::LocalFree(pSD);
pSD = nullptr;
return WXC::UniqueHandle();
}

if (::SetSecurityInfo(hNul.get(), SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION,
nullptr, nullptr, pNewDacl, nullptr) != ERROR_SUCCESS)
{
logger << L"Warning: Could not set NUL device DACL\n";
::LocalFree(pNewDacl);
::LocalFree(pSD);
pSD = nullptr;
return WXC::UniqueHandle();
}

CopilotAIMar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On failure after GetSecurityInfo succeeds, pOriginalDacl can remain non-null while pSD is freed and nulled, leaving pOriginalDacl as a dangling pointer (even if it’s not used today due to the empty handle). To prevent accidental future misuse, explicitly set pOriginalDacl = nullptr whenever pSD is freed/invalidated, and consider logging the relevant error codes (from GetSecurityInfo/SetEntriesInAclW/SetSecurityInfo) to improve diagnosability.

Copilot uses AI. Check for mistakes.
@shschaefer

Copy link
Copy Markdown
Contributor

Thanks for this. We think the correct solution to this is to resolve this problem from within the OS. We have checked in code there. Would prefer to not merge this and abandon the PR.

@bbonaby

Copy link
Copy Markdown
Collaborator

Thanks Scott. Closing based on Stuart's comment above + we've since converted the repo to Rust.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@shanselman@shschaefer@bbonaby@kanismohammed