1st impressions

[michaelk83]

Hi, I went over the code briefly, there a few things that stood out to me:

I noticed that you use the org.keepassxc.KeePassXC.MainWindow DBus service to start KeePassXC and unlock the database. Normally, at least with Secret Service setups, KeePassXC would be started through the org.freedesktop.secrets DBus service (though that doesn't handle unlocking). See keepassxreboot/keepassxc#6274 .

This is also not the method that was discussed in keepassxreboot/keepassxc#6055 (comment) , though your current approach is simpler. I understand that you may not want to depend on KeePassXC-side code to retrieve the password and unlock itself, but the implications need to be carefully thought through. (From the KeePassXC end, we're also waiting for Full QuickUnlock, which as I understand, should implement a similar credential retrieval mechanism in a way that's more acceptable to the KeePassXC team).

It would be good to document in the RAEDME how this PAM module is expected to work, and how it interacts with the Secret Service integration (a likely common use case for wanting auto-unlock on login):
What are the steps that you expect to occur from user login until the database unlocks, or until a program requests and receives a password though Secret Service, or any other use case you'd like to document. And also: why does this need to happen through a SystemD service? (e.g. for Secret Service, DBus is usually enough).

You currently have the /usr/bin/keepassxc path hard-coded, and try to verify the executable path against that. This is ok for a start, but keep in mind that KeePassXC may be installed to a different location in some use cases. Furthermore, it might be possible to spoof the executable path in some ways. There was some talk about that over at KeePassXC when its Secret Service client detection was being developed. The above-mentioned method is more flexible with the executable path, and more robustly identifies the executable (and if you follow the links from there, you can find some of those early discussions about client detection).

I can't comment much on security aspects, but as a general rule, you should be very careful with placing security-related stuff in normal user space. For example, it's quite easy for a malicious process to intercept (if not impersonate) the org.keepassxc.KeePassXC.MainWindow DBus service to obtain the KeePassXC password (see e.g. dbus-monitor). And since in this case you're trying to use the user login password as a KeePassXC password, this would often also be the sudo password, which could lead to fairly nasty privilege escalation.
(Although conventional wisdom is that if you have a malicious process running locally, you're screwed anyway, but we still try not to add more vulnerabilities.)

The Secret Service API uses encryption over DBus to protect against DBus eavesdropping, and there was also some unfinished work to bypass DBus entirely when transferring secrets from Secret Service. AFAIK, org.keepassxc.KeePassXC.MainWindow doesn't support that.

You may also need to protect the PAM module's memory similar to what KeePassXC does. (I saw that you're using SecretString, but don't know what kind of protection that offers.) You may want to look at other PAM modules and check what they do.

Finally, there are some relevant notes in another keepassxreboot/keepassxc#6055 (comment) .

[michaelk83]

Rather than passing the password through DBus, you can pipe it in through stdin:
https://keepassxc.org/docs/KeePassXC_UserGuide#_command_line_options
keepassxreboot/keepassxc#5879

[m00nwtchr]

I noticed that you use the org.keepassxc.KeePassXC.MainWindow DBus service to start KeePassXC and unlock the database. Normally, at least with Secret Service setups, KeePassXC would be started through the org.freedesktop.secrets DBus service (though that doesn't handle unlocking). See keepassxreboot/keepassxc#6274 .

The whole reason is just that I need org.keepassxc.KeePassXC.MainWindow to be available for the unlock anyway, so using the secret service path doesn't change anything. If KeePassXC adds dbus activation on the org.freedesktop.secrets name to its native packaging I'll probably switch it.

This is also not the method that was discussed in keepassxreboot/keepassxc#6055 (comment) , though your current approach is simpler. I understand that you may not want to depend on KeePassXC-side code to retrieve the password and unlock itself, but the implications need to be carefully thought through. (From the KeePassXC end, we're also waiting for keepassxreboot/keepassxc#7020, which as I understand, should implement a similar credential retrieval mechanism in a way that's more acceptable to the KeePassXC team).

The approach given in the comment isn't actually possible, because KeePassXC sets PTRACE_SET_DUMPABLE to 0, which makes it impossible for non-root users to read KeePassXC's /proc/[pid]/exe (which can be modified by the process, anyway, so isn't secure). Hence the systemd service and verification based on the info that systemd gives, because it does not rely on accessing KeePassXC's process info directly. As far as I know the value of ExecStart is based on what the systemd service file states, so if the PID given by systemd matches the D-Bus service PID, and the systemd service hasn't been maliciously modified (hence the path check), it should be the genuine KeePassXC process. But I also don't know too much about systemd internals. To improve compatibility I could change the check to verify that the executable is owned and writable only by root, instead.

I can't comment much on security aspects, but as a general rule, you should be very careful with placing security-related stuff in normal user space. For example, it's quite easy for a malicious process to intercept (if not impersonate) the org.keepassxc.KeePassXC.MainWindow DBus service to obtain the KeePassXC password (see e.g. dbus-monitor). And since in this case you're trying to use the user login password as a KeePassXC password, this would often also be the sudo password, which could lead to fairly nasty privilege escalation.
(Although conventional wisdom is that if you have a malicious process running locally, you're screwed anyway, but we still try not to add more vulnerabilities.)

Agreed.

You may also need to protect the PAM module's memory similar to what KeePassXC does. (I saw that you're using SecretString, but don't know what kind of protection that offers.) You may want to look at other PAM modules and check what they do.

Currently all it does is ensure that the secrets are wiped from memory when they're no longer needed, regardless of how many times they're copied around in memory. (Data is overwritten when rust's Drop is invoked.) Added it just because it was simple enough to do quickly, in the future I'll probably switch to some other crate, which implements more sophisticated memory protection functionality.

The Secret Service API uses encryption over DBus to protect against DBus eavesdropping, and there was also some unfinished work to bypass DBus entirely when transferring secrets from Secret Service. AFAIK, org.keepassxc.KeePassXC.MainWindow doesn't support that.

Definitely a problem, but when I saw that KeePassXC exposes this unlock over D-Bus functionality, I figured that at least some of the security faults with this are their problem. :P

Finally, there are some relevant notes in another keepassxreboot/keepassxc#6055 (comment) .

Another reason why I'm using the systemd service, to hopefully avoid some of the ugliness of the gnome-keyring method. Instead of leaving a process started from outside the user session hanging around, the fork only exists for as long as is necessary to pass data to KeePassXC, leaving a (relatively) nicely managed process.

(Addressing this:)

In this case, the keyring service is no longer a normal service managed by systemd or the DE's auto start mechanism. That's why I call it a hack and don't want to pursue it further. But if anyone else is interested, so far I feel replicating something like the above is the best we can do.

There are a few issues created by this, though, due to the fact that KeePassXC is a GUI program, and not a background daemon, however.

Rather than passing the password through DBus, you can pipe it in through stdin:
https://keepassxc.org/docs/KeePassXC_UserGuide#_command_line_options
keepassxreboot/keepassxc#5879

I'll look into it!

[m00nwtchr]

TL;DR What we need is for KeePassXC to provide a better way to pass the unlock secrets to it, and it'd be great if it was possible to run KeePassXC's secret service as background process (though unless the GUI application is massively rewritten, it'd probably act independently and require a separate database)

(Honestly, if this project doesn't pan out, I might look into making my own KeePassXC-database-compatible secret service implementation.)