Security fixes #921
Merged
Security fixes #921
Conversation
This file contains 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
Move the checks around to avoid missing cases where we might access memory that is no longer valid. Also avoid touching the underlying stream implicitly (e.g. via the destructor) as it might also no longer be valid. A malicious server could theoretically use this for remote code execution in the client. Issue found by Pavel Cheremushkin from Kaspersky Lab
Don't allow subclasses to just override dimensions or buffer details directly and instead force them to go via methods. This allows us to do sanity checks on the new values and catch bugs and attacks.
We do a lot of calculations based on pixel coordinates and we need to make sure they do not overflow. Restrict the maximum dimensions we support rather than try to switch over all calculations to use 64 bit integers. This prevents attackers from from injecting code by specifying a huge framebuffer size and relying on the values overflowing to access invalid areas of the heap. This primarily affects the client which gets both the screen dimensions and the pixel contents from the remote side. But the server might also be affected as a client can adjust the screen dimensions, as can applications inside the session. Issue found by Pavel Cheremushkin from Kaspersky Lab.
No one should every try to write to this buffer. Enforce that by throwing an exception if any one tries to get a writeable pointer to the data.
We always assumed there would be one pixel per row so a rect with a zero width would result in us writing to unknown memory. This could theoretically be used by a malicious server to inject code in to the viewer process. Issue found by Pavel Cheremushkin from Kaspersky Lab.
Otherwise we might be tricked in to reading and writing things at incorrect offsets for pixels which ultimately could result in an attacker writing things to the stack or heap and executing things they shouldn't. This only affects the server as the client never uses the pixel format suggested by th server. Issue found by Pavel Cheremushkin from Kaspersky Lab.
Provides safety against them accidentally becoming negative because of bugs in the calculations. Also does the same to CharArray and friends as they were strongly connection to the stream objects.
We use a lot of lengths given to us over the network, so be more paranoid about them causing an overflow as otherwise an attacker might trick us in to overwriting other memory. This primarily affects the client which often gets lengths from the server, but there are also some scenarios where the server might theoretically be vulnerable. Issue found by Pavel Cheremushkin from Kaspersky Lab.
Our fast paths assume that each channel fits in to a separate byte. That means the shift needs to be a multiple of 8. Start actually checking this so that a client cannot trip us up and possibly cause incorrect code exection. Issue found by Pavel Cheremushkin from Kaspersky Lab.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes a number of security issues in various places. They are all on the theme of the remote peer sending us large values in order to mess up calculations and result in unwanted memory access.
Some of these issue affect the server, some affect the client. All issues require a successfully authenticated connection though.
None of the issue currently have a working exploit, so it is unknown how difficult it would be to actually put together an attack using these.
A big thanks to Kaspersky Lab who provided us with details about these issues.