Skip to content

CWE-400: Uncontrolled Resource Consumption for sftp #1017

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
loganaden wants to merge 1 commit into
base: master
Choose a base branch
from
Open

CWE-400: Uncontrolled Resource Consumption for sftp #1017

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/wolfsftp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4658,11 +4658,16 @@ int wolfSSH_SFTP_RecvRename(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
}


#ifndef WOLFSSH_MAX_SFTP_HANDLES

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [Medium] New SFTP handle cap has no test coverage · Missing Tests

Consensus across the review and review-security scans (review rated Medium/SUGGEST; review-security rated Info; stricter Medium kept). The new cap branch (if (++count >= WOLFSSH_MAX_SFTP_HANDLES) return WS_MEMORY_E;) is not exercised by any test. The cap resolves to exactly 64 concurrent handles (<=63 existing -> add succeeds, >=64 -> reject). Existing regression coverage in tests/regress.c only adds one or two handles and already unit-tests SFTP_AddHandleNode directly (TestSftpRemoveHandleHeadUpdate, TestSftpValidateFileHandle), so a targeted boundary test is low-cost. No test fills the list to WOLFSSH_MAX_SFTP_HANDLES, verifies the max boundary, or checks that the rejected handle is not retained. This is exactly the off-by-one and cleanup-sensitive path that should have direct coverage; without it a future regression that off-by-ones the limit or drops the check would pass CI silently.

Fix: Add a regress.c test that inserts WOLFSSH_MAX_SFTP_HANDLES unique handles (asserting WS_SUCCESS each time), asserts the next SFTP_AddHandleNode returns WS_MEMORY_E, verifies the rejected handle is absent and the list count stays capped, then removes one and confirms a subsequent add succeeds again. An OPEN-level test with a small configured cap driving wolfSSH_SFTP_RecvOpen would also confirm the newly opened descriptor is closed on the cap-failure path and catch the pre-open side-effect bug.

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [Medium] Handle cap covers file handles only; directory OPENDIR path remains unbounded · Security

Consensus across the review and review-security scans (review rated Medium/SUGGEST; review-security rated Info; stricter Medium kept). The cap only bounds file handles in ssh->handleList. Directory handles opened via wolfSSH_SFTP_RecvOpenDir are tracked in the separate, uncapped ssh->dirList (WS_DIR_LIST): verified at src/wolfsftp.c:2731-2758 (and inserts at 2757-2758 / 2917-2918), that function WMALLOCs a new node and prepends it with no count check and never routes through SFTP_AddHandleNode. An attacker can therefore still drive unbounded memory/descriptor consumption by repeatedly sending WOLFSSH_FTP_OPENDIR without a matching CLOSE — the same CWE-400 resource-exhaustion class this PR targets. This is a pre-existing gap (not introduced by this PR); may be intentionally out of scope per the PR description ("open SFTP file handles"), so raised as a question.

Fix: Confirm whether directory handles are in scope; if so, apply an analogous per-session count/limit to ssh->dirList inside wolfSSH_SFTP_RecvOpenDir (or a combined open-handle budget) before retaining a new WS_DIR_LIST node, so the DoS mitigation is complete.

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] Cap constant defined mid-source instead of with other SFTP limits in the header · convention

WOLFSSH_MAX_SFTP_HANDLES is defined mid-.c-file, immediately above the function (verified at src/wolfsftp.c:4661-4663). The codebase convention places related SFTP limits in the public header — WOLFSSH_MAX_HANDLE lives in wolfssh/wolfsftp.h and WOLFSSH_MAX_FILENAME in wolfssh/ssh.h/port.h. Defining the tunable in wolfssh/wolfsftp.h (near WOLFSSH_MAX_HANDLE) makes it discoverable and configurable through the normal header/user_settings path. The #ifndef guard does still allow -DWOLFSSH_MAX_SFTP_HANDLES=... overrides, so this is placement/discoverability only.

Fix: Relocate the #ifndef WOLFSSH_MAX_SFTP_HANDLES / #define ... 64 / #endif block into wolfssh/wolfsftp.h alongside WOLFSSH_MAX_HANDLE for consistency and discoverability.

#define WOLFSSH_MAX_SFTP_HANDLES 64
#endif

/* add a name and handle to the handle list
* return WS_SUCCESS on success */
int SFTP_AddHandleNode(WOLFSSH* ssh, byte* handle, word32 handleSz, const char* name)
{
WS_HANDLE_LIST* cur;
word32 count = 0;
int sz;

if (ssh == NULL || handle == NULL || name == NULL) {
Expand All @@ -4673,6 +4678,12 @@ int SFTP_AddHandleNode(WOLFSSH* ssh, byte* handle, word32 handleSz, const char*
return WS_BUFFER_E;
}

for (cur = ssh->handleList; cur != NULL; cur = cur->next) {

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 [High] Handle cap is enforced only after opening the filesystem object · bug

The new cap rejection happens inside SFTP_AddHandleNode, but both wolfSSH_SFTP_RecvOpen paths call the platform open routine first: POSIX WOPEN at src/wolfsftp.c:2402 and Windows WS_CreateFileA at src/wolfsftp.c:2578, with SFTP_AddHandleNode only invoked afterward at src/wolfsftp.c:2420. Once the session is at WOLFSSH_MAX_SFTP_HANDLES, a client can send an OPEN with create/truncate flags; the server opens, creates, or truncates the file, then this new check rejects the handle and cleanup closes it. The descriptor is not retained, but the rejected open still has filesystem side effects. This PR turns what used to be a rare post-open allocation-failure path into the normal deterministic behavior at the configured cap. Verified against source: WOPEN at line 2402 precedes the SFTP_AddHandleNode cap check reached at line 2420. Note: the review-security scan reviewed this same reject path and concluded the descriptor is not leaked (cleanup closes fd and skips SFTP_RemoveHandleNode); the concern here is the pre-open filesystem side effect, not a leak.

Fix: Add a preflight capacity check before WOPEN / WS_CreateFileA in wolfSSH_SFTP_RecvOpen, and keep the SFTP_AddHandleNode check as a backstop. If the cap is already reached, build the SFTP failure response without calling the filesystem open routine so rejected opens do not create, truncate, or otherwise modify files.

if (++count >= WOLFSSH_MAX_SFTP_HANDLES) {

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] WS_MEMORY_E reused for resource-limit-reached case · style

Returning WS_MEMORY_E when the handle cap is hit conflates a policy/limit rejection with an allocation failure. Callers treat any non-success generically ("Unable to store handle" -> FXP FAILURE), so behavior is correct, but the error semantics are slightly misleading for anyone debugging or reading logs. There is no obvious dedicated error code today, so this is a note rather than a defect.

Fix: Consider a distinct code (e.g. a WS_RESOURCE/limit error) if one exists or is worth adding; otherwise leave WS_MEMORY_E and note the intent.

return WS_MEMORY_E;
}
}

cur = (WS_HANDLE_LIST*)WMALLOC(sizeof(WS_HANDLE_LIST), ssh->ctx->heap,
DYNTYPE_SFTP);
if (cur == NULL) {
Expand Down
Loading