search

keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
21.27k stars 1.47k forks source link

Data loss when database file changes without Yubikey available #5290

Open droidmonkey opened 4 years ago

droidmonkey commented 4 years ago

I was thinking of filing a new issue but maybe this is the same problem. I incur data loss on sync, silently, which is very very critical. Steps:

  1. Encrypt database with master password + Yubikey.
  2. Sync via a cloud provider and open same database from two machines.
  3. Modify database on machine A and save it. Let cloud sync client sync the new file.
  4. KeePassXC on machine B will detect the file update, and try to reload the database.
  5. Machine B will fail doing so, because the Yubikey is not inserted. Instead of displaying an error message or prompting for Yubikey entry, KeePass will ignore the new database update and will include a "*" character in its status bar, prompting the user to save unsaved changes (of which there are none). These unsaved changes is actually the older version of the database, and once a user saves this then boom, the new modifications from machine A are wiped out. Ouch.
  6. Repeat 1-5 but with Yubikey inserted in machine B when it receives the new database update, the program prompts me to press my Yubikey button (I've configured it so), and reloads the database as expected. New updates are visible.

Fix directions: this is a critical issue, so I think that two steps are necessary: 1- when saving unsaved changes, KeePassXC should check if the current database file does not include new entries or updates, in which case it should require user authorisation or merge the changes. This is the behaviour of Keepass2Android 2- when reloading an updated database file, fail HARD if the file could not be loaded, and force the user to provide a decryption mechanism.

Originally posted by @Zvezdin in https://github.com/keepassxreboot/keepassxc/issues/5284#issuecomment-675544976

Zvezdin commented 4 years ago

Any progress on the issue?

droidmonkey commented 4 years ago

Unfortunately the fix for this and other edge saving cases is bigger and more complex than I am comfortable releasing in a maintenance release. I'll be posting my fix very soon for review.

p0wertiger commented 4 years ago

Actually I've been thinking about local-master synchronization like original KeePass suggests with triggers, I can't find any suggestion on this and one of my computers is MacBook. What I want to achieve is an option to automatically sync (merge) two files in two locations on save - local in my private folder, master on cloud storage. The possibility would also be, as suggested, to enable keepass2android way of saving - check for changes, merge if different, save. This could be an additional toggle in settings. I've already faced data loss before when the files didn't get synced for a while for any reason (no Internet, sync app crashed).

This is partly a feature request, partly a bug as described in original thread post, hence my comment on this. I also suggest changing title of the issue since Yubikey is just one way to reach this case.

droidmonkey commented 4 years ago

It is true that this data loss can occur on a password change as well. This problem occurs because we do check for underlying changes prior to saving and attempt to merge them. When the db cannot be opened due to no yuibkey or different password or whatever then the open db enters a non synced state. Pressing save or save on exit overwrites the "on disk" db causing the data loss and also reversion of the database credentials.

thecolinw commented 3 years ago

Hope this is the right place to write, but it directly refers to Your last comment. You sayed You are trying to merge the database if there are underlying changes, but I tried to exactly test this and couldn't find a way to verifiy this.

Example case: Autosave on changes is enabled. For test purposes autoreload on external changes is disabled. You synchronize the database file between 2 devices. Database is opened in KeepassXC on device A and KeepassXC on device B. We start editing an entry on device A. Now we start editing an other entry on device B. We save the editing on device A. The databasefile is synced to device B. Because we are in editing mode database on device B didn't get a notification to reload the database because of changes. (It doesn't matter, that the database isn't reloaded because of the editing mode, the situation althought apply if the sync isn't working for a short time, maybe because one device is shortly offline. By the way same would happen if autoreload is enabled it just wouldn't reload) Now we save the editing on device B. The Databasefile is synced to device A. (As I mentioned this would happen too, if sync isn't working for short time, because database file on device B is now newer as on device A) On device A we have now the notification that a external change has happend and we have the option to reload it. If we say yes the changes of device B appear, but the changes we made before on device A disappear. => No merge on reload. If we say no and then save the database manually the changes we made on device A stay, but the changes from device B doesn't appear. => No merge on save.

