Skip to content

Harden the PID file, login shell and port parsing - #1326

Open
ejohnstown wants to merge 4 commits into
wolfSSL:masterfrom
ejohnstown:audit-batch
Open

ejohnstown wants to merge 4 commits into
wolfSSL:masterfrom
ejohnstown:audit-batch

Conversation

@ejohnstown

Copy link
Copy Markdown
Contributor

Four hardening fixes from an outside audit: the wolfsshd PID file and login shell, a missing NULL check, and port parsing in the client tools.

  • wolfsshd writes no PID file where O_NOFOLLOW is unavailable, since no pre-check can stop a swapped-in symlink there (F-14764).
  • wolfsshd execs the user's shell path whole instead of an 80-byte copy, and builds argv[0] from its basename; sshd_long_shell_test.sh covers both (F-14757).
  • SendChannelTerminalResize returns WS_BAD_ARGUMENT for a NULL ssh, like its sibling senders (F-14769).
  • The -p option of wolfssh, client, wolfscp, wolfsftp and echoserver rejects a port that is not wholly a number in range instead of wrapping it, and wolfssh rejects an empty ssh://host: port (F-14763, F-14767, F-14768, F-12552).

Where open() cannot refuse a final-component symlink, no pre-check can
stop a link swapped in before the open, and the fstat gate accepts a
root-owned target. wolfSSHD_ConfigSavePID now writes no PID file there
instead of relying on a racy lstat.

- test_ConfigSavePID gets a variant for that build that expects no file

Issue: F-14764
SHELL_Subsystem copied pw_shell into an 80-byte buffer before execv(),
so a longer path ran whatever its first 79 bytes named. The child now
passes pw_shell to execv() directly, and builds the login argv[0] from
its basename so a long path no longer falls back to "-sh".

- add sshd_long_shell_test.sh, which logs in as a throwaway user whose
  shell path is longer than 80 bytes and checks the shell's $0

Issue: F-14757
It was the one channel sender that read ssh->defaultPeerChannelId
without a NULL check. It now returns WS_BAD_ARGUMENT for a NULL ssh and
logs entry and exit like its siblings.

- add TestTerminalResizeNullSsh to regress.test

Issue: F-14769
The -p option of wolfssh, client, wolfscp, wolfsftp and echoserver now
goes through a strtol check, so a value that is not wholly a number in
range exits with an error instead of wrapping into a different port.

- apps/wolfssh: ClientParsePort(), shared with ClientParseDestination(),
  which also rejects an empty "ssh://host:" port instead of defaulting
- examples: a ClientParsePort() in examples/client/common.c; port 0 is
  rejected in every build, not only with a main driver
- echoserver: accepts 0 to 65535, keeping 0 for the test harness
- add TestClientParsePort and an empty URI port case to regress.test

Issue: F-14763, F-14767, F-14768, F-12552
Copilot AI balanced review requested due to automatic review settings October 16, 2026 22:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants