wolfsshd: expand AuthorizedKeysFile %u/%h/%% tokens per user

[yosuke-wolfssl]

wolfsshd: expand AuthorizedKeysFile %u/%h/%% tokens per user

Problem

ResolveAuthKeysPath copied an absolute AuthorizedKeysFile pattern into the
resolved path verbatim, with no token substitution (a TODO marked the gap).
A common OpenSSH-migration config such as:

AuthorizedKeysFile /etc/ssh/keys/%u

resolved to the literal /etc/ssh/keys/%u for every user. If a file existed
at that un-substituted path, its keys were accepted for any valid system user,
collapsing per-user key isolation.

Addressed by f_5827.

Fix

• Add ExpandAuthKeysTokens() — expands %u (user), %h (home dir), and %%
  (literal %) into a bounds-checked buffer. Unrecognized tokens and
  over-length expansions fail closed (WS_FATAL_ERROR) rather than reaching
  the filesystem.
• ResolveAuthKeysPath() now takes the connecting user and expands the pattern
  before the absolute/relative decision, so both branches honor substitution and
  each user resolves to a distinct path.
• Thread the username through SearchForPubKey() → CheckPublicKeyUnix (Unix)
  and CheckPublicKeyWIN (Windows). Both call sites already held it; no new
  getpwnam/lookup is introduced and no shared mutable state is added.

Token semantics

Token	Expands to
%u	connecting user
%h	user home dir
%%	literal %

A recognized token with no value (e.g. %u with a NULL user) and any
unrecognized token fail closed. A trailing lone % is treated as a literal.

Platform notes

Absolute-path detection is /-rooted on POSIX; on Windows it additionally
recognizes a \-root or a drive-letter (X:) root, guarded by #ifdef _WIN32
so a POSIX relative pattern beginning with \ or <letter>: is not misread.

Testing

• New test_ResolveAuthKeysPath unit test (13 scenarios): per-user %u
  (alice vs bob), %h, %%, relative, default-NULL fallback, trailing %,
  unrecognized token, NULL-user fail-closed, expansion over-length, relative
  home-dir over-length, and platform-aware drive-letter/backslash handling.
• MAX_PATH_SZ moved to configuration.h so the test buffer tracks the real
  contract; ResolveAuthKeysPath exposed to the tests via WOLFSSHD_STATIC.
• Verified under ASan + UBSan (clean), with negative controls confirming the
  tests catch both the verbatim-copy regression and a removed bounds check.
• Native Windows MSVC build (VS Build Tools 2022, x64): auth.c /
  configuration.c compile with no new warnings; default test set
  (api-test, unit-test, testsuite) passes.

Files changed

• apps/wolfsshd/auth.c
• apps/wolfsshd/auth.h
• apps/wolfsshd/configuration.h
• apps/wolfsshd/test/test_configuration.c

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1064

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[aidangarske]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped

Posted findings

• [High] Windows drive-relative AuthorizedKeysFile paths are treated as absolute — apps/wolfsshd/auth.c:573-610
• [Medium] New path-resolution tests mask missing string termination — apps/wolfsshd/test/test_configuration.c:1965-1974

Review generated by Skoll.

[aidangarske]

🟠 [High] Windows drive-relative AuthorizedKeysFile paths are treated as absolute
🚫 BLOCK bug

The PR adds Windows absolute-path detection, but it treats any <letter>: prefix as absolute. On Windows, paths like C:keys\alice or C:alice are drive-relative, not fully qualified. Because ResolveAuthKeysPath() skips the home-directory join when IsAbsoluteAuthKeysPath() returns true (copying the path unchanged for SearchForPubKey to open), an AuthorizedKeysFile C:keys\%u pattern is opened relative to the daemon process's current directory on drive C instead of under the user's home directory. Data flow: config/public setter -> ExpandAuthKeysTokens expands %u -> IsAbsoluteAuthKeysPath returns true solely because path[1] == ':' -> ResolveAuthKeysPath copies unchanged -> SearchForPubKey opens it. Under a misconfigured Windows deployment using X:relative syntax, an attacker with write access to the relevant current-directory tree can influence which authorized_keys file is loaded. This is introduced by the new _WIN32 branch and is not covered by the new Windows test vectors, which only check C:\keys\%u. Severity views differ across modes: review rates this High (BLOCK), security rates the practical impact Low (requires Windows, a non-default/mistyped config, and write access to the current-directory tree); the stricter High is kept.

Recommendation: Require a slash or backslash after the drive colon for fully qualified Windows paths (path[1] == ':' && (path[2] == '\\' || path[2] == '/')), and add Windows test vectors for C:%u and C:keys\%u that expect home-relative resolution (or rejection).

[aidangarske]

🟡 [Medium] New path-resolution tests mask missing string termination
💡 SUGGEST test

The new unit test clears resolved before every success case, so it cannot catch whether ResolveAuthKeysPath() actually writes a terminating NUL. That matters because the relative/default branch currently copies WSTRLEN(suffix) bytes even though the adjacent comment says it copies the NUL terminator. The production caller also zeroes its buffer, so this is mostly a test-quality gap, but the PR newly exposes and tests this helper and should not bake in a caller-side zeroing precondition.

Recommendation: Initialize resolved with a non-zero byte pattern (e.g. 0xA5) for success cases, then assert the expected string. Consider fixing the helper to copy WSTRLEN(suffix) + 1 in the relative/default branch.