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.
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
Fix issues with boundary access. #6019
Conversation
1237662
to
6b2bc41
Compare
|
Refer to this link for build results (access rights to CI server needed): |
thanks to @hardening for pointing that one out.
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| @@ -2161,7 +2161,7 @@ static CACHE_BITMAP_V3_ORDER* update_read_cache_bitmap_v3_order(rdpUpdate* updat | |||
| Stream_Read_UINT16(s, bitmapData->height); /* height (2 bytes) */ | |||
| Stream_Read_UINT32(s, new_len); /* length (4 bytes) */ | |||
|
|
|||
| if (Stream_GetRemainingLength(s) < new_len) | |||
| if ((new_len == 0) || (Stream_GetRemainingLength(s) < new_len)) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might misinterpret the spec but a length of 0 could be possible (2.2.9.2.1.1 Extended Bitmap Data ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe in theory, but an empty bitmap could break a lot later on and does not make much sense.
| *channel_id = 0; | ||
| *length = 8; /* Flow control PDU is 8 bytes */ | ||
| return TRUE; | ||
| } | ||
|
|
||
| if (((size_t)*length - 2) > Stream_GetRemainingLength(s)) | ||
| if ((len < 4) || ((len - 2) > Stream_GetRemainingLength(s))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for the len < 4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the header length required (the 2 bytes length itself and 2 bytes type)
| { | ||
| WLog_ERR(TAG, | ||
| "incorrect offset, type:0x%04" PRIX16 " actual:%" PRIuz " expected:%" PRIuz "", | ||
| type, Stream_Pointer(s) - bm, em - bm); | ||
| type, length + rest, length); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: here instead of printing the offset which is useless we could just show the remaining bytes...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what @hardening wrote, just show how many bytes were not handled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already done in latest commit.
| @@ -1384,7 +1393,7 @@ static int rdp_recv_tpkt_pdu(rdpRdp* rdp, wStream* s) | |||
| "pduType %s not properly parsed, %" PRIdz | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you could include my patch (gist sent on IRC), as it fixes the fact that you can announce a small pduLength but decoding functions will be able to read more than pduLength (because they do checks with Stream_GetRemainingLength(mainStream))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some remark, but LGTM, nice job.
The code is so much simpler when using substreams !
|
@hardening will address your comments in a separate PR, I think better merge this now as it is already big enough. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
fd21d24
to
6f00add
Compare
|
Refer to this link for build results (access rights to CI server needed): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice job
No description provided.