Keepassxc: Database does not lock with session if any dialog is visible

[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: caa49a8ef3ee28ed478192389b21d61107b3b8e0

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 721bec97949):

    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" [e-mail] 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
https://github.com/keepassxreboot/keepassxc/issues/721#issuecomment-328823250,
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”?

[ballrock2]

Could it be a solution to make an option in settings:
"Autosave changes (if nessecary) when auto-locking KeePass"?

The auto-lock is very unusefull when it doesn't lock the database sure. Because when notebook is closed f.E. and KeePass is forgotten, then an open database with security data is in system. This is -in my opinion- the worst case.

Best regards
Andreas

(using KPXC 2.3.4)

[droidmonkey]

Yes there should be a setting that the user can toggle which would be something like:

"When automatically locking databases, perform the following action:"

• Force save and lock (default)
• Ask to merge changes after unlock
• Do nothing (dangerous)

Option 2 would save the database to a temporary file (eg, passwords.kdbx.temp). When this database is next unlocked, the user would be prompted to merge changes from passwords.kdbx.temp. Ideally this would be coupled with the ability to actually see what those changes are. This is covered by a different issue.

[DJCrashdummy]

https://github.com/keepassxreboot/keepassxc/issues/721#issuecomment-474123157 seems the right way to go from the UX-point of view, but IMHO option 2 should be the default!

...i still don't get it why auto-save got the default, but that's another story. :confused: