ESP32_Serial_Swiss_Army_Knife/docs/ssh_parser_remaining_review.md

Remaining SSH parser review — 2026-09-16

Completed scope and decision

This finite review is complete for the current pinned server profile: PR899 key skips and CHANNEL_FAILURE, PR902 service validation, PR918/919 forwarding callback applicability, the remaining generic signature-API caller question, and exact DoChannelRequest name dispatch including application callback gates. Three bounded current-path corrections are implemented. This is not a complete wolfSSH/wolfSSL audit, a full backport of these PRs, firmware validation, or Phase9 sign-off. It supplements the historical/deferred statements in wolf review, key-validation review and the parser contract.

Only tools/security_overrides.py, tests/wolfssh_parser_contract/*, and this report are owned by this slice. No ordering delta, crypto configuration, version, managed component, application, production generated file, PlatformIO or device change. Concurrent packaging/IDF-review work is unrelated and left untouched.

Reviewed item	Current-profile disposition
PR899 RSA/ECC unchecked key skips	Confirmed in the pinned client parsers, not reachable through current server dispatch. No speculative client patch.
PR899 CHANNEL_FAILURE length predicate	Reachable after authentication. Corrected with a bounded exact recipient parser, not just the upstream predicate change. Existing fatal failure policy retained.
PR902	Service names, not channel callbacks. Exact ssh-userauth required by current server handler. Client accept half unused/unmodified.
PR918/919	Forwarding global/channel callback handling; compiled out with WOLFSSH_FWD absent. No forwarding patch.
DoChannelRequest prefix/NUL name aliases	Closed: all nine name predicates require exact length then exact bytes; branch bodies and unknown-request handling preserved.
Generic signature API / PR10131 remaining question	Weak generic API remains, but no attacker-selected short digest/OID path in the inspected current SSH caller set. No crypto/API patch justified for this profile.

Exact source and upstream provenance

The authoritative source is wolfSSH 1.4.20 original + existing ordering delta + existing parser/password edits + the three corrections below, not installed source alone. render_entry verifies original SHA-256 and exact-once edit anchors. Tests render into temporary files; production generated inputs are not overwritten.

Input	SHA-256
Original managed_components/wolfssl__wolfssh/src/internal.c	81ff1f9166708abd5c2911e9fe57c0aee01c88b5d3f68c909ee8a856d37f36a9
Reviewed prior original+ordering+parser generated internal.c	4948f8c447670eb54153dd1f3db69e4fa3092f7d7f7ed58a18a8fa05fcd168ca
Fresh generated internal.c after this review	1fdd608d45c4f33da82b0765dc66e1ec2060e78c744bc539906ef1b8a0f783ae
Unchanged tools/wolfssh_order/delta.json	6a81376fe3ffc5f449cde105402963f52d2d78cc844153e869a7e1e0f734fb76
wolfSSL 5.8.2 wolfcrypt/src/signature.c	62ab3db3dfd251b2a2c73b69ef05aab6085d2e0d673fd9159514b3ee261cea4f

Fetched official PR patches and independently fetched their commit patches on 2026-09-16; each pair was byte-identical. Exact archives and URL/SHA-256 records are in tests/wolfssh_parser_contract/pr*.patch and provenance.json. The runner also pins those commits/hashes/URLs independently; no download occurs during tests or configuration.

• PR899 commit d2eeec5e263a4821c90805963eeb0666e99868a6: RSA algorithm skip, ECC curve skip, CHANNEL_FAILURE predicate; Windows file/terminal hunks unused on ESP.
• PR902 commit ffa646a4b9d47d5d9d6127db140c433c58b1e276: service request/accept name checks.
• PR918 commit fd82a4bcf55935f0801b14bca6be9c71e32ae914: global forwarding callback-before-reply, missing-callback rejection and cancel reply framing.
• PR919 commit 0317c40fc131fab952d291d43c56c7b7ce5f4303: reject direct-tcpip channel without forwarding callback.

review.py independently reverses only the new notice, the two initial handler changes and nine exact channel-name predicates, then requires the complete prior generated-source hash. The complete original DoChannelRequest is recovered by reversing just those predicates. Any other source change, including ordering, client parsing, request branch bodies, password wiping or crypto callers, fails this fence. Original/version/anchor guards remain; no silent repin or removal of existing strict build-source checks.

PR899: distinguish client host keys from server authentication

ParseRSAPubKey reads an unchecked algorithm-name length then adds it to its index. ParseECCPubKey does the same for the curve name. An oversized/wrapping length can invalidate the intended cursor progression. PR899 replaces these skips with GetSkip; it does not establish full curve-name/key-blob semantic validation. These are real dependency defects, not evidence of current server exploitation.

Exact static call chain: DoKexDhReply → ParsePubKey → ParseRSAPubKey or ParseECCPubKey. There is one call to ParsePubKey, in DoKexDhReply. The current server user-key verifier instead uses DoUserAuthRequestPublicKey → DoUserAuthRequestEcc/DoUserAuthRequestEd25519; it does not call either PR899 key parser. Application authorization additionally validates and exactly matches the stored key blob before successful proof-of-possession verification.

This exclusion is not just a server-role assertion:

1. src/ssh_transport.c creates only WOLFSSH_ENDPOINT_SERVER contexts.
2. Generated DoPacket checks IsMessageAllowed(..., WS_MSG_RECV) before dispatch.
3. The current two KEX choices set the server expectation to MSGID_KEXDH_INIT (30), then MSGID_NEWKEYS (21), never MSGID_KEXDH_REPLY (31) or GEX reply (33). Both initial KEX and rekey use those restrictions. Before peer KEXINIT or with no nonzero matching expectation, reply messages are rejected as well.
4. Both dispatch routes to DoKexDhReply (31 and 33) therefore fail the generated expectation gate. Disabled DH/GEX must not be confused with the compiled case labels. The full generated ordering suite was rerun, including wrong-message rejection and both KEX exchanges/rekeys.
5. Production macro replay confirms WOLFSSH_NO_RSA, WOLFSSH_NO_DH, and disabled certificates. Client code is not generally compile-disabled: ECC client parser safety depends on the role/ordering contract, not on dead-code assumptions.

Disposition: retain both key parsers byte-identical, fence the call chain and ordering source, and require this decision to be revisited before enabling client use or widening KEX. No standalone client parsing safety claim.

CHANNEL_FAILURE correction and behavior

The pinned DoChannelFailure did not read a recipient at all: len != 0 returned WS_BAD_ARGUMENT; an empty payload returned WS_CHANOPEN_FAILED. It did not have the out-of-bounds read implied by blindly treating it as the newer parser. But CHANNEL_FAILURE is a connection-protocol message allowed after authentication by the current server gate, so its malformed-input contract is relevant even though the application has no useful outstanding channel-request workflow needing it.

The local adaptation validates pointers, uses GetUint32 on a local cursor, requires exactly one remaining recipient field (begin == len), and verifies the recipient through ChannelFind(..., WS_CHANNEL_ID_SELF). Only then does it publish the cursor and return the existing WS_CHANOPEN_FAILED. Truncation/wrapping offsets and trailing data return WS_BUFFER_E; an unknown recipient returns WS_INVALID_CHANID. Failure leaves the caller index unchanged; no channel/session state is mutated. No new queue, allocation, retry or callback is introduced.

This does not turn CHANNEL_FAILURE into a recoverable reply or implement request correlation. Both old nonempty rejection and new parsed failure remain fatal to this application's worker path, which treats only its explicit would-block/receive statuses as retryable. The correction establishes bounded framing and the appropriate existing failure result, not an authentication-bypass or memory-corruption exploit fix. It is a local adaptation, not a full PR899 patch.

PR902 and present channel callback policy

The old bounded DoServiceRequest accepted any short service string and advanced to CLIENT_USERAUTH_REQUEST_DONE. PR902 really is applicable before user authentication: ordering permits SERVICE_REQUEST at ACCEPT_KEYED but does not validate its name. The generated handler now requires length 12 and exact bytes ssh-userauth; mismatch returns WS_INVALID_STATE_E before index/state publication. Length comparison short-circuits before the fixed-span comparison. Existing bounds and the strict name-capacity limit remain. Unlike upstream's later-tree patch, this subset does not queue a best-effort disconnect: the owner already closes on this error. Valid-service transition is unchanged. DoServiceAccept is unchanged; the current server gate rejects SERVICE_ACCEPT before dispatch.

For the channel/forwarding question:

• Actual Xtensa replay confirms WOLFSSH_FWD, WOLFSSH_AGENT, WOLFSSH_CERTS, WOLFSSH_SFTP, and WOLFSSH_SCP absent. PR918's DoGlobalRequestFwd call sites and PR919's direct-tcpip handling are under WOLFSSH_FWD. Unsupported forwarding channel types take the default unknown-type failure before channel allocation; global forwarding requests fall through to failure if a reply is requested.
• create_context registers shell, exec and subsystem callbacks, not a channel-open or global-request callback. Default session-channel acceptance is intentional: the pinned handler limits it to one channel, and auth ordering precedes it.
• Shell callback marks shell_requested; exec/subsystem callbacks reject. process_handshake additionally requires an authenticated/current principal, that flag and WOLFSSH_SESSION_SHELL before broker/admin routing. Callback rejection alone is not the whole policy: the library stores session type and completion state even for rejected requests, while the application gate stops exec/subsystem admission. The registered callbacks never execute commands.
• The pinned generic channel-open callback rejection path appends the channel even after callback failure; no callback is installed here, so that dormant path is not patched by this review. Revisit before adding one. Do not infer that PR919 repairs generic channel-open callbacks; its archived hunk is forwarding-only.

Completed follow-up: exact channel-request names

The concrete prefix issue is closed, without refactoring the request parser. All nine WSTRNCMP(type, literal, typeSz) == 0 predicates are replaced by typeSz == sizeof(literal) - 1 && WMEMCMP(type, literal, sizeof(literal) - 1) == 0. The length check short-circuits before any comparison on a short name. memcmp compares through embedded NULs instead of accepting a terminated prefix. Existing bounded GetString copies at most 31 bytes; every recognized name is shorter, so an oversized name truncated to 31 bytes cannot alias a recognized name. No new allocation, helper, protocol response, state transition or feature setting.

The full handler and application gates were rechecked, not just the shell branch:

Exact name	Existing branch / actual production gate
env	Parses two strings; no environment-setting callback. Always compiled.
shell	Sets shell session type, calls registered accept_shell, marks library completion. Application still requires the callback's shell_requested flag and shell session type.
exec, subsystem	Parse command, store their session type, call registered rejecting callbacks. Library completion is not application admission; no command is executed by these callbacks.
pty-req	Under WOLFSSH_TERM, present in production. Parses term/dimensions/modes; resize callback is optional and not installed by this application. Does not authorize a shell.
window-change	Requires both WOLFSSH_TERM and WOLFSSH_SHELL; absent because production has no WOLFSSH_SHELL. Remains on the unknown path in that profile.
exit-status, exit-signal	Under TERM or SHELL; present via TERM. Existing payload parsing preserved.
auth-agent-req@openssh.com	Under WOLFSSH_AGENT; absent. Optional enabled-branch comparison tested only in a host fixture, not enabled in firmware.

Empty names, proper prefixes, same-prefix suffixes, same-length wrong bytes, embedded NULs and overlong names no longer select any recognized branch. They take the unchanged unknown-request path: no branch callback/session-type update, consume the payload and return success (send channel success if requested). This deliberately does not introduce unknown-request rejection or strict trailing payload validation. Malformed header/name/boolean framing still fails before lookup/callback. Existing exec/subsystem behavior of calling their rejecting callbacks even after a command-payload parse error is also preserved; those real callbacks cannot execute commands. A prior accepted shell does not authorize a later exec/subsystem: the actual application session-type gate still rejects it, with or without a requested reply. No unauthenticated route is introduced.

channel_request.c executes actual generated helpers and the complete handler. channel_request.py separately hash-pins and extracts the real accept_shell, reject_channel_request and complete process_handshake bodies, checks their registration/context wiring, and executes them with platform/routing doubles. Tests exercise both broker/admin shell admission and rejection for missing callback context/flag, missing authentication/principal, stale principal, non-shell session and unsupported role. This is not a live broker/admin or task-lifecycle test.

Generic signature API: finite caller closure, not library closure

Rechecked exact pinned signature.c and generated SSH calls, supplementing the key-validation trace:

• wc_SignatureVerifyHash and wc_SignatureGenerateHash_ex reject zero sizes and invalid hash types but do not require the supplied hash length to equal the algorithm's digest length. That generic weakness remains; no global PR10131 backport or crypto configuration change is made.
• Current server ECC authentication is the sole enabled SSH wc_SignatureVerifyHash caller. DoUserAuthRequestPublicKey derives the digest size from HashForId(pkTypeId) and wc_HashGetDigestSize, checks errors, hashes locally, then passes it to DoUserAuthRequestEcc. Authorized P256 implies SHA256, 32 bytes. A peer signature field does not supply this digest length. The other SSH VerifyHash call is certificate-gated and absent.
• SignHEcdsa hashes exchange H locally using the negotiated P256 host-key hash and calls wc_ecc_sign_hash with the full 32-byte digest. Ed25519 authentication uses streamed message verification, not generic prehash verification.
• Both wc_SignatureVerify call sites are in blocked client DoKexDhReply; that wrapper also derives/hashes a full digest internally. Client ECC auth signing and certificate signing are not current server paths; agent signing is disabled. No application src/ call to generic signature generation/verification APIs or raw wc_ecc_sign_hash/wc_ecc_verify_hash was found outside these vendor paths.
• No wolfSSL TLS context/connect/accept use was found in application src/; HTTPS uses mbedTLS. This is application reachability evidence, not a claim that wolfSSL TLS or generic ASN/signature APIs are compiled out or fixed.

Closed question: no short-digest/OID-confusion trigger in this inspected current SSH caller set. Reopen on: certificate/client/agent enablement, new raw API callers, key/KEX widening or a changed authorization/hash construction. General wolfSSL TLS/ASN/API auditing remains outside this finite scope.

Validation and remaining handoff

Executed in this slice:

Command (all prefixed CCACHE_DISABLE=1)	Result
python3 tests/wolfssh_parser_contract/run.py	PASS: existing 3,258 cases × two stack modes / 11 mutations, plus channel matrix below / 20 additional rejected mutations; independent full-source/provenance fences.
python3 tests/wolfssh_parser_contract/review.py --profile	PASS: actual saved Xtensa feature replay and fresh-source syntax. Explicitly reports production input is the reviewed prior baseline.
python3 tests/wolfssh_auth_contract/run.py --host-only	PASS: 135 password/control-flow/wipe cases.
python3 tests/sdk_security_overrides/run.py	PASS: generator and CMake fixtures, including existing SDK corrections. No actual build-registration option used.
python3 tests/wolfssh_order_contract/run.py	PASS: 8,028 full-generated-source/real-crypto checks and seven rejected mutations. No OpenSSH interop option used.
python3 tests/wolf_crypto_policy/run.py --host-only (initial review, not rerun for name-only follow-up)	PASS: 20 guards, PUBLIC CMake fixture, real vendor small-math/P256/ASN vectors. No strict production crypto rerun.
python3 tests/ssh_protocol_policy/run.py (initial review; not rerun while build remains stale)	Blocked as expected: Generated wolfSSH source differs from render_entry; reconfigure the build. Its strict guard was not changed or bypassed.

Follow-up channel matrix: 2,737 cases per stack mode for production TERM-only, TERM+SHELL, and TERM+SHELL+AGENT profiles; 2,735 per stack mode for no-terminal and SHELL-only profiles. All five profiles run both stack modes with guard pages and UBSan trap instrumentation. The alternative features are host-only coverage, not production settings. Tests cover every proper prefix, valid names, appended bytes/NUL suffixes, every embedded-NUL/same-length wrong-byte position, 31–65-byte names, every packet/payload truncation, oversized/wrapping declared lengths, nonzero offsets, want-reply both ways, known/unknown channels, PTY callbacks and real application admission. Instrumented comparison asserts that the compared span equals the initialized name length. 18 name/length mutations and two real application shell-admission gate mutations are rejected, in addition to the existing 11 parser mutations. Ordering/auth/SDK suites and Xtensa profile/syntax were rerun successfully after the follow-up; git diff --check also passed.

The new parser tests use crypto/channel doubles; they establish dispatch/gating and preserved state contracts, not cryptographic arithmetic or actual channel lifetime. The ordering suite supplies separate full-library host evidence. No resource/timing, firmware link, device, broad fuzzing, new network SSH or Phase9 acceptance claim.

Remaining handoff is bounded:

1. Parent-approved regeneration/build and strict production-source suites after integrating concurrent work; production generated bytes are deliberately stale.
2. The requested channel-name dispatch and callback-gate review is complete. Existing unknown/trailing-payload behavior is explicitly preserved, not certified as a generally strict parser and not expanded into another parser inventory.
3. Existing whole-phase hardware/rekey cleanup, compatibility and heap/stack/CPU gates remain as recorded in the ordering/key-validation reviews. No new target cost measurement is claimed for these allocation-free checks.
4. Dormant client-key/forwarding/certificate/generic-API defects are documented profile exclusions, not fixed dependency features. Re-audit only if those capabilities or the pinned source/role/ordering contract change.