Database does not lock if it has unsaved changes

[lopopolo]

Expected Behavior

When the setting "lock databases when session is locked or lid is closed" is enabled, the database should be locked on sleep even if the about menu is showing

Current Behavior

Database remains unlocked with about menu active

Possible Solution

Steps to Reproduce (for bugs)

1. Unlock database
2. Choose menu item KeepassXC > About KeepassXC
3. Select Debug Info tab
4. Put computer to sleep
5. Unsleep computer

Context

Debug Info

KeePassXC - Version 2.2.0
Revision: caa49a8

Libraries:

• Qt 5.9.0
• libgcrypt 1.7.7

Operating system: macOS Sierra (10.12)
CPU architecture: x86_64
Kernel: darwin 16.6.0

Enabled extensions:

• KeePassHTTP
• Auto-Type
• YubiKey

[sainaen]

Yep.
The issue is with MainWindow::lockDatabasesAfterInactivity(), which is called by MainWindow::handleScreenLock(): it doesn't lock DBs if there's an open modal dialog.

Making it always lock is an easy fix, but maybe the lockDatabasesAfterInactivity() should be changed instead, so it doesn't consider the About dialog as a reason not to lock.

[hifi]

Additionally it doesn't lock if you have unsaved changes. I don't know how that should be handled.

Save to a temporary file and lock both databases? Does it need its own issue or can it be discarded as intended behavior?

[qk4l]

Additionally it doesn't lock if you have unsaved changes. I don't know how that should be handled.

We can avoid this by disabling setting "lock databases" settings if "autosave db" is not enable.
Not the best option for user experience but better for security.

[louib]

@sainaen

Making it always lock is an easy fix, but maybe the lockDatabasesAfterInactivity() should be changed instead, so it doesn't consider the About dialog as a reason not to lock.

I agree with that. Here's the original reason for not locking when a modal is open (from commit 721bec9):

    Make sure we don't lock the database while a dialog is open.
    
    This can happen when
    - the user is picking out a file to save the database as
    - a dialog asking the user to save/discard/cancel the current database
      changes is active
    
    It is dangerous to lock the databases while these actions are still
    in progress.

MainWindow::lockDatabasesAfterInactivity() could prevent locking for the cases deemed dangerous, but proceed with locking for the other cases (including the about window, for example).

@keepassxreboot/core-developers what do you think? I don't know how easy that fix would be.

[TheZ3ro]

MainWindow::lockDatabasesAfterInactivity() could prevent locking for the cases deemed dangerous, but proceed with locking for the other cases (including the about window, for example).

I think it's the right thing to do. Maybe using a flag to prevent locking, setting the flag to true when a dangerous action is being made.
Can someone look into this?

[seatedscribe]

I'm available for this task! On 12 Sep 2017 1:22 pm, "TheZ3ro" <notifications@github.com> wrote: MainWindow::lockDatabasesAfterInactivity() could prevent locking for the cases deemed dangerous, but proceed with locking for the other cases (including the about window, for example). I think it's the right thing to do. Maybe using a flag to prevent locking, setting the flag to true when a dangerous action is being made. Can someone look into this? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#721 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMqpZPKVGmFg7S6R8IQAWR6U73xGVOEPks5shmmCgaJpZM4OJ1uD> .

[droidmonkey]

I am moving this to 2.3.0 because it may require refactoring how saving and other settings are handled.

[anshulbajpai]

Is this fixed in v2.3.0? Or will it be tackled in #1234 ?

[TheZ3ro]

It's not fixed and #1234 won't fix it

[anshulbajpai]

Thanks, I was confused because the milestone still says 2.3.0.

[droidmonkey]

v2.3.0 is still in development (ie not released)

[anshulbajpai]

Sorry, my bad. I read the release 2.2.4 as 2.4.2.

[sergeevabc]

I had locked the desktop by Win+L, but when came back there was the following window. It was expected the database would have been mandatory saved then locked (disclosure: longtime Keepass user). There are internal history and recycle bin along with external backups to roll back if something wrong happens. I am glad this was caught with the test database, outside the production environment. Perhaps we should create a Pitfalls section on the front page to warn about “known but so far unresolved sensitive issues”?

[michaelk83]

