Skip to content

Address Coverity findings in wolfscp, wolfsftp, wolfssh, and wolfsshd - #1139

Open
ejohnstown wants to merge 11 commits into
wolfSSL:masterfrom
ejohnstown:sf14
Open

Address Coverity findings in wolfscp, wolfsftp, wolfssh, and wolfsshd#1139
ejohnstown wants to merge 11 commits into
wolfSSL:masterfrom
ejohnstown:sf14

Conversation

@ejohnstown

Copy link
Copy Markdown
Contributor
  • wolfscp: remove dead sendCtx null re-check — FindNextDirEntry() already returns WS_BAD_ARGUMENT for a NULL ctx, so the second check is unreachable. (CID-572848)
  • wolfscp: fix path length check and terminate filePath — the guard allowed the joined path to exactly fill the buffer, leaving no room for the null; also terminate filePath after the WSTRNCPY of dirName. (CID-572864)
  • wolfsftp: check SFTP_SetHeader in RecvRealPath — ignored return; free the output buffer on error only when allocated locally, not when borrowed from the receive state. (CID-572923)
  • wolfsftp: check the resume seek in wolfSSH_SFTP_Put — a failed WFSEEK() left the local file at offset 0 while the remote write resumed at the offset, silently corrupting the upload. (CID-572932)
  • apps/wolfssh: wire up the -a agent option — useAgent was never set, making both agent setup blocks dead code; adds the option to match examples/client. (CID-572857)
  • apps/wolfssh, examples/client: retry readInput sends over a rekey — a rekey surfaces as WS_FATAL_ERROR with WS_REKEYING from wolfSSH_get_error(), so the old test never matched; resend the same buffer, and break out of the loop so the ECC cache cleanup is reachable. (CID-572833)
  • examples/client: drop dead select_ret tests in NonBlockSSH_connect — the loop condition already guarantees want-read/want-write, so the select_ret arms were unreachable; same change in apps/wolfssh. (CID-572884)
  • wolfsshd: check fcntl results in the pipe drain — check both fcntl calls and skip the drain read on a pipe that could not be made non-blocking, so the read cannot hang the connection process. (CID-572931)
  • wolfsshd: retry the final shell output flush — the post-waitpid drain ignored the send return, dropping the tail of command output on a full window, rekey, or would-block; retry bounded and log on failure. (CID-572907)
  • wolfsshd: drop dead dCert NULL check — without WOLFSSH_SMALL_STACK, dCert is a stack address and can't be NULL; keep the check only for the small-stack build where the WMALLOC can fail. (CID-573006)

FindNextDirEntry() returns WS_BAD_ARGUMENT when its ctx is NULL, so by
the time control reaches the second check ret is already WS_BAD_ARGUMENT
whenever sendCtx is NULL. The earlier check right after the call is kept.

Issue: CID-572848
The length guard allowed dirNameLen + 1 + dNameLen to equal the buffer
size, leaving no room for the terminating null, after which WSTRNCAT
appends nothing and the wrong path is used. Also terminate filePath
explicitly after the WSTRNCPY of dirName.

Issue: CID-572864
The return of SFTP_SetHeader() was ignored, unlike the other call sites.
Free the output buffer on the error path only when it was allocated here
rather than borrowed from the receive state.

Issue: CID-572923

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.

Pull request overview

This PR addresses multiple Coverity findings across the SFTP/SCP libraries and the example/utility clients/daemon, mainly tightening error handling, removing dead/unreachable code, and improving robustness during rekeying and non-blocking I/O.

Changes:

  • Fixes missed/unchecked return values and error paths in SFTP PUT/REALPATH and wolfsshd shell output draining.
  • Corrects SCP path length guarding and string termination, and removes dead/unreachable checks.
  • Updates non-blocking connect loops and stdin send retry behavior to correctly handle rekeying.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/wolfsftp.c Adds missing error checks for REALPATH response construction and resume-seek behavior during SFTP PUT.
src/wolfscp.c Fixes path length boundary check, ensures null-termination, and removes dead/unreachable validation logic.
examples/client/client.c Simplifies non-blocking connect retry loop and makes stdin send retry correctly handle rekeying.
apps/wolfsshd/wolfsshd.c Adds helpers for non-blocking pipe drain and bounded retry flushing of final shell output.
apps/wolfsshd/auth.c Removes unreachable NULL check for stack-allocated DecodedCert while preserving small-stack allocation checks.
apps/wolfssh/wolfssh.c Wires up -a agent option and aligns non-blocking connect + stdin send retry behavior with rekey handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/wolfsftp.c
Comment thread apps/wolfsshd/wolfsshd.c
A failed WFSEEK() left the local file at offset 0 while the remote write
continued from the resume offset, silently corrupting the upload. Fail
out through the local close instead, the file is open at that point.

