GUACAMOLE-2262: Heap-allocate clipboard buffers to prevent stack overflow.

[escra]

Summary

The clipboard receive and send buffers in cliprdr.c were allocated on the
stack using GUAC_COMMON_CLIPBOARD_MAX_LENGTH. Since configurable clipboard
limits (GUACAMOLE-2002)
raised this to 50 MiB while the default thread stack is only 8 MiB, large
clipboard operations cause a stack overflow (SIGSEGV) that silently kills the
connection.

Root cause

guac_rdp_cliprdr_format_data_response() declares:
c char received_data[GUAC_COMMON_CLIPBOARD_MAX_LENGTH];
With GUAC_COMMON_CLIPBOARD_MAX_LENGTH at 50 MiB (52428800 bytes) and the
default pthread stack size at 8 MiB, this overflows the stack immediately.

Similarly, guac_rdp_cliprdr_format_data_request() heap-allocates the output
buffer using guac_mem_alloc(GUAC_COMMON_CLIPBOARD_MAX_LENGTH) but could use
the actual configured clipboard size instead.

Fix

• Heap-allocate both buffers using guac_mem_alloc() with the actual configured
  clipboard size (clipboard->clipboard->available) instead of the compile-time
  maximum
• Free the buffers on all exit paths (including the early return for unsupported
  formats)

Affected versions

• staging/1.6.1 and main - any build with configurable clipboard limits
  (merged via PR GUACAMOLE-2002: Allow connection clipboard limits to be configured. #564)
• Not affected: 1.5.5 and earlier (256 KiB fixed limit fits on stack)

JIRA

GUACAMOLE-2002 - this is
a regression from the configurable clipboard limits feature.

Test plan

• Configure a connection with clipboard limit > 8 MiB
• Copy large text to clipboard on RDP server, paste in browser
• Copy large text in browser, paste on RDP server
• Verify no SIGSEGV / silent disconnection
• Verify normal clipboard operations still work with default settings

[necouchman]

@mike-jumper Any drawback that you can think of to allocating clipboard buffers based on the (dynamic) output buffer size rather than the (static) maximum length?

[eugen-keeper]

@escra
Thank you for the fix.
Anyway, wouldn't it be better here to have something like int output_buf_size = clipboard->clipboard->available * 2?
If format_data_request->requestedFormatId == CF_UNICODETEXT, then one byte character may produce two byte UTF-16.

[escra]

Good catch. You're right, available alone doesn't account for the encoding expansion when converting to UTF-16.

Worst case with CRLF normalization enabled is 4x (a single \n becomes \r\n in UTF-16 = 4 output bytes for 1 input byte). Without CRLF it's 2x as you noted. guac_iconv() won't overflow the buffer, it just stops writing, so this causes truncation rather than corruption. The original fixed-size buffer had the same limitation.

I'll update to use available * 4 which covers all format + CRLF combinations.

[bbennett-ks]

FYI I made a VNC clipboard change from stack to heap based allocation in a PR (#669). I came across a downstream merge conflict which has lead me to this thread.

I had a thought, re: Clipboard size: the UI (and the comment for GUAC_COMMON_CLIPBOARD_MAX_LENGTH) does specify the size in bytes, not # of characters. Also, worst case UTF-8 (and UTF-16) can be as much as 4 bytes. e.g. emojis. With a maximum of 50MB clipboard size, *4 is getting pretty big. Good news, though, is that for such large allocations RAM is only mapped for the address ranges actually accessed.