Merge pull request #56 from google/panicsv0.2.10.2.1

Fixed failures in PAM module

8 files changed, 146 insertions, 121 deletions

diff --git a/actions/policy.go b/actions/policy.go
index 510afa1..cbdcb3a 100644
--- a/actions/policy.go
+++ b/actions/policy.go
@@ -60,7 +60,7 @@ func PurgeAllPolicies(ctx *Context) error {
 		err = security.RemoveKey(service+policyDescriptor, ctx.TargetUser)
 
 		switch errors.Cause(err) {
-		case nil, security.ErrKeyringSearch:
+		case nil, security.ErrKeySearch:
 			// We don't care if the key has already been removed
 		default:
 			return err
diff --git a/cmd/fscrypt/commands.go b/cmd/fscrypt/commands.go
index 43c9cb0..fd90626 100644
--- a/cmd/fscrypt/commands.go
+++ b/cmd/fscrypt/commands.go
@@ -119,7 +119,7 @@ func encryptAction(c *cli.Context) error {
 // keyring unless --skip-unlock is used. On failure, an error is returned, any
 // metadata creation is reverted, and the directory is unmodified.
 func encryptPath(path string) (err error) {
-	target, err := parseUserFlag()
+	target, err := parseUserFlag(!skipUnlockFlag.Value)
 	if err != nil {
 		return
 	}
@@ -274,7 +274,7 @@ func unlockAction(c *cli.Context) error {
 		return expectedArgsErr(c, 1, false)
 	}
 
-	target, err := parseUserFlag()
+	target, err := parseUserFlag(true)
 	if err != nil {
 		return newExitError(c, err)
 	}
@@ -357,7 +357,7 @@ func purgeAction(c *cli.Context) error {
 		}
 	}
 
-	target, err := parseUserFlag()
+	target, err := parseUserFlag(true)
 	if err != nil {
 		return newExitError(c, err)
 	}
@@ -507,7 +507,7 @@ func createProtectorAction(c *cli.Context) error {
 		return expectedArgsErr(c, 1, false)
 	}
 
-	target, err := parseUserFlag()
+	target, err := parseUserFlag(false)
 	if err != nil {
 		return newExitError(c, err)
 	}
diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index 9731efc..81a6798 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -35,6 +35,7 @@ import (
 	"github.com/google/fscrypt/crypto"
 	"github.com/google/fscrypt/filesystem"
 	"github.com/google/fscrypt/metadata"
+	"github.com/google/fscrypt/security"
 	"github.com/google/fscrypt/util"
 )
 
@@ -93,6 +94,14 @@ func getErrorSuggestions(err error) string {
 			needs to be enabled for this filesystem. See the
 			documentation on how to enable encryption on ext4
 			systems (and the risks of doing so).`
+	case security.ErrSessionUserKeying:
+		return `This is usually the result of a bad PAM configuration.
+			Either correct the problem in your PAM stack, enable
+			pam_keyinit.so, or run "keyctl link @u @s".`
+	case security.ErrAccessUserKeyring:
+		return fmt.Sprintf(`You can only use %s to access the user
+			keyring of another user if you are running as root.`,
+			shortDisplay(userFlag))
 	case actions.ErrBadConfigFile:
 		return `Run "sudo fscrypt setup" to recreate the file.`
 	case actions.ErrNoConfigFile:
diff --git a/cmd/fscrypt/flags.go b/cmd/fscrypt/flags.go
index e883a6d..af03ad2 100644
--- a/cmd/fscrypt/flags.go
+++ b/cmd/fscrypt/flags.go
@@ -33,6 +33,7 @@ import (
 	"github.com/urfave/cli"
 
 	"github.com/google/fscrypt/actions"
+	"github.com/google/fscrypt/security"
 	"github.com/google/fscrypt/util"
 )
 
@@ -283,17 +284,23 @@ func getPolicyFromFlag(flagValue string, target *user.User) (*actions.Policy, er
 
 // parseUserFlag returns the user specified by userFlag or the current effective
 // user if the flag value is missing. If the effective user is root, however, a
-// user must specified in the flag.
-func parseUserFlag() (*user.User, error) {
+// user must specified in the flag. If checkKeyring is true, we also make sure
+// there are no problems accessing the user keyring.
+func parseUserFlag(checkKeyring bool) (targetUser *user.User, err error) {
 	if userFlag.Value != "" {
-		return user.Lookup(userFlag.Value)
+		targetUser, err = user.Lookup(userFlag.Value)
+	} else {
+		if util.IsUserRoot() {
+			return nil, ErrSpecifyUser
+		}
+		targetUser, err = util.EffectiveUser()
 	}
-	effectiveUser, err := util.EffectiveUser()
 	if err != nil {
 		return nil, err
 	}
-	if util.IsUserRoot() {
-		return nil, ErrSpecifyUser
+
+	if checkKeyring {
+		_, err = security.UserKeyringID(targetUser)
 	}
-	return effectiveUser, nil
+	return targetUser, err
 }
diff --git a/pam/pam.go b/pam/pam.go
index 657e9fb..a3642cc 100644
--- a/pam/pam.go
+++ b/pam/pam.go
@@ -131,17 +131,17 @@ func (h *Handle) GetItem(i Item) (unsafe.Pointer, error) {
 // StartAsPamUser sets the effective privileges to that of the PAM user, and
 // configures the PAM user's keyrings to be properly linked.
 func (h *Handle) StartAsPamUser() error {
-	if err := security.KeyringsSetup(h.PamUser, h.OrigUser); err != nil {
-		return err
+	if _, err := security.UserKeyringID(h.PamUser); err != nil {
+		log.Printf("Setting up keyrings in PAM: %v", err)
 	}
-	return security.SetThreadPrivileges(h.PamUser, false)
+	return security.SetThreadPrivileges(h.PamUser)
 }
 
 // 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.SetThreadPrivileges(h.OrigUser, false)
+	err := security.SetThreadPrivileges(h.OrigUser)
 	if err != nil {
 		log.Print(err)
 	}
diff --git a/pam_fscrypt/run_fscrypt.go b/pam_fscrypt/run_fscrypt.go
index c02b05f..6414d99 100644
--- a/pam_fscrypt/run_fscrypt.go
+++ b/pam_fscrypt/run_fscrypt.go
@@ -36,6 +36,7 @@ import (
 	"log/syslog"
 	"os"
 	"path/filepath"
+	"runtime/debug"
 	"unsafe"
 
 	"golang.org/x/sys/unix"
@@ -62,19 +63,29 @@ const (
 type PamFunc 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) C.int {
+func RunPamFunc(f PamFunc, pamh unsafe.Pointer, argc C.int, argv **C.char) (ret C.int) {
 	args := parseArgs(argc, argv)
 	errorWriter := setupLogging(args)
-	handle, err := pam.NewHandle(pamh)
 
+	// Log any panics to the errorWriter
+	defer func() {
+		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())
+		}
+	}()
+
+	handle, err := pam.NewHandle(pamh)
 	if err == nil {
 		err = f(handle, args)
 	}
-
 	if err != nil {
-		fmt.Fprint(errorWriter, err)
+		fmt.Fprintf(errorWriter, "pam func failed: %s", err)
 		return C.PAM_SERVICE_ERR
 	}
+	log.Print("pam func succeeded")
 	return C.PAM_SUCCESS
 }
 
diff --git a/security/keyring.go b/security/keyring.go
index 4cfb4af..ed723fc 100644
--- a/security/keyring.go
+++ b/security/keyring.go
@@ -20,10 +20,10 @@
 package security
 
 import (
+	"fmt"
 	"log"
 	"os"
 	"os/user"
-	"runtime"
 	"sync"
 
 	"github.com/pkg/errors"
@@ -37,32 +37,19 @@ const KeyType = "logon"
 
 // Keyring related error values
 var (
-	ErrFindingKeyring = util.SystemError("could not find user keyring")
-	ErrKeyringInsert  = util.SystemError("could not insert key into the keyring")
-	ErrKeyringSearch  = errors.New("could not find key with descriptor")
-	ErrKeyringDelete  = util.SystemError("could not delete key from the keyring")
-	ErrKeyringLink    = util.SystemError("could not link keyring")
+	ErrKeySearch         = errors.New("could not find key with descriptor")
+	ErrKeyRemove         = util.SystemError("could not remove key from the keyring")
+	ErrKeyInsert         = util.SystemError("could not insert key into the keyring")
+	ErrSessionUserKeying = errors.New("user keyring not linked into session keyring")
+	ErrAccessUserKeyring = errors.New("could not access user keyring")
+	ErrLinkUserKeyring   = util.SystemError("could not link user keyring into root keyring")
 )
 
-// KeyringsSetup configures the desired keyring linkage by linking the target
-// user's keying into the privileged user's keyring.
-func KeyringsSetup(target, privileged *user.User) error {
-	targetKeyringID, err := userKeyringID(target)
-	if err != nil {
-		return err
-	}
-	privilegedKeyringID, err := userKeyringID(privileged)
-	if err != nil {
-		return err
-	}
-	return keyringLink(targetKeyringID, privilegedKeyringID)
-}
-
 // FindKey tries to locate a key in the kernel keyring with the provided
 // description. The key ID is returned if we can find the key. An error is
 // returned if the key does not exist.
 func FindKey(description string, target *user.User) (int, error) {
-	keyringID, err := userKeyringID(target)
+	keyringID, err := UserKeyringID(target)
 	if err != nil {
 		return 0, err
 	}
@@ -70,7 +57,7 @@ func FindKey(description string, target *user.User) (int, error) {
 	keyID, err := unix.KeyctlSearch(keyringID, KeyType, description, 0)
 	log.Printf("KeyctlSearch(%d, %s, %s) = %d, %v", keyringID, KeyType, description, keyID, err)
 	if err != nil {
-		return 0, errors.Wrap(ErrKeyringSearch, err.Error())
+		return 0, errors.Wrap(ErrKeySearch, err.Error())
 	}
 	return keyID, err
 }
@@ -88,7 +75,7 @@ func RemoveKey(description string, target *user.User) error {
 	_, err = unix.KeyctlInt(unix.KEYCTL_INVALIDATE, keyID, 0, 0, 0)
 	log.Printf("KeyctlInvalidate(%d) = %v", keyID, err)
 	if err != nil {
-		return errors.Wrap(ErrKeyringDelete, err.Error())
+		return errors.Wrap(ErrKeyRemove, err.Error())
 	}
 	return nil
 }
@@ -96,7 +83,7 @@ func RemoveKey(description string, target *user.User) error {
 // InsertKey puts the provided data into the kernel keyring with the provided
 // description.
 func InsertKey(data []byte, description string, target *user.User) error {
-	keyringID, err := userKeyringID(target)
+	keyringID, err := UserKeyringID(target)
 	if err != nil {
 		return err
 	}
@@ -105,7 +92,7 @@ func InsertKey(data []byte, description string, target *user.User) error {
 	log.Printf("KeyctlAddKey(%s, %s, <data>, %d) = %d, %v",
 		KeyType, description, keyringID, keyID, err)
 	if err != nil {
-		return errors.Wrap(ErrKeyringInsert, err.Error())
+		return errors.Wrap(ErrKeyInsert, err.Error())
 	}
 	return nil
 }
@@ -115,72 +102,77 @@ var (
 	cacheLock      sync.Mutex
 )
 
-// userKeyringID returns the key id of the target user's keyring. The returned
-// keyring will also be linked into the process keyring so that it will be
-// accessible thoughout the program.
-func userKeyringID(target *user.User) (int, error) {
+// UserKeyringID returns the key id of the target user's user keyring. We also
+// ensure that the keyring will be accessible by linking it into the process
+// keyring and linking it into the root user keyring (permissions allowing). An
+// error is returned if a normal user requests their user keyring, but it is not
+// in the current session keyring.
+func UserKeyringID(target *user.User) (int, error) {
 	uid := util.AtoiOrPanic(target.Uid)
-	// We will cache the result of this function.
-	cacheLock.Lock()
-	defer cacheLock.Unlock()
-	if keyringID, ok := keyringIDCache[uid]; ok {
-		return keyringID, nil
+	targetKeyring, err := userKeyringIDLookup(uid)
+	if err != nil {
+		return 0, errors.Wrap(ErrAccessUserKeyring, err.Error())
 	}
 
-	// The permissions of the keyrings API is a little strange. The euid is
-	// used to determine if we can access/modify a key/keyring. However, the
-	// ruid is used to determine KEY_SPEC_USER_KEYRING. This means both the
-	// ruid and euid must match the user's uid for the lookup to work.
-	if uid == os.Getuid() && uid == os.Geteuid() {
-		log.Printf("Normal keyring lookup for uid=%d", uid)
-		return userKeyringIDLookup(uid)
-	}
-
-	// We drop permissions in a separate thread (guaranteed as the main
-	// thread is locked) because we need to drop the real AND effective IDs.
-	log.Printf("Threaded keyring lookup for uid=%d", uid)
-	idChan := make(chan int)
-	errChan := make(chan error)
-	// OSThread locks ensure the privilege change is only for the lookup.
-	runtime.LockOSThread()
-	defer runtime.UnlockOSThread()
-	go func() {
-		runtime.LockOSThread()
-		if err := SetThreadPrivileges(target, true); err != nil {
-			errChan <- err
-			return
+	if !util.IsUserRoot() {
+		// Make sure the returned keyring will be accessible by checking
+		// that it is in the session keyring.
+		if !isUserKeyringInSession(uid) {
+			return 0, ErrSessionUserKeying
 		}
-		keyringID, err := userKeyringIDLookup(uid)
-		if err != nil {
-			errChan <- err
-			return
-		}
-		idChan <- keyringID
-	}()
+		return targetKeyring, nil
+	}
 
-	// We select so the thread will have to complete
-	select {
-	case err := <-errChan:
-		return 0, err
-	case keyringID := <-idChan:
-		if uid == os.Getuid() && uid == os.Geteuid() {
-			return 0, util.SystemError("thread privileges now incorrect")
+	// Make sure the returned keyring will be accessible by linking it into
+	// the root user's user keyring (which will not be garbage collected).
+	rootKeyring, err := userKeyringIDLookup(0)
+	if err != nil {
+		return 0, errors.Wrap(ErrLinkUserKeyring, err.Error())
+	}
+
+	if rootKeyring != targetKeyring {
+		if err = keyringLink(targetKeyring, rootKeyring); err != nil {
+			return 0, errors.Wrap(ErrLinkUserKeyring, err.Error())
 		}
-		return keyringID, nil
 	}
+	return targetKeyring, nil
 }
 
 func userKeyringIDLookup(uid int) (int, error) {
-	// This will trigger the creation of the user keyring, if necessary.
-	keyringID, err := unix.KeyctlGetKeyringID(unix.KEY_SPEC_USER_KEYRING, false)
+	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
+		}
+	}
+	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, errors.Wrap(ErrFindingKeyring, err.Error())
+		return 0, err
 	}
 
-	// For some silly reason, a thread does not automatically "possess" keys
-	// in the user keyring. So we link it into the process keyring so that
-	// we will not get "permission denied" when purging or modifying keys.
+	// 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 {
 		return 0, err
 	}
@@ -189,11 +181,25 @@ func userKeyringIDLookup(uid int) (int, error) {
 	return keyringID, nil
 }
 
+// isUserKeyringInSession tells us if the user's uid keyring is in the current
+// session keyring.
+func isUserKeyringInSession(uid int) bool {
+	// We cannot use unix.KEY_SPEC_SESSION_KEYRING directly as that might
+	// create a session keyring if one does not exist.
+	sessionKeyring, err := unix.KeyctlGetKeyringID(unix.KEY_SPEC_SESSION_KEYRING, false)
+	log.Printf("keyringID(session) = %d, %v", sessionKeyring, err)
+	if err != nil {
+		return false
+	}
+
+	description := fmt.Sprintf("_uid.%d", uid)
+	id, err := unix.KeyctlSearch(sessionKeyring, "keyring", description, 0)
+	log.Printf("KeyctlSearch(%d, keyring, %s) = %d, %v", sessionKeyring, description, id, err)
+	return err == nil
+}
+
 func keyringLink(keyID int, keyringID int) error {
 	_, err := unix.KeyctlInt(unix.KEYCTL_LINK, keyID, keyringID, 0, 0)
 	log.Printf("KeyctlLink(%d, %d) = %v", keyID, keyringID, err)
-	if err != nil {
-		return errors.Wrap(ErrKeyringLink, err.Error())
-	}
-	return nil
+	return err
 }
diff --git a/security/privileges.go b/security/privileges.go
index 2a1bdae..7d69da9 100644
--- a/security/privileges.go
+++ b/security/privileges.go
@@ -26,6 +26,7 @@ package security
 
 import (
 	"log"
+	"os"
 	"os/user"
 
 	"github.com/pkg/errors"
@@ -34,44 +35,35 @@ import (
 	"github.com/google/fscrypt/util"
 )
 
-// SetThreadPrivileges drops drops the privileges of the current thread to have
-// the uid/gid of the target user. If permanent is true, this operation cannot
-// be reversed in the thread (the real and effective IDs are set). If
-// permanent is false, only the effective IDs are set, allowing the privileges
-// to be changed again with another call to SetThreadPrivileges.
-func SetThreadPrivileges(target *user.User, permanent bool) error {
+// SetThreadPrivileges temporarily drops the privileges of the current thread to
+// have the effective uid/gid of the target user. The privileges can be changed
+// again with another call to SetThreadPrivileges.
+func SetThreadPrivileges(target *user.User) error {
 	euid := util.AtoiOrPanic(target.Uid)
 	egid := util.AtoiOrPanic(target.Gid)
-	var ruid, rgid int
-	if permanent {
-		log.Printf("Permanently dropping to user %q", target.Username)
-		ruid, rgid = euid, egid
-	} else {
-		log.Printf("Temporarily dropping to user %q", target.Username)
-		// Real IDs of -1 mean they will not be changed.
-		ruid, rgid = -1, -1
+	if os.Geteuid() == euid {
+		log.Printf("Privileges already set to %q", target.Username)
+		return nil
 	}
+	log.Printf("Setting privileges to %q", target.Username)
 
 	// 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(ruid, euid); err != nil {
+		if err := setUids(-1, euid); err != nil {
 			return err
 		}
 	}
-
-	if err := setGids(rgid, egid); err != nil {
+	if err := setGids(-1, egid); err != nil {
 		return err
 	}
-
 	if err := setGroups(target); err != nil {
 		return err
 	}
-
 	// If not setting privs to root, we want to avoid dropping the uid
 	// util the very end.
 	if euid != 0 {
-		if err := setUids(ruid, euid); err != nil {
+		if err := setUids(-1, euid); err != nil {
 			return err
 		}
 	}