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

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

Open
lucas-flowers opened this issue May 3, 2021 · 23 comments

Comments

@lucas-flowers
Copy link

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:

  • KeePassXC-Browser (Firefox)
  • KeeShare enabled
@droidmonkey
Copy link
Member

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

lucas-flowers commented May 3, 2021

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:

  • One group for a personal account I also use at work
  • One group for work accounts that I also use on my personal computer

(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):

  • The share file with work-related entries pointed to an absolute path on my work laptop (macOS)
  • The share file with personal entries pointed to an absolute path on my personal computer (Windows)

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

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

@droidmonkey
Copy link
Member

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

lucas-flowers commented May 4, 2021

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:

  • For the first share, I opened my work-related database, configured a preexisting group to share to a new file, and saved the database (creating the new share file). I created a corresponding new group in the personal database pointing to that share file.
  • For the second share, I opened my personal database, created a new group, moved an entry into it, configured the group for sharing, and saved the database. I created a corresponding group in the work database pointing to that shared file.

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

@droidmonkey
Copy link
Member

Gotcha, I'll try to replicate this scenario.

@droidmonkey
Copy link
Member

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

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

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

droidmonkey commented May 31, 2021

@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:

  • 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.
  • When an entry is moved out of a shared container, record the original UUID as a deleted entry then give it a new UUID so that it no longer interacts with the share.

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

mgeramb commented Jan 18, 2022

@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:

  • create new database "Project" and create one Entry in this DB.
  • save this db as "Project.kdbx"
  • close the database "Project"
  • create new database "Import"
  • create group "Project" and enable KeeShare with the mode "Syncronize"
  • enter path to "Project.kdbx"
  • enter password of "Project" and press 'OK'
  • a red banner with an error "access denied" with the path to the "Project.kdbx" appears.

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

mgeramb commented Jan 19, 2022

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

droidmonkey commented Jan 19, 2022

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

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

image

@phoerious phoerious modified the milestones: v2.7.1, v2.7.2 Apr 12, 2022
@tysonclugg
Copy link

tysonclugg commented May 29, 2022

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

@droidmonkey droidmonkey modified the milestones: v2.7.2, v2.8.0 Jul 24, 2022
@corj-aolabor
Copy link

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

@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. 😿

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

droidmonkey commented May 31, 2023

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

@droidmonkey droidmonkey changed the title KeePassXC wipes out most passwords without warning Adding KeeShare database may wipe out passwords if related to original database Aug 20, 2023
@sawft99
Copy link

sawft99 commented Jan 26, 2024

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

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

droidmonkey commented Jan 26, 2024

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

@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:

  • hide the keeshare feature behind an opt-in with a bit fat disclaimer, telling users how "don't do those things" applies, or;
  • accept that the problem needs to be fixed, and decide how that's going to happen.

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

No branches or pull requests

8 participants