From 3432f5757293dda39b9fa936a717160cd788ab68 Mon Sep 17 00:00:00 2001 From: Joseph Richey Date: Fri, 1 Sep 2017 00:47:34 -0700 Subject: pam_fscrypt: PAM module no longer crashes on panic Now the offending panic will just be logged and the module will fail. This is important as to not crash the login process. --- pam_fscrypt/run_fscrypt.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) 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 } -- cgit v1.2.3 From d5f64c1ecd8f13f01681d0a18b8f3174ff9bd225 Mon Sep 17 00:00:00 2001 From: Joseph Richey Date: Fri, 1 Sep 2017 00:50:42 -0700 Subject: security: No more permenant privilege dropping This was creating an issue becasuse fully dropping privileges required spawning a goroutine and using rutime.DropOSThread(). --- pam/pam.go | 4 ++-- security/privileges.go | 32 ++++++++++++-------------------- 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/pam/pam.go b/pam/pam.go index 657e9fb..998772c 100644 --- a/pam/pam.go +++ b/pam/pam.go @@ -134,14 +134,14 @@ func (h *Handle) StartAsPamUser() error { if err := security.KeyringsSetup(h.PamUser, h.OrigUser); err != nil { return 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/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 } } -- cgit v1.2.3 From 1ce72a7367967152948dbe332ea8d9834f194c27 Mon Sep 17 00:00:00 2001 From: Joseph Richey Date: Fri, 1 Sep 2017 00:53:07 -0700 Subject: security: Change user keyring lookup algorithm Now instead of spawning a seperate thread we alternate between changing the euid and ruid to both find the keyring and link it to the process keyring. Note that we also ensure that the user keyring is linked into the root keyring whenever possible. --- actions/policy.go | 2 +- pam/pam.go | 4 +- security/keyring.go | 168 +++++++++++++++++++++++++++------------------------- 3 files changed, 90 insertions(+), 84 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/pam/pam.go b/pam/pam.go index 998772c..a3642cc 100644 --- a/pam/pam.go +++ b/pam/pam.go @@ -131,8 +131,8 @@ 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) } 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, , %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 } -- cgit v1.2.3 From 079ee257d27e28b166965f1fa0136f694598b6c7 Mon Sep 17 00:00:00 2001 From: Joseph Richey Date: Fri, 1 Sep 2017 00:55:22 -0700 Subject: cmd/fscrypt: Check that keyrings are setup Chaning the --user flag to (optionally) check for a proper keyring setup allows us to fail early in cases where we need a working keyring. --- cmd/fscrypt/commands.go | 8 ++++---- cmd/fscrypt/flags.go | 21 ++++++++++++++------- 2 files changed, 18 insertions(+), 11 deletions(-) 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/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 } -- cgit v1.2.3 From 0dfbbf62fae3d4051dd5f0686835ac393f8a0247 Mon Sep 17 00:00:00 2001 From: Joseph Richey Date: Fri, 1 Sep 2017 00:56:44 -0700 Subject: cmd/fscrypt: Add explanations for keyring failures Now the user is persented with help when they try to access a keyring that isn't theirs or try to use fscrypt without a user keyring linked into the session keyring. --- cmd/fscrypt/errors.go | 9 +++++++++ 1 file changed, 9 insertions(+) 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: -- cgit v1.2.3