PAM fscrypt adds root(0) group to all users called by su

Mark, nice discovery, thanks for the report. I've asked upstream fscrypt authors for assistance.

Thanks

Mark, thank you so much for finding this bug.

I'm the maintainer for fscrypt, and the attached patch (when applied to fscrypt's master branch) seems to resolve this issue. I ran a few tests on my local system (along with fscrypt's existing tests) and everything still seems to be working.

The bug was caused by the fscrypt PAM module's privilege dropping code. It would drop privileges (for security, ironically enough) before performing unprivileged actions on behalf of the user. However, when restoring privileges before the PAM module exited, the EUID EGID and groups were all set back to root. However, when running su, the GID and groups in a PAM module are those of the user (while the UID is root). So this restored _too many_ privileges making the user part of the root group.

Let me know if there are any issues, if this patch breaks things, or if it doesn't fix the bug.

Joe

Hi Joe,
thank you very much for such a swift response and fixing the bug!

And also for the very useful explanation, which let's me better understand the patch.
I'm sorry though, I won't be able to test your patch (I'm traveling to a conference and I won't be able to setup my system to compile fscrypt from source soon). But I bet you've tested it well.

If you don't mind, I have some comments to the code/patch (bear with me, I don't speak GO and know the code of pam-fscrypt only from this patch, so I may be completely stupid..)

> // StopAsPamUser restores the original privileges that were running the
 // PAM module (this is usually root). As this error is often ignored in a defer
 // statement, any error is also logged.
 func (h *Handle) StopAsPamUser() error {
- err := security.SetProcessPrivileges(h.OrigUser)
+ err := security.SetProcessPrivileges(h.origPrivs)
  if err != nil {
   log.Print(err)
  }

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

> + n, err := C.getgroups(C.int(len(groups)), &groups[0])
+ if n < 0 {
+ return nil, err
+ }

Couple of things here, although likely just nitpiks.
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?
if could be if n < 1: error;
on the line "n, err :=" you don't check for err, you do it everywhere else. But probably getgroups cannot fail and only returns n>0 if no err?

> +// specified by privs. The original privileges can be by first saving the output
typo: can be ..obtained?..by

The patch looks like a nice cleanup that simplifies things.
Thank you for your work on the project!
cheers, Mark

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

The issue has been assigned CVE-2018-6558

@Joe I just noticed that the proposed fix breaks the ability to do a simple 'su - <USER>' where <USER> is not root or the current user and the current user is unprivileged:

$ su - fs
Password:
su: System error

Here's the debug logging from pam_fscrypt:

pam_fscrypt[5468]: Authenticate()
pam_fscrypt[5468]: keyringID(_uid.1001) = 834965334, <nil>
pam_fscrypt[5468]: KeyctlLink(834965334, -2) = <nil>
pam_fscrypt[5468]: keyringID(session) = 132871448, <nil>
pam_fscrypt[5468]: KeyctlSearch(132871448, keyring, _uid.1001) = -1, required key not available
pam_fscrypt[5468]: Setting up keyrings in PAM: user keyring not linked into session keyring
pam_fscrypt[5468]: Setting euid=1001 egid=1001 groups=[1001]
pam_fscrypt[5468]: pam func failed: setting egid: operation not permitted

In the comand above, UID 1000 is invoking su and attempting to change UID 1001.

The debug logging doesn't mention what the original euid, egid, and supplementary groups are when the pam_fscrypt Authenticate() function is first entered but I suspect that the euid is the is that of the original user invoking su (1000). The process doesn't have sufficient privs to switch to egid 1001, Authenicate() returns an error, and the whole thing fails. I'm not certain but maybe the correct thing to do in this case is return PAM_IGNORE rather than PAM_SERVICE_ERR.

So the attached patch set should solve the issue Tyler pointed out. It ended up not being an issue with the PAM return codes, but an issue with the privs themselves. The "System Error" comes the privs not being properly restored.

It was a little more involved to fix all of the privilege changing code, but now everything should be nice and reversible, no matter what the original privs.

Thanks, the new patches do fix the regression that I mentioned in comment 6.

Release v0.2.4 is out containing the fix:
https://github.com/google/fscrypt/releases/tag/v0.2.4

I've uploaded an fscrypt security update to the Ubuntu Security PPA. Ubuntu Security will release it once they've reviewed and approved the changes.

This bug was fixed in the package fscrypt - 0.2.2-0ubuntu2.1

---------------
fscrypt (0.2.2-0ubuntu2.1) bionic-security; urgency=medium

  * SECURITY UPDATE: Privilege escalation via improperly restored
    supplementary groups in libpam-fscrypt (LP: #1787548)
    - CVE-2018-6558.patch: Save the euid, egid, and supplementary groups when
      entering the PAM module, drop privileges to perform actions on behalf of
      the user, and then properly restore the saved values before exiting the
      PAM module. Based on patch from upstream.
    - CVE-2018-6558
  * 0001-security-drop-and-regain-privileges-in-all-threads.patch: Drop and
    regain privileges in all threads of the current process
  * 0001-Ensure-keyring-privilege-changes-are-reversible.patch: Ensure keyring
    privilege changes are reversible to prevent failures when, for example,
    "su <user>" is executed as an unprivileged user

 -- Tyler Hicks <email address hidden> Wed, 22 Aug 2018 18:57:26 +0000