GUACAMOLE-306: Guard against NULL client in VNC size handler - #706
Open
denkovrov wants to merge 1 commit into
Open
GUACAMOLE-306: Guard against NULL client in VNC size handler#706denkovrov wants to merge 1 commit into
denkovrov wants to merge 1 commit into
Conversation
guac_vnc_user_size_handler() passes vnc_client->rfb_client directly into guac_vnc_display_set_size(), which dereferences it immediately via rfbClientGetClientData(). If the Guacamole "size" instruction arrives from the client before the VNC handshake completes and rfb_client is set, this is a NULL dereference. This is the same race already fixed by GUACAMOLE-306 for the mouse and keyboard handlers in the same file (input.c), both of which guard on rfb_client != NULL before use - just missed for this newer auto-resize handler, added later by GUACAMOLE-1196. Reproduced under load from many concurrent VNC connections opening at once, which measurably slows down handshake completion and makes the race far more likely to be hit (rare to nonexistent under normal single-connection use). Confirmed via gdb backtrace on a core dump, consistently faulting inside guac_vnc_display_set_size() at the same offset. The check is placed in guac_vnc_display_set_size() itself rather than only in guac_vnc_user_size_handler(), since it is the single choke point both of its callers go through; the other caller (guac_vnc_display_set_owner_size(), used for the initial post-handshake resize) always passes an already-valid client, so the added check is a no-op on that path.
Contributor
|
@denkovrov The GUACAMOLE-306 issue was marked as fixed several years ago and closed (it was fixed in version 0.9.13-incubating). You will need to open a new Jira issue for this and update the tagging appropriately. |
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
2 participants
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.
Summary
guac_vnc_user_size_handler()insrc/protocols/vnc/input.cpassesvnc_client->rfb_clientdirectly intoguac_vnc_display_set_size()(src/protocols/vnc/display.c), which dereferences it immediately viarfbClientGetClientData(). If the Guacamole "size" instruction (sent by the client as soon as the canvas opens) arrives before the VNC handshake completes andrfb_clientis set, this is a NULL pointer dereference and crashesguacd.This is the same class of bug as GUACAMOLE-306 ("VNC may segfault during the connection process"), which was fixed for the mouse and keyboard handlers in the same file (
input.c) — both already guard onrfb_client != NULLbefore use. The auto-resize handler was added later by GUACAMOLE-1196 and appears to have missed the same guard.Reproduction
Hit under load testing: opening many concurrent VNC connections measurably slows down handshake completion (creating the
rfbClient), widening the window during which an already-open browser tab's "size" instruction can race ahead of it. Under normal single-connection use the window is small enough to be rare to nonexistent; under concurrent connection load it reproduced reliably (segfault visible in kernel logs, consistent fault address/offset every time).A
gdb thread apply all bt fullbacktrace on a core dump traced the fault intoguac_vnc_display_set_size(), which is what led to this fix. I don't have that raw backtrace text on hand to attach to this PR, but wanted to flag it exists in case a maintainer would find it useful for confirming - happy to try to reproduce and re-capture it if that would help review.The fix itself has been validated by re-running the same concurrent-connection load test against a build with this patch applied: zero segfaults, versus a reliable reproduction on the same test without it.
Fix
Adds a NULL check at the top of
guac_vnc_display_set_size()rather than only in the size handler, since that function is the single choke point both of its callers go through. The other caller,guac_vnc_display_set_owner_size()(used for the initial post-handshake resize), always passes an already-valid client, so the added check is a no-op on that path and doesn't change existing behavior there.Notes
issues.apache.org(JIRA) appears to be closed, so I wasn't able to file a new ticket for this - this PR references GUACAMOLE-306 as the precedent for the same bug class already being fixed for the sibling handlers. Happy to have a committer file a proper follow-up ticket if that's preferred, or point me at the right way to request a JIRA account.1.6.0tag and currentmain, so this applies cleanly to either.