Bugzilla – Bug 1208684
AUDIT-0: pam_kwallet: reconsider adding pam_kwallet to default installation patterns
Last modified: 2023-03-13 12:01:54 UTC
It appears as though that due to security audits in the past, pam_kwallet wasn't able to make it default. Please consider a new audit. If a recent audit passed please let me know and I will figure out where to go next to figure out how to request this gets shipped default w/ plasma versions of TW. SOURCE of REQUEST: I found about this issue, and learned that this package would make life so much easier for so many users. LINK: https://bugzilla.opensuse.org/show_bug.cgi?id=1193348 "This will lead to the expected behavior of the kwallet automatically being unlocked on (manual) login. This package is included by default on the KDE versions of many other distributions (ex. Debian, Kubuntu) and is very convenient for users utilizing functionality that requires opening the wallet at login. For example, the user is prompted to set/enter their kwallet password immediately after login when connecting to a wireless network that does not have its keys saved globally. It seems like this would be a quality of life improvement for many users. For example, on the r/opensuse subreddit there are many posts complaining about KDE wallet asking for a password on every login (with the solution almost universally being to either install pam_kwallet or to set a blank/unencrypted KDE wallet password)"
The major audit of the module was done in bug 993806 and the PAM module was not in good shape back then. But can you please explain what you mean by "make it default"? As security team we only decide whether a PAM module is allowed into the distribution at all, not really how and when it is installed and configured. So maybe you need to involve the package's maintainer rather than us. The package does not seem to have a dedicated maintainer any more though. Adding Fabian as main contributer and KDE project maintainer instead.
(In reply to Matthias Gerstner from comment #1) > The major audit of the module was done in bug 993806 and the PAM module was > not in good shape back then. > > But can you please explain what you mean by "make it default"? As security > team we only decide whether a PAM module is allowed into the distribution at > all, not really how and when it is installed and configured. The module is in the distro (it was even long before bug 993806), but not installed by default. Quoting you from that bug report: > Enabling it by default is a no-go at the moment from security perspective. If this changed meanwhile, we can include it in the Plasma pattern selection. > So maybe you need > to involve the package's maintainer rather than us. The package does not seem > to have a dedicated maintainer any more though. Adding Fabian as main > contributer and KDE project maintainer instead.
(In reply to fabian@ritter-vogt.de from comment #2) > The module is in the distro (it was even long before bug 993806), but not installed by default. Quoting you from that bug report: > > > Enabling it by default is a no-go at the moment from security perspective. > > If this changed meanwhile, we can include it in the Plasma pattern selection. Ah I missed that point. Well then I will take another tour of the current codebase and judge from that whether this restriction can be lifted.
It will take some more time (about two weeks) before I can look into this due to other tasks.
I will look into this now.
So most of the worrying stuff in the code of this module has been fixed since the last audit. Still the code base is not very straightforward or clean. A couple of things I noted: - how should it ever happen that `pam_sm_open_session` is called before `pam_sm_authenticate`? There is some logic in there to handle that case, but that would be a bug int he PAM stack configuration IMO. - in `readSaltFile()` the order of waitpid() / read pipe is wrong. The child process might block writing on the pipe, never able to exit. Usually the parent should be reading from the pipe until an EOF condition is seen on the pipe, only then call `waitpid()`. Since only some fifty bytes are exchanged here it doesn't matter though, because the pipe is able to buffer that data. - there are a couple of error exits in `pam_sm_open_session` that leak the `to_wallet_pipe` file descriptors. This could cause user processes to inherit these in error conditions. - stderr is closed in the kwallet child without being replaced by something sensible (like /dev/null), the logic behind that is unclear. - the password wiping in `wipeString()` seems overzealous in the light that this password string remains in PAM data structures in two places. The privilege drops look okay for both the salt extraction and the invocation of kwallet in user context. In the su(do) context some environment variable trickery could still be played out e.g. to have the socket created somewhere else than intended. Since all of this happens with target user privileges the impact should be uncritical.
So in summary I am okay with adding this to a default installation pattern. @Fabian, can you take care of that change or who else might be a proper assignee for it?
(In reply to Matthias Gerstner from comment #7) > So in summary I am okay with adding this to a default installation pattern. > @Fabian, can you take care of that change or who else might be a proper > assignee for it? \o/ Submitted: https://build.opensuse.org/request/show/1070190 (In reply to Matthias Gerstner from comment #6) > So most of the worrying stuff in the code of this module has been fixed since > the last audit. Still the code base is not very straightforward or clean. Agreed. In the age of systemd user sessions, credentials and related stuff I wonder whether there's a better way to do this. > A couple of things I noted: > > - how should it ever happen that `pam_sm_open_session` is called before > `pam_sm_authenticate`? There is some logic in there to handle that case, > but > that would be a bug int he PAM stack configuration IMO. It's unfortunately really easy to have misconfigured PAM or some software which uses PAM slightly wrong (also due to implementation differences and bugs), so unless there's a way this fallback can have a negative impact I'd keep it around. For Plasma 6 we could remove it and see what happens. > - in `readSaltFile()` the order of waitpid() / read pipe is wrong. The child > process might block writing on the pipe, never able to exit. Usually the > parent should be reading from the pipe until an EOF condition is seen on > the > pipe, only then call `waitpid()`. Since only some fifty bytes are exchanged > here it doesn't matter though, because the pipe is able to buffer that > data. Agreed. > - there are a couple of error exits in `pam_sm_open_session` that leak the > `to_wallet_pipe` file descriptors. This could cause user processes to > inherit these in error conditions. Confirmed. The annoying part is that at least half of the pipe can't be CLOEXEC :-/ > - stderr is closed in the kwallet child without being replaced by something > sensible (like /dev/null), the logic behind that is unclear. Looks like that's a combination of smaller (individually ok-ish) changes which ended up doing something weird. In the beginning it closed all FDs from [2,64), but this caused the env socket to end up at FD 2 which kwalletd treated as stderr (36311b6f, initial implementation) That was changed to [3,64) to keep FD 2 occupied until the socket is open. After that, FD 2 is closed (8da1a470). The loop was changed to no longer close [3,64), but assign CLOEXEC to them, but the close(2) further below was kept (8f899902). Technically the close(2) could be removed, considering that stdin and stdout are kept anyway. I suppose opening /dev/null for all three is the best option though? > - the password wiping in `wipeString()` seems overzealous in the light that > this password string remains in PAM data structures in two places. If wiping memory is seen as valuable, should those other places also be wiped (by PAM internally)? > The privilege drops look okay for both the salt extraction and the invocation > of kwallet in user context. In the su(do) context some environment variable > trickery could still be played out e.g. to have the socket created somewhere > else than intended. Since all of this happens with target user privileges the > impact should be uncritical.
(In reply to fabian@ritter-vogt.de from comment #8) > > - how should it ever happen that `pam_sm_open_session` is called before > > `pam_sm_authenticate`? There is some logic in there to handle that case, > > but > > that would be a bug int he PAM stack configuration IMO. > > It's unfortunately really easy to have misconfigured PAM or some software which uses PAM slightly wrong (also due to implementation differences and bugs), so unless there's a way this fallback can have a negative impact I'd keep it around. The fallback as such is probably okay but the fact that a buggy PAM configuration is "fixed" here can be dangerous, because this can cause good knows what, depending on the actual misconfiguration and modules involved. > The annoying part is that at least half of the pipe can't be CLOEXEC :-/ Actually, it can be, by creating the pipes CLOEXEC and only mark the write end non-CLOEXEC after the fork() but before the execve() is done. > Technically the close(2) could be removed, considering that stdin and stdout are kept anyway. I suppose opening /dev/null for all three is the best option though? Yes, having /dev/null for std descriptors (if not set to anything else) is common practice for running daemons. > > - the password wiping in `wipeString()` seems overzealous in the light that > > this password string remains in PAM data structures in two places. > > If wiping memory is seen as valuable, should those other places also be > wiped (by PAM internally)? It would be valuable hardening but doing this with full coverage, especially in this PAM context, is next to impossible. Mostly because the data is also stored in the PAM library and thus not under full control. Also because other PAM modules also process the password in the same process and likely won't wipe it. Therefore I would rather drop the logic completely.
Okay I believe this bug can be closed. Feel free to contact me in the bug or by other means if any questions remain about implementation details.
https://invent.kde.org/plasma/kwallet-pam/-/merge_requests/12 includes some fixes for other issues but also gets rid of the fallback for authentication after open_session.