TightDecoder: bound rectangle area to prevent OOM from a malicious server - #2132
Open
acts-1631 wants to merge 1 commit into
Open
TightDecoder: bound rectangle area to prevent OOM from a malicious server#2132acts-1631 wants to merge 1 commit into
acts-1631 wants to merge 1 commit into
Conversation
…rver readRect() only capped the rectangle width (TIGHT_MAX_WIDTH), not its height, which arrives on the wire as an unbounded uint16. A malicious server can advertise a large framebuffer in ServerInit and then send a single Tight rectangle up to 2048x65535, causing decodeRect() to allocate a ~537MB zlib output buffer and, when the pixel format needs translation, another ~537MB translation buffer for the same rectangle. A simple height cap equal to TIGHT_MAX_WIDTH would reject rectangles that a compliant server can legitimately send: EncodeManager only splits a changed rectangle once its area exceeds SubRectMaxArea (65536), so a narrow, tall rectangle (e.g. a thin scrollbar or resize handle on a display or monitor stack taller than 2048px) can be sent unsplit with a height well above 2048. Enforce the same area bound the encoder already honours instead of an arbitrary height limit. This keeps the worst-case allocation to TIGHT_MAX_AREA * bpp (~256KB) while accepting every rectangle a compliant encoder can produce.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
readRect() only caps the rectangle width (TIGHT_MAX_WIDTH), not its
height, which arrives on the wire as an unbounded uint16. A malicious
server can advertise a large framebuffer in ServerInit and then send
a single Tight rectangle up to 2048x65535, causing decodeRect() to
allocate a ~537MB zlib output buffer and, when the pixel format needs
translation, another ~537MB translation buffer for the same
rectangle.
A flat height cap equal to TIGHT_MAX_WIDTH would reject rectangles
that a compliant server can legitimately send: EncodeManager only
splits a changed rectangle once its area exceeds SubRectMaxArea
(65536), so a narrow, tall rectangle (a thin scrollbar or resize
handle on a display or monitor stack taller than 2048px) can be sent
unsplit with a height well above 2048.
This enforces the same area bound the encoder already honours
instead of an arbitrary height limit, keeping the worst-case
allocation to TIGHT_MAX_AREA * bpp (~256KB) while still accepting
every rectangle a compliant encoder can produce.