Issue: CID-572932

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.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

apps/wolfssh/wolfssh.c:799

  • The getopt option string includes "a" unconditionally, but the switch only handles 'a' when WOLFSSH_AGENT is enabled. In non-agent builds, "-a" will be parsed as a known option and then fall into the default case, showing usage that does not list -a and exiting. Make the option string conditional on WOLFSSH_AGENT (or handle 'a' with an explicit "agent not enabled" message).
    int ch;

    while ((ch = mygetopt(argc, argv, "aE:Gl:Np:V")) != -1) {
        switch (ch) {
        #ifdef WOLFSSH_AGENT

examples/client/client.c:381

  • This function still returns early on stdin read failure, which bypasses the ECC per-thread cache cleanup after the loop. The PR description mentions breaking out so the cleanup is reachable (as done in apps/wolfssh); this call site should use break instead of return as well.
        if (ret <= 0) {
            fprintf(stderr, "Error reading stdin\n");
            return THREAD_RET_SUCCESS;
        }
        do {

examples/client/client.c:401

  • This function still returns early on send failure, which bypasses the ECC per-thread cache cleanup after the loop. To match the intended behavior described in the PR (and the change made in apps/wolfssh), break out of the loop instead of returning here.
        if (ret <= 0) {
            fprintf(stderr, "Couldn't send data\n");
            return THREAD_RET_SUCCESS;
        }

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1139

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.

Comment thread examples/client/client.c Outdated
The useAgent flag was never set, so both agent setup blocks in
wolfSSH_Client were dead code. Add the -a option to the parser and
carry it through the config struct, matching examples/client/client.c.

Issue: CID-572857
Both loop exits in readInput() returned early, leaving the per-thread
ECC cache cleanup after the loop unreachable. Break out of the loop
instead.

wolfSSH_stream_send() reports a rekey in progress by returning
WS_FATAL_ERROR with the ssh error set to WS_REKEYING, so testing the
return value against WS_REKEYING never matches. Read the code with
wolfSSH_get_error() and resend the same buffer once the rekey
finishes, in both the app and the example client. Looping back to
read() instead would drop the input already taken from stdin.

Issue: CID-572833
The loop condition already guarantees a want-read or want-write error,
so the select_ret arms of the retry test and the else chain could never
run. Retry unconditionally; tcp_select still throttles the loop. Same
change applied to the copy in apps/wolfssh/wolfssh.c.

Issue: CID-572884
The leftover-data drain after waitpid ignored both fcntl calls. Check
the get and the set, and skip the drain read for a pipe that could not
be made non-blocking so the read cannot hang the connection process.

Issue: CID-572931
The drain after waitpid ignored the send return, so on a non-blocking
socket the tail of a command's output was dropped on a full window, a
rekey or a would block. Retry a bounded number of times and log when
the data still can not be sent.

Issue: CID-572907
Without WOLFSSH_SMALL_STACK dCert is the address of a stack variable, so
the NULL check could never fire. Keep the check under the small stack
build where the WMALLOC can actually fail.

Issue: CID-573006
- handleSz is the handle buffer size going into wolfSSH_SFTP_Open(), so
  a failed open left it non-zero and STATE_PUT_CLOSE_REMOTE closed a
  zeroed handle for a file that was never opened.
- The stray close overwrote ret and ssh->error, hiding the real cause.
Comment thread examples/client/client.c Outdated

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.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

examples/client/client.c:171

  • tcp_select() can return WS_SELECT_FAIL (select() error). Ignoring that return value can turn this loop into a tight retry (e.g., EBADF/EINTR), potentially burning CPU and masking the underlying socket error. Handle WS_SELECT_FAIL by breaking out and returning a fatal error.
        (void)tcp_select(sockfd, 1);

        ret = wolfSSH_connect(ssh);
        error = wolfSSH_get_error(ssh);

apps/wolfssh/wolfssh.c:136

  • tcp_select() can return WS_SELECT_FAIL (select() error). Ignoring the return value can cause a tight retry loop (e.g., EBADF/EINTR), which burns CPU and obscures the real failure. Handle WS_SELECT_FAIL explicitly and break out as fatal.
        (void)tcp_select(sockfd, 1);

        ret = wolfSSH_connect(ssh);
        error = wolfSSH_get_error(ssh);

apps/wolfsshd/wolfsshd.c:1452

  • The select() return value is ignored. If select() fails (e.g., EINTR or EBADF), this loop still consumes a retry and proceeds, which can prematurely drop output or spin quickly on repeated errors. Check select() and retry on EINTR; treat other failures as fatal.
            if (cnt_w == WS_WANT_WRITE) {
                select((int)sshFd + 1, NULL, &fds, NULL, &to);
            }
            else {
                /* waiting on the peer's window adjust or kex packets */

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1139

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

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.

3 participants