Merge pull request #103 from google/pam

Cleanup privilege dropping/raising in pam_fscrypt

5 files changed, 163 insertions, 134 deletions

diff --git a/pam/pam.go b/pam/pam.go
index ba254c8..c48dd13 100644
--- a/pam/pam.go
+++ b/pam/pam.go
@@ -35,16 +35,14 @@ import (
 	"unsafe"
 
 	"github.com/google/fscrypt/security"
-	"github.com/google/fscrypt/util"
 )
 
 // Handle wraps the C pam_handle_t type. This is used from within modules.
 type Handle struct {
-	handle *C.pam_handle_t
-	status C.int
-	// OrigUser is the user who invoked the PAM module (usually root)
-	OrigUser *user.User
-	// PamUser is the user who the PAM module is for
+	handle    *C.pam_handle_t
+	status    C.int
+	origPrivs *security.Privileges
+	// PamUser is the user for whom the PAM module is running.
 	PamUser *user.User
 }
 
@@ -62,13 +60,8 @@ func NewHandle(pamh unsafe.Pointer) (*Handle, error) {
 		return nil, err
 	}
 
-	if h.PamUser, err = user.Lookup(C.GoString(pamUsername)); err != nil {
-		return nil, err
-	}
-	if h.OrigUser, err = util.EffectiveUser(); err != nil {
-		return nil, err
-	}
-	return h, nil
+	h.PamUser, err = user.Lookup(C.GoString(pamUsername))
+	return h, err
 }
 
 func (h *Handle) setData(name string, data unsafe.Pointer, cleanup C.CleanupFunc) error {
@@ -140,14 +133,20 @@ func (h *Handle) StartAsPamUser() error {
 	if _, err := security.UserKeyringID(h.PamUser, true); err != nil {
 		log.Printf("Setting up keyrings in PAM: %v", err)
 	}
-	return security.SetProcessPrivileges(h.PamUser)
+	userPrivs, err := security.UserPrivileges(h.PamUser)
+	if err != nil {
+		return err
+	}
+	if h.origPrivs, err = security.ProcessPrivileges(); err != nil {
+		return err
+	}
+	return security.SetProcessPrivileges(userPrivs)
 }
 
 // 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.
+// PAM module (this is usually root).
 func (h *Handle) StopAsPamUser() error {
-	err := security.SetProcessPrivileges(h.OrigUser)
+	err := security.SetProcessPrivileges(h.origPrivs)
 	if err != nil {
 		log.Print(err)
 	}
diff --git a/pam_fscrypt/pam_fscrypt.go b/pam_fscrypt/pam_fscrypt.go
index e611bc6..85bd934 100644
--- a/pam_fscrypt/pam_fscrypt.go
+++ b/pam_fscrypt/pam_fscrypt.go
@@ -50,10 +50,17 @@ const (
 	cacheFlag = "drop_caches"
 )
 
+var (
+	// PamFuncs for our 4 provided methods
+	authenticateFunc = PamFunc{"Authenticate", Authenticate}
+	openSessionFunc  = PamFunc{"OpenSession", OpenSession}
+	closeSessionFunc = PamFunc{"CloseSession", CloseSession}
+	chauthtokFunc    = PamFunc{"Chauthtok", Chauthtok}
+)
+
 // Authenticate copies the AUTHTOK (if necessary) into the PAM data so it can be
 // used in pam_sm_open_session.
 func Authenticate(handle *pam.Handle, _ map[string]bool) error {
-	log.Print("Authenticate()")
 	if err := handle.StartAsPamUser(); err != nil {
 		return err
 	}
@@ -76,7 +83,6 @@ func Authenticate(handle *pam.Handle, _ map[string]bool) error {
 
 // OpenSession provisions any policies protected with the login protector.
 func OpenSession(handle *pam.Handle, _ map[string]bool) error {
-	log.Print("OpenSession()")
 	// We will always clear the the AUTHTOK data
 	defer handle.ClearData(authtokLabel)
 	// Increment the count as we add a session
@@ -150,7 +156,6 @@ func OpenSession(handle *pam.Handle, _ map[string]bool) error {
 // CloseSession can deprovision all keys provisioned at the start of the
 // session. It can also clear the cache so these changes take effect.
 func CloseSession(handle *pam.Handle, args map[string]bool) error {
-	log.Printf("CloseSession(%v)", args)
 	// Only do stuff on session close when we are the last session
 	if count, err := AdjustCount(handle, -1); err != nil || count != 0 {
 		log.Printf("count is %d and we are not locking", count)
@@ -212,7 +217,6 @@ func lockLoginPolicies(handle *pam.Handle) error {
 
 // Chauthtok rewraps the login protector when the passphrase changes.
 func Chauthtok(handle *pam.Handle, _ map[string]bool) error {
-	log.Print("Chauthtok()")
 	if err := handle.StartAsPamUser(); err != nil {
 		return err
 	}
@@ -257,7 +261,7 @@ func Chauthtok(handle *pam.Handle, _ map[string]bool) error {
 
 //export pam_sm_authenticate
 func pam_sm_authenticate(pamh unsafe.Pointer, flags, argc C.int, argv **C.char) C.int {
-	return RunPamFunc(Authenticate, pamh, argc, argv)
+	return authenticateFunc.Run(pamh, argc, argv)
 }
 
 // pam_sm_stecred needed because we use pam_sm_authenticate.
@@ -268,12 +272,12 @@ func pam_sm_setcred(pamh unsafe.Pointer, flags, argc C.int, argv **C.char) C.int
 
 //export pam_sm_open_session
 func pam_sm_open_session(pamh unsafe.Pointer, flags, argc C.int, argv **C.char) C.int {
-	return RunPamFunc(OpenSession, pamh, argc, argv)
+	return openSessionFunc.Run(pamh, argc, argv)
 }
 
 //export pam_sm_close_session
 func pam_sm_close_session(pamh unsafe.Pointer, flags, argc C.int, argv **C.char) C.int {
-	return RunPamFunc(CloseSession, pamh, argc, argv)
+	return closeSessionFunc.Run(pamh, argc, argv)
 }
 
 //export pam_sm_chauthtok
@@ -282,8 +286,7 @@ func pam_sm_chauthtok(pamh unsafe.Pointer, flags, argc C.int, argv **C.char) C.i
 	if pam.Flag(flags)&pam.PrelimCheck != 0 {
 		return C.PAM_SUCCESS
 	}
-
-	return RunPamFunc(Chauthtok, pamh, argc, argv)
+	return chauthtokFunc.Run(pamh, argc, argv)
 }
 
 // main() is needed to make a shared library compile
diff --git a/pam_fscrypt/run_fscrypt.go b/pam_fscrypt/run_fscrypt.go
index da336df..8622a64 100644
--- a/pam_fscrypt/run_fscrypt.go
+++ b/pam_fscrypt/run_fscrypt.go
@@ -59,11 +59,16 @@ const (
 	countFileFormat           = "%d\n"
 )
 
-// PamFunc is used to define the various actions in the PAM module
-type PamFunc func(handle *pam.Handle, args map[string]bool) error
+// PamFunc is used to define the various actions in the PAM module.
+type PamFunc struct {
+	// Name of the function being executed
+	name string
+	// Go implementation of this function
+	impl func(handle *pam.Handle, args map[string]bool) error
+}
 
-// RunPamFunc is used to convert between the Go functions and exported C funcs.
-func RunPamFunc(f PamFunc, pamh unsafe.Pointer, argc C.int, argv **C.char) (ret C.int) {
+// Run is used to convert between the Go functions and exported C funcs.
+func (f *PamFunc) Run(pamh unsafe.Pointer, argc C.int, argv **C.char) (ret C.int) {
 	args := parseArgs(argc, argv)
 	errorWriter := setupLogging(args)
 
@@ -72,20 +77,21 @@ func RunPamFunc(f PamFunc, pamh unsafe.Pointer, argc C.int, argv **C.char) (ret
 		if r := recover(); r != nil {
 			ret = C.PAM_SERVICE_ERR
 			fmt.Fprintf(errorWriter,
-				"pam func panicked: %s\nPlease open an issue.\n%s",
-				r, debug.Stack())
+				"%s(%v) panicked: %s\nPlease open a bug.\n%s",
+				f.name, args, r, debug.Stack())
 		}
 	}()
 
+	log.Printf("%s(%v) starting", f.name, args)
 	handle, err := pam.NewHandle(pamh)
 	if err == nil {
-		err = f(handle, args)
+		err = f.impl(handle, args)
 	}
 	if err != nil {
-		fmt.Fprintf(errorWriter, "pam func failed: %s", err)
+		fmt.Fprintf(errorWriter, "%s(%v) failed: %s", f.name, args, err)
 		return C.PAM_SERVICE_ERR
 	}
-	log.Print("pam func succeeded")
+	log.Printf("%s(%v) succeeded", f.name, args)
 	return C.PAM_SUCCESS
 }
 
diff --git a/security/keyring.go b/security/keyring.go
index ab65631..c4603bc 100644
--- a/security/keyring.go
+++ b/security/keyring.go
@@ -22,7 +22,6 @@ package security
 import (
 	"fmt"
 	"log"
-	"os"
 	"os/user"
 	"sync"
 
@@ -138,42 +137,47 @@ func UserKeyringID(target *user.User, checkSession bool) (int, error) {
 	return targetKeyring, nil
 }
 
-func userKeyringIDLookup(uid int) (int, error) {
+func userKeyringIDLookup(uid int) (keyringID int, err error) {
 	cacheLock.Lock()
 	defer cacheLock.Unlock()
-	if keyringID, ok := keyringIDCache[uid]; ok {
-		return keyringID, nil
-	}
-
-	ruid, euid := os.Getuid(), os.Geteuid()
-	// If all the ids do not agree, we will have to change them
-	needSetUids := uid != ruid || uid != euid
-
-	// The value of KEY_SPEC_USER_KEYRING is determined by the real uid, so
-	// we must set the ruid appropriately. Note that this will also trigger
-	// the creation of the uid keyring if it does not yet exist.
-	if needSetUids {
-		defer setUids(ruid, euid) // Always reset privileges
-		if err := setUids(uid, 0); err != nil {
-			return 0, err
+	var ok bool
+	if keyringID, ok = keyringIDCache[uid]; ok {
+		return
+	}
+
+	// Our goals here are to:
+	//    - Find the user keyring (for the provided uid)
+	//    - Link it into the current process keyring (so we can use it)
+	//    - Make no permenant changes to the process privileges
+	// Complicating this are the facts that:
+	//    - The value of KEY_SPEC_USER_KEYRING is determined by the ruid
+	//    - Keyring linking permissions use the euid
+	// So we have to change both the ruid and euid to make this work,
+	// setting the suid to 0 so that we can later switch back.
+	ruid, euid, suid := getUids()
+	if ruid != uid || euid != uid {
+		if err = setUids(uid, uid, 0); err != nil {
+			return
 		}
+		defer func() {
+			resetErr := setUids(ruid, euid, suid)
+			if resetErr != nil {
+				err = resetErr
+			}
+		}()
 	}
-	keyringID, err := unix.KeyctlGetKeyringID(unix.KEY_SPEC_USER_KEYRING, true)
+
+	// We get the value of KEY_SPEC_USER_KEYRING. Note that this will also
+	// trigger the creation of the uid keyring if it does not yet exist.
+	keyringID, err = unix.KeyctlGetKeyringID(unix.KEY_SPEC_USER_KEYRING, true)
 	log.Printf("keyringID(_uid.%d) = %d, %v", uid, keyringID, err)
 	if err != nil {
 		return 0, err
 	}
 
-	// We still want to use this key after our privileges are reset. If we
-	// link the key into the process keyring, we will possess it and still
-	// be able to use it. However, the permissions to link are based on the
-	// effective uid, so we must set the euid appropriately.
-	if needSetUids {
-		if err := setUids(0, uid); err != nil {
-			return 0, err
-		}
-	}
-	if err := keyringLink(keyringID, unix.KEY_SPEC_PROCESS_KEYRING); err != nil {
+	// We still want to use this keyring after our privileges are reset. So
+	// we link it into the process keyring, preventing a loss of access.
+	if err = keyringLink(keyringID, unix.KEY_SPEC_PROCESS_KEYRING); err != nil {
 		return 0, err
 	}
 
diff --git a/security/privileges.go b/security/privileges.go
index 24cd345..c9bfde7 100644
--- a/security/privileges.go
+++ b/security/privileges.go
@@ -43,30 +43,15 @@ package security
 // cgo maps them to different Go types.
 
 /*
+#define _GNU_SOURCE    // for getresuid and setresuid
 #include <sys/types.h>
-#include <unistd.h>	// setreuid, setregid
-#include <grp.h>	// setgroups
-
-static int my_setreuid(uid_t ruid, uid_t euid)
-{
-	return setreuid(ruid, euid);
-}
-
-static int my_setregid(gid_t rgid, gid_t egid)
-{
-	return setregid(rgid, egid);
-}
-
-static int my_setgroups(size_t size, const gid_t *list)
-{
-	return setgroups(size, list);
-}
+#include <unistd.h>    // getting and setting uids and gids
+#include <grp.h>       // setgroups
 */
 import "C"
 
 import (
 	"log"
-	"os"
 	"os/user"
 	"syscall"
 
@@ -75,72 +60,104 @@ import (
 	"github.com/google/fscrypt/util"
 )
 
-// SetProcessPrivileges temporarily drops the privileges of the current process
-// to have the effective uid/gid of the target user. The privileges can be
-// changed again with another call to SetProcessPrivileges.
-func SetProcessPrivileges(target *user.User) error {
-	euid := util.AtoiOrPanic(target.Uid)
-	egid := util.AtoiOrPanic(target.Gid)
-	if os.Geteuid() == euid {
-		log.Printf("Privileges already set to %q", target.Username)
-		return nil
-	}
-	log.Printf("Setting privileges to %q", target.Username)
+// Privileges encapulate the effective uid/gid and groups of a process.
+type Privileges struct {
+	euid   C.uid_t
+	egid   C.gid_t
+	groups []C.gid_t
+}
+
+// ProcessPrivileges returns the process's current effective privileges.
+func ProcessPrivileges() (*Privileges, error) {
+	ruid := C.getuid()
+	euid := C.geteuid()
+	rgid := C.getgid()
+	egid := C.getegid()
 
-	// If setting privs to root, we want to set the uid first, so we will
-	// then have the necessary permissions to perform the other actions.
-	if euid == 0 {
-		if err := setUids(-1, euid); err != nil {
-			return err
+	var groups []C.gid_t
+	n, err := C.getgroups(0, nil)
+	if n < 0 {
+		return nil, err
+	}
+	// If n == 0, the user isn't in any groups, so groups == nil is fine.
+	if n > 0 {
+		groups = make([]C.gid_t, n)
+		n, err = C.getgroups(n, &groups[0])
+		if n < 0 {
+			return nil, err
 		}
+		groups = groups[:n]
 	}
-	if err := setGids(-1, egid); err != nil {
-		return err
+	log.Printf("Current privs (real, effective): uid=(%d,%d) gid=(%d,%d) groups=%v",
+		ruid, euid, rgid, egid, groups)
+	return &Privileges{euid, egid, groups}, nil
+}
+
+// UserPrivileges returns the defualt privileges for the specified user.
+func UserPrivileges(user *user.User) (*Privileges, error) {
+	privs := &Privileges{
+		euid: C.uid_t(util.AtoiOrPanic(user.Uid)),
+		egid: C.gid_t(util.AtoiOrPanic(user.Gid)),
 	}
-	if err := setGroups(target); err != nil {
-		return err
+	userGroups, err := user.GroupIds()
+	if err != nil {
+		return nil, util.SystemError(err.Error())
 	}
-	// If not setting privs to root, we want to avoid dropping the uid
-	// util the very end.
-	if euid != 0 {
-		if err := setUids(-1, euid); err != nil {
-			return err
-		}
+	privs.groups = make([]C.gid_t, len(userGroups))
+	for i, group := range userGroups {
+		privs.groups[i] = C.gid_t(util.AtoiOrPanic(group))
 	}
-	return nil
+	return privs, nil
 }
 
-func setUids(ruid, euid int) error {
-	res, err := C.my_setreuid(C.uid_t(ruid), C.uid_t(euid))
-	log.Printf("setreuid(%d, %d) = %d (errno %v)", ruid, euid, res, err)
-	if res == 0 {
-		return nil
+// SetProcessPrivileges sets the privileges of the current process to have those
+// specified by privs. The original privileges can be obtained by first saving
+// the output of ProcessPrivileges, calling SetProcessPrivileges with the
+// desired privs, then calling SetProcessPrivileges with the saved privs.
+func SetProcessPrivileges(privs *Privileges) error {
+	log.Printf("Setting euid=%d egid=%d groups=%v", privs.euid, privs.egid, privs.groups)
+
+	// If setting privs as root, we need to set the euid to 0 first, so that
+	// we will have the necessary permissions to make the other changes to
+	// the groups/egid/euid, regardless of our original euid.
+	C.seteuid(0)
+
+	// Seperately handle the case where the user is in no groups.
+	numGroups := C.size_t(len(privs.groups))
+	groupsPtr := (*C.gid_t)(nil)
+	if numGroups > 0 {
+		groupsPtr = &privs.groups[0]
 	}
-	return errors.Wrapf(err.(syscall.Errno), "setting uids")
-}
 
-func setGids(rgid, egid int) error {
-	res, err := C.my_setregid(C.gid_t(rgid), C.gid_t(egid))
-	log.Printf("setregid(%d, %d) = %d (errno %v)", rgid, egid, res, err)
-	if res == 0 {
-		return nil
+	if res, err := C.setgroups(numGroups, groupsPtr); res < 0 {
+		return errors.Wrapf(err.(syscall.Errno), "setting groups")
+	}
+	if res, err := C.setegid(privs.egid); res < 0 {
+		return errors.Wrapf(err.(syscall.Errno), "setting egid")
+	}
+	if res, err := C.seteuid(privs.euid); res < 0 {
+		return errors.Wrapf(err.(syscall.Errno), "setting euid")
 	}
-	return errors.Wrapf(err.(syscall.Errno), "setting gids")
+	ProcessPrivileges()
+	return nil
 }
 
-func setGroups(target *user.User) error {
-	groupStrings, err := target.GroupIds()
-	if err != nil {
-		return util.SystemError(err.Error())
-	}
-	gids := make([]C.gid_t, len(groupStrings))
-	for i, groupString := range groupStrings {
-		gids[i] = C.gid_t(util.AtoiOrPanic(groupString))
+func setUids(ruid, euid, suid int) error {
+	log.Printf("Setting ruid=%d euid=%d suid=%d", ruid, euid, suid)
+	// We elevate the all the privs before setting them. This prevents
+	// issues with (ruid=1000,euid=1000,suid=0), where just a single call
+	// to setresuid might fail with permission denied.
+	if res, err := C.setresuid(0, 0, 0); res < 0 {
+		return errors.Wrapf(err.(syscall.Errno), "setting uids")
 	}
-	res, err := C.my_setgroups(C.size_t(len(groupStrings)), &gids[0])
-	log.Printf("setgroups(%v) = %d (errno %v)", gids, res, err)
-	if res == 0 {
-		return nil
+	if res, err := C.setresuid(C.uid_t(ruid), C.uid_t(euid), C.uid_t(suid)); res < 0 {
+		return errors.Wrapf(err.(syscall.Errno), "setting uids")
 	}
-	return errors.Wrapf(err.(syscall.Errno), "setting groups")
+	return nil
+}
+
+func getUids() (int, int, int) {
+	var ruid, euid, suid C.uid_t
+	C.getresuid(&ruid, &euid, &suid)
+	return int(ruid), int(euid), int(suid)
 }