From efcea181fb2e136ed2c3fd2df3b6a9e2a2e67952 Mon Sep 17 00:00:00 2001 From: Stuart Schaefer Date: Sat, 21 Feb 2026 16:19:13 -0800 Subject: [PATCH 1/2] Ensure that BFS does not apply container inherit on root of system drive. --- wxc_common/FileSystemBfsManager.cpp | 31 ++++++++++++++++++----- wxc_common/include/FileSystemBfsManager.h | 7 +++-- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/wxc_common/FileSystemBfsManager.cpp b/wxc_common/FileSystemBfsManager.cpp index 932b5d06b..aa0d607c7 100644 --- a/wxc_common/FileSystemBfsManager.cpp +++ b/wxc_common/FileSystemBfsManager.cpp @@ -16,7 +16,8 @@ bool FileSystemBfsManager::Configure(ContainerPolicy policy, std::wstring& error // Configure BFS for allowed paths for (const auto& path : policy.readwritePaths) { - if (!AddBfsPath(path, errorMsg)) + bool inherit = TestForRootPath(path); + if (!AddBfsPath(path, errorMsg, inherit)) { RemoveConfiguration(); return false; @@ -30,7 +31,8 @@ bool FileSystemBfsManager::Configure(ContainerPolicy policy, std::wstring& error // Configure BFS for allowed read-only paths for (const auto& path : policy.readonlyPaths) { - if (!AddReadOnlyBfsPath(path, errorMsg)) + bool inherit = TestForRootPath(path); + if (!AddReadOnlyBfsPath(path, errorMsg, inherit)) { RemoveConfiguration(); return false; @@ -81,26 +83,41 @@ bool FileSystemBfsManager::ExecuteBfsCfgOperation(std::span a return true; } -bool FileSystemBfsManager::AddBfsPath(std::wstring_view path, std::wstring& errorMsg) +bool FileSystemBfsManager::AddBfsPath(std::wstring_view path, std::wstring& errorMsg, bool inherit) { std::vector args = { - L"--addpolicy", L"--policybroker", L"--filename", path, L"--appid", _appContainerName, L"--containerinherit", + L"--addpolicy", L"--policybroker", + L"--filename", path, L"--appid", _appContainerName }; + if (inherit) + { + args.push_back(L"--containerinherit"); + } return ExecuteBfsCfgOperation( args, L"Failed to add BFS path " + std::wstring{path} + L" for AppContainer " + _appContainerName, errorMsg); } -bool FileSystemBfsManager::AddReadOnlyBfsPath(std::wstring_view path, std::wstring& errorMsg) +bool FileSystemBfsManager::AddReadOnlyBfsPath(std::wstring_view path, std::wstring& errorMsg, bool inherit) { std::vector args = { - L"--addpolicy", L"--policybrokerreadonly", L"--filename", path, - L"--appid", _appContainerName, L"--containerinherit", + L"--addpolicy", L"--policybrokerreadonly", + L"--filename", path, L"--appid", _appContainerName }; + if (inherit) + { + args.push_back(L"--containerinherit"); + } return ExecuteBfsCfgOperation( args, L"Failed to add read-only BFS path " + std::wstring{path} + L" for AppContainer " + _appContainerName, errorMsg); } +bool FileSystemBfsManager::TestForRootPath(std::wstring_view path) +{ + // Test to see if the path is "C:\", if so DO NOT inherit + return (path == L"C:\\") ? false: true; +} + bool FileSystemBfsManager::RemoveConfiguration(std::wstring& errorMsg) { std::vector args = {L"--clearpolicy", L"--appid", _appContainerName}; diff --git a/wxc_common/include/FileSystemBfsManager.h b/wxc_common/include/FileSystemBfsManager.h index 3626da795..5ff0baa0b 100644 --- a/wxc_common/include/FileSystemBfsManager.h +++ b/wxc_common/include/FileSystemBfsManager.h @@ -29,9 +29,9 @@ class FileSystemBfsManager WXC::Logger& _logger; bool _configured = false; - bool AddBfsPath(std::wstring_view path, std::wstring& errorMsg); + bool AddBfsPath(std::wstring_view path, std::wstring& errorMsg, bool inherit = true); - bool AddReadOnlyBfsPath(std::wstring_view path, std::wstring& errorMsg); + bool AddReadOnlyBfsPath(std::wstring_view path, std::wstring& errorMsg, bool inherit = true); bool RemoveConfiguration(std::wstring& errorMsg); @@ -41,4 +41,7 @@ class FileSystemBfsManager // Helper: Run bfscfg.exe with arguments std::wstring RunBfsCfg(std::span args, std::wstring& errorMsg); + + // Helper: Test a path to see if it is the root of a drive + bool TestForRootPath(std::wstring_view path); }; From 5ee3182c9a266418306d804363590366b236676f Mon Sep 17 00:00:00 2001 From: Stuart Schaefer Date: Mon, 23 Feb 2026 11:53:34 -0800 Subject: [PATCH 2/2] Ensure handles are not inherited by the container and shutdown is done correctly. --- build_debug.bat | 3 ++ cli/src/cli.ts | 3 +- sdk/src/sandbox.ts | 7 ---- test_configs/pwsh_setlocation.json | 17 +++++++++ test_scripts/run_pwsh_test.bat | 2 ++ wxc_common/AppContainerScriptRunner.cpp | 47 +++++++++++++++++++------ wxc_common/ProcessUtilities.cpp | 2 ++ 7 files changed, 61 insertions(+), 20 deletions(-) create mode 100644 build_debug.bat create mode 100644 test_configs/pwsh_setlocation.json create mode 100644 test_scripts/run_pwsh_test.bat diff --git a/build_debug.bat b/build_debug.bat new file mode 100644 index 000000000..52a427863 --- /dev/null +++ b/build_debug.bat @@ -0,0 +1,3 @@ +@echo off +echo Building Debug configuration... +msbuild wxc.sln /p:Configuration=Debug /p:Platform=x64 /t:Rebuild /nologo /verbosity:minimal diff --git a/cli/src/cli.ts b/cli/src/cli.ts index b7c0e1312..f90ad86fd 100644 --- a/cli/src/cli.ts +++ b/cli/src/cli.ts @@ -148,8 +148,7 @@ program // Spawn the process // NOTE: For now, we will force winpty. const pty = spawnSandbox(config.script, policy, { - debug: options.debug ?? false, - useConpty: false + debug: options.debug ?? false }, config.workingDirectory, config.appContainer?.name); // Handle output diff --git a/sdk/src/sandbox.ts b/sdk/src/sandbox.ts index 2b21409f6..9be184755 100644 --- a/sdk/src/sandbox.ts +++ b/sdk/src/sandbox.ts @@ -67,12 +67,6 @@ export interface SandboxSpawnOptions { */ debug?: boolean; - /** - * Use the conpty DLL instead of the default winpty backend on Windows 11. - * This may provide better performance and compatibility. - */ - useConpty?: boolean; - /** * PTY options to pass to node-pty */ @@ -158,7 +152,6 @@ export function spawnSandbox( rows: 80, cwd: workingDirectory || process.cwd(), env: process.env, - useConpty: options.useConpty, ...options.ptyOptions, }; diff --git a/test_configs/pwsh_setlocation.json b/test_configs/pwsh_setlocation.json new file mode 100644 index 000000000..c9f60eade --- /dev/null +++ b/test_configs/pwsh_setlocation.json @@ -0,0 +1,17 @@ +{ + "script": "pwsh.exe -nop -nol -c \"Set-PSReadLineOption -HistorySaveStyle SaveNothing; Set-Location c:\\temp\"; Get-ChildItem", + "appContainer": { + "name": "CLI-Pwsh" + }, + "filesystem": { + "readwritePaths": [ + "C:\\Program Files\\PowerShell\\7", + "C:\\temp", + "C:\\Users", + "C:\\Users\\st\\AppData\\Roaming\\Microsoft\\Windows\\PowerShell\\PSReadLine" + ], + "readonlyPaths": [ + "C:\\" + ] + } +} \ No newline at end of file diff --git a/test_scripts/run_pwsh_test.bat b/test_scripts/run_pwsh_test.bat new file mode 100644 index 000000000..b3fe1f50e --- /dev/null +++ b/test_scripts/run_pwsh_test.bat @@ -0,0 +1,2 @@ +@echo off +..\outputs\wxc\x64\Debug\wxc-exec.exe --debug ..\test_configs\pwsh_setlocation.json diff --git a/wxc_common/AppContainerScriptRunner.cpp b/wxc_common/AppContainerScriptRunner.cpp index f76a83eb5..d86ada938 100644 --- a/wxc_common/AppContainerScriptRunner.cpp +++ b/wxc_common/AppContainerScriptRunner.cpp @@ -146,8 +146,8 @@ ScriptResponse AppContainerScriptRunner::RunInternal(const CodexRequest& request siEx.StartupInfo.dwFlags |= STARTF_USESTDHANDLES; siEx.StartupInfo.lpDesktop = const_cast(L"winsta0\\default"); - // Initialize attribute list (security caps + optional LPAC policy) - DWORD attrCount = request.policy.leastPrivilegeMode ? 2 : 1; + // Initialize attribute list (security caps + handle list + optional LPAC policy) + DWORD attrCount = request.policy.leastPrivilegeMode ? 3 : 2; SIZE_T attributeListSize = 0; ::InitializeProcThreadAttributeList(nullptr, attrCount, 0, &attributeListSize); @@ -185,6 +185,16 @@ ScriptResponse AppContainerScriptRunner::RunInternal(const CodexRequest& request } } + // Explicitly list only the pipe handles the child container needs to inherit. + // This lets us pass bInheritHandles=TRUE to CreateProcessW while still tightly + // controlling which handles the child can access. + HANDLE inheritHandles[] = {hStdInRead.get(), hStdOutWrite.get(), hStdErrWrite.get()}; + if (!::UpdateProcThreadAttribute(siEx.lpAttributeList, 0, PROC_THREAD_ATTRIBUTE_HANDLE_LIST, inheritHandles, + sizeof(inheritHandles), nullptr, nullptr)) + { + return CreateErrorResponse(L"Failed to update HANDLE_LIST attribute."); + } + // Create the process std::vector cmdLineBuffer(request.scriptCode.begin(), request.scriptCode.end()); cmdLineBuffer.push_back(L'\0'); @@ -239,19 +249,34 @@ ScriptResponse AppContainerScriptRunner::RunInternal(const CodexRequest& request params3.hWrite = hParentStdErr; hThread3.reset(::CreateThread(nullptr, 0, WXC::PipeThread, ¶ms3, 0, nullptr)); - // Wait for child process to exit - ::WaitForSingleObject(hProcess.get(), GetTimeoutMilliseconds(request.scriptTimeout)); + // Wait for the child process to exit, or for an output relay thread to finish. + // Threads 2 and 3 exit when the child closes its stdout/stderr (which happens on exit), + // so any of these handles signaling indicates the child session is over. + HANDLE completionHandles[] = {hProcess.get(), hThread2.get(), hThread3.get()}; + DWORD waitResult = ::WaitForMultipleObjects(3, completionHandles, FALSE, + GetTimeoutMilliseconds(request.scriptTimeout)); - DWORD exitCode = 0; - ::GetExitCodeProcess(hProcess.get(), &exitCode); + if (waitResult == WAIT_TIMEOUT) + { + // Timeout elapsed before the child exited: forcibly terminate it. + ::TerminateProcess(hProcess.get(), static_cast(-1)); + // Block until the OS confirms the process is gone so GetExitCodeProcess is valid. + ::WaitForSingleObject(hProcess.get(), INFINITE); + } + + // Shut down Thread 1 (stdin relay). CancelSynchronousIo interrupts its blocking + // ReadFile call, causing PipeThread to break out of its loop. Closing hStdInWrite + // ensures that any WriteFile already in flight also fails promptly. + ::CancelSynchronousIo(hThread1.get()); + hStdInWrite.reset(); - // Wait for threads to finish (with 1 second timeout) - HANDLE threads[] = {hThread1.get(), hThread2.get(), hThread3.get()}; - WaitForMultipleObjects(3, threads, TRUE, 1000); + // Wait for all relay threads to finish draining and exit cleanly. + HANDLE allThreads[] = {hThread1.get(), hThread2.get(), hThread3.get()}; + ::WaitForMultipleObjects(3, allThreads, TRUE, 2000); - // TODO: If the process is still running after timeout, terminate it + DWORD exitCode = 0; + ::GetExitCodeProcess(hProcess.get(), &exitCode); - // TODO: Decide if we need one shot still and script response, or just error code and logging ScriptResponse result; result.ExitCode = static_cast(exitCode); diff --git a/wxc_common/ProcessUtilities.cpp b/wxc_common/ProcessUtilities.cpp index b8c2946ce..08ecf5f3a 100644 --- a/wxc_common/ProcessUtilities.cpp +++ b/wxc_common/ProcessUtilities.cpp @@ -19,11 +19,13 @@ DWORD WINAPI PipeThread(LPVOID param) while (true) { + // If the process has closed the pipe or an error occurs, exit the loop if (!ReadFile(hRead, buffer, BUFFER_SIZE, &bytesRead, nullptr) || bytesRead == 0) { break; } + // Write to the destination pipe. If an error occurs, exit the loop. if (!WriteFile(hWrite, buffer, bytesRead, &bytesWritten, nullptr) || bytesWritten != bytesRead) { break;