SSHD/Echoserver: Fix memory leaks and public-key lookup#1071
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1071
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
a4fa57f to
a3e4a11
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1071
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
a3e4a11 to
164dc44
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1071
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
164dc44 to
7958c10
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1071
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
aidangarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped
Posted findings
- [Medium] Privilege-separation permission behavior lacks regression tests —
apps/wolfsshd/auth.c:1853-1961 - [Medium] Multiple public-key lookup fix has no regression coverage —
examples/echoserver/echoserver.c:2677-2727 - [Low] New bare scope block violates local C convention —
apps/wolfsshd/auth.c:1952-1974
Review generated by Skoll.
| struct passwd* pwInfo; | ||
| int ret = WS_SUCCESS; | ||
|
|
||
| if (wolfSSHD_ConfigGetPrivilegeSeparation(auth->conf) == |
There was a problem hiding this comment.
🟡 [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:
| 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. |
| @@ -2676,12 +2675,12 @@ static int wsUserAuth(byte authType, | |||
| authData->type == map->type) { | |||
|
|
|||
| if (authData->type == WOLFSSH_USERAUTH_PUBLICKEY) { | |||
There was a problem hiding this comment.
🟡 [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:
| 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. |
| int ret = WS_SUCCESS; | ||
|
|
||
| wolfSSH_Log(WS_LOG_INFO, "[SSHD] Attempting to raise permissions level"); | ||
| #ifndef WIN32 |
There was a problem hiding this comment.
🔵 [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:
| #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 |
Fixed: Memory leaks, multiple public-key lookups per user in the echoserver, and authentication privilege errors.
promptData.promptsandpromptData.promptLengthson allocation failure pathways.wsUserAuthin echoserver.c to traverse allregistered public keys for a matching user name instead of immediately returning an invalid
key error on the first hash mismatch.
keyLoadBufunderWOLFSSH_SMALL_STACKconfigurations for all early exit paths insideechoserver_test.