Skip to content

wolfsshd: complete the Windows user profile fallback - #1243

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/13326
Open

wolfsshd: complete the Windows user profile fallback#1243
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/13326

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Problem

_GetHomeDirectory()'s fallback never set PROFILEINFO.dwSize, which LoadUserProfileW() rejects with ERROR_INVALID_PARAMETER. That fallback runs for any user with no profile on the host — the network logon wolfsshd performs does not create one — so a first login by such a user was refused: public-key auth could not locate authorized_keys, and SFTP and shell sessions could not resolve a working directory. Both Windows CI workflows worked around this by hand-writing a ProfileList registry entry.

Two further defects sat on the same path. The home directory came from ExpandEnvironmentStringsW(L"%USERPROFILE%"), which expands against the service account's environment, so fixing dwSize alone would have handed systemprofile back as every user's home. And the loaded registry hive was never unloaded.

Fix (apps/wolfsshd/auth.c)

  • dwSize is set, and the profile is loaded whenever WOLFSSHD_AUTH.profile is NULL — on every session, not only when the profile is missing. The hive is machine-wide, so a session holding no handle had it unloaded underneath it when another session for the same user ended.
  • _GetProfileDirectory() reads the path with GetUserProfileDirectoryW() on the auth token, replacing both SHGetKnownFolderPath() and the %USERPROFILE% expansion.
  • wolfSSHD_AuthCloseToken() unloads the profile before closing the token.
  • RevertToSelf() moves ahead of the token close in SHELL_Subsystem()'s cleanup — UnloadUserProfile() needs the service account's SE_RESTORE_NAME/SE_BACKUP_NAME, which the impersonated user's token lacks.
Caller Helper Builds a profile
CheckPublicKeyWIN(), before authenticating _GetProfileDirectory() no
SFTP_Subsystem(), SHELL_Subsystem() _GetHomeDirectory() yes

The split matters: userAuthCb runs before signature verification, so a signature-less public-key probe reaches CheckPublicKeyWIN(). Keeping the load out of that path stops an unauthenticated client from forcing profile creation for any account it names.

Behaviour worth noting: public-key auth is still impossible for a user who has never logged on, since there is no home directory to hold authorized_keys. That matches master, and is deliberate — profile creation is deferred to an authenticated session.

Closes f-13326, and covers the service-account home directory and the unreleased hive, neither of which was filed separately.

Why loading on every session is safe

Microsoft's LoadUserProfile documentation states the obligation directly: when a service impersonates a user the system does not load that user's profile, so the service should, and "it is your responsibility to load the user's registry hive into the HKEY_USERS registry key with the LoadUserProfile function before you call CreateProcessAsUser" — otherwise access to HKEY_CURRENT_USER "may not produce results consistent with a normal interactive logon". wolfsshd calls CreateProcessAsUserW(), so a load is owed on every session; loading only when the profile was absent was the anomaly.

Loading a profile that is already mounted does not reinitialise it — the second caller simply gets its own handle, and Windows keeps the hive mounted until the last handle is released. Neither the LoadUserProfile nor the UnloadUserProfile page documents that reference counting, so it was confirmed on Windows rather than assumed: with two overlapping sessions for one user, the unfixed code unmounts the hive the second session is still running against when the first ends, and this change keeps it mounted until the second session ends. The concurrent-session test below is that experiment, and it runs on every push.

For a user whose home directory exists but is not a registered valid profile, Windows builds a fresh profile beside it — the same thing it does on an interactive logon. That is what the workflow rework accounts for.

Tests (.github/workflows/)

A no_profile leg in windows-sftp.yml creates users with net user alone and asserts they start with no ProfileList entry or home directory. It then checks that an exec session builds the profile, runs as testuser and releases the hive; that with two overlapping sessions for a second profile-less user, ending the first leaves the hive loaded for the second and the second releases it; and that an SFTP session lands in testuser's own home rather than systemprofile and releases the hive at connection teardown.

Both Windows workflows now log testuser on once so Windows builds a real profile, in place of creating the home directory and ProfileList entry by hand. The fabricated profile had no NTUSER.DAT, so once wolfsshd actually loaded it Windows built a second profile beside it.

