Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yosuke-wolfssl wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 128 additions & 28 deletions apps/wolfsshd/auth.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,6 @@ struct WOLFSSHD_AUTH {
#ifndef MAX_LINE_SZ
#define MAX_LINE_SZ 900
#endif
#ifndef MAX_PATH_SZ
#define MAX_PATH_SZ 80
#endif

#if 0
/* this could potentially be useful in a deeply embedded future port */
Expand Down Expand Up @@ -477,15 +474,121 @@ static int CheckPasswordUnix(const char* usr, const byte* pw, word32 pwSz, WOLFS

static const char authKeysDefault[] = ".ssh/authorized_keys";

/* Resolve the authorized keys file path for a user. The pattern is the user's
* configured AuthorizedKeysFile (resolved per request from the per-user config)
* and is passed in explicitly rather than read from shared state so concurrent
* authentications (e.g. Windows threaded mode) cannot race on it. A NULL or
* empty pattern falls back to the default authorized_keys location. */
static int ResolveAuthKeysPath(const char* homeDir, const char* pattern,
/* Expand AuthorizedKeysFile tokens (%% literal, %h home dir, %u user name)
* from pattern into out. Unrecognized tokens fail closed so a per-user pattern
* cannot collapse to one shared path. Returns WS_SUCCESS or a negative error. */
static int ExpandAuthKeysTokens(const char* pattern, const char* homeDir,
const char* user, char* out, word32 outSz)
{
int ret = WS_SUCCESS;
word32 outIdx = 0;
word32 i = 0;
word32 patSz;
word32 insSz;
const char* ins;
char lit[2];

if (pattern == NULL || out == NULL || outSz == 0) {
ret = WS_BAD_ARGUMENT;
}

if (ret == WS_SUCCESS) {
patSz = (word32)WSTRLEN(pattern);
lit[1] = '\0';

while (ret == WS_SUCCESS && i < patSz) {
ins = NULL;

if (pattern[i] == '%' && (i + 1) < patSz) {
switch (pattern[i + 1]) {
case '%':
lit[0] = '%';
ins = lit;
break;
case 'h':
ins = homeDir;
break;
case 'u':
ins = user;
break;
default:
wolfSSH_Log(WS_LOG_ERROR,
"[SSHD] Unsupported AuthorizedKeysFile token");
ret = WS_FATAL_ERROR;
break;
}
/* token recognized but its value is unavailable */
if (ret == WS_SUCCESS && ins == NULL) {
wolfSSH_Log(WS_LOG_ERROR,
"[SSHD] No value for AuthorizedKeysFile token");
ret = WS_FATAL_ERROR;
}
i += 2;
}
else {
/* literal character (including a trailing lone '%') */
lit[0] = pattern[i];
ins = lit;
i += 1;
}

if (ret == WS_SUCCESS) {
insSz = (word32)WSTRLEN(ins);
/* leave room for the terminating null */
if (outIdx + insSz >= outSz) {
wolfSSH_Log(WS_LOG_ERROR,
"[SSHD] Path for key file larger than max allowed");
ret = WS_FATAL_ERROR;
}
else {
XMEMCPY(out + outIdx, ins, insSz);
outIdx += insSz;
}
}
}
}

if (ret == WS_SUCCESS) {
out[outIdx] = '\0';
}

return ret;
}

/* True for an absolute path. POSIX roots only at '/'; Windows also roots at a
* '\' or a drive letter ("X:"). The Windows forms stay guarded so a Unix
* relative pattern beginning with '\' or "<letter>:" is not misread. */
static int IsAbsoluteAuthKeysPath(const char* path)
{
int ret = 0;

if (path != NULL) {
if (path[0] == '/') {
ret = 1;
}
#ifdef _WIN32
else if (path[0] == '\\') {
ret = 1;
}
else if (((path[0] >= 'A' && path[0] <= 'Z') ||

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [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).

(path[0] >= 'a' && path[0] <= 'z')) && path[1] == ':') {
ret = 1;
}
#endif
}

return ret;
}

/* Resolve the authorized keys file path for a user. The pattern is passed in
* explicitly so concurrent authentications cannot race on it, and its tokens
* are expanded so each user resolves to a distinct path. */
WOLFSSHD_STATIC int ResolveAuthKeysPath(const char* homeDir,
const char* pattern, const char* user,
char* resolved)
{
int ret = WS_SUCCESS;
char expanded[MAX_PATH_SZ];
char* idx;
int homeDirSz;
const char* suffix = authKeysDefault;
Expand All @@ -496,24 +599,19 @@ static int ResolveAuthKeysPath(const char* homeDir, const char* pattern,

if (ret == WS_SUCCESS) {
if (pattern != NULL && *pattern != 0) {
/* TODO: token substitutions (e.g. %h) */
if (*pattern == '/') {
/* Absolute path is used as-is. Error out rather than
* silently truncate when it does not fit, mirroring the
* relative-path branch below. */
if (WSTRLEN(pattern) >= MAX_PATH_SZ) {
wolfSSH_Log(WS_LOG_ERROR,
"[SSHD] Path for key file larger than max allowed");
ret = WS_FATAL_ERROR;
}
else {
WSTRNCPY(resolved, pattern, MAX_PATH_SZ - 1);
resolved[MAX_PATH_SZ - 1] = '\0';
}
ret = ExpandAuthKeysTokens(pattern, homeDir, user, expanded,
(word32)sizeof(expanded));
if (ret != WS_SUCCESS) {
wolfSSH_Log(WS_LOG_ERROR,
"[SSHD] Failed to expand AuthorizedKeysFile pattern");
}
/* expanded is NUL-terminated and shorter than MAX_PATH_SZ */
else if (IsAbsoluteAuthKeysPath(expanded)) {
WMEMCPY(resolved, expanded, WSTRLEN(expanded) + 1);
return ret;
}
else {
suffix = pattern;
suffix = expanded;
}
}
}
Expand Down Expand Up @@ -756,6 +854,7 @@ int wolfSSHD_OpenSecureFile(const char* path, WUID_T ownerUid,
}

static int SearchForPubKey(const char* path, const char* authKeysFile,
const char* user,
const WS_UserAuthData_PublicKey* pubKeyCtx,
WUID_T uid, int strictModes)
{
Expand All @@ -769,7 +868,7 @@ static int SearchForPubKey(const char* path, const char* authKeysFile,
int rc = 0;

WMEMSET(authKeysPath, 0, sizeof(authKeysPath));
rc = ResolveAuthKeysPath(path, authKeysFile, authKeysPath);
rc = ResolveAuthKeysPath(path, authKeysFile, user, authKeysPath);
if (rc != WS_SUCCESS) {
wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Failed to resolve authorized keys"
" file path.");
Expand Down Expand Up @@ -952,8 +1051,9 @@ static int CheckPublicKeyUnix(const char* name,
}

if (ret == WSSHD_AUTH_SUCCESS) {
ret = SearchForPubKey(pwInfo->pw_dir, authorizedKeysFile, pubKeyCtx,
pwInfo->pw_uid, wolfSSHD_ConfigGetStrictModes(authCtx->conf));
ret = SearchForPubKey(pwInfo->pw_dir, authorizedKeysFile, name,
pubKeyCtx, pwInfo->pw_uid,
wolfSSHD_ConfigGetStrictModes(authCtx->conf));
}
}

Expand Down Expand Up @@ -1297,7 +1397,7 @@ static int CheckPublicKeyWIN(const char* usr,
if (ret == WSSHD_AUTH_SUCCESS) {
r[rSz-1] = L'\0';

ret = SearchForPubKey(r, authorizedKeysFile, pubKeyCtx, 0,
ret = SearchForPubKey(r, authorizedKeysFile, usr, pubKeyCtx, 0,
wolfSSHD_ConfigGetStrictModes(authCtx->conf));
if (ret != WSSHD_AUTH_SUCCESS) {
wolfSSH_Log(WS_LOG_ERROR,
Expand Down
2 changes: 2 additions & 0 deletions apps/wolfsshd/auth.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ int CheckPasswordHashUnix(const char* input, char* stored);
#endif
int CheckAuthKeysLine(char* line, word32 lineSz, const byte* key,
word32 keySz);
int ResolveAuthKeysPath(const char* homeDir, const char* pattern,
const char* user, char* resolved);
int CAKeysFileDiffers(const char* a, const char* b);
int wolfSSHD_GetUserAuthTypes(const WOLFSSHD_CONFIG* usrConf);
#endif
Expand Down
4 changes: 4 additions & 0 deletions apps/wolfsshd/configuration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ typedef struct WOLFSSHD_CONFIG WOLFSSHD_CONFIG;
#define WOLFSSHD_PRIV_SANDBOX 1
#define WOLFSSHD_PRIV_OFF 2

#ifndef MAX_PATH_SZ
#define MAX_PATH_SZ 80
#endif

WOLFSSHD_CONFIG* wolfSSHD_ConfigNew(void* heap);
void wolfSSHD_ConfigFree(WOLFSSHD_CONFIG* conf);
int wolfSSHD_ConfigLoad(WOLFSSHD_CONFIG* conf, const char* filename);
Expand Down
118 changes: 118 additions & 0 deletions apps/wolfsshd/test/test_configuration.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1900,6 +1900,123 @@ static int test_OpenSecureFile(void)
}
#endif /* !_WIN32 */

/* Verify AuthorizedKeysFile token substitution so an absolute pattern with %u
* resolves to a per-user path instead of the same literal string for every
* user. */
static int test_ResolveAuthKeysPath(void)
{
int ret = WS_SUCCESS;
int rc;
word32 i;
char resolved[MAX_PATH_SZ];
char longPat[MAX_PATH_SZ + 16];
char longHome[MAX_PATH_SZ];
static const struct {
const char* home;
const char* pattern;
const char* user;
int expectRet;
const char* expect;
} vectors[] = {
/* absolute pattern with %u resolves to a distinct path per user */
{ "/home/alice", "/etc/ssh/keys/%u", "alice", WS_SUCCESS,
"/etc/ssh/keys/alice" },
{ "/home/bob", "/etc/ssh/keys/%u", "bob", WS_SUCCESS,
"/etc/ssh/keys/bob" },
/* %h expands to the home directory */
{ "/home/alice", "%h/.ssh/authorized_keys", "alice", WS_SUCCESS,
"/home/alice/.ssh/authorized_keys" },
/* %% is a literal percent */
{ "/home/alice", "/keys/100%%/%u", "alice", WS_SUCCESS,
"/keys/100%/alice" },
/* relative pattern is taken under the home directory */
{ "/home/alice", "keys/%u", "alice", WS_SUCCESS,
"/home/alice/keys/alice" },
#ifdef _WIN32
/* drive-letter and backslash roots are absolute on Windows */
{ "/home/alice", "C:\\keys\\%u", "alice", WS_SUCCESS,
"C:\\keys\\alice" },
{ "/home/alice", "\\keys\\%u", "alice", WS_SUCCESS,
"\\keys\\alice" },
#else
/* on POSIX they are relative, taken under the home directory */
{ "/home/alice", "C:\\keys\\%u", "alice", WS_SUCCESS,
"/home/alice/C:\\keys\\alice" },
{ "/home/alice", "\\keys\\%u", "alice", WS_SUCCESS,
"/home/alice/\\keys\\alice" },
#endif
/* NULL pattern falls back to the default location */
{ "/home/alice", NULL, "alice", WS_SUCCESS,
"/home/alice/.ssh/authorized_keys" },
/* a trailing lone '%' is treated as a literal */
{ "/home/alice", "/etc/keys/%u%", "alice", WS_SUCCESS,
"/etc/keys/alice%" },
/* unrecognized token fails closed */
{ "/home/alice", "/etc/keys/%q", "alice", WS_FATAL_ERROR, NULL },
/* recognized token with no available value (NULL user) fails closed */
{ "/home/alice", "/etc/keys/%u", NULL, WS_FATAL_ERROR, NULL },
};

for (i = 0; ret == WS_SUCCESS && i < sizeof(vectors) / sizeof(vectors[0]);
i++) {
Log(" Testing scenario: pattern \"%s\" user \"%s\".",
vectors[i].pattern != NULL ? vectors[i].pattern : "(null)",
vectors[i].user != NULL ? vectors[i].user : "(null)");
WMEMSET(resolved, 0, sizeof(resolved));

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 [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.

rc = ResolveAuthKeysPath(vectors[i].home, vectors[i].pattern,
vectors[i].user, resolved);
if (rc != vectors[i].expectRet) {
Log(" FAILED (rc=%d).\n", rc);
ret = WS_FATAL_ERROR;
}
else if (vectors[i].expect != NULL &&
WSTRCMP(resolved, vectors[i].expect) != 0) {
Log(" FAILED (got \"%s\").\n", resolved);
ret = WS_FATAL_ERROR;
}
else {
Log(" PASSED.\n");
}
}

/* an expansion that exceeds MAX_PATH_SZ fails closed */
if (ret == WS_SUCCESS) {
Log(" Testing scenario: over-length pattern is rejected.");
WMEMSET(longPat, 'a', sizeof(longPat) - 1);
longPat[0] = '/';
longPat[sizeof(longPat) - 1] = '\0';
WMEMSET(resolved, 0, sizeof(resolved));
rc = ResolveAuthKeysPath("/home/alice", longPat, "alice", resolved);
if (rc == WS_FATAL_ERROR) {
Log(" PASSED.\n");
}
else {
Log(" FAILED (rc=%d).\n", rc);
ret = WS_FATAL_ERROR;
}
}

/* a relative pattern under a long home directory fails closed */
if (ret == WS_SUCCESS) {
Log(" Testing scenario: relative pattern under long home is "
"rejected.");
WMEMSET(longHome, 'a', sizeof(longHome) - 1);
longHome[0] = '/';
longHome[sizeof(longHome) - 1] = '\0';
WMEMSET(resolved, 0, sizeof(resolved));
rc = ResolveAuthKeysPath(longHome, "keys/%u", "alice", resolved);
if (rc == WS_FATAL_ERROR) {
Log(" PASSED.\n");
}
else {
Log(" FAILED (rc=%d).\n", rc);
ret = WS_FATAL_ERROR;
}
}

return ret;
}

const TEST_CASE testCases[] = {
TEST_DECL(test_ConfigDefaults),
TEST_DECL(test_ParseConfigLine),
Expand All @@ -1917,6 +2034,7 @@ const TEST_CASE testCases[] = {
TEST_DECL(test_IncludeRecursionBound),
TEST_DECL(test_GetUserAuthTypes),
TEST_DECL(test_ConfigSetAuthKeysFile),
TEST_DECL(test_ResolveAuthKeysPath),
TEST_DECL(test_ConfigFree),
#ifndef _WIN32
TEST_DECL(test_OpenSecureFile),
Expand Down
Loading