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

Data loss when database file changes without Yubikey available #5290

Open
droidmonkey opened this issue Aug 18, 2020 · 17 comments · May be fixed by #10612
Open

Data loss when database file changes without Yubikey available #5290

droidmonkey opened this issue Aug 18, 2020 · 17 comments · May be fixed by #10612

Comments

@droidmonkey
Copy link
Member

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 #5284 (comment)

@Zvezdin
Copy link

Zvezdin commented Sep 11, 2020

Any progress on the issue?

@droidmonkey
Copy link
Member Author

droidmonkey commented Oct 5, 2020

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.

@droidmonkey droidmonkey modified the milestones: v2.6.2, v2.7.0 Oct 5, 2020
@p0wertiger
Copy link

p0wertiger commented Nov 4, 2020

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
Copy link
Member Author

droidmonkey commented Nov 4, 2020

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
Copy link

thecolinw commented Dec 10, 2020

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:

  • Auto-Type
  • Browser-Integration
  • SSH-Agent
  • KeeShare (bestätigtes und unbestätigtes Teilen)
  • YubiKey

Kryptographische Bibliotheken:
libgcrypt 1.8.6

@l-mb
Copy link

l-mb commented Dec 18, 2020

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
Copy link

l-mb commented Mar 9, 2021

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

@droidmonkey
Copy link
Member Author

droidmonkey commented Mar 9, 2021

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
Copy link

l-mb commented Mar 27, 2021

@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
Copy link
Member Author

It's on the list

@Zvezdin
Copy link

Zvezdin commented May 21, 2022

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

@liayn
Copy link

liayn commented Apr 19, 2023

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
Copy link

krikk commented May 17, 2023

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

@j-lakeman
Copy link

j-lakeman commented Jul 16, 2023

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
Copy link
Member Author

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
Copy link

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
Copy link

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."

vuurvli3g pushed a commit to vuurvli3g/keepassxc that referenced this issue Apr 19, 2024
- External modifications to the db file can no longer be missed.

- Fixed dialogFinished signal of DatabaseOpenDialog was not emitted when dialog was closed via the 'X' (close) button

- For reloading with a modified db, an additional choice has been added to allow the user to ignore the changes in the file on disk.

- User is now presented with an unlock database dialog if reload fails to open the db automatically.
  For example when the user removed the YubiKey, failed to touch the YubiKey within the timeout period, or db pw has been changed.

Fixes keepassxreboot#5290
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.