Merge pull request #56 from google/panics

Fixed failures in PAM module

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

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

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:

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
 }

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

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
 }