Commander1024/ESP32_Serial_Swiss_Army_Knife
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Harden SSH parsing and add notice tooling

Browse Source 
- Enforce exact service and channel names with bounded failure parsing
- Add hash-pinned offline notice assembly and regression coverage
- Record advisory dispositions, provenance, integration evidence, and
  remaining gates
This commit is contained in:
Commander1024
2026-09-16 15:06:38 +02:00
parent bea33e1c95
commit 51f835c22f
29 changed files with 3332 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/ssh_parser_remaining_review.md
+276
View File
@@ -0,0 +1,276 @@
# 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](wolf_security_review.md), [key-validation review](ssh_key_validation_review.md)
and the [parser contract](../tests/wolfssh_parser_contract/README.md).
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](https://github.com/wolfSSL/wolfssh/commit/d2eeec5e263a4821c90805963eeb0666e99868a6): RSA algorithm skip, ECC curve skip, CHANNEL_FAILURE predicate; Windows file/terminal hunks unused on ESP.
- [PR902 commit ffa646a4b9d47d5d9d6127db140c433c58b1e276](https://github.com/wolfSSL/wolfssh/commit/ffa646a4b9d47d5d9d6127db140c433c58b1e276): service request/accept name checks.
- [PR918 commit fd82a4bcf55935f0801b14bca6be9c71e32ae914](https://github.com/wolfSSL/wolfssh/commit/fd82a4bcf55935f0801b14bca6be9c71e32ae914): global forwarding callback-before-reply, missing-callback rejection and cancel reply framing.
- [PR919 commit 0317c40fc131fab952d291d43c56c7b7ce5f4303](https://github.com/wolfSSL/wolfssh/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](ssh_key_validation_review.md#raw-signatures-and-cve-2026-5194-applicability):
- `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.