keepassxreboot / keepassxc

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

Adding KeeShare database may wipe out passwords if related to original database #6477

Open lucas-flowers opened 3 years ago

lucas-flowers commented 3 years ago

Overview

Since enabling KeeShare, KeePassXC has been erasing a large number of my passwords without warning.

Steps to Reproduce

Restore backup database, which is 3.55 MB:

image

Open the restored database, which is now 360 kB:

image

I can repeat this any number of times with the original backed up files. I reversed the deletions by opening the corrupt kdbx file and merging in the original backed-up database (and I am disabling exports/imports in KeeShare from now on).

Expected Behavior

I expect opening the database not to erase my passwords.

Actual Behavior

Opening the database erases my passwords.

Context

KeePassXC 2.6.4 Revision: 34a78f0

Operating System: Windows Desktop Env: N/A Windowing System: N/A

Also:

droidmonkey commented 3 years ago

We know this is a big problem with keeshare for some configurations for unknown reasons. This is tracked in #6013. Can you please explain exactly the steps you followed to create your share and mount it in your database?

lucas-flowers commented 3 years ago

Background

I have two databases, one is personal and one is for work. They are located in the same directory, which is synced across all my devices with Syncthing. I've mounted two shares:

(The device that I know has this issue is the Windows computer described in the original post, but I also have KeePassXC running on Ubuntu and macOS. I haven't noticed them causing this issue, though I can't say for sure that they don't. The version for macOS, which I use more frequently than Ubuntu, is also 2.6.4 revision 34a78f0.)

Steps

I can't recall the exact steps that I performed to create the mounts, but broadly I followed the steps in https://keepassxc.org/docs/KeePassXC_UserGuide.html#_database_sharing_with_keeshare with two issues:

Issue 1

I did have some confusion about the usage of KeeShare personal certificates. It looks like I've generated certificates for each keepassxc instance, but there are no signatures shown in the database's KeeShare settings for either share.

(I'm not sure this was related but I'm mentioning it for completeness.)

Issue 2

I initially gave absolute paths for the share files, since I mistakenly thought the paths were configured once for each keepassxc installation (instead of once in the database itself):

Since these entries don't change often, it took a while for me to notice that they weren't actually syncing. After a week, I realized the icons had a red X overlay on them indicating that syncing failed (I assume due to the using of absolute paths, i.e., Windows paths on macOS and vice versa).

I corrected the paths to be relative instead of absolute (i.e., just the filename of the share).

The entry erasure incident has occurred at least twice on my personal KeePass database (it does not appear to have occurred on my work database): It happened today (which was after switching to relative paths), and once a few days ago (I forget if relative paths were configured back then).

lucas-flowers commented 3 years ago

(Also, I have not moved any entries to or from the share groups since their creation.)

droidmonkey commented 3 years ago

Did you start with a fresh database for your shares or did you start from your original database, copied it, and deleted entries you didn't need?

Are your shares ending in .kdbx or .kdbx.share?

lucas-flowers commented 3 years ago

Both databases (which I'm referring to as the work and personal databases) already existed with no entries in common prior to the creation of the shares. Specifically:

In both cases, they are .kdbx.share files.

droidmonkey commented 3 years ago

Gotcha, I'll try to replicate this scenario.

droidmonkey commented 3 years ago

Figured out why this happens, it has to do with storing deleted item uuids and during a merge operation those cause existing entries in the database to be deleted without warning. This will be fixed in 2.7.0.

borisdigital commented 3 years ago

Figured out why this happens, it has to do with storing deleted item uuids and during a merge operation those cause existing entries in the database to be deleted without warning. This will be fixed in 2.7.0.

Hey @droidmonkey, i'd like to ask carefully, if there is some kind of timeline when 2.7.0 will be released? Or alternatively will there be some kind of backport patch for 2.6.x? This bug actually made our company's access database unusable (we're using a lot of keeshares).

Thank you in advance for your time!

droidmonkey commented 3 years ago

The fix requires a non backwards compatible overhaul of keeshare. Basically the way it was originally designed leads to this situation. We are working on a lot of changes and refactors for 2.7 and it won't be possible to Backport this much change without bringing the rest of it. I'm getting closer to just calling 2.7 done, but it still needs these changes to be finished.

droidmonkey commented 3 years ago

@borisdigital I am sorry that your company lost information, that is certainly not a good reflection on our application. KeeShare was introduced as a simple/isolate feature, but ended up being more complex and interconnected with standard database entries.

FWIW, this issue occurs when you copy the database file, delete entries from it, then use it as a KeeShare database. The deleted entry UUID's will propagate to the main database and delete all the original entries. This also happens if someone adds an entry to the share from a main database and then deletes the entry from the share. Entries with the same UUID that exist outside the share will be deleted.

To fix this I am doing the following:

Basically we need to move to a share as an isolated inner container instead of allowing it to interact with entries outside the shared group. This also makes it far easier to allow for temporarily mounting a share without negative interactions.

mgeramb commented 2 years ago

@droidmonkey

I have the same or at least similar issue with KeeShare which can by easily reproduced. I used the current snapshot from '2022-01-08 23:52'.

Steps to reproduce:

The Project.kdbx files have now a size of 0 bytes.

For me this sounds that the problem is not in the syncronization logic itself. It seems the problem occurs in the file handling which causes the 0 bytes file.

Update: One more idea, maybe it have something to do with our maleware scanner. I will check this and give update here.

mgeramb commented 2 years ago

@droidmonkey I have verified now my issue above without maleware scanner. It seems, that the KeeShare feature does currently not work at all in the 2.7 snapshot. Are you aware of this or should create a new issue for it?

droidmonkey commented 2 years ago

Yes please open a new issue for pre release. In my testing of my keeshare code changes I did not ever get a 0 byte file, but might be hitting an exception case

tysonclugg commented 2 years ago

FWIW, this issue occurs when you copy the database file, delete entries from it, then use it as a KeeShare database. The deleted entry UUID's will propagate to the main database and delete all the original entries. This also happens if someone adds an entry to the share from a main database and then deletes the entry from the share. Entries with the same UUID that exist outside the share will be deleted.

To fix this I am doing the following:

  • When a KeeShare database is first added to a main database ("share initiation"), remove all deleted item UUIDs from the share that match entries in the main database. This ensures a share won't delete existing entries outside the share.

@droidmonkey How will this affect the situation where the database file is copied and items are deleted from the "main" database file before a KeeShare group is created with the sharing type is set to either syncrhonize or export? Would items still be lost from the share with the suggested change in place?

droidmonkey commented 2 years ago

image

tysonclugg commented 2 years ago

@phoerious Thanks for scheduling this bug for the next release.

Can this issue also be tagged with "high severity" to highlight the user impact this is having?

EDIT: Severity 1 (Loss of data) would be a more appropriate tag.

corj-aolabor commented 1 year ago

@tysonclugg I saw your other Issue that got closed and I'm on your side. This is a important feature of Keepass and lossing Data because of it is a big problem. It should have high priority and should be disabled in a Hotfix until it works again. I almost lost around 250 entries trying to make a file to share Team Passwords in.

tysonclugg commented 1 year ago

@tysonclugg I saw your other Issue that got closed and I'm on your side. This is a important feature of Keepass and lossing Data because of it is a big problem. It should have high priority and should be disabled in a Hotfix until it works again. I almost lost around 250 entries trying to make a file to share Team Passwords in.

Thanks for the mention, the reality is that I have stopped using KeePassXC because I no longer trust that my passwords will be kept safe. :crying_cat_face:

I hit by the bug over 2 years ago. I tried to help out in #4199, digging into the problem and looking closely at the source, then making various suggestions about how the issue might be resolved. Then 11 months later #4199 was closed without a fix, and after another couple of weeks #4199 was nominated as duplicate of this issue.

Now here we are, 3⅓ years after this issue was first logged in #4199, and users are still losing data. Is there any progress to report?

If the problem with fixing this issue is indeed that the kdbx specifications may need to change, then surely the time to do so is well overdue. If a fix can be made without changing the spec, I'd love to hear about it - my intention of communicating ideas for the fix in #4199 was actually in preparation for making a patch, but no positive feedback was received regarding any of my suggestions, so I did not proceed.

droidmonkey commented 1 year ago

The problem is that the system is working AS DESIGNED. You copied a database, deleted entries in it, and effectively merged it into the same database (entry UUID's are EQUAL) and that causes the deletion of those entries. That is how merging is supposed to work. The problem is that KeeShare was originally designed and built to take advantage of merging and not as an "embedded database". #4199 is absolutely a duplicate of this one and I chose to keep this one... as mentioned in my comment.

Now I would love to fix this but it basically requires recoding the entirety of KeeShare which I have not had time to devote to.

To avoid this problem is absolutely trivial. Just create a NEW database and move the entries you want into that database. Then setup the KeeShare. Oh and keep backups of your database

sawft99 commented 9 months ago

So 2.7 has come and gone but since i opened an issue #10229 for it and it was marked as a duplicate for this issue which is still open. It doesn't seemed like that's happened then.

As others said years ago at this point it's odd this hasn't been a higher priority (besides a label). Its been causing large data loss at some organizations and also in my case. Thankfully we have backups of it at least.

If i cant rely on the core functionality of a password manager to store credentials both securely and while ensuring integrity of the data, then it's no longer viable personally or professionally. I would consider securely sharing information without data loss within the core functionality and really any bug that causes such an issue. Anything outside of basic security and data integrity should be secondary.

Perhaps instead of a slew of GUI changes and more advanced use cases, this issue could finally be addressed as it was originally promised.

Also from the last comment this seems be be "by design" but the issue is still open. So there is some kind of intention to fix this at the very least I hope. However, the fact that yet another major revision is underway while hotkeys and cli arguments are on the project board and this isn't is disapointing.

corj-aolabor commented 9 months ago

So 2.7 has come and gone but since i opened an issue #10229 for it and it was marked as a duplicate for this issue which is still open. It doesn't seemed like that's happened then.

As others said years ago at this point it's odd this hasn't been a higher priority (besides a label). Its been causing large data loss at some organizations and also in my case. Thankfully we have backups of it at least.

If i cant rely on the core functionality of a password manager to store credentials both securely and while ensuring integrity of the data, then it's no longer viable personally or professionally. I would consider securely sharing information without data loss within the core functionality and really any bug that causes such an issue. Anything outside of basic security and data integrity should be secondary.

Perhaps instead of a slew of GUI changes and more advanced use cases, this issue could finally be addressed as it was originally promised.

Also from the last comment this seems be be "by design" but the issue is still open. So there is some kind of intention to fix this at the very least I hope. However, the fact that yet another major revision is underway while hotkeys and cli arguments are on the project board and this isn't is disapointing.

I just gave up. I use Keepass only for my personal passwords. We as a team use Passbolt now. This issue is open since 2021. Yeah this is an free and open source project so we cant really demand something.

droidmonkey commented 9 months ago

Nobody is forcing you to use the keeshare feature, and that was a major contribution from a third party, so we kinda inherited this mess...

Also, this is not normal behavior. You have to do very specific things to trigger this situation that are abnormal behavior. So don't do those things. And yes, this is by design because that is how keepass databases work with merging, even keepass original works this way.

tysonclugg commented 6 months ago

@droidmonkey - you've dismissed this issue numerous times despite offers to help fix the code.

It's a major problem that is driving users away. It may well be an inherited problem, but it is a problem all the same.

I suggest you choose between the following: