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]

After investigating this a bit, I came up with this scheme:

• Write a small wrapper for KeePassXC which runs as a systemd service kpxc-starter.{service,socket}.
• It listens on a UNIX socket for the database password, and starts KeePassXC with --pw-stdin when it is received.

Theoretically plain UNIX sockets are much more secure than D-Bus, and I can implement whatever encryption I'd like on top of that.
The wrapper is needed because KeePassXC's behaviour with --pw-stdin doesn't really allow for retries, or any kind of more complex logic. Also this way I can delay the start of KeePassXC until the graphical session is fully initialised much more easily.

[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!

[michaelk83]

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).

I was under the impression that PAM needs to run as root, since it's part of the system's security layer. But apparently, not always. But according to that link, at least the user authentication module does need to run as root, to read /etc/shadow.

What was supposed to make my approach more secure is the root:root 600 client authentication file, and subsequent executable hash check. But if the client can impersonate the legit KeePassXC, then this doesn't help, because you'd just be checking the KeePassXC hash, instead of the actual client. I re-skimmed the client detection discussion over at KeePassXC (link + subsequent comments), and you're right: /proc/[pid]/exe can still be spoofed.

So given that, calling the correct executable explicitly, and then passing it the password, is indeed the better approach than trying to deal with "Who's asking?".

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.

If the SystemD service file is installed in user space (systemctl --user), then it may be writable by the user. In that case, it's trivial to modify.

If you're going to use the stdin method with a hard-coded path, then I'm not sure if these checks are still useful at all. You may as well just go with a root:root 600 (or 644, if you don't want to run this as root) configuration file telling you exactly which executable to call, and what the expected hash of it is. Faking the executable hash should be near-impossible, as long you're sure that that's where you're passing the password to (which should indeed be the case with the stdin method).

[m00nwtchr]

If the SystemD service file is installed in user space (systemctl --user), then it may be writable by the user. In that case, it's trivial to modify.

That's why I'm checking the validity of the service by verifying that the path it is referring to is the correct one. It doesn't matter that the service was modified if the binary it executes is still /usr/bin/keepassxc, because overwriting that would take root access.

[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.)

[michaelk83]

There's an ongoing effort over at KeePassXC to separate their core from their GUI and make a libkdbx. The next steps are keepassxreboot/keepassxc#11003 and keepassxreboot/keepassxc#8480 , and the Secret Service part is in keepassxreboot/keepassxc#8591 , waiting for the the other two.

[michaelk83]

One more thought (which I'm putting in a separate thread):

You could require that the actual database password be the sha256 (or whatever) hash of the login password, and recommend that it be a separate database from the user's main passwords database. If the use case is for Secret Service, it's a good idea to separate that from other passwords; and if the user is trying to unlock their main database on login, they can use the AutoOpen feature in the helper database.

This would have several advantages:

1. If the password (hash) is compromised, at least this wouldn't expose sudo.
2. If you hash the login password as soon as you get it, and only keep the hash in memory, your memory protection doesn't need to be as strict.
3. Separation of (local) Secret Service passwords from the user's other password: good for security and portability.
4. The main database can use whatever other credentials, and isn't tied to the login password.

As a byproduct of 3, after they separate their GUI from core, the Secret Service database can run via daemon, and main database via GUI, and it wouldn't matter how those two interacted (client-server or parallel).