Verification

  • All 10 Windows jobs pass: 3 windows-sftp legs and 7 windows-cert-store-test matrix cells.
  • Negative control: the concurrent-session test fails on the unfixed code with ending session A unmounted the hive under session B.
  • MSVC Debug x64 builds clean, no warnings on the changed files; api-test and unit-test pass.
  • GCC preflight sweep: lint clean, 6/6 configs.

Not in this PR

  • CreateProcessAsUserW() passes a NULL environment, so a spawned shell inherits the service's environment rather than the user's.
  • A failed UnloadUserProfile() leaves the hive mounted; reaching that branch needs fault injection the project does not have.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Sep 8, 2026
Copilot AI lite review requested due to automatic review settings September 8, 2026 05:44

Copilot AI 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.

🟢 Approval recommended

The changes directly address the documented Windows failure mode, add proper resource cleanup, and include targeted CI coverage for the previously untested no-profile scenario.

Pull request overview

This PR fixes wolfsshd’s Windows home-directory fallback so that accounts without a pre-existing local profile can successfully log in and get a correct per-user home directory (instead of the service account’s), while also ensuring the temporary profile hive is unloaded at session teardown.

Changes:

  • Fix _GetHomeDirectory() fallback by correctly initializing PROFILEINFO.dwSize, loading the user profile at most once per session, and using GetUserProfileDirectoryW() (token-based) instead of %USERPROFILE%.
  • Track the loaded user profile hive in WOLFSSHD_AUTH.profile and unload it in wolfSSHD_AuthCloseToken().
  • Add a new Windows CI “no_profile” job that validates profile creation, correct landing directory, and hive unload after disconnect.
File summaries
File Description
apps/wolfsshd/wolfsshd.c Reorders RevertToSelf() ahead of token/profile teardown so profile unloading has required service privileges.
apps/wolfsshd/auth.c Fixes Windows profile-loading fallback and adds per-session profile hive tracking + unloading on token close.
.github/workflows/windows-sftp.yml Adds a matrix job covering a user with no pre-existing Windows profile and asserts creation + cleanup behavior.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1243

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Comment thread apps/wolfsshd/auth.c Outdated
Comment thread apps/wolfsshd/auth.c
Comment thread apps/wolfsshd/auth.c Outdated
Comment thread apps/wolfsshd/auth.c

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1243

Scan targets checked: wolfssh-bugs, wolfssh-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

@wolfSSL-Fenrir-bot
wolfSSL-Fenrir-bot dismissed their stale review September 8, 2026 07:18

Fenrir's latest completed scan found no issues; clearing the prior automated change request.

@yosuke-wolfssl
yosuke-wolfssl force-pushed the fix/13326 branch 2 times, most recently from a5b44ec to 840bf68 Compare September 9, 2026 00:59
@yosuke-wolfssl
yosuke-wolfssl force-pushed the fix/13326 branch 3 times, most recently from b41cd05 to 5b6575e Compare September 9, 2026 02:02
- _GetHomeDirectory loads the user's profile when WOLFSSHD_AUTH's new
  profile member is NULL, setting PROFILEINFO.dwSize first and keeping
  the returned hProfile there.
- _GetProfileDirectory reads the home directory with
  GetUserProfileDirectoryW, in place of SHGetKnownFolderPath and the
  %USERPROFILE% expansion. CheckPublicKeyWIN calls it directly, so a
  caller that has not authenticated the user builds no profile.
- wolfSSHD_AuthCloseToken unloads the profile before closing the token,
  calling RegCloseKey when the unload fails.
- The Windows shell cleanup calls RevertToSelf() before closing the auth
  token rather than after.
- windows-sftp.yml gains a no_profile job that covers an exec session,
  two overlapping sessions, and SFTP for users created with net user
  alone; it skips the earlier SFTP step so its exec session connects
  first.
- Both Windows workflows log testuser on once so Windows builds a real
  profile, in place of writing the home directory and ProfileList entry
  by hand, and the recursive icacls grants on it are gone.

Issue: F-13326

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1243

Scan targets checked: wolfssh-src, wolfssh-bugs

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Comment thread apps/wolfsshd/auth.c
Sign up for free to 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.

4 participants