In both cases the changes for one of the devices disappear. Which means I lose important data.

As much as I could test this I doesn't saw a merge happening. Is there an option I missed to get it working? Is this a bug or the expected behavoir. Please tell what do You meant with merge. Same test with KeePass would result in a question to override or merge on save.

I currently thinking about switching to KeePassXC, because of the nicer design, totp, crossdevice and keeshare features all in one package without the hassle to configure plugins, but maybe losing important data is a no-go for me. Besides this, this is great work!

KeePassXC - Version 2.6.2 Revision: e9b9582

Qt 5.15.1 Diagnosemodus ist deaktiviert.

Betriebssystem: Windows 10 Version 2004 CPU-Architektur: x86_64 Kernel: winnt 10.0.19041

Aktivierte Erweiterungen:

Kryptographische Bibliotheken: libgcrypt 1.8.6

l-mb commented 3 years ago

Just for completeness (in case this code path is somewhat different):

This will also happen in a slight variant when the YubiKey is configured to require touch, but one misses the window to interact before the timeout.

(I thought I'd leave one plugged in but at least set a barrier that requires someone to be physically present.)

l-mb commented 3 years ago

Dear all, I just wanted to touch base here - the proper fix is a bit beyond me. How to best proceed here? Thanks!

droidmonkey commented 3 years ago

If this is a problem for your workflow then you need to lock the database on the machine you do not have the yubikey plugged into

l-mb commented 3 years ago

@droidmonkey I was kind-of hoping that a possible resolution would instead detect this, and defer the merge until after the key was back.

I understand how to work-around it, but is this behaviour by design and intended then and going to stay? (I understand Open Source ain't free, so if there's a way to offer a bug bounty, count me in.)

droidmonkey commented 3 years ago

It's on the list

Zvezdin commented 2 years ago

Hi, any update on when the fix for this issue would come out? Thanks!

liayn commented 1 year ago

In relation to #9336:

I think this could easily be resolved by providing a dedicated button to force sync (*), which I can press when I inserted the key. What do you think?

(*) That means: Initiate the exact same procedure manually, that happens when a change of the database file is detected.

krikk commented 1 year ago

i think this issue should also get the label BUG!!

j-lakeman commented 1 year ago

Hey all, I'm syncing my kdbx with a YubiKey as 2nd factor through Nextcloud. Whenever I'm modifying an entry using either https://github.com/PhilippC/keepass2android or https://github.com/Kunzisoft/KeePassDX, it breaks my browser integration for the Firefox add-on. To resolve that, I'm using the file versioning of Nextcloud to download the old database and merge them. I'm not having two databases open at the same time though, they simply get synced by Nextcloud. Could this be related to the above issue? I couldn't find any other similar issues. Cheers for your work! Great app apart from that!

droidmonkey commented 1 year ago

Not related, although it shouldn't be happening. This usually happens when people use the same name for their browser connection on two different computers.

Neturius commented 9 months ago

First, thanks for creating such a great software, Much appreciated. However, if such a critical flaw is known for 3.5 years now I wonder if it will ever be fixed. There are not even warnings in the docs. So, what are you planning to do with it?

itbastian commented 7 months ago

Another idea to solve this issue: Instead of just marking the database with an asterix in the window title - which triggers an unresistable urge to press Ctrl+S in me ;) - I would rather like to see:

  1. keep the big banner with "need to reload from disk" open
  2. have the whole DB in a read only state
  3. offer a button to trigger that reload

Since 2. and 3. are probably quite a bit work, I would settle with 1.: If the big red banner could just stay until you close it.

Possible Text: "External changes detected. To merge you need to manually close and reopen the database BEFORE applying any changes."