Restricting the amount of clipboard data copying from the server clipboard in/out of the client

[Calvin Ko]

The goal is to restrict the amount of data being transferred from server in the client (or vice versa - out) through clipboard copy/paste operation.

Two options specifying the max size

1. max_clipboard_copyin_size (into client - raw to wire)
2. max_clipboard_copyout_size (out from client - wire to raw)

In addition, notify the client when the data is truncated.

[Antoine Martin]

Please attach the patch.

[Calvin Ko]

attached the patch

[Antoine Martin]

improved patch

[Antoine Martin]

The updated patch changes:

• makes the default send and receive limits configurable via env vars
• indentation tweaks
• simplify max datalen calculation and make it python3 friendly: use integer division

@calvinko: this modifies a private clipboard method, please include the changes that call it.

[Calvin Ko]

patch with control command to set the size limits

[Calvin Ko]

The update patch - clipboard-size-v2.patch

• add the private function set_limits in clipboard proxy for changing the limits
• add the control command clipboard_limits for changing the limits

[Antoine Martin]

OK, some minor changes are still needed:

• it should be possible to go back to the default values (-1 and -1), so use None default values for max_send_size and max_receive_size in set_direction and check for None in set_limits - then you can just do xpra control :100 clipboard-limits -1 -1
• have you tested the control command? unless I am missing something, without input validation the control values will be strings and then this breaks badly, in python2 the size check fails silently (removing the restrictions):

  > 3 > "1"
  False
  

with python3 you probably get an error in the clipboard code on every request (which may be worse as this is known to cause GTK to hang)

• in control_command_clipboard_limits, you alias the clipboard helper to a local variable (good), but then you don't use it
• the setting_changed method takes native values, not strings, either send a pair of values or a dictionary

[Calvin Ko]

address the minor changes stated in comment:5

[Antoine Martin]

As per comment:5:

use None default values for max_send_size and max_receive_size in set_direction

Otherwise regular clipboard clients, which call set_direction without the extra arguments will end up removing the size restrictions.

IMO max_send in setting_changed is redundant and should be called send

[Calvin Ko]

address comment:6

[Antoine Martin]

Did you test this? Ideally this feature should be added to the unit tests.

I found at least two major problems during my quick testing:

• r21202 fixes a long standing bug that is uncovered by the extra "truncated" attribute in the "clipboard-contents" packets - do you really need this? (it will create backwards compatibility issues with older versions - as some users just don't update as often as they should..)
• the checks assume that a limit is always set:

  2018-12-11 13:56:49,818 Data copied out truncated because of clipboard policy 1 to -1
  

And this ends up truncating all clipboard packets by 1 character.

---

How I tested:

• xpra start then xpra info:

  xpra start :100
  

  $ xpra info | grep clipboard | grep max_
  clipboard.max_recv_size=-1
  clipboard.max_send_size=-1
  clipboard.max_size=4194304
  

• change it via control channel

  $ xpra control :13 clipboard-limits 8 8
  clipboard send limit set to 8, recv limit set to 8 (single copy/paste)
  

  $ xpra info | grep clipboard | grep max_
  clipboard.max_recv_size=8
  clipboard.max_send_size=8
  clipboard.max_size=4194304
  

• same with env vars:

  XPRA_MAX_CLIPBOARD_RECEIVE_SIZE=10 XPRA_MAX_CLIPBOARD_SEND_SIZE=10 xpra start
  

  $ xpra info | grep clipboard | grep max_
  clipboard.max_recv_size=10
  clipboard.max_send_size=10
  clipboard.max_size=4194304
  

Then check that the client does receive a truncated string:

• from the server:

  echo hello12345 | xclip -i -selection CLIPBOARD
  

• on the client:

  xclip -o -selection CLIPBOARD
  

Then the other way (client to server) - preferably after changing the limits to ensure that send and receive limits are used the right way around.
Also worth testing: client limits via env vars.

[Calvin Ko]

r21202 fixes a long standing bug that is uncovered by the extra "truncated" >attribute in the "clipboard-contents" packets - do you really need this? (it will create backwards compatibility issues with older versions - as some users just don't update as often as they should..)

You are right. I am actually thinking to add a clipboard-content-truncated message instead of changing the clipboard-content packets. All I need is to notify the client in some way. What do you think?

[Calvin Ko]

I have not test the code since I don't have an environment set up to test xpra.
Lets ask Max to help testing.

[Antoine Martin]

I am actually thinking to add a clipboard-content-truncated message instead of changing the clipboard-content packets. All I need is to notify the client in some way. What do you think?

If you need to notify the client, then I would rather not add a new packet type.
We can workaround older clients by adding a new capability flag in the hello packet - something like clipboard-contents-slice-fix=true.

[Calvin Ko]

address comment:10

[Calvin Ko]

clipboard-size-v5.patch

• check whether limits are set (only truncated when limits are set, i.e., not -1)
• add clipboard.contents_slice_fix capability for backward compatibility. New client should set this to False.

[Calvin Ko]

Max, see comment:7 for testing.

[Antoine Martin]

Merged in r21213 with the following changes:

• added get_format_size function to ensure we return the correct size for the given data format (because CARD32 can actually be 64-bit...)
• renamed capability to clipboard.contents-slice-fix, capabilities are False by default
• expose the capability from both server and client, and call set_clipboard_contents_slice_fix from both too
• log an informational message if the other end is missing that clipboard fix
• catch "clipboard-limits" setting change in client to avoid warning

Backports:

• r21207 for the contents slice fix
• r21214 exposes the clipboard.contents-slice-fix flag

[J. Max Mena]

Fired up a trunk r21225 server, and followed the steps for using the control channel - primarily xpra control :DISPLAY clipboard-limits 8 8 and it broke the server side clipboard. I could copy text from my client into the server clipboard, but could not copy anything on the server. If I did, the clipboard would be empty, and I couldn't paste anything client side.

I ran a session with -d clipboard demonstrating what I saw. My repro steps are pretty easy:

• Start a server with xfce4-terminal or some terminal emulator with a Copy/Paste? menu

• Run xpra control :DISPLAY clipboard-limits 8 8

• Copy some text from the client into the server

• Type something into the terminal, highlight it, and try to paste it into a client application (I just used kwrite)

I tried that on three different sessions, and it failed each time. I'll work on using the Environment variables and using xclip.

[Antoine Martin]

I tried that on three different sessions, and it failed each time.

Failed how? Wrong data? No data?
Do you have logs?

[J. Max Mena]

Requested logs.

[J. Max Mena]

When I copy from the client to server, I get as much text as I should within the limits - 8 characters. But when I highlight text on the server, copy it, and then paste it client-side, there's no data.

I just installed xclip, and I restarted my server and now it's working?

[Antoine Martin]

I just installed xclip, and I restarted my server and now it's working?

What steps?
The ones from comment:7 do use xclip so it should not come as a surprise that you need to have it installed.

[Antoine Martin]

Always test with the most basic tools, like xclip.
Do not trust applications to do the right thing with the clipboard.

[J. Max Mena]

Noted.

Both the control channel and the environment variables work with xclip, so as far as I can tell there isn't anything else to do.

I'm gonna go ahead and close this, unless there's any follow-ups.