Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent Klipper from storing secrets in clipboard history #1969

Merged
merged 2 commits into from Aug 29, 2018

Conversation

Projects
None yet
6 participants
@TheZ3ro
Copy link
Member

TheZ3ro commented May 16, 2018

Description

Fix #584
Porting of keepassx/keepassx#211 "Add hint for Klipper to not add passwords to history" by @roberthoffmann
Relevant Klipper issue/PR https://phabricator.kde.org/D12539

Motivation and context

Klipper adds every item to the clipboard history, unless an additional mime type 'x-kde-passwordManagerHint is set to secret.

How has this been tested?

I cannot test this since I don't have klipper installed.
Maybe @phoerious or @Germano0 can test this

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • All new and existing tests passed. [REQUIRED]
  • I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@TheZ3ro TheZ3ro added the new feature label May 16, 2018

@TheZ3ro TheZ3ro added this to the v2.4.0 milestone May 16, 2018

@TheZ3ro TheZ3ro requested a review from keepassxreboot/core-developers May 16, 2018

@@ -31,7 +31,8 @@ class Clipboard : public QObject
Q_OBJECT

public:
void setText(const QString& text);
void setText(const QString& text, bool secret = false);

This comment has been minimized.

@phoerious

phoerious May 17, 2018

Member

You also need to replace all usages of setText() with the call to the secret version.

This comment has been minimized.

@TheZ3ro

TheZ3ro May 17, 2018

Author Member

Why? Non secret field will be visible. Only passwords will be omitted from the clipboard history.
I've already changed the copy-password to clipboard usages to use the secret version.
Other fields will call setText with secret set to false by default

This comment has been minimized.

@phoerious

phoerious May 17, 2018

Member

I don't see any use of the new functions in this PR, except in setClipboardSecretTextAndMinimize() (which is also new).
Besides, I would prefer this flag to be set for everything. Usernames etc. are also cleared from the clipboard after the timeout, so they should also not be in the history.

This comment has been minimized.

@TheZ3ro

TheZ3ro May 17, 2018

Author Member

setClipboardSecretTextAndMinimize() itself it's used when copying the password
(line 551 and 944 of DatabaseWidget.cpp)

I will change it since I agree that all field should be treated like secrets

This comment has been minimized.

@Germano0

Germano0 May 22, 2018

Contributor

If you have any dubts, you may want to ask Martin Flöser, the Klipper developer.
When the patch will be finished, please let me know, so I will test it

@TheZ3ro TheZ3ro force-pushed the feature/klipper-password branch from c38d533 to a4f7923 May 29, 2018

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Jun 8, 2018

Somehow this PR shrunk. What happened to the rest of the changes and to what I nagged about in my first review?

@TheZ3ro

This comment has been minimized.

Copy link
Member Author

TheZ3ro commented Jun 11, 2018

@phoerious look at the difference between the two commits.

In the old commit:

  • I defined a new setSecretText function in the Clipboard class with the secret MIMEtype (used by Klipper to delete entries from history)
  • I defined a new setClipboardSecretTextAndMinimize function in the DatabaseWidget class calling the setSecretText one
  • I used the setClipboardSecretTextAndMinimize function whenever the password was copied to clipboard

In the new commit:

  • I edited the already existing setText function in the Clipboard class to always expose the secret MIMEtype when copying any text to clipboard.
    Klipper will catch this and delete the entries from his history but other clipboard manager will happily ignore and perform as standard

IMHO it seems pretty easy to understand from the code

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Jun 11, 2018

All I see in the Diff are +11 -3

@TheZ3ro

This comment has been minimized.

Copy link
Member Author

TheZ3ro commented Jun 11, 2018

Checkout the feature/klipper-password branch, do a git log to see the 2 commit i'm talking about:

commit a4f7923
Author: thez3ro
Date: Tue May 29 22:35:00 2018 +0200
all fields are protected

commit 9f244eb
Author: thez3ro
Date: Wed May 16 15:00:41 2018 +0200
prevent klipper from storing password in clipboard history

Then you can:

  1. use git diff HEAD~2 HEAD~1 to display the second-last commit (labeled old in my previous comment)
  2. use git diff HEAD~2 HEAD to display the final branch status +11 -3 (labeled new in my previous comment)
  3. use git diff HEAD~1 HEAD to display only the last commit where I revert changes made in 1

