https://bugzilla.suse.com/show_bug.cgi?id=1208684 https://bugzilla.suse.com/show_bug.cgi?id=1208684#c6 Matthias Gerstner <matthias.gerstner@suse.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |IN_PROGRESS --- Comment #6 from Matthias Gerstner <matthias.gerstner@suse.com> --- 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. -- You are receiving this mail because: You are on the CC list for the bug.