Added -secure mode to wipe selection buffers and paste once

[kennbr34]

I've added a '-secure' mode that users may wish to use if passing sensitive information in or out of the selection buffers. The mode also sets the selection loops to 1 so that the selection is cleared after a single paste (in lieu of a timeout feature that many users have requested). The documentation refers to this as implying '-loops 1' and the '-loops' setting will subsequently override this if specified.

The function to zero out the selection buffers is somewhat special because it uses a volatile type function pointer that prevents the compiler from optimizing out the process as a dead-store elimination. This is the same method that OpenBSD's explicit_bzero and OpenSSL's OPENSSl_cleanse use. I've also ensured that the parent process wipes its version of sel_buf out before forking if in silent mode.

This is the first pull request for an open-source project I've made, so if I've done something stupid let me know. :)

Edit: As the '-loops' feature is not working correctly (https://github.com/astrand/xclip/issues/73) I've instead implemented Rik Snel's '-wait' patch from https://github.com/astrand/xclip/issues/8

[hackerb9]

@kennbr34 I think it might make sense for xclip to always zero its memory when it exits. Is there a reason you did not implement it like that? Would you be willing to implement it as the default for xclip and make another pull request?

The only downside I can see is that X paste requests might hang if there is a huge amount of memory that needs to be zeroed. But that could be worked around by calling XSetSelectionOwner(..., None, ...) to tell X that xclip no longer owns the selection before calling xcmemzero().

[hackerb9]

@kennbr34 could you please write up a little paragraph about what -secure does and does not do in the man page? We don't want people thinking -secure gives them more protection than it does. In particular, since xclip has to rely on XStoreBuffer(), I believe that means there may still be a copy of the buffer in the X server process, so it'd be good if the man page discussed whether that is the case and how much protection the -secure flag actually gives. Thanks!

[kennbr34]

@kennbr34 I think it might make sense for xclip to always zero its memory when it exits. Is there a reason you did not implement it like that? Would you be willing to implement it as the default for xclip and make another pull request?

The only downside I can see is that X paste requests might hang if there is a huge amount of memory that needs to be zeroed. But that could be worked around by calling XSetSelectionOwner(..., None, ...) to tell X that xclip no longer owns the selection before calling xcmemzero().

Sure I could make it done by default, I would just need to remove the conditional test before calling the xcmemzero() function.

The main reason I did it as an option was to combine it with the behavior to remove the sensitive information once it was pasted once, because to me it stood to reason that the user wouldn't care if the memory was zero'd afterwards if it was not sensitive data. This seems to be like combining two operations with one option flag though, so it does seem like it would be better suited to be done as default or handled by a distinct optional parameter.

Unfortunately, I am unable to confirm myself how X handles these memory buffers. This was more of a best-effort approach of cleaning up our own mess, so to speak: At the very least we are sanitizing our own memory before exiting.

There are basically 3 possibilities of how the memory is handled in X's buffers: 1) These buffers are shared memory, so that zero'ing out the buffer in xclip will zero out the memory area held by X as well 2) X does its own independent sanitation 3) X does no sanitation at all, and a duplicate of the selection is indeed held in memory. We could probably deduce whether 3 happens simply by monitoring memory consumption of X after handing it a large piece of data into a selection buffer.

The main security consideration intended here was removing sensitive information from selection buffers after it is pasted once, so that the user does not inadvertently paste it again somewhere else they do not intend, and so that it is not simply held in a still-open 'xclip' process in memory.

Maybe renaming the '-secure' function to '-sensitive' might be more apropos so that it implies it is for sending sensitive information to the selection buffers, but does not imply any level of extra security in terms of memory handling. This could be implemented immediately while examination of how X is storing and handling memory is being confirmed.

Let me know how you think we should proceed before I submit another pull request for review.

[hackerb9]

Yes, I think -sensitive is better than -secure. The man page ought to say something about the main purpose being to avoid inadvertent pasting.

Also — although this maybe should be a different bug — it would be good if -sensitive really did enforce pasting only once. Instead of relying on -wait (or -loop) to quit the process, -sensitive ought to handle that immediately after paste.

[hackerb9]

Oh, and yes, removing the conditionals before xcmemzero sounds perfect. If you can also use XSetSelectionOwner to set the owner to None before clearing memory that would be super as we won't have to worry about making pastes hang while X waits for xclip to finish up.

[kennbr34]

Also — although this maybe should be a different bug — it would be good if -sensitive really did enforce pasting only once. Instead of relying on -wait (or -loop) to quit the process, -sensitive ought to handle that immediately after paste.

Yeah I would like to see this handled better, but it's a bit over my head with how to accomplish it with X. I have an open issue regarding '-loops' not working like it should, but as I understand the X window system it is not actually possible to predict upon what loop iteration the selection will have been pasted. In other words, sometimes the selection has been pasted once the 2nd iteration has been reached, and others it has not been pasted until the 3rd iteration, and this is basically inherent to the way X creates windows and requestors.

My method is basically a bit of a hack based on https://github.com/astrand/xclip/issues/8. This creates a timer that resets after each time the selection is pasted, and if the selection is not pasted again before the timer expires, then the xclip fork clears the selection buffer and closes. What I've done is just set the timeout interval to be so low that a person cannot physically paste the selection a second time fast enough before xclip clears the selection buffer. Leaving the timeout period configurable is advantageous in case a person needs to be able to paste it more than once. For example, if confirming a new password, the user could use '-wait' to set the timeout sufficiently high to facilitate pasting twice (or any number of times), but allow xclip to timeout at the specified amount of milliseconds after the last paste.

The problem with this method is it creates a race condition where, if the timeout interval specified is too low, the xclip fork will exit before the selection is actually even sent to the buffer; this will of course make pasting the selection impossible. Since releasing the initial pull request, I have done some testing and found the best default timeout interval to be 50 ms. In practical terms, this is still fast enough that it will clear the selection buffer faster than a user can paste a password a second time, but not so fast that it prevents pasting the selection at all. There is of course the possibility that 50 ms might be too low on very slow hardware, but I tested this on some pretty old hardware and found I could not reproduce the race condition at any interval lower than 25 ms, so it is very unlikely 50 ms will be too low. In those instances, the timeout interval can be configured to suit the environment.

So with those things plus the previous mentioned proposals I will release a pull request with the following changes:

• '-secure' be changed to '-sensitive' and the manual more aptly describe its function
• Call XSetSelectionOwner() to set window owner to None before calling xcmemzero()
• xcmemzero() be used unconditionally to clear selection buffers upon exit
• '-wait' be changed to '-timeout' and the manual more aptly describe the timer that this entails
• The default '-timeout' period be set to 50 ms instead of 1 ms.

[hackerb9]

Hold off on changing -wait to -timeout. I think we might want -timeout for an option that times out even if it is never pasted. I think 50ms sounds okay for now. We can discuss getting xclip to exit after a single paste (no waiting) after we get your other changes checked in.