Fix pty-req terminal mode size handling when stdin is not a tty

[yosuke-wolfssl]

Problem

CreateMode() returned -1 when tcgetattr(STDIN_FILENO) failed, and
SendChannelTerminalRequest() stored that in word32 modeSz, making it
0xFFFFFFFF. The PreparePacket() size arithmetic is word32, so it wrapped and
allocated a small output buffer; the guarded copy below then ran
WMEMCPY(output + idx, mode, 0xFFFFFFFF) — a 4 GiB read out of a 4096-byte stack
buffer into a ~60-byte heap buffer. High severity, CWE-681 leading to CWE-787.

Triggered by any client that requests a pty while stdin is not a terminal:
wolfssh -u user host < /dev/null, cron/CI runs, or an embedder with no
controlling terminal. Both in-tree clients set WOLFSSH_SESSION_TERMINAL
unconditionally in interactive mode. Closes f-7202.

Fix (src/internal.c)

• CreateModeTermios() is a new helper holding the termios body; it returns
  WS_FATAL_ERROR instead of printing and returning -1. CreateMode() falls
  back to the default mode list (38400 + TTY_OP_END) when there is no terminal —
  the path the no-termios build already used, so the #else branch is gone.
• The mode writes are bounded at the source. TTYWordSet()/TTYSet() took a
  bare byte* and advanced idx blindly, so the caller could only detect damage
  already done. Both now refuse to write past the buffer:

    if (*idx + TERMINAL_MODE_SZ + 1 > TERMINAL_MODES_MAX_SZ)
        return;

The reserved byte leaves room for the terminator, whose write is guarded the same
way — CreateMode() returns WS_FATAL_ERROR if it will not fit. A full buffer
truncates to a well-formed TTY_OP_END-terminated list. TERMINAL_MODES_MAX_SZ
replaces the bare 4096 literals so the buffer and its bound cannot drift.

• GetTerminalInfo() had the same gap for the window size: the
  ioctl(TIOCGWINSZ) return was ignored, so a client with no terminal negotiated a
  0x0 pty, which wolfsshd passes straight to TIOCSWINSZ. The no-terminal defaults
  are now assigned once up front and each platform arm overwrites only what it
  actually read, which also removed the duplicated defaults and the #else arm.

Behavior change: a non-tty client completes the pty-req with default modes and an
80x24 window and runs to EOF instead of crashing, matching ssh -tt host </dev/null.

Tests (tests/unit.c)

test_SendChannelTerminalRequestNoTty() points stdin and stdout at /dev/null
(TIOCGWINSZ reads stdout), pins TERM, sends the request, restores the
environment, then asserts every field of the captured packet by value: "pty-req",
the term string, an 80x24 window with zero pixel dimensions, and the exact 11-byte
fallback mode list.

Verification

• make check: 10 passed, 1 skipped, 0 failed. Clean under gcc-13 -Werror across
  6 configs. ASan + UBSan clean.
• Negative controls: reverting src/internal.c makes the test die under ASan with
  stack-buffer-overflow ... SendChannelTerminalRequest; deleting the setter guards
  with the buffer shrunk to 8 bytes gives stack-buffer-overflow ... in TTYWordSet;
  restoring the unconditional ioctl() yields window 0x0 (0x0 px); changing the
  fallback baud to 9600 fails the mode-content assertion.

[Copilot]

Pull request overview

This pull request fixes a high-severity size/wraparound bug in client pty-req handling when stdin is not a TTY, preventing a potentially massive out-of-bounds copy caused by storing a negative mode-size into a word32. It refactors terminal-mode encoding to safely fall back to default modes when termios settings can’t be read, and adds a unit test that exercises the non-tty path by redirecting stdin to /dev/null.

Changes:

• Refactored termios mode encoding into a helper and made the non-tty case fall back to default terminal modes (src/internal.c).
• Added defensive validation of the mode-size result before packet-size arithmetic in SendChannelTerminalRequest() (src/internal.c).
• Added a unit test that redirects stdin to /dev/null and asserts the encoded terminal modes are bounded and properly terminated (tests/unit.c).

Reviewed changes

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

