Skip to content

Commit

Permalink
Try to detect process being forked during PAM transaction
Browse files Browse the repository at this point in the history
Update #350
  • Loading branch information
ebiggers committed Apr 17, 2022
1 parent 53dc5f3 commit ed0be69
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 0 deletions.
9 changes: 9 additions & 0 deletions pam/pam.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,15 @@ func (h *Handle) GetItem(i Item) (unsafe.Pointer, error) {
return data, nil
}

// GetServiceName retrieves the name of the application running the PAM transaction.
func (h *Handle) GetServiceName() string {
data, err := h.GetItem(Service)
if err != nil {
return "[unknown service]"
}
return C.GoString((*C.char)(data))
}

// StartAsPamUser sets the effective privileges to that of the PAM user.
func (h *Handle) StartAsPamUser() error {
userPrivs, err := security.UserPrivileges(h.PamUser)
Expand Down
46 changes: 46 additions & 0 deletions pam_fscrypt/pam_fscrypt.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ import "C"
import (
"fmt"
"log"
"log/syslog"
"os"
"strconv"
"unsafe"

"github.com/pkg/errors"
Expand All @@ -46,6 +49,8 @@ const (
moduleName = "pam_fscrypt"
// authtokLabel tags the AUTHTOK in the PAM data.
authtokLabel = "fscrypt_authtok"
// pidLabel tags the pid in the PAM data.
pidLabel = "fscrypt_pid"
// These flags are used to toggle behavior of the PAM module.
debugFlag = "debug"

Expand Down Expand Up @@ -76,6 +81,12 @@ func Authenticate(handle *pam.Handle, _ map[string]bool) error {
}
defer handle.StopAsPamUser()

// Save the PID in the PAM data so that the Session hook can try to
// detect the unsupported situation where the process was forked.
if err := handle.SetString(pidLabel, strconv.Itoa(os.Getpid())); err != nil {
return errors.Wrap(err, "could not save pid in PAM data")
}

// If this user doesn't have a login protector, no unlocking is needed.
if _, err := loginProtector(handle); err != nil {
log.Printf("no protector, no need for AUTHTOK: %s", err)
Expand Down Expand Up @@ -128,6 +139,37 @@ func setupUserKeyringIfNeeded(handle *pam.Handle, policies []*actions.Policy) er
return handle.StartAsPamUser()
}

// The Go runtime doesn't support being forked, as it is multithreaded but
// fork() deletes all threads except one. Some programs, such as xrdp, misuse
// libpam by fork()-ing the process between pam_authenticate() and
// pam_open_session(). Try to detect such unsupported cases and bail out early
// rather than deadlocking the Go runtime, which would prevent the user from
// logging in entirely. This isn't guaranteed to work, as we are already
// running Go code here, so we may have already deadlocked. But in practice the
// deadlock doesn't occur until hashing the login passphrase is attempted.
func isUnsupportedFork(handle *pam.Handle) bool {
pidString, err := handle.GetString(pidLabel)
if err != nil {
return false
}
expectedPid, err := strconv.Atoi(pidString)
if err != nil {
log.Printf("%s parse error: %v", pidLabel, err)
return false
}
if os.Getpid() == expectedPid {
return false
}
handle.InfoMessage(fmt.Sprintf("%s couldn't automatically unlock directories, see syslog", moduleName))
if logger, err := syslog.New(syslog.LOG_WARNING, moduleName); err == nil {
fmt.Fprintf(logger,
"not unlocking directories because %s forked the process between authenticating the user and opening the session, which is incompatible with %s. See https://github.com/google/fscrypt/issues/350",
handle.GetServiceName(), moduleName)
logger.Close()
}
return true
}

// OpenSession provisions any policies protected with the login protector.
func OpenSession(handle *pam.Handle, _ map[string]bool) error {
// We will always clear the AUTHTOK data
Expand All @@ -154,6 +196,10 @@ func OpenSession(handle *pam.Handle, _ map[string]bool) error {
return nil
}

if isUnsupportedFork(handle) {
return nil
}

if err = setupUserKeyringIfNeeded(handle, policies); err != nil {
return errors.Wrapf(err, "setting up user keyring")
}
Expand Down

0 comments on commit ed0be69

Please sign in to comment.