CWE-400: Uncontrolled Resource Consumption for sftp#1017
Conversation
Signed-off-by: Loganaden Velvindron <logan@cyberstorm.mu>
|
Can one of the admins verify this patch? |
|
Can we treat this as a bug report and fix it another way? Or would you fill out a contributor agreement? The issue is I'm removing the functions this modifies. I have another spot something like this can be added. |
|
@ejohnstown sure. I can fill in the CLA. |
|
I'll need you to send an email to support@wolfssl.com. Please include this PR number and your github username. Say that you need a contributor agreement. They will set you up. Note, this code won't apply any more after one of my PRs is merged. Would you mind porting this over when mine is merged? |
|
Sure. I've sent the request for CLA. |
|
CLA signed. |
|
@ejohnstown can I push now ? |
Hi @loganaden you are welcome to push code all you want. The CLA will be reviewed internally and once approved we will let you know. But don't let it stop you for pushing changes to your PR's or opening new ones. Thank you |
|
@loganaden, the other PR has been merged. Your PR needs some conflict resolution. Your CLA needs to move through our system. Once both are complete, I can merge this. Thank you for the fix! |
|
@ejohnstown CLA signed. Working on updating the PR. |
aidangarske
left a comment
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: REQUEST_CHANGES
Findings: 5 total — 5 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] [review] Handle cap is enforced only after opening the filesystem object —
src/wolfsftp.c:4681-4684 - [Medium] [review+review-security] New SFTP handle cap has no test coverage —
src/wolfsftp.c:4661-4685 - [Medium] [review+review-security] Handle cap covers file handles only; directory OPENDIR path remains unbounded —
src/wolfsftp.c:4661-4685 - [Low] [review] Cap constant defined mid-source instead of with other SFTP limits in the header —
src/wolfsftp.c:4661-4663 - [Low] [review] WS_MEMORY_E reused for resource-limit-reached case —
src/wolfsftp.c:4682-4684
Review generated by Skoll
| return WS_BUFFER_E; | ||
| } | ||
|
|
||
| for (cur = ssh->handleList; cur != NULL; cur = cur->next) { |
There was a problem hiding this comment.
🔴 [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.
| } | ||
|
|
||
|
|
||
| #ifndef WOLFSSH_MAX_SFTP_HANDLES |
There was a problem hiding this comment.
🟠 [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.
| } | ||
|
|
||
|
|
||
| #ifndef WOLFSSH_MAX_SFTP_HANDLES |
There was a problem hiding this comment.
🟠 [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.
| } | ||
|
|
||
|
|
||
| #ifndef WOLFSSH_MAX_SFTP_HANDLES |
There was a problem hiding this comment.
🔵 [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.
| } | ||
|
|
||
| for (cur = ssh->handleList; cur != NULL; cur = cur->next) { | ||
| if (++count >= WOLFSSH_MAX_SFTP_HANDLES) { |
There was a problem hiding this comment.
🔵 [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.
5 participants
Enforce a per-session cap on open SFTP file handles before accepting additional retained handles. When the cap is reached, the server must reject the new handle and must not retain the newly opened file descriptor.