File	Description
src/internal.c	Prevents mode-size wraparound/overflow in pty-req by using a safe fallback and validating size before packet allocation/copy.
tests/unit.c	Adds coverage for the no-tty stdin scenario to ensure the request remains bounded and correctly terminated.

---

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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1130

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[ejohnstown]

Skoll Code Review

Scan type: reviewOverall recommendation: COMMENT
Findings: 7 total — 3 posted, 4 skipped
3 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Medium] No-tty pty-req now negotiates a 0x0 terminal window — src/internal.c:20071-20087
• [Medium] CreateMode() bound check happens after the buffer has already been written — src/internal.c:20083-20087
• [Medium] New test asserts packet shape but never that the default modes were used — tests/unit.c:5735-5745

Skipped findings

• [Low] Test packet walk can wrap word32 and read past the 256-byte capture buffer
• [Low] Test hardcodes 4096 instead of tracking the library's mode buffer size
• [Low] Test failure codes -800..-814 collide with three existing tests
• [Low] modeRet failure branch in SendChannelTerminalRequest is now unreachable and untested

Review generated by Skoll

[ejohnstown]

🟠 [Medium] No-tty pty-req now negotiates a 0x0 terminal window

The PR turns the no-terminal case from a fatal (crashing) path into a completed pty-req. The mode string now falls back to sane defaults (38400 baud), but the window size has no matching fallback. GetTerminalInfo() calls ioctl(STDOUT_FILENO, TIOCGWINSZ, &windowSize) and never checks the return value; windowSize is zero-initialized (struct winsize windowSize = { 0,0,0,0 };), so when there is no controlling terminal the client sends widthChar = 0, heightRows = 0, widthPixels = 0, heightPixels = 0. The server takes those verbatim: apps/wolfsshd/wolfsshd.c:1646 does s.ws_col = ssh->widthChar; ... ioctl(childFd, TIOCSWINSZ, &s);, so the allocated pty really is 0 columns by 0 rows. Note the #else arm of the same function already documents 80x24 as the 'sane defaults for terminal size if not yet supported' -- the ioctl arm silently produces 0x0 instead. The 0x0 case was technically reachable before the PR (tty on stdin, pipe on stdout), but the PR is what makes the fully non-interactive client complete the request rather than die, so this is now the normal outcome of the newly supported use case. Since the whole point of the change is 'complete the pty-req with defaults instead of crashing', the size deserves the same treatment as the modes.

Fix: Give the window size the same no-terminal fallback the modes just got (80x24), or state explicitly in the commit message that a 0x0 pty is the intended result for non-tty clients. Worth a test assertion on the width/height fields either way, since the new test currently skips over them.

[yosuke-wolfssl]

Fixed. GetTerminalInfo() now assigns the no-terminal defaults (80x24, zero
pixels) once up front, and each platform arm overwrites only what it actually
read — the ioctl() return is checked, and a zero ws_col/ws_row counts as
"no terminal". That also collapsed the duplicated defaults, so the #else arm
is gone and 80/24 became TERMINAL_WIDTH_DEFAULT / TERMINAL_HEIGHT_DEFAULT.

[ejohnstown]

🟠 [Medium] CreateMode() bound check happens after the buffer has already been written

The new guard if (modeRet < 0 || (word32)modeRet > (word32)sizeof(mode)) ret = WS_FATAL_ERROR; runs after CreateMode() has already written into mode. CreateMode() still takes only byte* mode with no size parameter, and neither it nor CreateModeTermios() bounds its writes -- TTYWordSet/TTYSet/TTYCharSet advance idx blindly and the final mode[idx++] = WOLFSSH_TTY_OP_END; is unchecked. Today the worst case is roughly 55 entries * 5 bytes plus the terminator (~280 bytes) against a 4096-byte buffer, so nothing overflows, but the caller's check can only ever detect damage that has already been done. The check also encodes the buffer size in two places (the caller's sizeof(mode) and the callee's implicit assumption). Since the PR is specifically hardening this call, passing the size through would make the invariant enforceable rather than merely observable.

