Comment # 6 on bug 1208684 from Matthias Gerstner

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.