keepassxreboot / keepassxc

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

Database does not lock when the session is locked #8194

Open WhyNotHugo opened 2 years ago

WhyNotHugo commented 2 years ago

Overview

The "Lock databases when session is locked or lid is closed" checkbox is check. However, the databases are never locked when locking the screen.

Steps to Reproduce

  1. Make sure the checkbox is checked.
  2. loginctl lock-session

Expected Behavior

The databases should become locked.

Actual Behavior

The databases remain unlocked.

Context

I'm using swaywm. I'm clarifying this in case some Xorg-specific hints are used to detect if the screen is locked. AFAIK, wayland has no mechanism for clients to determine if the session is locked (hence why logind is handling it for me).

Tried disabling my Firejail sandbox just in case that was getting in the way.

KeePassXC - Version 2.7.1
Revision: 5916a8f

Qt 5.15.5
Debugging mode is disabled.

Operating system: Arch Linux
CPU architecture: x86_64
Kernel: linux 5.18.5-arch1-1

Enabled extensions:
- Auto-Type
- Browser Integration
- SSH Agent
- KeeShare
- YubiKey
- Secret Service Integration

Cryptographic libraries:
- Botan 2.19.2

Operating System: Linux Desktop Env: swaywm Windowing System: Wayland

WhyNotHugo commented 2 years ago

Huh, based on https://github.com/keepassxreboot/keepassxc/issues/3339, this should work: https://github.com/keepassxreboot/keepassxc/blob/4e7aeca3b7ab4e943f22b6bab5d720ea0471290f/src/core/ScreenLockListenerDBus.cpp

droidmonkey commented 2 years ago

Use dbus-monitor to confirm that the message is actually sent when you lock the screen.

WhyNotHugo commented 2 years ago

Use dbus-monitor to confirm that the message is actually sent when you lock the screen.

loginctl lock-session is exiting zero, and systemd-lock-handler indicates that org.freedesktop.login1.Session.Lock is being emitted. Additionally, systemd-lock-handler runs my actual screen locker (swaylock) in response to this signal, so I'm very certain the signal is working correctly.

droidmonkey commented 2 years ago

At this point you need to confirm that keepassxc is actually receiving that signal. If we do receive it and don't process it correctly then we have a bug.

WhyNotHugo commented 2 years ago

How can I confirm if it receives the signal? There's nothing in stdout.

WhyNotHugo commented 2 years ago

I can't imagine a single client not receiving a signal. I'm not sure how to confirm if KP is receiving in in this case, any hints are welcome.

droidmonkey commented 2 years ago

Use dbus monitoring tools, they show who is listening to the bus and whether messages are delivered and acknowledged. Bustle or dbus-monitor.

WhyNotHugo commented 2 years ago

dbus-monitor does not indicate when an application receives a signal, only when it is emitted. I've worked with several differetnpf D-Bus clients and servers and I haven't seen any way to confirm that a client receives a signal other than the client itself logging it. But I don't see keepassxc having anything like a --verbose flag tho.

When the signal is emitted, dbus-monitor only shows a single line referencing it:

signal time=1656172166.324371 sender=:1.8 -> destination=(null destination) serial=5475 path=/org/freedesktop/login1/session/_31; interface=org.freedesktop.login1.Session; member=Lock

systemd-lock-handler does receive a signal (it also logs that it's received the signal) and starts the screenlocker, so this confirms that (a) clients receive the signal and (b) dbus-monitor yields no indicator that clients receive the signal.

I believe that if KeePassXC itself does not receive the signal, it would imply a rather unusual bug in the D-Bus daemon itself -- I don't expect that to be the case TBH.

WhyNotHugo commented 2 years ago

Bustle yields no additional info either:

image

WhyNotHugo commented 2 years ago

Oh, right, figured out what's broken. KeePassXC tries to determine the current session id reading the XDG_SESSION_ID environment variable:

https://github.com/keepassxreboot/keepassxc/blob/4e7aeca3b7ab4e943f22b6bab5d720ea0471290f/src/core/ScreenLockListenerDBus.cpp#L59-L65

This variable is unset in my case. I could figure out how to export it, but that sounds like a bit of a hack. I assume that maybe some desktop environments make sure this variable is passed to all application, hence why it works in some cases?

The current session id can be obtained via the /org/freedesktop/login1/session/auto object, reading the org.freedesktop.login1.Session.Id property. Reading it from this prop should fix the issue.

droidmonkey commented 2 years ago

You probably need to fix your firejail config to pull that value in. I am sure other things could be 'off by not having this defined. https://www.freedesktop.org/software/systemd/man/pam_systemd.html#%24XDG_SESSION_ID

The current session id can be obtained via the /org/freedesktop/login1/session/auto object, reading the org.freedesktop.login1.Session.Id property. Reading it from this prop should fix the issue

Are you sure this always yields the expected results?

WhyNotHugo commented 2 years ago

It’s undefined in an unsandboxed terminal too. I don’t think we should assume that a variable is set. The same DBus API where we await the signal exposes the correct session id.

WhyNotHugo commented 2 years ago

Are you sure this always yields the expected results?

Indeed, this is the correct way to determine the current session. That is the sole purpose of this API too.

I cannot reconfigure firejail to somehow inject the XDG_SESSION_ID variable, because Firejail has not knowledge of the current XDG session either. I would need some wrapper script that read the /org/freedesktop/login1/session/auto d-bus object to determine the session id, and then pass that onto Firejail/KeePassXC as XDG_SESSION_ID... but that's a huge hack, it would make sense for KPXC to simply do that itself.

It seems relatively simple in this context, but as previous PRs have surely proven, my C++ abilities are terrible :disappointed:

ShellCode33 commented 1 year ago

I think it would be possible to read /proc/self/sessionid instead of relying on XDG_SESSION_ID.

I didn't try it but in the meantime, I think you could workaround this by creating a wrapper script in /usr/local/bin/keepassxc that would contain the following:

#!/bin/sh
export XDG_SESSION_ID="$(cat /proc/self/sessionid)"
exec /usr/bin/keepassxc

For this to work, /usr/local/bin must be before /usr/bin in your PATH.