Repository navigation
Check public API arguments and caller buffers - #1368
ejohnstown wants to merge 6 commits into
Conversation
wolfSSH_SendIgnore() carries buf in the SSH_MSG_IGNORE it sends. A NULL buf with a zero bufSz still sends 128 zero bytes, and a NULL buf with a nonzero size is WS_BAD_ARGUMENT. So is a payload whose packet, with worst-case padding and MAC, would pass the peer's MAX_PACKET_SZ check; this also keeps the PreparePacket() size sums from wrapping. - internal: MAX_IGNORE_DATA_SZ bounds the payload like the peer check - regress: check the payload and the NULL default on the wire - regress: 0xFFFFFFFB and one over the bound fail cleanly - regress: the largest payload reaches a peer, plain and keyed, with the byte highwater off so a low DEFAULT_HIGHWATER_MARK cannot rekey Issue: F-7512
DoAsn1Key() encodes a public key into the caller's *out when one is supplied, as the private-key path already does, and returns WS_BUFFER_E when *outSz is too small. It allocates only when *out is NULL. - GetPubKeyOut() picks the buffer for every public-key encoder - api: RSA, ECC, Ed25519 and ML-DSA-44 reads into a caller buffer, and one byte short Issue: F-11652
wolfSSH_SetTerminalResizeCb() and wolfSSH_SetTerminalResizeCtx() do nothing for a NULL session, like the other session setters. - api: NULL and valid session through both setters Issue: F-17101
wolfSSH_GetText() builds the text locally and copies it into str, so str is always terminated and the return is the full text length, whichever function WSNPRINTF maps to. A return of strSz or more means truncation; an unknown id or unnegotiated KEX gives "" and 0. - echoservers: test truncation with ret >= strSz - api: empty, exact, truncated, and DH texts Issue: F-17102
There was a problem hiding this comment.
🟡 Changes recommended
The API test uses a shared fixed directory path, allowing concurrent runs to interfere with creation and cleanup and cause assertion failures.
1 open issue
What changed in this PR
This PR corrects public API argument and buffer handling in wolfSSH, with regression tests and updated examples.
Changes:
- Honor caller-provided IGNORE payloads, public-key buffers, and SFTP directory attributes.
- Make terminal resize setters NULL-safe and standardize text termination and truncation reporting.
- Add API and packet-boundary tests, update documentation and examples, and remove the unused internal path helper.
| File | Description |
|---|---|
wolfssh/wolfsftp.h |
Documents MKDIR attribute defaults. |
wolfssh/ssh.h |
Clarifies text output and IGNORE payload contracts. |
wolfssh/internal.h |
Defines the IGNORE payload limit and removes the unused path declaration. |
tests/regress.c |
Tests IGNORE payloads and plain/keyed packet limits. |
tests/api.c |
Tests caller buffers, NULL setters, text output, and MKDIR attributes. |
src/wolfsftp.c |
Serializes caller-supplied MKDIR attributes. |
src/ssh.c |
Fixes payload, buffer, terminal setter, and text handling. |
src/internal.c |
Bounds IGNORE payloads and removes the unused path helper. |
ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c |
Corrects text truncation detection. |
examples/sftpclient/sftpclient.c |
Requests default MKDIR attributes explicitly. |
examples/echoserver/echoserver.c |
Corrects text truncation detection. |
Review effort: Balanced
wolfSSH_SFTP_MKDIR() encodes atr with SFTP_SetAttributes(), as the other attribute-carrying requests do. A NULL atr requests permissions 0777, the request every MKDIR used to send. - sftpclient: pass NULL instead of an uninitialized atr - api: a 0700 request makes a 0700 directory on the echoserver Issue: F-14634
wolfSSH_GetPath() has no callers and is WOLFSSH_LOCAL, so it is not part of the API. Issue: F-8812
be29403 to
a052000
Compare
|
@claude review |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1368
Scan targets checked: wolfssh-src, wolfssh-bugs
Not reviewed (no scan target covers them): ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c
Coverage: 6 of 10 in-scope changed file(s) opened by the reviewer; not opened: examples/echoserver/echoserver.c, examples/sftpclient/sftpclient.c, wolfssh/ssh.h, wolfssh/wolfsftp.h
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It changes public API behaviour in an SSH library: key encoding into caller buffers, SSH_MSG_IGNORE packet sizing and the SFTP MKDIR wire format. Because of that, a maintainer should still take a look.
What was reviewed:
DoAsn1Key/GetPubKeyOut: the caller buffer is checked against the computed key size before any write,*outSzis set only on success, and the cleanup frees only memory it allocated (*out == NULL). This matches the existing private-key path.wolfSSH_GetText: thestrSz == 0and NULLstrcases return early, so thestr[strSz - 1]truncation can't underflow.dhText[80]fits the longest DH format.SendIgnorenow checksMAX_IGNORE_DATA_SZbeforePreparePacket. InwolfSSH_SFTP_MKDIR, the buffer and header lengths both useUINT32_SZ + atrSz, and a NULLatrkeeps the 0777 default.
Extended reasoning...
The change touches 11 files in wolfSSH: key parsing in src/ssh.c, the internal SendIgnore size limit, SFTP MKDIR serialization, removal of the unused wolfSSH_GetPath, plus about 500 lines of new tests. It touches security-sensitive surface: buffer handling for ASN.1 public key output, the size limit on outgoing transport packets, and a changed SFTP request encoding. A Copilot inline comment on tests/api.c was resolved by the PR author, not by an independent reviewer. A human should review because this is a security library and the public API contract for caller buffers changes.
Six small fixes to public API functions that ignored or misused their
arguments.
packet passes the peer's MAX_PACKET_SZ check (34,907 bytes by default)
WS_BUFFER_E when it is too small
Issue: F-7512, F-11652, F-17101, F-17102, F-14634, F-8812