Skip to content

fix: correct DoChannelData dataSz bounds check #1090

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

Closed
MarkAtwood wants to merge 1 commit into from
Closed

fix: correct DoChannelData dataSz bounds check #1090

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
2 changes: 1 addition & 1 deletion src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11090,7 +11090,7 @@ static int DoChannelData(WOLFSSH* ssh,

/* Validate dataSz */
if (ret == WS_SUCCESS) {
if (len < begin) {
if (dataSz > len - begin) {

@aidangarske aidangarske Jul 10, 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 dataSz guard remains unreachable after GetSize
💡 SUGGEST bug

The PR replaces the dead len < begin check with dataSz > len - begin, but dataSz was just read by GetSize(&dataSz, buf, len, &begin). GetSize() already returns non-success when *v > len - *idx, and begin is that same idx. Therefore, whenever execution reaches this new ret == WS_SUCCESS block, dataSz <= len - begin is already guaranteed. Short channel-data payloads still return WS_BUFFER_E from GetSize() before this branch can run, so the changed code does not add the stated WS_RECV_OVERFLOW_E validation path and remains dead validation logic.

Recommendation: Clarify the intended error behavior and add a regression test for a truncated MSGID_CHANNEL_DATA payload before treating this as a completed bounds-check fix. If the intent is only to rely on existing bounds checking, remove the redundant validation block and point the comment at GetSize(). If the intent is to return WS_RECV_OVERFLOW_E specifically for an over-declared channel-data payload, parse the length with GetUint32() here and keep the explicit dataSz > len - begin check, plus a unit case where dataSz exceeds the remaining payload length.

ret = WS_RECV_OVERFLOW_E;
}
Comment on lines 11092 to 11095
}
Expand Down
Loading