xpra icon
Bug tracker and wiki

Opened 5 months ago

Closed 5 months ago

#2072 closed defect (fixed)

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

Reported by: Calvin Ko Owned by: J. Max Mena
Priority: major Milestone: 2.5
Component: android Version: 2.4.x
Keywords: Cc:

Description

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.

Attachments (7)

clipboard.patch (4.0 KB) - added by Calvin Ko 5 months ago.
clipboard-size.patch (4.6 KB) - added by Antoine Martin 5 months ago.
improved patch
clipboard-size-v2.patch (6.5 KB) - added by Calvin Ko 5 months ago.
patch with control command to set the size limits
clipboard-size-v3.patch (6.8 KB) - added by Calvin Ko 5 months ago.
address the minor changes stated in comment:5
clipboard-size-v4.patch (7.0 KB) - added by Calvin Ko 5 months ago.
address comment:6
clipboard-size-v5.patch (9.2 KB) - added by Calvin Ko 5 months ago.
address comment:10
2072dclipboard.log (108.1 KB) - added by J. Max Mena 5 months ago.
Requested logs.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 5 months ago by Antoine Martin

Owner: changed from Antoine Martin to Calvin Ko

Please attach the patch.

Changed 5 months ago by Calvin Ko

Attachment: clipboard.patch added

comment:2 Changed 5 months ago by Calvin Ko

attached the patch

Changed 5 months ago by Antoine Martin

Attachment: clipboard-size.patch added

improved patch

comment:3 Changed 5 months ago by 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.

Changed 5 months ago by Calvin Ko

Attachment: clipboard-size-v2.patch added

patch with control command to set the size limits

comment:4 Changed 5 months ago by 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
Last edited 5 months ago by Calvin Ko (previous) (diff)

comment:5 Changed 5 months ago by 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

Changed 5 months ago by Calvin Ko

Attachment: clipboard-size-v3.patch added

address the minor changes stated in comment:5

comment:6 Changed 5 months ago by 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

Changed 5 months ago by Calvin Ko

Attachment: clipboard-size-v4.patch added

address comment:6

comment:7 Changed 5 months ago by 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.

Last edited 5 months ago by Antoine Martin (previous) (diff)

comment:8 Changed 5 months ago by 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?

comment:9 Changed 5 months ago by 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.

comment:10 Changed 5 months ago by 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.

Changed 5 months ago by Calvin Ko

Attachment: clipboard-size-v5.patch added

address comment:10

comment:11 Changed 5 months ago by 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.

comment:12 Changed 5 months ago by Calvin Ko

Owner: changed from Calvin Ko to J. Max Mena

comment:13 Changed 5 months ago by Calvin Ko

Max, see comment:7 for testing.

comment:14 Changed 5 months ago by 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

comment:15 Changed 5 months ago by 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.

comment:16 Changed 5 months ago by Antoine Martin

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

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

Changed 5 months ago by J. Max Mena

Attachment: 2072dclipboard.log added

Requested logs.

comment:17 Changed 5 months ago by 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?

comment:18 Changed 5 months ago by 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.

comment:19 Changed 5 months ago by Antoine Martin

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

comment:20 Changed 5 months ago by J. Max Mena

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.