(continuing from #6593)

It does deserve some attention though. It's been marked high priority, but has been put off since version 2.2.

Not put off, we have been fixing this per-dialog as we find problems. That issue mainly centers around what to do with a modified database on lock.

Wouldn't this comment be the general solution that should be applied to all dialogs except the few dangerous cases that were outlined in this earlier comment?

Rather than chasing this bug one dialog at a time, identify which specific cases are dangerous, and then apply the general solution everywhere else.

[michaelk83]

Actually, I'd even argue that the middle option of the proposed solution (save to temporary file, then merge) is still safe even for the two cases already identified as dangerous:

• the user is picking out a file to save the database as
• a dialog asking the user to save/discard/cancel the current database changes is active

In both cases, the changes are saved to a temporary file, and both DBs are locked. Thus, both the changes and the original DB are safely preserved, and the process can be resumed when appropriate.

So I propose the following extended solution:

1. If any dialog is open when the DB needs to be locked, save any changes to a temporary file, discard changes in the original, and lock both.
2. When the save dialog completes, if the the DB was locked as above:
   • If the user selected a file name and OK'ed, delete the original, and rename the temporary file (same as during a regular save with atomic move).
   • If the user canceled, ask whether to merge or discard the changes from the temporary file.
3. For the save/discard/cancel dialog, if the DB was locked:
   • Save proceeds to rename the temporary file.
   • Discard deletes the temporary file.
   • Cancel should probably be disabled when the DB is locked, otherwise you'll just have to ask almost the same question again: "merge or discard temporary file?". Maybe add a tooltip: "Database was locked. The operation cannot be canceled."
4. For all other dialogs, ask whether to merge or discard the temporary file at the appropriate time (e.g. when the DB is unlocked again).
5. For the "merge or discard temporary file?" dialog itself, the DB was already locked. If this dialog is shown as soon as the original DB is unlocked, then there are no new changes to the DB, and the previous changes are in the already existing temporary file, so the DB can be safely relocked if needed. Since both "merge" and "discard" are just atomic file delete and rename operations, the DB doesn't need to be unlocked for this dialog to complete. This can even be handled by the same dialog as point 3.

Anyway, the MVP is still just to add that setting and apply the selected action to all safe dialogs. The above extra handling of the dangerous dialogs can be implemented separately.

[jurgenhaas]

Not sure if this is the right place but as all the other lock issues got linked here and closed elsewhere, I'd like to add my lock issue here too: I'm on Ubuntu 20 and always keep KeePassXC in the background, but no other dialog is open and no changes have been made. When locking the desktop and coming back later, the database of KeePassXC is locked too, as expected. But it seems, that only works for a period of time (locking once per day for a couple of days) and then suddenly doesn't work anymore. As if a service or system status wasn't functioning properly after a while.

I know, this is such a vague issue description, that it's impossible to help based on this. I'd be happy to dig into the logs, if I only knew what I should be looking for. Anyone with an idea, what component could possibly influence that behavior for that matter?

[droidmonkey]

Locking keepassxc when the computer locks depends on a dbus signal that is sent out by your screen lock program. No signal, no lock. Check that dbus signal is being emitted every time your desktop locks.

[jurgenhaas]

There is this signal when locking:

signal time=1631695790.794009 sender=:1.33 -> destination=(null destination) serial=15758 path=/org/gnome/SessionManager; interface=org.freedesktop.DBus.Properties; member=PropertiesChanged
   string "org.gnome.SessionManager"
   array [
      dict entry(
         string "SessionIsActive"
         variant             boolean false
      )
   ]
   array [
   ]

and that one when coming back:

signal time=1631695790.794009 sender=:1.33 -> destination=(null destination) serial=15758 path=/org/gnome/SessionManager; interface=org.freedesktop.DBus.Properties; member=PropertiesChanged
   string "org.gnome.SessionManager"
   array [
      dict entry(
         string "SessionIsActive"
         variant             boolean false
      )
   ]
   array [
   ]

What's interesting: having a radio stream on, that gets properly interrupted when locking this way, but KeePassXC doesn't.

[droidmonkey]

Yah that isn't going to cut it, here is the code which is very readable that shows the signals listened for. This is decidedly another issue then this one but had also come up before.

https://github.com/keepassxreboot/keepassxc/blob/develop/src/gui/osutils/nixutils/ScreenLockListenerDBus.cpp

[Simplenuity]

As lock issues have been linked and discussed here, I'm commenting here.

#9992: The situation got worse due to the change of what is triggering the save button to be enabled (related to #9697?). Compared to version 2.7.0 (tested with KeePassXC-2.7.0-x86_64.AppImage), version 2.7.6 (deb installation) now enables the save button also when changes occur to the state of the group tree, i.e. by opening or closing a group. In my case, choosing an entry or even opening it is not enabling the save button as long as I don't change any data.

Beside the other aspects already brought up in this thread, it seems necessary to decide:

• what actions should enable the save button, like:

  • data change
  • non-data change, like:
    • state of navigation
    • search results
    • state of a selected entry

• whether the user should be able to select these/some of these via options in settings. Currently there is only an option to auto-save non-data changes.