Fix: Add a modeMaxSz parameter to CreateMode()/CreateModeTermios() and bound the writes inside, keeping the caller's check as a belt-and-braces assertion. If that is considered out of scope for this fix, add a comment at the top of CreateMode() stating the maximum number of bytes it can emit so the 4096 buffer size is provably sufficient.

[yosuke-wolfssl]

Fixed at the source. Rather than add a size parameter, the limit is a shared
constant — all three setters are static and every call site writes the one
mode[] buffer:

    if (*idx + TERMINAL_MODE_SZ + 1 > TERMINAL_MODES_MAX_SZ)
        return;

In TTYWordSet() and TTYSet(), that covers all 58 writes (TTYCharSet()
delegates). The reserved byte leaves room for the terminator, whose write is
also guarded — CreateMode() returns WS_FATAL_ERROR if it won't fit, covering
the path where termRet was used directly as a write offset. A full buffer
truncates to a well-formed TTY_OP_END-terminated list. TERMINAL_MODES_MAX_SZ
replaces the bare 4096 in both the declaration and the test.

Negative control: with the guards deleted and the buffer shrunk to 8 bytes, ASan
reports stack-buffer-overflow ... in TTYWordSet; restored at the same size it
runs clean.

[ejohnstown]

🟠 [Medium] New test asserts packet shape but never that the default modes were used

The test's assertions on the encoded modes are purely structural: modeSz != 0, modeSz <= 4096, the modes fit inside the capture, and the last byte is WOLFSSH_TTY_OP_END. Nothing checks that the fallback content was produced. The fallback is fully deterministic -- TTYWordSet(38400, WOLFSSH_TTY_OP_ISPEED) + TTYWordSet(38400, WOLFSSH_TTY_OP_OSPEED) + terminator = exactly 11 bytes -- so it can be asserted exactly. As written, the only thing that would catch the termios path being taken by accident is the 256-byte cap on s_chanReqCapture (a real-tty packet is ~330 bytes and would trip the -812 bound check), which is an incidental property of the capture harness rather than a deliberate assertion. A future change that made CreateMode() emit different or partial default content would still pass.

Fix: Assert the exact fallback encoding (length 11, ISPEED/OSPEED opcodes, 38400 values) so the test pins the behavior the fix introduces, not just that the packet is well-formed.

[ejohnstown]

Concretely, the fallback is 11 deterministic bytes, so the assertion can be exact:

static const byte expectedModes[] = {
    WOLFSSH_TTY_OP_ISPEED, 0x00, 0x00, 0x96, 0x00,
    WOLFSSH_TTY_OP_OSPEED, 0x00, 0x00, 0x96, 0x00,
    WOLFSSH_TTY_OP_END
};

if (modeSz != (word32)sizeof(expectedModes) ||
        idx + modeSz > s_chanReqCaptureSz) { result = -812; goto done; }
if (WMEMCMP(s_chanReqCapture + idx, expectedModes, modeSz) != 0) {
    result = -813; goto done;
}

Three regressions the current assertions accept: (1) if CreateModeTermios() ever returned 0 rather than a negative, CreateMode() tests if (termRet < 0), so 0 takes the else, idx stays 0, both TTYWordSet() calls are skipped, and mode becomes just { WOLFSSH_TTY_OP_END } -- modeSz == 1 passes every check while the pty-req carries no modes at all; (2) a wrong opcode or baud (9600 for 38400) changes neither size nor terminator; (3) both paths writing leaves ~290 bytes, still under 4096. Also worth asserting typeSz == 7 and the "pty-req" bytes before trusting the offset walk.

[yosuke-wolfssl]

Adopted, including the exact 11-byte expectedModes[] compare and the
typeSz == 7 / "pty-req" check. Regression (1) from your follow-up —
CreateModeTermios() returning 0 and leaving a bare { TTY_OP_END } — is now
caught by the content compare rather than passing as modeSz == 1.

Two adjustments: the window assertion needed stdout redirected too (TIOCGWINSZ
reads STDOUT_FILENO), and bounds are written len > captureSz - idx instead of
idx + len > captureSz, closing the skipped word32 wrap finding.

[yosuke-wolfssl]

Hello @ejohnstown ,
Thank you for reviewing. I fixed the issue you mentioned.
Could you please review it again ?

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1130

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.