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
42 changes: 33 additions & 9 deletions apps/wolfsshd/auth.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@
#if defined(WOLFSSHD_UNIT_TEST) && !defined(_WIN32)
int (*wsshd_setregid_cb)(WGID_T, WGID_T) = setregid;
int (*wsshd_setreuid_cb)(WUID_T, WUID_T) = setreuid;
int (*wsshd_setegid_cb)(WGID_T) = setegid;
int (*wsshd_seteuid_cb)(WUID_T) = seteuid;
#endif

struct WOLFSSHD_AUTH {
Expand Down Expand Up @@ -1841,7 +1843,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 +1852,15 @@ static int SetDefualtUserID(WOLFSSHD_AUTH* auth)
struct passwd* pwInfo;
int ret = WS_SUCCESS;

if (wolfSSHD_ConfigGetPrivilegeSeparation(auth->conf) ==
Comment thread
aidangarske marked this conversation as resolved.
WOLFSSHD_PRIV_OFF) {
auth->gid = getgid();
auth->uid = getuid();
auth->sGid = auth->gid;
auth->sUid = auth->uid;
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 +1921,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 +1953,37 @@ 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
Comment thread
aidangarske marked this conversation as resolved.
if (auth) {
byte flag = 0;

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

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");
#ifdef WOLFSSHD_UNIT_TEST
if (wsshd_setegid_cb(auth->sGid) != 0) {
#else
if (setegid(auth->sGid) != 0) {
#endif
wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Error raising gid");
ret = WS_FATAL_ERROR;
}

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

return ret;
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 @@ -90,6 +90,8 @@ int wolfSSHD_OpenSecureFile(const char* path, WUID_T ownerUid,
#ifndef _WIN32
extern int (*wsshd_setregid_cb)(WGID_T, WGID_T);
extern int (*wsshd_setreuid_cb)(WUID_T, WUID_T);
extern int (*wsshd_setegid_cb)(WGID_T);
extern int (*wsshd_seteuid_cb)(WUID_T);
int wolfSSHD_GetUserGroupNames(void* heap, const char* usr, WGID_T primaryGid,
char*** outNames, word32* outCount);
void wolfSSHD_FreeUserGroupNames(void* heap, char** names, word32 count);
Expand Down
266 changes: 266 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 @@ -1559,6 +1559,266 @@ static int test_AuthReducePermissionsUser_uid_fail(void)
wsshd_setreuid_cb = savedReuid;
return ret;
}

static WGID_T s_setegid_arg;
static WUID_T s_seteuid_arg;
static int s_setegid_ret;
static int s_seteuid_ret;
static int s_setegid_called;
static int s_seteuid_called;

static int stub_setegid(WGID_T egid)
{
s_setegid_called = 1;
s_setegid_arg = egid;
return s_setegid_ret;
}

static int stub_seteuid(WUID_T euid)
{
s_seteuid_called = 1;
s_seteuid_arg = euid;
return s_seteuid_ret;
}

static void InstallPrivRaiseStubs(int egidRet, int euidRet,
int (**savedEgid)(WGID_T), int (**savedEuid)(WUID_T))
{
*savedEgid = wsshd_setegid_cb;
*savedEuid = wsshd_seteuid_cb;
wsshd_setegid_cb = stub_setegid;
wsshd_seteuid_cb = stub_seteuid;
s_setegid_ret = egidRet;
s_seteuid_ret = euidRet;
s_setegid_called = 0;
s_seteuid_called = 0;
s_setegid_arg = 0;
s_seteuid_arg = 0;
}

/* UsePrivilegeSeparation no must let SetDefaultUserID succeed without a
* configured sshd system user, since no uid/gid switching will ever happen. */
static int test_AuthCreateUser_privSepOff(void)
{
int ret = WS_SUCCESS;
WOLFSSHD_CONFIG* conf;
WOLFSSHD_AUTH* auth;
static const char line[] = "UsePrivilegeSeparation no";

conf = wolfSSHD_ConfigNew(NULL);
if (conf == NULL) {
return WS_MEMORY_E;
}

if (ParseConfigLine(&conf, line, (int)WSTRLEN(line), 0) != WS_SUCCESS) {
ret = WS_FATAL_ERROR;
}

if (ret == WS_SUCCESS) {
auth = wolfSSHD_AuthCreateUser(NULL, conf);
if (auth == NULL) {
ret = WS_FATAL_ERROR;
}
else {
wolfSSHD_AuthFreeUser(auth);
}
}

wolfSSHD_ConfigFree(conf);
return ret;
}

/* wolfSSHD_AuthRaisePermissions must not touch setegid/seteuid at all when
* privilege separation is off, since the process never dropped privileges. */
static int test_AuthRaisePermissions_offSkipsSyscalls(void)
{
int ret = WS_SUCCESS;
WOLFSSHD_CONFIG* conf;
WOLFSSHD_AUTH* auth;
int (*savedEgid)(WGID_T);
int (*savedEuid)(WUID_T);
static const char line[] = "UsePrivilegeSeparation no";

conf = wolfSSHD_ConfigNew(NULL);
if (conf == NULL) {
return WS_MEMORY_E;
}

if (ParseConfigLine(&conf, line, (int)WSTRLEN(line), 0) != WS_SUCCESS) {
ret = WS_FATAL_ERROR;
}

if (ret == WS_SUCCESS) {
auth = wolfSSHD_AuthCreateUser(NULL, conf);
if (auth == NULL) {
ret = WS_FATAL_ERROR;
}
else {
InstallPrivRaiseStubs(0, 0, &savedEgid, &savedEuid);

if (wolfSSHD_AuthRaisePermissions(auth) != WS_SUCCESS)
ret = WS_FATAL_ERROR;
if (ret == WS_SUCCESS
&& (s_setegid_called || s_seteuid_called))
ret = WS_FATAL_ERROR;

wsshd_setegid_cb = savedEgid;
wsshd_seteuid_cb = savedEuid;
wolfSSHD_AuthFreeUser(auth);
}
}

wolfSSHD_ConfigFree(conf);
return ret;
}

/* With privilege separation on, wolfSSHD_AuthRaisePermissions restores the
* uid/gid the process started with (captured at AuthCreateUser time). Skipped
* when the environment has no "sshd" system user, since SetDefaultUserID
* requires one to succeed for this mode. */
static int test_AuthRaisePermissions_separateCallsSyscalls(void)

@aidangarske aidangarske Jul 14, 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 privilege-separation-ON tests silently skip when no "sshd" system user exists · Missing Tests

Verified: the guard at test_configuration.c:1687-1691 returns WS_SUCCESS when getpwnam("sshd") is NULL, and the auth.c diff confirms SetDefaultUserID only consults getpwnam() on the privilege-separation-ON path (the PRIV_OFF path now short-circuits to getgid()/getuid()). Three of the six new tests — the only ones that exercise the security-relevant branch where privilege separation is ON and setegid/seteuid are actually invoked — bail out with return WS_SUCCESS when getpwnam("sshd") returns NULL, because they cannot construct an auth object without that account. Many CI images and minimal containers have no sshd system account, so in those environments all three tests report PASS while asserting nothing. The result is that the assertions guarding the changed code (that Raise calls setegid/seteuid with the saved uid/gid, that a setegid failure short-circuits seteuid via the new ret == WS_SUCCESS && guard, and that a seteuid failure is reported) may never actually execute in CI, while the suite still shows green. The tests do emit a Log() skip line, so this is not fully silent, but the pass/fail signal does not distinguish "verified" from "not run". Only test_AuthRaisePermissions_offSkipsSyscalls and test_AuthCreateUser_privSepOff (the priv-sep-OFF paths) run unconditionally.

Fix: Decouple these tests from the host's user database so they always run: either add a wsshd_getpwnam_cb stub (matching the existing wsshd_setegid_cb/wsshd_seteuid_cb pattern this PR already adds to auth.c/auth.h) so SetDefaultUserID can be driven with a synthetic passwd entry, or expose a WOLFSSHD_UNIT_TEST-only setter for auth->sUid/sGid. Failing that, have the runner report skipped tests distinctly from passing ones so a green suite does not imply these branches were verified.

{
int ret = WS_SUCCESS;
WOLFSSHD_CONFIG* conf;
WOLFSSHD_AUTH* auth;
int (*savedEgid)(WGID_T);
int (*savedEuid)(WUID_T);

if (getpwnam("sshd") == NULL) {
/* no sshd system user available in this environment to switch to */
Log(" No \"sshd\" system user available, skipping.\n");
return WS_SUCCESS;
}

conf = wolfSSHD_ConfigNew(NULL);
if (conf == NULL) {
return WS_MEMORY_E;
}

/* privilege separation defaults to on */
auth = wolfSSHD_AuthCreateUser(NULL, conf);
if (auth == NULL) {
ret = WS_FATAL_ERROR;
}
else {
InstallPrivRaiseStubs(0, 0, &savedEgid, &savedEuid);

if (wolfSSHD_AuthRaisePermissions(auth) != WS_SUCCESS)
ret = WS_FATAL_ERROR;
if (ret == WS_SUCCESS && (!s_setegid_called || !s_seteuid_called))
ret = WS_FATAL_ERROR;
if (ret == WS_SUCCESS && s_setegid_arg != getgid())
ret = WS_FATAL_ERROR;
if (ret == WS_SUCCESS && s_seteuid_arg != getuid())
ret = WS_FATAL_ERROR;

wsshd_setegid_cb = savedEgid;
wsshd_seteuid_cb = savedEuid;
wolfSSHD_AuthFreeUser(auth);
}

wolfSSHD_ConfigFree(conf);
return ret;
}

/* wolfSSHD_AuthRaisePermissions must reject a NULL auth argument instead of
* dereferencing it. */
static int test_AuthRaisePermissions_nullArg(void)
{
if (wolfSSHD_AuthRaisePermissions(NULL) != WS_BAD_ARGUMENT)
return WS_FATAL_ERROR;
return WS_SUCCESS;
}

/* When setegid fails, wolfSSHD_AuthRaisePermissions must report the failure
* and short-circuit seteuid rather than attempting it anyway. */
static int test_AuthRaisePermissions_gidFailSkipsUid(void)
{
int ret = WS_SUCCESS;
WOLFSSHD_CONFIG* conf;
WOLFSSHD_AUTH* auth;
int (*savedEgid)(WGID_T);
int (*savedEuid)(WUID_T);

if (getpwnam("sshd") == NULL) {
/* no sshd system user available in this environment to switch to */
Log(" No \"sshd\" system user available, skipping.\n");
return WS_SUCCESS;
}

conf = wolfSSHD_ConfigNew(NULL);
if (conf == NULL) {
return WS_MEMORY_E;
}

/* privilege separation defaults to on */
auth = wolfSSHD_AuthCreateUser(NULL, conf);
if (auth == NULL) {
ret = WS_FATAL_ERROR;
}
else {
InstallPrivRaiseStubs(-1, 0, &savedEgid, &savedEuid);

if (wolfSSHD_AuthRaisePermissions(auth) != WS_FATAL_ERROR)
ret = WS_FATAL_ERROR;
if (ret == WS_SUCCESS && !s_setegid_called)
ret = WS_FATAL_ERROR;
if (ret == WS_SUCCESS && s_seteuid_called)
ret = WS_FATAL_ERROR;

wsshd_setegid_cb = savedEgid;
wsshd_seteuid_cb = savedEuid;
wolfSSHD_AuthFreeUser(auth);
}

wolfSSHD_ConfigFree(conf);
return ret;
}

/* When setegid succeeds but seteuid fails, wolfSSHD_AuthRaisePermissions must
* still report the failure. */
static int test_AuthRaisePermissions_uidFail(void)
{
int ret = WS_SUCCESS;
WOLFSSHD_CONFIG* conf;
WOLFSSHD_AUTH* auth;
int (*savedEgid)(WGID_T);
int (*savedEuid)(WUID_T);

if (getpwnam("sshd") == NULL) {
/* no sshd system user available in this environment to switch to */
Log(" No \"sshd\" system user available, skipping.\n");
return WS_SUCCESS;
}

conf = wolfSSHD_ConfigNew(NULL);
if (conf == NULL) {
return WS_MEMORY_E;
}

/* privilege separation defaults to on */
auth = wolfSSHD_AuthCreateUser(NULL, conf);
if (auth == NULL) {
ret = WS_FATAL_ERROR;
}
else {
InstallPrivRaiseStubs(0, -1, &savedEgid, &savedEuid);

if (wolfSSHD_AuthRaisePermissions(auth) != WS_FATAL_ERROR)
ret = WS_FATAL_ERROR;
if (ret == WS_SUCCESS && !s_setegid_called)
ret = WS_FATAL_ERROR;
if (ret == WS_SUCCESS && !s_seteuid_called)
ret = WS_FATAL_ERROR;

wsshd_setegid_cb = savedEgid;
wsshd_seteuid_cb = savedEuid;
wolfSSHD_AuthFreeUser(auth);
}

wolfSSHD_ConfigFree(conf);
return ret;
}
#endif /* !_WIN32 */

/* Locks in the NULL-safe comparison used by RequestAuthentication to fail
Expand Down Expand Up @@ -1929,6 +2189,12 @@ const TEST_CASE testCases[] = {
TEST_DECL(test_AuthReducePermissionsUser_ok),
TEST_DECL(test_AuthReducePermissionsUser_gid_fail),
TEST_DECL(test_AuthReducePermissionsUser_uid_fail),
TEST_DECL(test_AuthCreateUser_privSepOff),
TEST_DECL(test_AuthRaisePermissions_offSkipsSyscalls),
TEST_DECL(test_AuthRaisePermissions_separateCallsSyscalls),
TEST_DECL(test_AuthRaisePermissions_nullArg),
TEST_DECL(test_AuthRaisePermissions_gidFailSkipsUid),
TEST_DECL(test_AuthRaisePermissions_uidFail),
#endif
#if defined(WOLFSSH_HAVE_LIBCRYPT) || defined(WOLFSSH_HAVE_LIBLOGIN)
TEST_DECL(test_CheckPasswordHashUnix),
Expand Down
Loading
Loading