Comment 4 for bug 1787548

Hey Mark,

Thanks for the comments. I incorporated them into the revised patch.

> I'm surprised by the comment "this err is often ignored..", as this seems the critical part (returning to orig privileges). Although it returns error(?) so a failure is probably handeled elsewhere on a higher layer. (Actually, how is this handeled? - if there's an err in privilege escalation, it's ok, you'd just say "sorry, fscrypt failed to run"; but if deescalation fails - what'd you do? Kill the process, kernel panic?)

This turns out to be a fairly tricky issue as pam_fscrypt runs as "optional" for Auth/Session/Password, meaning a PAM_SERVICE_ERR it returns will only end up being logged in the default configuration (as errors with the user's fscrypt setup _should not_ block login). Right now I've just removed the comment about the error being ignored (you're right it should just be handled at a higher level).

Killing the process isn't an option because it would also block login. However, now the the privileges changing code is correct, a failure at any point in pam_fscrypt should no longer result in an insecure state (although it may put su or the login process in a bad state). For example, now if StopAsPamUser fails to run at all, running "su test" results in a system error.

I should note that the eventual goal is to remove most of the PAM code and integrate most the session control stuff with the init system. But that's a long way off. See https://github.com/google/fscrypt/issues/95

> You index groups w/o checking for size>0, although I wasn't able to create a user who is not in atleast 1 grp. maybe the make() can fail?

Good catch on this! I've updated the code to handle the "user in 0 groups case". I was able to get a process executing in which getgroups() returned an empty array, so the code should handle it. The main issue here isn't the make() failing, but the call to setgroups() killing the process when using &privs.groups[0] without a bounds check.

> typo: can be ..obtained?..by

Fixed