On the next rebase, I will squash those 2 commit into 1 so the revert part will not be present in git history


Alternatively in the commit tab you can see the two specific commit
istantanea_2018-06-12_00-24-53

@TheZ3ro

This comment has been minimized.

Copy link
Member Author

TheZ3ro commented Jul 3, 2018

Can someone review this? @keepassxreboot/core-developers

@droidmonkey
Copy link
Member

droidmonkey left a comment

One comment, not major

clipboard->setText(text, QClipboard::Clipboard);
const QString secretStr = "secret";
QByteArray secretBa = secretStr.toUtf8();
mime->setText(text);

This comment has been minimized.

@droidmonkey

droidmonkey Jul 8, 2018

Member

Can we move this line and line 53 above the #ifdef, they are the same regardless of OSX or not

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Jul 29, 2018

I know how to operate Git. ;-)
I'm a bit confused why GitHub is only showing a tiny part of the actual DIff when I click on "Files changed". I'm not quite sure what will happen when I click the merge button.

@TheZ3ro

This comment has been minimized.

Copy link
Member Author

TheZ3ro commented Jul 30, 2018

@phoerious What?

I've deleted the older branch, created a new one with the same name, added the new changes, did a rebase excluding the old commits and force-push.
This is a normal thing in a git environment and it has been done many times before on this repo, nothing strange.

It's useless to keep in a branch waiting for a merge old changes that are being reverted the next commit.
It only use extra repository space in the commit history and in git objects.

So, what are you talking about?

Checkout the branch with your git client, what you see is what gets merged.

@shantanugoel

This comment has been minimized.

Copy link

shantanugoel commented Aug 28, 2018

Is there any more work to be done on this PR before this can be merged? Looks like @droidmonkey approved the changes and the change requested by @phoerious is not applicable any more?

Changes were made my @TheZ3ro

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Aug 28, 2018

It looks complete to me, I need to test it using klipper before I merge unless someone can get certainty that it works.

@shantanugoel

This comment has been minimized.

Copy link

shantanugoel commented Aug 29, 2018

Thanks @droidmonkey. I built the develop branch just now along with this pull request and tested on ArchLinux. I can confirm that klipper now ignores the password copied from keepassxc.

@droidmonkey droidmonkey force-pushed the feature/klipper-password branch from a4f7923 to 2992e10 Aug 29, 2018

@droidmonkey droidmonkey merged commit 08a36b4 into develop Aug 29, 2018

2 checks passed

CodeFactor No issues found.
Details
TeamCity CI (KeepassXC) TeamCity build finished
Details

@droidmonkey droidmonkey deleted the feature/klipper-password branch Aug 29, 2018

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
@sts10

This comment has been minimized.

Copy link
Contributor

sts10 commented Mar 21, 2019

Is there any chance that this fix doesn't work for Snap installations?

I ask because, unfortunately, running 2.4.0 installed as a Snap on Kubuntu 18.04, my passwords remain in my clipboard indefinitely (rather than 10 seconds, as expected).

I'm more than happy to file a fresh issue for this, but just wanted to check if (a) there was something I had to do in KDE preferences to allow KPXC to clear the clipboard or (b) this wasn't tested on a Snap install and that could be the issue.

Debug info

KeePassXC - Version 2.4.0
Revision: c51752d
Distribution: Snap

Libraries:

  • Qt 5.9.5
  • libgcrypt 1.8.1

Operating system: Ubuntu Core 18
CPU architecture: x86_64
Kernel: linux 4.18.0-16-generic

Enabled extensions:

  • Auto-Type
  • Browser Integration
  • SSH Agent
  • KeeShare (signed and unsigned sharing)
  • YubiKey
@shantanugoel

This comment has been minimized.

Copy link

shantanugoel commented Mar 22, 2019

@sts10 At least I tested it on a regular install only, not on snap. And it's working with the official 2.4.0 release as well. Are you sure you have the right version of klipper installed that has the counterpart patch to look at the metadata sent by keepassxc and ignore it? That patch (https://phabricator.kde.org/D12539) was merged only around May last year but you seem to be on Kubuntu 18.04, so there's a high chance that it's not present in your OS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.