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

AirDropping database can cause database corruption #1113

Closed
sts10 opened this Issue Oct 24, 2017 · 10 comments

Comments

Projects
None yet
5 participants
@sts10
Contributor

sts10 commented Oct 24, 2017

Given one setting choice, an AirDrop (a macOS feature), and a "no" choice, it's possible to corrupt a database.

Expected Behavior

Users should be free to AirDrop their database with any combinations of settings and not corrupt their database.

Current Behavior

Given a setting choice and a prompt choice, AirDropping a database can result in a corruption.

Possible Solution

Make the prompt described in step 5 below ("The database file has changed. Do you want to load the changes?") be more of a warning? I guess the AirDropping "modifies" the locked file somehow, so this could be a problem with AirDrop/macOS, but maybe there's some way of having AirDropping a database not modify it somehow?

Steps to Reproduce (for bugs)

  1. In KeePassXC have the setting "Automatically reload the database when modified externally" unchecked
  2. Create a new database and create a few entries.
  3. Save and lock database, leaving KeePassXC open.
  4. Use Apple AirDrop to transfer your database to an iPhone that has MiniKeePass on it. (Right click database file > Share > AirDrop > select your iPhone)
  5. On laptop, you should immediately get prompt/dialogue from KeePassXC: "The database file has changed. Do you want to load the changes?"
  6. Choose "no"
  7. Attempt to unlock database on laptop. User gets "error: Invalid transform seed size". Unable to open database on laptop (seems to be same error described in #639 ). Note: User can still unlock database on iPhone with MiniKeePass. Unclear if user can recover an uncorrupted copy of their database from their phone.

Note: I have not been able to see if I can reproduce this bug by AirDropping a database file from one laptop to another laptop, but if I had to guess it would also happen in that case?

Screenshot of Settings Used While Reproducing

screen shot 2017-10-23 at 10 23 41 pm

Context

A user wishing to remain anonymous told me about this bug. (I successfully reproduced it following the steps listed above, with the settings and debug info listed here.) User was simply attempting to send his database to his or her iPhone without using a cloud service.

I can see a possible objection that unchecking the setting in step 1 and choosing "no" at step 6 is the "wrong" choice, but given how unexpected the dialogue prompt is (an AirDrop transfer doesn't feel like a file modification), and the (likely) catastrophic results of a corrupted database, I'd say this is a pretty gnarly bug. I'd hope that it would be harder (if not impossible) to corrupt a database using KeePassXC-- is that realistic?

Debug Info

KeePassXC - Version 2.2.2
Revision: 6d46717

Libraries:

  • Qt 5.9.1
  • libgcrypt 1.8.1

Operating system: OS X Yosemite (10.10)
CPU architecture: x86_64
Kernel: darwin 14.5.0

Enabled extensions:

  • KeePassHTTP
  • Auto-Type
  • YubiKey
@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Oct 24, 2017

Member

To be sure I follow correctly: the database is closed, but transferring it to an iPhone changes it? That shouldn't happen and first of all sounds like a bug in AirDrop. There is no reason why that should cause a file modification.

Can you please test what happens if you

  • have automatic reload on
  • choose yes
  • choose no and then restart KeePassXC
  • leave the DB open while transferring it (in all three scenarios above)
  • check if the DB that was sent to the iPhone is also corrupt
Member

phoerious commented Oct 24, 2017

To be sure I follow correctly: the database is closed, but transferring it to an iPhone changes it? That shouldn't happen and first of all sounds like a bug in AirDrop. There is no reason why that should cause a file modification.

Can you please test what happens if you

  • have automatic reload on
  • choose yes
  • choose no and then restart KeePassXC
  • leave the DB open while transferring it (in all three scenarios above)
  • check if the DB that was sent to the iPhone is also corrupt
@droidmonkey

This comment has been minimized.

Show comment
Hide comment
@droidmonkey

droidmonkey Oct 24, 2017

Member

Yah the sequence does not make sense. When you lock a database it is closed. There is nothing else KeePassXC is doing with that file. As phoerious said, why would you get notified of a file change because you air dropped it to another device?

Member

droidmonkey commented Oct 24, 2017

Yah the sequence does not make sense. When you lock a database it is closed. There is nothing else KeePassXC is doing with that file. As phoerious said, why would you get notified of a file change because you air dropped it to another device?

@louib

This comment has been minimized.

Show comment
Hide comment
@louib

louib Oct 25, 2017

Member

@droidmonkey we do get a The database file has changed. Do you want to load the changes? prompt when the file is modified, even though the database is locked in KeePassXC.

Member

louib commented Oct 25, 2017

@droidmonkey we do get a The database file has changed. Do you want to load the changes? prompt when the file is modified, even though the database is locked in KeePassXC.

@droidmonkey

This comment has been minimized.

Show comment
Hide comment
@droidmonkey

droidmonkey Oct 25, 2017

Member

That is interesting, there is a bug there since the file watcher should be disabled when the db is not "open".

Member

droidmonkey commented Oct 25, 2017

That is interesting, there is a bug there since the file watcher should be disabled when the db is not "open".

@sts10

This comment has been minimized.

Show comment
Hide comment
@sts10

sts10 Oct 25, 2017

Contributor

Glad to see we've maybe caught one bug!

I was busy today but I got to try a few of the procedures @phoerious requested above.

Let's call my original procedure of steps described in my opening of this issue as "Procedure A"

Procedure B: Procedure A, but have the automatic reload setting on (checked)

  • What happened: no prompt, database not corrupted

Procedure C. Procedure B, but choose yes in step 6

  • What happened: I get the prompt, choose "yes" and database is not corrupted

Procedure D. Procedure A, but restart KeePassXC immediately following step 6

  • What happened: I get the prompt, I choose no, then quit KXC. Re-open KXC, attempt to unlock --> DB corrupted

Procedure E. Procedure A, but don't lock the database in step 3. In other words: leave the DB open (unlocked) while AirDropping it .

  • What happened: I get the prompt, choose "no". Database remains open. When I next attempt to lock it, I get different prompt which reads: "This database has been modified. Do you want to save the database before locking it? Otherwise your changes are lost." I think I selected not to save. It did not result in a corrupted database.

These remaining three procedures I didn't have time to check tonight, but I'm assuming similar results to Procedure E.
Procedure F: B, but leave DB unlocked while AirDropping it
Procedure G: C, but leave DB unlocked while AirDropping it
Procedure H: D, but leave DB unlocked while AirDropping it

Throughout my tests I was always able to open the test databases on my iPhone using MiniKeePass. However I admit I didn't test this after completing each procedure.

Contributor

sts10 commented Oct 25, 2017

Glad to see we've maybe caught one bug!

I was busy today but I got to try a few of the procedures @phoerious requested above.

Let's call my original procedure of steps described in my opening of this issue as "Procedure A"

Procedure B: Procedure A, but have the automatic reload setting on (checked)

  • What happened: no prompt, database not corrupted

Procedure C. Procedure B, but choose yes in step 6

  • What happened: I get the prompt, choose "yes" and database is not corrupted

Procedure D. Procedure A, but restart KeePassXC immediately following step 6

  • What happened: I get the prompt, I choose no, then quit KXC. Re-open KXC, attempt to unlock --> DB corrupted

Procedure E. Procedure A, but don't lock the database in step 3. In other words: leave the DB open (unlocked) while AirDropping it .

  • What happened: I get the prompt, choose "no". Database remains open. When I next attempt to lock it, I get different prompt which reads: "This database has been modified. Do you want to save the database before locking it? Otherwise your changes are lost." I think I selected not to save. It did not result in a corrupted database.

These remaining three procedures I didn't have time to check tonight, but I'm assuming similar results to Procedure E.
Procedure F: B, but leave DB unlocked while AirDropping it
Procedure G: C, but leave DB unlocked while AirDropping it
Procedure H: D, but leave DB unlocked while AirDropping it

Throughout my tests I was always able to open the test databases on my iPhone using MiniKeePass. However I admit I didn't test this after completing each procedure.

@louib

This comment has been minimized.

Show comment
Hide comment
@louib

louib Oct 25, 2017

Member

This is pretty bad. I was able to reproduce just with touch ~/database.kdbx. The corruption arises when we try to save a database that is not unlocked!

  1. Disable autoreload
  2. Open any database
  3. Lock it
  4. touch path/to/db.kdbx
  5. Say "no" to reload database prompt.
  6. If autosave is disabled, then quit the application and save changes. If autosave is enabled, the database is already saved and corrupted.
Member

louib commented Oct 25, 2017

This is pretty bad. I was able to reproduce just with touch ~/database.kdbx. The corruption arises when we try to save a database that is not unlocked!

  1. Disable autoreload
  2. Open any database
  3. Lock it
  4. touch path/to/db.kdbx
  5. Say "no" to reload database prompt.
  6. If autosave is disabled, then quit the application and save changes. If autosave is enabled, the database is already saved and corrupted.
@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Oct 25, 2017

Member

Okay, so it seems that messing with the file modification time, although the file wasn't modified somehow causes corruption.

Member

phoerious commented Oct 25, 2017

Okay, so it seems that messing with the file modification time, although the file wasn't modified somehow causes corruption.

@droidmonkey

This comment has been minimized.

Show comment
Hide comment
@droidmonkey

droidmonkey Oct 25, 2017

Member

The key to this is a bad Interaction between autosave and auto reload when the database is locked

Member

droidmonkey commented Oct 25, 2017

The key to this is a bad Interaction between autosave and auto reload when the database is locked

@phoerious

This comment has been minimized.

Show comment
Hide comment
@phoerious

phoerious Oct 25, 2017

Member

We already have a safeguard to prevent auto-reload while the database is being saved. But perhaps it's not working as expected?

Member

phoerious commented Oct 25, 2017

We already have a safeguard to prevent auto-reload while the database is being saved. But perhaps it's not working as expected?

@louib

This comment has been minimized.

Show comment
Hide comment
@louib

louib Oct 25, 2017

Member

I doesn't have to happen at the same time (autoreload and autosave). The save can happen after the reload and the problem will still occur. Also, both options can be disabled and there's still gonna be a way for a user to corrupt it's database. the problem is twofold

  1. when refusing to autoreload when the database is locked, we mark it as dirty, making it available for saving (this is fixed by #1119)
  2. when saving the database, we have no way to tell if it's a legit database or if it's a placeholder database created when the legit database was locked (see https://github.com/keepassxreboot/keepassxc/blob/develop/src/gui/DatabaseWidget.cpp#L1084)

I think this is where the corruption comes from. We overwrite the database with an empty database that was created as a placeholder when locking

Member

louib commented Oct 25, 2017

I doesn't have to happen at the same time (autoreload and autosave). The save can happen after the reload and the problem will still occur. Also, both options can be disabled and there's still gonna be a way for a user to corrupt it's database. the problem is twofold

  1. when refusing to autoreload when the database is locked, we mark it as dirty, making it available for saving (this is fixed by #1119)
  2. when saving the database, we have no way to tell if it's a legit database or if it's a placeholder database created when the legit database was locked (see https://github.com/keepassxreboot/keepassxc/blob/develop/src/gui/DatabaseWidget.cpp#L1084)

I think this is where the corruption comes from. We overwrite the database with an empty database that was created as a placeholder when locking

@droidmonkey droidmonkey removed the in triage label Nov 23, 2017

droidmonkey added a commit that referenced this issue Nov 23, 2017

@TheZ3ro TheZ3ro closed this Dec 6, 2017

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