SSHD/Echoserver: Fix memory leaks and public-key lookup #1071
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
| { | ||
| 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 | ||
|
|
@@ -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), | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
testThe PR changes daemon privilege behavior by returning success from
SetDefaultUserIDwhenUsePrivilegeSeparation nois configured, and by makingwolfSSHD_AuthRaisePermissionsa no-op unless the config isWOLFSSHD_PRIV_SEPARATorWOLFSSHD_PRIV_SANDBOX. Grep found config parsing tests and reduce-permission tests, but no tests that exercisewolfSSHD_AuthRaisePermissionsor auth creation under the new privilege modes. This is a user-visible daemon startup/auth behavior change, so it should have regression coverage.Suggestion:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added unit coverage for the UsePrivilegeSeparation no case to ensure it returns success without requiring the configured sshd user. I also added tests verifying that wolfSSHD_AuthRaisePermissions only triggers the uid/gid restore path for the yes and sandbox modes as expected.