Skip to content

SSHD/Echoserver: Fix memory leaks and public-key lookup #1071

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
stenslae wants to merge 1 commit into
base: master
Choose a base branch
from
Open

SSHD/Echoserver: Fix memory leaks and public-key lookup #1071

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
40 changes: 26 additions & 14 deletions apps/wolfsshd/auth.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1841,7 +1841,7 @@ static int SetDefaultPublicKeyCheck(WOLFSSHD_AUTH* auth)
#define WOLFSSH_USER_GET_STRING(x) #x
#define WOLFSSH_USER_STRING(x) WOLFSSH_USER_GET_STRING(x)

static int SetDefualtUserID(WOLFSSHD_AUTH* auth)
static int SetDefaultUserID(WOLFSSHD_AUTH* auth)
{
#ifdef _WIN32
/* TODO: Implement for Windows. */
Expand All @@ -1850,6 +1850,11 @@ static int SetDefualtUserID(WOLFSSHD_AUTH* auth)
struct passwd* pwInfo;
int ret = WS_SUCCESS;

if (wolfSSHD_ConfigGetPrivilegeSeparation(auth->conf) ==

@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] Privilege-separation permission behavior lacks regression tests
💡 SUGGEST test

The PR changes daemon privilege behavior by returning success from SetDefaultUserID when UsePrivilegeSeparation no is configured, and by making wolfSSHD_AuthRaisePermissions a no-op unless the config is WOLFSSHD_PRIV_SEPARAT or WOLFSSHD_PRIV_SANDBOX. Grep found config parsing tests and reduce-permission tests, but no tests that exercise wolfSSHD_AuthRaisePermissions or auth creation under the new privilege modes. This is a user-visible daemon startup/auth behavior change, so it should have regression coverage.

Suggestion:

Suggested change
if (wolfSSHD_ConfigGetPrivilegeSeparation(auth->conf) ==
Add unit coverage for `UsePrivilegeSeparation no` returning success without requiring the configured sshd user, and for `wolfSSHD_AuthRaisePermissions` calling the uid/gid restore path only for `yes` and `sandbox` modes.

WOLFSSHD_PRIV_OFF) {
return WS_SUCCESS;
}

pwInfo = getpwnam(WOLFSSH_USER_STRING(WOLFSSH_SSHD_USER));
if (pwInfo == NULL) {
/* user name not found on system */
Expand Down Expand Up @@ -1910,7 +1915,7 @@ WOLFSSHD_AUTH* wolfSSHD_AuthCreateUser(void* heap, const WOLFSSHD_CONFIG* conf)
}

if (ret == WS_SUCCESS) {
ret = SetDefualtUserID(auth);
ret = SetDefaultUserID(auth);
if (ret != WS_SUCCESS) {
wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Error setting default "
"user ID.");
Expand Down Expand Up @@ -1942,24 +1947,31 @@ int wolfSSHD_AuthFreeUser(WOLFSSHD_AUTH* auth)
/* return WS_SUCCESS on success */
int wolfSSHD_AuthRaisePermissions(WOLFSSHD_AUTH* auth)
{
int ret = 0;
int ret = WS_SUCCESS;

wolfSSH_Log(WS_LOG_INFO, "[SSHD] Attempting to raise permissions level");
#ifndef WIN32

@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.

🔵 [Low] New bare scope block violates local C convention
🔧 NIT convention

The PR adds a standalone scope block under #ifndef WIN32 only to limit flag. The project coding guidance treats newly added bare scope blocks as a convention issue; this can be written with the declaration inside the existing preprocessor region instead.

Suggestion:

Suggested change
#ifndef WIN32
#ifndef WIN32
byte flag = 0;
if (auth == NULL) {
return WS_BAD_ARGUMENT;
}
flag = wolfSSHD_ConfigGetPrivilegeSeparation(auth->conf);
if (flag == WOLFSSHD_PRIV_SEPARAT || flag == WOLFSSHD_PRIV_SANDBOX) {
...
}
#endif

if (auth) {
if (setegid(auth->sGid) != 0) {
wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Error raising gid");
ret = WS_FATAL_ERROR;
{
byte flag = 0;

if (auth == NULL) {
return WS_BAD_ARGUMENT;
}

if (seteuid(auth->sUid) != 0) {
wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Error raising uid");
ret = WS_FATAL_ERROR;
flag = wolfSSHD_ConfigGetPrivilegeSeparation(auth->conf);
if (flag == WOLFSSHD_PRIV_SEPARAT || flag == WOLFSSHD_PRIV_SANDBOX) {
wolfSSH_Log(WS_LOG_INFO,
"[SSHD] Attempting to raise permissions level");
if (setegid(auth->sGid) != 0) {
wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Error raising gid");
ret = WS_FATAL_ERROR;
}

if (ret == WS_SUCCESS && seteuid(auth->sUid) != 0) {
wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Error raising uid");
ret = WS_FATAL_ERROR;
}
}
}
else {
ret = WS_BAD_ARGUMENT;
}
#endif

return ret;
Expand Down
62 changes: 52 additions & 10 deletions examples/echoserver/echoserver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1785,7 +1785,6 @@ static int load_key(byte isEcc, byte* buf, word32 bufSz)
return sz;
}


#ifndef WOLFSSH_NO_ED25519
/* returns buffer size on success */
static int load_key_ed25519(byte* buf, word32 bufSz)
Expand Down Expand Up @@ -1922,7 +1921,6 @@ static int LoadMlDsaHostKeys(WOLFSSH_CTX* ctx, const char* keyList)
}
#endif /* WOLFSSH_NO_MLDSA */


typedef struct StrList {
const char* str;
struct StrList* next;
Expand Down Expand Up @@ -2475,7 +2473,7 @@ static int EchoserverInitTpmHostKey(WOLFSSH_CTX* ctx, const char* keyFile,
wolfTPM2_Cleanup(&tpmHostDev);
}

/* keyBlob holds the private blob and key auth; the session may hold auth. */
/* zeroize key material; session may also hold auth data */
wc_ForceZero(&keyBlob, sizeof(keyBlob));
wc_ForceZero(&tpmSession, sizeof(tpmSession));
#ifndef NO_FILESYSTEM
Expand Down Expand Up @@ -2556,6 +2554,7 @@ static int wsUserAuth(byte authType,
PwMapList* list;
PwMap* map;
byte authHash[WC_SHA256_DIGEST_SIZE] = {0};
int userFound = 0;

if (ctx == NULL) {
fprintf(stderr, "wsUserAuth: ctx not set");
Expand Down Expand Up @@ -2676,12 +2675,12 @@ static int wsUserAuth(byte authType,
authData->type == map->type) {

if (authData->type == WOLFSSH_USERAUTH_PUBLICKEY) {

@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] Multiple public-key lookup fix has no regression coverage
💡 SUGGEST test

The PR fixes wsUserAuth so a hash mismatch for the first public key registered to a user does not immediately reject later keys. Existing public-key tests use their own serverPubkeyUserAuth callback and do not exercise the echoserver PwMapList traversal. The same fix is mirrored in the ESP-IDF echoserver, so a regression could silently return if this path is refactored.

Suggestion:

Suggested change
if (authData->type == WOLFSSH_USERAUTH_PUBLICKEY) {
Add a regression scenario with two public-key entries for the same username where the first key hash mismatches and the second matches.

userFound = 1;
if (WMEMCMP(map->p, authHash, WC_SHA256_DIGEST_SIZE) == 0) {
return WOLFSSH_USERAUTH_SUCCESS;
}
else {
return WOLFSSH_USERAUTH_INVALID_PUBLICKEY;
}
/* Hash mismatch: continue checking other registered keys
* for this user (a user may have multiple public keys). */
}
else if (authData->type == WOLFSSH_USERAUTH_PASSWORD) {
if (WMEMCMP(map->p, authHash, WC_SHA256_DIGEST_SIZE) == 0) {
Expand Down Expand Up @@ -2724,6 +2723,8 @@ static int wsUserAuth(byte authType,
map = map->next;
}

if (userFound)
return WOLFSSH_USERAUTH_INVALID_PUBLICKEY;
return WOLFSSH_USERAUTH_INVALID_USER;
}

Expand Down Expand Up @@ -2807,7 +2808,8 @@ static void ShowUsage(void)
" load in a SSH public key to accept from peer\n");
printf(" -s <file> load in a TPM public key file to replace default hansel key\n");
#ifdef WOLFSSH_TPM
printf(" -G <file> load an ECC/RSA host key blob from the TPM (private key stays in the TPM)\n");
printf(" -G <file> load ECC/RSA host key blob from TPM"
" (private key stays in TPM)\n");
#endif
printf(" -J <name>:<file>\n"
" load in an X.509 PEM cert to accept from peer\n");
Expand Down Expand Up @@ -3213,14 +3215,17 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
if (kbAuthData.prompts == NULL) {
ES_ERROR("Error allocating prompts");
}
kbAuthData.prompts[0] = (byte*)"KB Auth Password: ";
kbAuthData.promptLengths = (word32*)WMALLOC(sizeof(word32), NULL, 0);
if (kbAuthData.prompts == NULL) {
if (kbAuthData.promptLengths == NULL) {
WFREE(kbAuthData.prompts, NULL, 0);
ES_ERROR("Error allocating promptLengths");
}
kbAuthData.prompts[0] = (byte*)"KB Auth Password: ";
kbAuthData.promptLengths[0] = 18;
kbAuthData.promptEcho = (byte*)WMALLOC(sizeof(byte), NULL, 0);
if (kbAuthData.prompts == NULL) {
if (kbAuthData.promptEcho == NULL) {
WFREE(kbAuthData.prompts, NULL, 0);
WFREE(kbAuthData.promptLengths, NULL, 0);
ES_ERROR("Error allocating promptEcho");
}
kbAuthData.promptEcho[0] = 0;
Expand Down Expand Up @@ -3254,6 +3259,9 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
if (tpmHostKeyPath != NULL) {
if (EchoserverInitTpmHostKey(ctx, tpmHostKeyPath,
ECHOSERVER_TPM_KEY_AUTH_DEFAULT) != 0) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
ES_ERROR("Couldn't load TPM host key from %s.\n",
Comment thread
stenslae marked this conversation as resolved.
tpmHostKeyPath);
}
Expand All @@ -3264,10 +3272,16 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
if (loadDefaultHostKeys) {
bufSz = load_key(peerEcc, keyLoadBuf, bufSz);
if (bufSz == 0) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
ES_ERROR("Couldn't load first key file.\n");
}
if (wolfSSH_CTX_UsePrivateKey_buffer(ctx, keyLoadBuf, bufSz,
WOLFSSH_FORMAT_ASN1) < 0) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
ES_ERROR("Couldn't use first key buffer.\n");
}

Expand All @@ -3277,10 +3291,16 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)

bufSz = load_key(peerEcc, keyLoadBuf, bufSz);
if (bufSz == 0) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
ES_ERROR("Couldn't load second key file.\n");
}
if (wolfSSH_CTX_UsePrivateKey_buffer(ctx, keyLoadBuf, bufSz,
WOLFSSH_FORMAT_ASN1) < 0) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
ES_ERROR("Couldn't use second key buffer.\n");
}
#endif
Expand All @@ -3289,10 +3309,16 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
bufSz = EXAMPLE_KEYLOAD_BUFFER_SZ;
bufSz = load_key_ed25519(keyLoadBuf, bufSz);
if (bufSz == 0) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
ES_ERROR("Couldn't load Ed25519 key file.\n");
}
if (wolfSSH_CTX_UsePrivateKey_buffer(ctx, keyLoadBuf, bufSz,
WOLFSSH_FORMAT_ASN1) < 0) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
ES_ERROR("Couldn't use Ed25519 key buffer.\n");
}
#endif /* WOLFSSH_NO_ED25519 */
Expand Down Expand Up @@ -3321,11 +3347,17 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)

/* create temp buffer and load in file */
if (userBufSz == 0) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
ES_ERROR("Couldn't find size of file %s.\n", userPubKey);
}

userBuf = (byte*)WMALLOC(userBufSz, NULL, 0);
if (userBuf == NULL) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
ES_ERROR("WMALLOC failed\n");
}
load_file(userPubKey, userBuf, &userBufSz);
Expand All @@ -3343,17 +3375,27 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
load_file(caCert, NULL, &certBufSz);

if (certBufSz == 0) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
ES_ERROR("Couldn't find size of file %s.\n", caCert);
}

certBuf = (byte*)WMALLOC(certBufSz, NULL, 0);
if (certBuf == NULL) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
ES_ERROR("WMALLOC failed\n");
}
load_file(caCert, certBuf, &certBufSz);
ret = wolfSSH_CTX_AddRootCert_buffer(ctx, certBuf, certBufSz,
WOLFSSH_FORMAT_PEM);
if (ret != 0) {
#ifdef WOLFSSH_SMALL_STACK
WFREE(keyLoadBuf, NULL, 0);
#endif
WFREE(certBuf, NULL, 0);
ES_ERROR("Couldn't add root cert\n");
Comment thread
stenslae marked this conversation as resolved.
}
WFREE(certBuf, NULL, 0);
Expand Down
Loading
Loading