Skip to content

examples/portfwd: drop the password from the options banner - #1198

Merged
ejohnstown merged 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_11673
Aug 26, 2026
Merged

examples/portfwd: drop the password from the options banner#1198
ejohnstown merged 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_11673

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Problem

examples/portfwd/portfwd.c mishandled the SSH password in two independent ways
(f-11673 and f-10606).

  • The startup banner printed " * password: %s\n" with the -P value, so the
    plaintext credential went to stdout before the connection was even attempted —
    into every redirected log, CI transcript and script capture. The banner has
    done this since the example was added in a42075d8, 2018-08-31 — eight years.
  • wsUserAuth() copied the password (from -P or the interactive prompt) into a
    non-static global byte userPassword[256] that nothing ever erased, so it
    stayed resident for the duration of the forwarding session and past
    wolfSSH_Cleanup().

Fix (examples/portfwd/portfwd.c)

  • Banner: the password line and its argument are dropped from the printf;
    the ssh host, username and forward endpoint lines are untouched. Nothing
    consumed the removed line — no script or test greps portfwd options or any
    banner line, and scripts/fwd.test.expect syncs on portfwd's -R ready file
    and the nc data, never on client stdout.
  • userPassword is static, matching the already-static sibling in
    examples/client/common.c.
  • wc_ForceZero() at the wolfSSH_connect() return, before the error check
    so the failure path is covered too — the surrounding style is err_sys(),
    which exits and never reaches the cleanup at the end of the function:
    ret = wolfSSH_connect(ssh);
    /* User authentication is done with the buffer either way. */
    wc_ForceZero(userPassword, sizeof(userPassword));
    if (ret != WS_SUCCESS)
        err_sys("Couldn't connect SFTP");

Safe because SendUserAuthRequest() builds the packet inside the same call
that invokes the callback; the library keeps no pointer into the buffer, and
wolfSSH_connect() returns WS_SUCCESS only after user-auth completes.

  • wolfssl/wolfcrypt/memory.h added for the wc_ForceZero() declaration.

Closes f-11673 and f-10606. The -P option itself is unchanged (fwd.test
depends on it), as is the argv copy the auth context points at: the password
given to -P remains in the process argument vector for the whole run, visible
in ps and /proc/<pid>/cmdline to a local observer, and in shell history.
Scrubbing that is deliberately out of scope — it would mean copying the value
into userPassword and repointing the auth context there, a larger change than
this PR is scoped for. The exposure closed here is the durable one: a credential
written into a redirected log outlives the process; argv does not.

Tests

None added. The suites exercise the library, using the examples as vehicles;
there is no harness that asserts on an example's own stdout, and zeroization of
a global is not observable from a test.

Verification

  • Build clean (--enable-fwd --enable-sftp); gcc-13 -Werror sweep clean across
    6 configs.
  • make check: 8 passed, 1 skipped, 0 failed. scripts/fwd.test passes all
    three phases — local, reverse, and peer-allocated port — which is what proves
    auth still succeeds with the buffer zeroed.
  • Manual: with -P upthehill, the captured client log contains no occurrence of
    the password and the remaining four banner lines print.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Aug 25, 2026
Copilot AI lite review requested due to automatic review settings August 25, 2026 02:10

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

Removes plaintext password exposure in the portfwd example by eliminating credential printing at startup and ensuring the in-memory password buffer is wiped after the SSH connection attempt completes (success or failure).

Changes:

  • Drop the password line (and its argument) from the portfwd options startup banner.
  • Change userPassword to static (internal linkage) and add wc_ForceZero() to clear it immediately after wolfSSH_connect() returns.
  • Include <wolfssl/wolfcrypt/memory.h> to provide the wc_ForceZero() declaration.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@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 #1198

Scan targets checked: wolfssh-bugs, wolfssh-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

- The startup banner's password line and its argument are dropped
  from the printf; ssh host, username and the two forward endpoints
  remain.
- userPassword has internal linkage, and portfwd_worker() zeroes it
  with wc_ForceZero() as soon as wolfSSH_connect() returns, on both
  the success and the failure path.
- portfwd.c includes wolfssl/wolfcrypt/memory.h.

Issue: F-11673
@ejohnstown
ejohnstown merged commit 8e8b62d into wolfSSL:master Aug 26, 2026
165 checks passed
@yosuke-wolfssl
yosuke-wolfssl deleted the fix/f_11673 branch August 26, 2026 23:10
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.

5 participants