From 3022c1603d968c22f147b4a2c49c4637dd1be91b Mon Sep 17 00:00:00 2001 From: "Joe Richey joerichey@google.com" Date: Wed, 22 Aug 2018 05:17:32 -0700 Subject: Ensure setting user privileges is reversible This change makes sure after dropping then elevating privileges for a process, the euid, guid, and groups are all the same as they were originally. This significantly simplifies the privilege logic. This fixes CVE-2018-6558, which allowed an unprivleged user to gain membership in the root group (gid 0) due to the groups not being properly reset in the process. --- pam/pam.go | 33 ++++++----- security/privileges.go | 146 +++++++++++++++++++++++++------------------------ 2 files changed, 92 insertions(+), 87 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/security/privileges.go b/security/privileges.go index 24cd345..eaee808 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 -#include // setreuid, setregid -#include // 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 // getting and setting uids and gids +#include // setgroups */ import "C" import ( "log" - "os" "os/user" "syscall" @@ -75,72 +60,93 @@ 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 +} - // 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 - } - } - if err := setGids(-1, egid); err != nil { - return err - } - if err := setGroups(target); err != nil { - return err +// ProcessPrivileges returns the process's current effective privileges. +func ProcessPrivileges() (*Privileges, error) { + ruid := C.getuid() + euid := C.geteuid() + rgid := C.getgid() + egid := C.getegid() + + var groups []C.gid_t + n, err := C.getgroups(0, nil) + if n < 0 { + return nil, err } - // 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 + // 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] } - return nil + 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 } -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 +// 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)), } - return errors.Wrapf(err.(syscall.Errno), "setting uids") + userGroups, err := user.GroupIds() + if err != nil { + return nil, util.SystemError(err.Error()) + } + privs.groups = make([]C.gid_t, len(userGroups)) + for i, group := range userGroups { + privs.groups[i] = C.gid_t(util.AtoiOrPanic(group)) + } + return privs, nil } -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 +// 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 gids") -} -func setGroups(target *user.User) error { - groupStrings, err := target.GroupIds() - if err != nil { - return util.SystemError(err.Error()) + 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") } - gids := make([]C.gid_t, len(groupStrings)) - for i, groupString := range groupStrings { - gids[i] = C.gid_t(util.AtoiOrPanic(groupString)) + if res, err := C.seteuid(privs.euid); res < 0 { + return errors.Wrapf(err.(syscall.Errno), "setting euid") } - res, err := C.my_setgroups(C.size_t(len(groupStrings)), &gids[0]) - log.Printf("setgroups(%v) = %d (errno %v)", gids, res, err) + ProcessPrivileges() + return nil +} + +func setUids(ruid, euid int) error { + res, err := C.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 } - return errors.Wrapf(err.(syscall.Errno), "setting groups") + return errors.Wrapf(err.(syscall.Errno), "setting uids") } -- cgit v1.2.3 From 315f9b042237200174a1fb99427f74027e191d66 Mon Sep 17 00:00:00 2001 From: "Joe Richey joerichey@google.com" Date: Wed, 22 Aug 2018 05:23:00 -0700 Subject: Ensure keyring privilege changes are reversible This change makes sure that, when we set the ruid and euid in order to get the user keyring linked into the current process keyring, we will always be able to reverse these changes (using a suid of 0). This fixes an issue where "su " would result in a system error when called by an unprivileged user. It also explains exactly how and why we are making these privilege changes. --- security/keyring.go | 60 +++++++++++++++++++++++++++----------------------- security/privileges.go | 23 ++++++++++++++----- 2 files changed, 49 insertions(+), 34 deletions(-) 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 eaee808..c9bfde7 100644 --- a/security/privileges.go +++ b/security/privileges.go @@ -142,11 +142,22 @@ func SetProcessPrivileges(privs *Privileges) error { return nil } -func setUids(ruid, euid int) error { - res, err := C.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 +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") } - return errors.Wrapf(err.(syscall.Errno), "setting uids") + 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 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) } -- cgit v1.2.3 From 11b09739cbcb25e6602267efe3d48eb063233f5a Mon Sep 17 00:00:00 2001 From: "Joe Richey joerichey@google.com" Date: Wed, 22 Aug 2018 05:28:21 -0700 Subject: Improve debug and error output for pam_fscrypt --- pam_fscrypt/pam_fscrypt.go | 21 ++++++++++++--------- pam_fscrypt/run_fscrypt.go | 24 +++++++++++++++--------- 2 files changed, 27 insertions(+), 18 deletions(-) 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 } -- cgit v1.2.3