pam-ussh: fix bug reported by Solar Designer.

if pam-ussh is being called by sudo (as in the example in the readme),
it can be tricked into reading someone else's $SSH_AUTH_SOCK.

debian/changelog

@@ -1,3 +1,9 @@
+pam-ussh (1.0.1) all; urgency=low
+
+  * fix security issue reported by Solar Designer.
+
+ -- Peter Moody <pmoody@uber.com>  Fri, 10 Feb 2017 01:23:45 +0000
+
 pam-ussh (1.0) all; urgency=low
 
   * Initial release.

debian/control

@@ -1,7 +1,7 @@
 Source: pam-ussh
 Section: admin
 Maintainer: Peter Moody <pmoody@uber.com>
-Build-Depends: debhelper (>= 9), golang-1.6 (>= 1.6.2), git, libpam0g-dev (>= 1.1.3-7)
+Build-Depends: debhelper (>= 9), golang (>= 1.6.2), git, libpam0g-dev (>= 1.1.3-7)
 Standards-Version: 3.9.6
 Homepage: https://github.com/uber/pam-ussh
 

pam.go

@@ -28,7 +28,6 @@ package main
 
 import (
 	"os"
-	"os/user"
 	"unsafe"
 )
 
@@ -38,9 +37,17 @@ import (
 #include <stdlib.h>
 
 char *string_from_argv(int, char**);
+char *get_user(pam_handle_t *pamh);
+int get_uid(char *user);
 */
 import "C"
 
+func init() {
+	if !disablePtrace() {
+		pamLog("unable to disable ptrace")
+	}
+}
+
 func sliceFromArgv(argc C.int, argv **C.char) []string {
 	r := make([]string, 0, argc)
 	for i := 0; i < int(argc); i++ {
@@ -53,12 +60,18 @@ func sliceFromArgv(argc C.int, argv **C.char) []string {
 
 //export pam_sm_authenticate
 func pam_sm_authenticate(pamh *C.pam_handle_t, flags, argc C.int, argv **C.char) C.int {
-	user, err := user.Current()
-	if err != nil {
-		return C.PAM_AUTH_ERR
+	cUsername := C.get_user(pamh)
+	if cUsername == nil {
+		return C.PAM_USER_UNKNOWN
+	}
+	defer C.free(unsafe.Pointer(cUsername))
+
+	uid := int(C.get_uid(cUsername))
+	if uid < 0 {
+		return C.PAM_USER_UNKNOWN
 	}
 
-	r := pamAuthenticate(os.Stderr, user.Username, sliceFromArgv(argc, argv))
+	r := pamAuthenticate(os.Stderr, uid, C.GoString(cUsername), sliceFromArgv(argc, argv))
 	if r == AuthError {
 		return C.PAM_AUTH_ERR
 	}

pam_c.go

@@ -23,10 +23,144 @@ THE SOFTWARE.
 */
 
 /*
+#include <errno.h>
+#include <pwd.h>
+#include <security/pam_appl.h>
+#include <stdlib.h>
+#include <stdio.h>
 #include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#ifdef __APPLE__
+  #include <sys/ptrace.h>
+#elif __linux__
+  #include <sys/prctl.h>
+#endif
 
 char *string_from_argv(int i, char **argv) {
   return strdup(argv[i]);
 }
+
+// get_user pulls the username out of the pam handle.
+char *get_user(pam_handle_t *pamh) {
+  if (!pamh)
+    return NULL;
+
+  int pam_err = 0;
+  const char *user;
+  if ((pam_err = pam_get_item(pamh, PAM_USER, (const void**)&user)) != PAM_SUCCESS)
+    return NULL;
+
+  return strdup(user);
+}
+
+// file_uid returns the uid that owns a file.
+int file_uid(char *path) {
+  struct stat sb;
+  int ret = -1;
+  if ((ret = stat(path, &sb)) < 0) {
+    return -1;
+  }
+
+  return (int)sb.st_uid;
+}
+
+// get_uid returns the uid for the given char *username
+int get_uid(char *user) {
+  if (!user)
+    return -1;
+  struct passwd pw, *result;
+  char buf[8192]; // 8k should be enough for anyone
+
+  int i = getpwnam_r(user, &pw, buf, sizeof(buf), &result);
+  if (!result || i != 0)
+    return -1;
+  return pw.pw_uid;
+}
+
+// get_username returns the username for the given uid.
+char *get_username(int uid) {
+  if (uid < 0)
+    return NULL;
+
+  struct passwd pw, *result;
+  char buf[8192]; // 8k should be enough for anyone
+
+  int i = getpwuid_r(uid, &pw, buf, sizeof(buf), &result);
+  if (!result || i != 0)
+    return NULL;
+
+  return strdup(pw.pw_name);
+}
+
+// change_euid sets the euid to the given euid
+int change_euid(int uid) {
+  return seteuid(uid);
+}
+
+int disable_ptrace() {
+#ifdef __APPLE__
+  return ptrace(PT_DENY_ATTACH, 0, 0, 0);
+#elif __linux__
+  return prctl(PR_SET_DUMPABLE, 0);
+#endif
+  return 1;
+}
 */
 import "C"
+
+import (
+	"fmt"
+	"os/user"
+	"strconv"
+	"unsafe"
+)
+
+// fileUID returns the uid of the owner of a given file, and
+// true if the file is owned by the given uid.
+func fileUID(path string) int {
+	cPath := C.CString(path)
+	defer C.free(unsafe.Pointer(cPath))
+
+	return int(C.file_uid(cPath))
+}
+
+// getUID is used for testing.
+func getUID() int {
+	u, err := user.Current()
+	if err != nil {
+		fmt.Printf("user.Current error: %v\n", err)
+		return -1
+	}
+
+	i, err := strconv.Atoi(u.Uid)
+	if err == nil {
+		return i
+	}
+
+	cUsername := C.CString(u.Uid)
+	defer C.free(unsafe.Pointer(cUsername))
+	return int(C.get_uid(cUsername))
+}
+
+// getUsername returns the username associated with the given uid.
+func getUsername(uid int) string {
+	cUsername := C.get_username(C.int(uid))
+	if cUsername == nil {
+		return "<unknown>"
+	}
+	defer C.free(unsafe.Pointer(cUsername))
+	return C.GoString(cUsername)
+}
+
+// seteuid drops privs.
+func seteuid(uid int) bool {
+	return C.change_euid(C.int(uid)) == C.int(0)
+}
+
+// likely redundant, but try and make sure we can't be traced.
+func disablePtrace() bool {
+	return C.disable_ptrace() == C.int(0)
+}

pam_linux.go

@@ -23,10 +23,9 @@ THE SOFTWARE.
 package main
 
 /*
-#include <unistd.h>
-#include <sys/types.h>
 #include <grp.h>
 #include <stdlib.h> // for C.free
+#include <sys/types.h>
 
 int group_member(int gid);
 struct group *getgrnam(const char *name);

pam_ussh.go

@@ -66,16 +66,36 @@ func pamLog(format string, args ...interface{}) {
 
 // authenticate validates certs loaded on the ssh-agent at the other end of
 // AuthSock.
-func authenticate(w io.Writer, ca string, principals map[string]struct{}) AuthResult {
+func authenticate(w io.Writer, uid int, username, ca string, principals map[string]struct{}) AuthResult {
 	authSock := os.Getenv("SSH_AUTH_SOCK")
 	if authSock == "" {
 		fmt.Fprint(w, "No SSH_AUTH_SOCK")
 		return AuthError
 	}
 
+	// store for later error message just in case.
+	ownerUID := fileUID(authSock)
+
+	origEUID := os.Geteuid()
+	if os.Getuid() != origEUID || origEUID == 0 {
+		// Note: this only sets the euid and doesn't do anything with the egid.
+		// That should be fine for most cases, but it's worth calling out.
+		if !seteuid(uid) {
+			pamLog("error dropping privs from %d to %d", origEUID, uid)
+			return AuthError
+		}
+		defer func() {
+			if !seteuid(origEUID) {
+				pamLog("error resetting uid to %d", origEUID)
+			}
+		}()
+	}
+
 	agentSock, err := net.Dial("unix", authSock)
 	if err != nil {
-		fmt.Fprintf(w, "%v", err)
+		fmt.Fprintf(w, "error connecting to %s: %v\n", authSock, err)
+		pamLog("error opening auth sock (sock owner: %d/%s) by (caller: %d/%s)",
+			ownerUID, getUsername(ownerUID), os.Getuid(), username)
 		return AuthError
 	}
 
@@ -174,7 +194,7 @@ func loadValidPrincipals(principals string) (map[string]struct{}, error) {
 	return p, nil
 }
 
-func pamAuthenticate(w io.Writer, user string, argv []string) AuthResult {
+func pamAuthenticate(w io.Writer, uid int, username string, argv []string) AuthResult {
 	runtime.GOMAXPROCS(1)
 
 	userCA := defaultUserCA
@@ -207,7 +227,7 @@ func pamAuthenticate(w io.Writer, user string, argv []string) AuthResult {
 	}
 
 	if len(group) == 0 || isMemberOf(group) {
-		return authenticate(w, userCA, authorizedPrincipals)
+		return authenticate(w, uid, username, userCA, authorizedPrincipals)
 	}
 
 	return AuthSuccess

pam_ussh_test.go

@@ -57,7 +57,7 @@ func TestNoAuthSock(t *testing.T) {
 	defer os.Setenv("SSH_AUTH_SOCK", oldAgent)
 	os.Unsetenv("SSH_AUTH_SOCK")
 	b := new(bytes.Buffer)
-	require.Equal(t, AuthError, authenticate(b, "", nil))
+	require.Equal(t, AuthError, authenticate(b, 0, "", "", nil))
 	require.Contains(t, b.String(), "No SSH_AUTH_SOCK")
 }
 
@@ -69,7 +69,7 @@ func TestBadAuthSock(t *testing.T) {
 		defer os.Setenv("SSH_AUTH_SOCK", oldAgent)
 		os.Setenv("SSH_AUTH_SOCK", s)
 		b := new(bytes.Buffer)
-		require.Equal(t, AuthError, authenticate(b, "", nil))
+		require.Equal(t, AuthError, authenticate(b, 0, "", "", nil))
 		require.Contains(t, b.String(), "connect: no such file or directory")
 	})
 }
@@ -81,7 +81,7 @@ func TestBadCA(t *testing.T) {
 			k, e := rsa.GenerateKey(rand.Reader, 1024)
 			require.NoError(t, e)
 			require.NoError(t, a.Add(agent.AddedKey{PrivateKey: k}))
-			require.Equal(t, AuthError, authenticate(new(bytes.Buffer), ca, nil))
+			require.Equal(t, AuthError, authenticate(new(bytes.Buffer), 0, "", ca, nil))
 		})
 	})
 }
@@ -98,7 +98,7 @@ func TestAuthorize_NoKeys(t *testing.T) {
 		e = ioutil.WriteFile(ca, ssh.MarshalAuthorizedKey(pub), 0444)
 
 		WithSSHAgent(func(a agent.Agent) {
-			r := authenticate(new(bytes.Buffer), ca, p)
+			r := authenticate(new(bytes.Buffer), 0, "", ca, p)
 			require.Equal(t, AuthError, r)
 		})
 	})
@@ -129,27 +129,27 @@ func TestPamAuthorize(t *testing.T) {
 			a.Add(agent.AddedKey{PrivateKey: userPriv, Certificate: c})
 
 			// test with no principal
-			r := pamAuthenticate(new(bytes.Buffer), "foober", []string{caPamOpt})
+			r := pamAuthenticate(new(bytes.Buffer), getUID(), "foober", []string{caPamOpt})
 			require.Equal(t, AuthSuccess, r,
 				"authenticate failed when it should've succeeded")
 
-			// negative test with authorized_principals pam option
-			r = pamAuthenticate(new(bytes.Buffer), "foober", []string{caPamOpt,
+			// negative test with authorized_principals pam 2option
+			r = pamAuthenticate(new(bytes.Buffer), getUID(), "foober", []string{caPamOpt,
 				fmt.Sprintf("authorized_principals=group:boober")})
 			require.Equal(t, AuthError, r)
 
 			// positive test with authorized_principals_file pam option
-			r = pamAuthenticate(new(bytes.Buffer), "foober", []string{caPamOpt,
+			r = pamAuthenticate(new(bytes.Buffer), getUID(), "foober", []string{caPamOpt,
 				fmt.Sprintf("authorized_principals_file=%s", principals)})
 			require.Equal(t, AuthSuccess, r)
 
 			// negative test with a bad authorized_principals_file pam option
-			r = pamAuthenticate(new(bytes.Buffer), "foober", []string{caPamOpt,
+			r = pamAuthenticate(new(bytes.Buffer), getUID(), "foober", []string{caPamOpt,
 				"authorized_principals_file=foober"})
 			require.Equal(t, AuthError, r)
 
 			// test that a user not in the required group passes.
-			r = pamAuthenticate(new(bytes.Buffer), "foober", []string{caPamOpt,
+			r = pamAuthenticate(new(bytes.Buffer), getUID(), "foober", []string{caPamOpt,
 				"group=nosuchgroup"})
 			require.Equal(t, AuthSuccess, r)
 		})