Klipper: Remove first history item on clipboard clear
AbandonedPublic

Authored by davidedmundson on Apr 20 2018, 9:42 AM.

Details

Reviewers
hoffmannrobert
Summary

Password manager tools like Keepassx offer an option to clear
the clipboard/selection after some time, e.g. 10 seconds,
after the password was copied to the clipboard. This works fine,
but unfortunately the password isn't removed from Klipper's
history. This is a great security risk, which may make the use
of password managers impossible.

This patch offers an option to remove the top history item if
Klipper::checkClipData was called, but actually there isn't any
data. This is the case, if QClipboard::clear was called (by
Keepassx), which does setMimeData(0, mode).

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
hoffmannrobert created this revision.Apr 20 2018, 9:42 AM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 20 2018, 9:42 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
hoffmannrobert requested review of this revision.Apr 20 2018, 9:42 AM

Aren't you just duplicating the existing "prevent empty clipboard" option ?

Aren't you just duplicating the existing "prevent empty clipboard" option ?

No, the "prevent empty clipboard" option ensures that the clipboard doesn't stay empty, if it has been cleared, and sets it to the top history item. But it doesn't alter the history in any way.

Right, but pressing control+v again won't paste the item after clearing which I assumed was the intended goal

Right, but pressing control+v again won't paste the item after clearing which I assumed was the intended goal

Yes, pressing Control+V won't paste the item after clearing. But the problem is, that the cleared item stays readable in the clipboard history. Left-click on the clipboard icon in the system tray, and there still it is. That is the problem my patch solves by removing the item from history top.

Ideally, there was a mime data flag an application could set to indicate it wouldn't want it to end up in clipboard history but this is probably impossible to get applications to use..

We have two settings:
The first, when the X selection owner gets cleared, replaces the clipboard with the top entry
This one when the X selection owner gets cleared, deletes the top entry in the UI.

Both default to on, which together makes no sense. We're replacing the clipboard and then immediately deleting that same entry.

At a minimum it's a conflicting option that needs to be handled properly.

Even then I'm not 100% convinced.

If a client copies two pieces of sensitive data and then calls clear you've not solved anything.

But more importantly if another X client clears the selection owner even if it doesn't own the previous data, you're just deleting random things.
Something we know happens a lot which is why we have that preventEmpty situation on by default.

