Repository navigation
wolfsshd: shell child fixes and secret wipes - #1341
ejohnstown wants to merge 7 commits into
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1341
Scan targets checked: wolfssh-src, wolfssh-bugs
Not reviewed (no scan target covers them): apps/wolfsshd/auth.h, apps/wolfsshd/test/run_all_sshd_tests.sh, apps/wolfsshd/test/sshd_exit_signal_test.sh, apps/wolfsshd/test/sshd_scp_home_test.sh, configure.ac
Coverage: 3 of 6 in-scope changed file(s) opened by the reviewer; not opened: apps/wolfsshd/test/test_configuration.c, tests/api.c, wolfssh/ssh.h
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Lite
4bb9bbe to
f9ced89
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1341
Scan targets checked: wolfssh-src, wolfssh-bugs
Not reviewed (no scan target covers them): apps/wolfsshd/auth.h, apps/wolfsshd/test/run_all_sshd_tests.sh, apps/wolfsshd/test/sshd_exit_signal_test.sh, apps/wolfsshd/test/sshd_scp_home_test.sh, configure.ac
Coverage: 1 of 1 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
wolfSSH_DoModes() now returns -1 without touching the terminal when tcgetattr() fails, instead of applying an uninitialized termios. A failed tcsetattr() is reported the same way, and wolfsshd logs it. - api.c: DoModes on a pipe returns -1 Issue: F-10546
A shell child killed by a signal now reports 128 plus the signal as its exit status. A failure after the fork kills and reaps the child and closes the parent's descriptors to it, and the resize context is cleared before the terminal it points at is closed. - SHELL_AbortChild() serves the post-fork setup failures, which now run before the parent drops privileges so it can still kill the child - -r with no command now fails at the argument check in main() - sshd_exit_signal_test.sh checks a plain exit and a SIGKILL Issue: F-13327, F-14203, F-15571, F-14204
SCP_Subsystem() changes to pw_dir after dropping privileges, as the shell and SFTP do, so a relative remote path resolves from home rather than from the daemon's working directory, inside a chroot too. When home is unreachable, SCP and the shell start from "/", as OpenSSH does. SCP logs that only outside a chroot. - sshd_scp_home_test.sh uploads to "." and finds the file in home Issue: F-15502
Account lookups go through wolfSSHD_GetPwnam(), which fills a caller-owned store with getpwnam_r() and wipes it on release. Shadow entries are read the same way with getspnam_r() where libc has it. The crypt() result is wiped on every path. - auth.c and the session lookup in HandleConnection use the store - configure probes the five-argument getspnam_r() as HAVE_GETSPNAM_R - ScanShadowFile() wipes the line the loop exit reads - the getpwnam/getspnam test hooks default to NULL, the real lookup - test_GetPwnam checks the store, the wipe, and the errno contract Issue: F-11683, F-11684, F-13439, F-8842, F-12646
The wide-character copies of the peer's command are zeroed before they are freed, in SHELL_Subsystem() and in SetupConsole(). A CreateProcessW() failure in SetupConsole() now runs its cleanup before returning WS_FATAL_ERROR. Issue: F-8851, F-10596
The shell child frees its session and the server context before it drops to the user, so the host private key and the session keys are wiped from its image ahead of the exec. It keeps copies of the user name and command it still needs. A short read of a key file wipes the buffer before freeing it. Issue: F-8841, F-14227
CheckPasswordHashUnix() hashes into a heap struct crypt_data when configure finds crypt_r(), and wipes the whole struct on return, so neither the result nor crypt's working state is left behind. Builds without it, such as macOS, keep crypt() and its result wipe. - configure probes crypt_r() through <crypt.h> as HAVE_CRYPT_R Issue: F-8842
f9ced89 to
0317365
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1341
Scan targets checked: none
Unchanged since last review (not re-run): wolfssh-src, wolfssh-bugs
Not reviewed (no scan target covers them): apps/wolfsshd/auth.h, apps/wolfsshd/test/run_all_sshd_tests.sh, apps/wolfsshd/test/sshd_exit_signal_test.sh, apps/wolfsshd/test/sshd_scp_home_test.sh, configure.ac
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
Fixes to wolfsshd's shell and SCP sessions, then wiping of account
entries, command lines, host keys and password hashes.
and reap the child; -r with no command is rejected.
passwords hash with crypt_r() where configure finds it.
session are wiped. F-11683, F-11684 and F-13439 are mitigated, not
fixed: libc's own lookup buffers are not wiped.
Issue: F-10546, F-13327, F-14203, F-15571, F-14204, F-15502, F-11683, F-11684, F-13439, F-8842, F-12646, F-8851, F-10596, F-8841, F-14227