wslc: prevent session name squatting for default WSLc sessions#40144
wslc: prevent session name squatting for default WSLc sessions#40144benhillis wants to merge 1 commit intofeature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/windows/wslc/services/SessionService.cpp:45
- When attaching to the default session (empty sessionName -> nullptr passed to OpenSessionByName), failure paths still format errors using sessionName.c_str(), which is an empty string. This produces user-facing messages like "Session '' not found". Consider special-casing the empty-name/default-session case and emitting a clearer localized message (e.g., "Default session not found" / "Failed to open default session") instead of printing an empty name.
HRESULT hr = manager->OpenSessionByName(sessionName.empty() ? nullptr : sessionName.c_str(), &session);
if (FAILED(hr))
{
if (hr == HRESULT_FROM_WIN32(ERROR_NOT_FOUND))
{
wslutil::PrintMessage(Localization::MessageWslcSessionNotFound(sessionName.c_str()), stderr);
return 1;
}
auto errorString = wsl::windows::common::wslutil::ErrorCodeToString(hr);
wslutil::PrintMessage(
Localization::MessageErrorCode(Localization::MessageWslcOpenSessionFailed(sessionName.c_str()), errorString), stderr);
return 1;
src/windows/wslc/services/SessionService.cpp:186
- TerminateSession now treats an empty displayName as "default session" (nullptr passed to OpenSessionByName), but the "not found" / "terminate failed" messages still use displayName.c_str() and can end up printing an empty name. Please special-case the default-session call path so the error message is meaningful when no session name was provided.
wil::com_ptr<IWSLCSession> session;
HRESULT hr = sessionManager->OpenSessionByName(displayName.empty() ? nullptr : displayName.c_str(), &session);
if (FAILED(hr))
{
if (hr == HRESULT_FROM_WIN32(ERROR_NOT_FOUND))
{
wslutil::PrintMessage(Localization::MessageWslcSessionNotFound(displayName.c_str()), stderr);
return 1;
}
6df494e to
c991c9d
Compare
c991c9d to
46d783e
Compare
46c11a9 to
7cb7ef9
Compare
7cb7ef9 to
784b41f
Compare
784b41f to
d0b3819
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/windows/common/WSLCUserSettings.cpp:12
- File header comment still says
Module Name: UserSettings.cpp, but the file is nowWSLCUserSettings.cpp. Please update the module name to match the actual filename to avoid confusion when tracing build/log output and code ownership.
| @@ -88,6 +167,14 @@ void WSLCSessionManagerImpl::CreateSession(const WSLCSessionSettings* Settings, | |||
|
|
|||
| wslutil::StopWatch stopWatch; | |||
|
|
|||
| // Initialize settings for the default session. | |||
| std::unique_ptr<SessionSettings> defaultSettings; | |||
| if (Settings == nullptr) | |||
| { | |||
| defaultSettings = SessionSettings::Default(callerToken.get(), tokenInfo.Elevated); | |||
| Settings = &defaultSettings->Settings; | |||
| } | |||
There was a problem hiding this comment.
CreateSession holds m_wslcSessionsLock while loading/parsing user settings (YAML) for default sessions (SessionSettings::Default(...)). This introduces file I/O and YAML parsing under the session manager lock, which can block other session operations and increase contention. Consider building the resolved/default SessionSettings outside the lock (or caching per-user settings) and only holding the lock for the session lookup/mutation portion.
| <data name="MessageWslcDefaultSessionNotFound" xml:space="preserve"> | ||
| <value>Default session not found</value> | ||
| </data> | ||
| <data name="MessageWslcOpenDefaultSessionFailed" xml:space="preserve"> | ||
| <value>Failed to open default session</value> | ||
| </data> | ||
| <data name="MessageWslcTerminateDefaultSessionFailed" xml:space="preserve"> | ||
| <value>Default session termination failed</value> | ||
| </data> |
There was a problem hiding this comment.
New user-facing resource keys were added only to en-US/Resources.resw. Other locales currently lack these entries (the localization validator only warns), which can cause missing translations/fallback-to-English behavior for non-en-US users. Consider adding placeholder entries for these keys in the other locale .resw files so localization can pick them up consistently.
- Server now determines default session name and settings from caller's token, preventing malicious users from squatting reserved session names - CreateSession rejects explicit use of reserved names (wslc-cli, wslc-cli-admin) with E_ACCESSDENIED - Null StoragePath remains valid for ephemeral sessions; empty string is rejected as E_INVALIDARG - Add dedicated EnterSession API for custom sessions - Extract UserSettings into shared wslcsettings library used by both wslc.exe and wslservice.exe - Move EnumVariantMap.h to common - wslc.exe no longer parses settings.yaml; service handles all config - Fix std::terminate crash in CustomDmesgOutput test when CreateSession fails by adding a scope_exit guard to join the reader thread - Add session name squatting E2E test
d0b3819 to
50b93fb
Compare
Summary
Prevents malicious users from squatting reserved WSLc session names (
wslc-cli,wslc-cli-admin) by moving default session name and settings resolution to the server.Changes
CreateSession(nullptr)andOpenSessionByName(nullptr)resolve the default session name from the caller's token. The client no longer knows or sends session names for default sessions.CreateSessionreturnsE_ACCESSDENIED.EnterSessionAPI: Custom sessions (wslc session enter) use a separate API that readssettings.yamlserver-side and setsNoCreatestorage flags.wslcsettingsshared library: ExtractedUserSettingsinto a shared library used by bothwslc.exeandwslservice.exe.SettingsCommandnow callsUserSettingsdirectly instead of duplicating file creation logic.SessionOptionsclass,SessionModel.cpp,WslcSettingsTemplate.h. MovedEnumVariantMap.hto common.SessionSettingsisNON_MOVABLE(usesunique_ptrto avoidc_str()pointer invalidation).Testing