Personally I think the smarter solution (though I understand it's harder to get it in) would be to have keepassx put some hint in the mimeData x-kde-clipboard-manager-skip-this: true and then have klipper completely ignore the entry when it gets copied rather than trying to solve a problem after it's already happened.

You are right, the "prevent empty clipboard" option needs to be set to false to make the "RemoveTopHistoryItemOnClear" option work correctly.

Sorry, but I'm against this. The options keepass provides are just bullshit (sorry to be that direct). That cannot work and will not work. One cannot remove anything from the clipboard, trying to do so is insane. What would make most sense for things like clipboard is setting a mimetime like text/password. This could be used by Klipper to never add it to history. That would be a useful feature. But anything that just tries to support keepass's utterly broken workflow from an X11/Wayland and security point of view doesn't make any sense.

Klipper: Fix option RemoveTopHistoryItemOnClear

Some facts:

  • The clipboard in KDE is a QClipboard instance.
  • Klipper is a frontend to the clipboard with a history. It exchanges data with the QClipboard instance.
  • QClipboard provides the facility of setting mime types with data it stores. All data it stores (QMimeData) has a mime type associated.
  • There is no such thing like a hint in QClipboard. There is only the mime type every data is associated with.
  • Keepassx (and others) store their passwords and other data with mime type 'text/plain' in the clipboard.

I've tried using a different mime type 'text/confidential' in Keepassx and Klipper. Results are:

  • Selection only works with 'text/plain'. Storing a string with mime type 'text/confidential' (converted to a QByteArray and packed into a QMimeData object) in the clipboard with mode 'QClipboard::Selection' results in the error 'QClipboard: Cannot transfer data, no data available'.
  • Storing such a 'text/confidential' string in the clipboard with mode 'QClipboard::Clipboard' works, and Klipper could handle it by not storing it in its history. But unfortunately the clipboard contents are of no use to any application. They check the clipboard with QMimeData::hasText(), which only returns true if there is data with a 'text/plain' mime type and retrieve data from clipboard by calling QClipboard::text(), which only returns 'text/plain' typed data.

So telling Klipper to not save something in the clipboard to its history by setting a different mime type doesn't work.

If only the QClipboard instance can be used to transfer this not-save-to-history-information to Klipper, the information would have to be contained in the data itself, like some tag in front: 'NOT_FOR_KLIPPER_HISTORY: secret_password'. But then, Klipper must remove the tag from the clipboard content before it's pasted somewhere. If there is no Klipper, the tag must not be used. Seems not feasible.

Other options would be to extend QClipboard or QMimeData. Hm, don't think so.

So, for an easy but maybe not perfect solution, I changed the history removal patch, so the options RemoveTopHistoryItemOnClear and PreventEmptyClipboard are mutually exclusive now and set RemoveTopHistoryItemOnClear default to false. This way, users can decide by themselves, if they want this functionality and larger organisations can preconfigure these options as needed.

Storing such a 'text/confidential' string in the clipboard with mode 'QClipboard::Clipboard' works, and Klipper could handle it by not storing it in its history. But unfortunately the clipboard contents are of no use to any application. They check the clipboard with QMimeData::hasText(), which only returns true if there is data with a 'text/plain' mime type and retrieve data from clipboard by calling QClipboard::text(), which only returns 'text/plain' typed data.

I didn't explain well.

mimeData is key value pairs, you can have many pieces of mimeData.

So we have text/plain with the password as before
and x-kde-passwordManagerHint data "secret"

We still use the text/plain mimeData for context, but klipper ignores entries where that other mimeData is present.

mimeData is key value pairs, you can have many pieces of mimeData.

So we have text/plain with the password as before
and x-kde-passwordManagerHint data "secret"

We still use the text/plain mimeData for context, but klipper ignores entries where that other mimeData is present.

Thanks, I wasn't aware of this.

It works well for mode QClipboard::Clipboard. In Klipper::applyClipChanges() these entries can be sorted out, so they aren't inserted into history.

But it doesn't work for QClipboard::Selection. As soon as there is some other mimeData present with unsupported type, the selection stays empty with the warning 'QClipboard: Cannot transfer data, no data available'.

This is run in Keepassx:

QClipboard* clipboard = QApplication::clipboard();
QMimeData* mimeData = new QMimeData();
const QString secretHint = "secret";
QByteArray ba = secretHint.toUtf8();
mimeData->setText(text);
mimeData->setData("x-kde-passwordManagerHint", ba);
clipboard->setMimeData(mimeData, QClipboard::Clipboard);   // this works

if (clipboard->supportsSelection()) {
    clipboard->setMimeData(mimeData, QClipboard::Selection);  // this not, warning: QClipboard: Cannot transfer data, no data available
}

Thanks for following this up!

In keepassx you need a second QMimeData

void QClipboard::setMimeData(QMimeData *src, Mode mode = Clipboard)
Sets the clipboard data to src. Ownership of the data is transferred to the clipboard.

So the second call you're passing data you don't own. Not sure if that's the cause of your issue. Maybe dump your full code somewhere and I'll take a look?

Interestingly your warning doesn't come from setting the data. That warning is in QXcbClipboard::handleSelectionRequest i.e getting the current clipboard information back.

hoffmannrobert added a comment.EditedApr 26 2018, 10:46 AM

Thank you, that caused the issue.

Now it works well with keepassx. That's a much better solution, than removing from history. I've created a new revision for it:

https://phabricator.kde.org/D12539

If this is obsolete now, you can choose "Abandon revision" from the {nav Add Action..." dropdown menu button that's above the comment box.

davidedmundson commandeered this revision.Apr 26 2018, 1:58 PM
davidedmundson abandoned this revision.
davidedmundson added a reviewer: hoffmannrobert.