From 0da41281822ae09674487a921d32a3ba29d76820 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 09:55:15 -0500
Subject: Refactor to pull the long running stuff out of the if statement and
 into a function

---
 src/pam-freerdp.c | 69 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 7770970..4e43ec4 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -249,6 +249,43 @@ done:
 	return retval;
 }
 
+static int
+session_socket_handler (const char * buffer, int buffer_len, struct passwd * pwdent, int socketfd)
+{
+	/* Locks to carry over */
+	mlock(buffer, buffer_len);
+
+	if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
+			setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
+		return EXIT_FAILURE;
+	}
+
+	if (listen(socketfd, 1) < 0) {
+		return EXIT_FAILURE;
+	}
+
+	socklen_t connected_addr_size;
+	int connectfd;
+	struct sockaddr_un connected_addr;
+
+	connected_addr_size = sizeof(struct sockaddr_un);
+	connectfd = accept(socketfd, (struct sockaddr *)&connected_addr, &connected_addr_size);
+	if (connectfd < 0) {
+		return EXIT_FAILURE;
+	}
+
+	int writedata;
+	writedata = write(connectfd, buffer, buffer_len);
+
+	close(connectfd);
+
+	if (writedata == buffer_len) {
+		return 0;
+	}
+
+	return EXIT_FAILURE;
+}
+
 pid_t session_pid = 0;
 /* Open Session.  Here we need to fork a little process so that we can
    give the credentials to the session itself so that it can startup the
@@ -330,40 +367,14 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char ** argv
 
 	pid_t pid = fork();
 	if (pid == 0) {
-		/* Locks to carry over */
-		mlock(buffer, buffer_len);
+		int retval = 0;
 
-		if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
-				setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
-			_exit(EXIT_FAILURE);
-		}
-
-		if (listen(socketfd, 1) < 0) {
-			_exit(EXIT_FAILURE);
-		}
-
-		socklen_t connected_addr_size;
-		int connectfd;
-		struct sockaddr_un connected_addr;
+		retval = session_socket_handler(buffer, buffer_len, pwdent, socketfd);
 
-		connected_addr_size = sizeof(struct sockaddr_un);
-		connectfd = accept(socketfd, (struct sockaddr *)&connected_addr, &connected_addr_size);
-		if (connectfd < 0) {
-			_exit(EXIT_FAILURE);
-		}
-
-		int writedata;
-		writedata = write(connectfd, buffer, buffer_len);
-
-		close(connectfd);
 		close(socketfd);
 		free(buffer);
 
-		if (writedata == buffer_len) {
-			_exit(0);
-		} else {
-			_exit(EXIT_FAILURE);
-		}
+		_exit(retval);
 	} else if (pid < 0) {
 		retval = PAM_SYSTEM_ERR;
 		close(socketfd);
-- 
cgit v1.2.3


From c8bbce31973ad6a2c86549f88b7cae4286e1be20 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 10:20:59 -0500
Subject: Move the socket creation into the fork'd function

---
 src/pam-freerdp.c | 80 +++++++++++++++++++++++++++----------------------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 4e43ec4..1aab5dd 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -250,17 +250,45 @@ done:
 }
 
 static int
-session_socket_handler (const char * buffer, int buffer_len, struct passwd * pwdent, int socketfd)
+session_socket_handler (const char * buffer, int buffer_len, struct passwd * pwdent)
 {
-	/* Locks to carry over */
-	mlock(buffer, buffer_len);
-
 	if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
 			setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
 		return EXIT_FAILURE;
 	}
 
+	/* Make our socket and bind it */
+	int socketfd;
+	struct sockaddr_un socket_addr;
+
+	socketfd = socket(AF_UNIX, SOCK_STREAM, 0);
+	if (socketfd < 0) {
+		return EXIT_FAILURE;
+	}
+
+	memset(&socket_addr, 0, sizeof(struct sockaddr_un));
+	socket_addr.sun_family = AF_UNIX;
+	strncpy(socket_addr.sun_path, pwdent->pw_dir, sizeof(socket_addr.sun_path) - 1);
+	strncpy(socket_addr.sun_path + strlen(pwdent->pw_dir), "/.freerdp-socket", (sizeof(socket_addr.sun_path) - strlen(pwdent->pw_dir)) - 1);
+
+	/* We bind the socket before forking so that we ensure that
+	   there isn't a race condition to get to it.  Things will block
+	   otherwise. */
+	if (bind(socketfd, (struct sockaddr *)&socket_addr, sizeof(struct sockaddr_un)) < 0) {
+		close(socketfd);
+		return EXIT_FAILURE;
+	}
+
+	/* Set the socket file permissions to be 600 and the user and group
+	   to be the guest user.  NOTE: This won't protect on BSD */
+	if (chmod(socket_addr.sun_path, S_IRUSR | S_IWUSR) != 0 ||
+			chown(socket_addr.sun_path, pwdent->pw_uid, pwdent->pw_gid) != 0) {
+		close(socketfd);
+		return EXIT_FAILURE;
+	}
+
 	if (listen(socketfd, 1) < 0) {
+		close(socketfd);
 		return EXIT_FAILURE;
 	}
 
@@ -271,12 +299,14 @@ session_socket_handler (const char * buffer, int buffer_len, struct passwd * pwd
 	connected_addr_size = sizeof(struct sockaddr_un);
 	connectfd = accept(socketfd, (struct sockaddr *)&connected_addr, &connected_addr_size);
 	if (connectfd < 0) {
+		close(socketfd);
 		return EXIT_FAILURE;
 	}
 
 	int writedata;
 	writedata = write(connectfd, buffer, buffer_len);
 
+	close(socketfd);
 	close(connectfd);
 
 	if (writedata == buffer_len) {
@@ -319,39 +349,6 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char ** argv
 		goto done;
 	}
 	
-	/* Make our socket and bind it */
-	int socketfd;
-	struct sockaddr_un socket_addr;
-
-	socketfd = socket(AF_UNIX, SOCK_STREAM, 0);
-	if (socketfd < 0) {
-		retval = PAM_SYSTEM_ERR;
-		goto done;
-	}
-
-	memset(&socket_addr, 0, sizeof(struct sockaddr_un));
-	socket_addr.sun_family = AF_UNIX;
-	strncpy(socket_addr.sun_path, pwdent->pw_dir, sizeof(socket_addr.sun_path) - 1);
-	strncpy(socket_addr.sun_path + strlen(pwdent->pw_dir), "/.freerdp-socket", (sizeof(socket_addr.sun_path) - strlen(pwdent->pw_dir)) - 1);
-
-	/* We bind the socket before forking so that we ensure that
-	   there isn't a race condition to get to it.  Things will block
-	   otherwise. */
-	if (bind(socketfd, (struct sockaddr *)&socket_addr, sizeof(struct sockaddr_un)) < 0) {
-		close(socketfd);
-		retval = PAM_SYSTEM_ERR;
-		goto done;
-	}
-
-	/* Set the socket file permissions to be 600 and the user and group
-	   to be the guest user.  NOTE: This won't protect on BSD */
-	if (chmod(socket_addr.sun_path, S_IRUSR | S_IWUSR) != 0 ||
-			chown(socket_addr.sun_path, pwdent->pw_uid, pwdent->pw_gid) != 0) {
-		close(socketfd);
-		retval = PAM_SYSTEM_ERR;
-		goto done;
-	}
-
 	/* Build this up as a buffer so we can just write it and see that
 	   very, very clearly */
 	int buffer_len = 0;
@@ -369,15 +366,18 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char ** argv
 	if (pid == 0) {
 		int retval = 0;
 
-		retval = session_socket_handler(buffer, buffer_len, pwdent, socketfd);
+		/* Locks to carry over */
+		mlock(buffer, buffer_len);
 
-		close(socketfd);
+		retval = session_socket_handler(buffer, buffer_len, pwdent);
+
+		munlock(buffer, buffer_len);
+		memset(buffer, 0, buffer_len);
 		free(buffer);
 
 		_exit(retval);
 	} else if (pid < 0) {
 		retval = PAM_SYSTEM_ERR;
-		close(socketfd);
 	} else {
 		session_pid = pid;
 	}
-- 
cgit v1.2.3


From 7513bc73b72d2478047efac55970470c7ce2cba5 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 11:02:37 -0500
Subject: Moving buffer allocation into the function

---
 src/pam-freerdp.c | 43 ++++++++++++++++---------------------------
 1 file changed, 16 insertions(+), 27 deletions(-)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 1aab5dd..a090f02 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -250,15 +250,28 @@ done:
 }
 
 static int
-session_socket_handler (const char * buffer, int buffer_len, struct passwd * pwdent)
+session_socket_handler (struct passwd * pwdent, const char * ruser, const char * rhost, const char * rdomain, const char * password)
 {
 	if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
 			setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
 		return EXIT_FAILURE;
 	}
 
+	/* Build this up as a buffer so we can just write it and see that
+	   very, very clearly */
+	int buffer_len = 0;
+	buffer_len += strlen(ruser) + 1;    /* Add one for the space */
+	buffer_len += strlen(rhost) + 1;    /* Add one for the space */
+	buffer_len += strlen(rdomain) + 1;  /* Add one for the space */
+	buffer_len += strlen(password) + 1; /* Add one for the NULL */
+
+	char * buffer = malloc(buffer_len);
+	/* Lock the buffer before writing */
+	mlock(buffer, buffer_len);
+	snprintf(buffer, buffer_len, "%s %s %s %s", ruser, password, rdomain, rhost);
+
 	/* Make our socket and bind it */
-	int socketfd;
+	int socketfd = 0;
 	struct sockaddr_un socket_addr;
 
 	socketfd = socket(AF_UNIX, SOCK_STREAM, 0);
@@ -348,32 +361,12 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char ** argv
 		retval = PAM_SYSTEM_ERR;
 		goto done;
 	}
-	
-	/* Build this up as a buffer so we can just write it and see that
-	   very, very clearly */
-	int buffer_len = 0;
-	buffer_len += strlen(ruser) + 1;    /* Add one for the space */
-	buffer_len += strlen(rhost) + 1;    /* Add one for the space */
-	buffer_len += strlen(rdomain) + 1;  /* Add one for the space */
-	buffer_len += strlen(password) + 1; /* Add one for the NULL */
-
-	char * buffer = malloc(buffer_len);
-	/* Lock the buffer before writing */
-	mlock(buffer, buffer_len);
-	snprintf(buffer, buffer_len, "%s %s %s %s", ruser, password, rdomain, rhost);
 
 	pid_t pid = fork();
 	if (pid == 0) {
 		int retval = 0;
 
-		/* Locks to carry over */
-		mlock(buffer, buffer_len);
-
-		retval = session_socket_handler(buffer, buffer_len, pwdent);
-
-		munlock(buffer, buffer_len);
-		memset(buffer, 0, buffer_len);
-		free(buffer);
+		retval = session_socket_handler(pwdent, ruser, rhost, rdomain, password);
 
 		_exit(retval);
 	} else if (pid < 0) {
@@ -382,10 +375,6 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char ** argv
 		session_pid = pid;
 	}
 
-	memset(buffer, 0, buffer_len);
-	munlock(buffer, buffer_len);
-	free(buffer);
-
 done:
     return retval;
 }
-- 
cgit v1.2.3


From a2ece3e4d7694e940e0cca31d663233cf623272b Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 11:11:19 -0500
Subject: Restructure so that clean up is all at the end of the function

---
 src/pam-freerdp.c | 71 +++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 21 deletions(-)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index a090f02..33105d4 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -252,31 +252,55 @@ done:
 static int
 session_socket_handler (struct passwd * pwdent, const char * ruser, const char * rhost, const char * rdomain, const char * password)
 {
+	/* Socket stuff */
+	int socketfd = 0;
+	struct sockaddr_un socket_addr;
+
+	/* Connected user */
+	socklen_t connected_addr_size;
+	int connectfd = 0;
+	struct sockaddr_un connected_addr;
+
+	/* Our buffer */
+	char * buffer = NULL;
+	int buffer_len = 0;
+
+	/* Track write out */
+	int writedata = 0;
+
 	if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
 			setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
+		/* Don't need to clean up yet */
 		return EXIT_FAILURE;
 	}
 
 	/* Build this up as a buffer so we can just write it and see that
 	   very, very clearly */
-	int buffer_len = 0;
 	buffer_len += strlen(ruser) + 1;    /* Add one for the space */
 	buffer_len += strlen(rhost) + 1;    /* Add one for the space */
 	buffer_len += strlen(rdomain) + 1;  /* Add one for the space */
 	buffer_len += strlen(password) + 1; /* Add one for the NULL */
 
-	char * buffer = malloc(buffer_len);
+	if (buffer_len < 5) {
+		/* Don't need to clean up yet */
+		return EXIT_FAILURE;
+	}
+
+	buffer = malloc(buffer_len);
+
+	if (buffer == NULL) {
+		/* Don't need to clean up yet */
+		return EXIT_FAILURE;
+	}
+
 	/* Lock the buffer before writing */
 	mlock(buffer, buffer_len);
 	snprintf(buffer, buffer_len, "%s %s %s %s", ruser, password, rdomain, rhost);
 
 	/* Make our socket and bind it */
-	int socketfd = 0;
-	struct sockaddr_un socket_addr;
-
 	socketfd = socket(AF_UNIX, SOCK_STREAM, 0);
 	if (socketfd < 0) {
-		return EXIT_FAILURE;
+		goto cleanup;
 	}
 
 	memset(&socket_addr, 0, sizeof(struct sockaddr_un));
@@ -288,40 +312,45 @@ session_socket_handler (struct passwd * pwdent, const char * ruser, const char *
 	   there isn't a race condition to get to it.  Things will block
 	   otherwise. */
 	if (bind(socketfd, (struct sockaddr *)&socket_addr, sizeof(struct sockaddr_un)) < 0) {
-		close(socketfd);
-		return EXIT_FAILURE;
+		goto cleanup;
 	}
 
 	/* Set the socket file permissions to be 600 and the user and group
 	   to be the guest user.  NOTE: This won't protect on BSD */
 	if (chmod(socket_addr.sun_path, S_IRUSR | S_IWUSR) != 0 ||
 			chown(socket_addr.sun_path, pwdent->pw_uid, pwdent->pw_gid) != 0) {
-		close(socketfd);
-		return EXIT_FAILURE;
+		goto cleanup;
 	}
 
 	if (listen(socketfd, 1) < 0) {
-		close(socketfd);
-		return EXIT_FAILURE;
+		goto cleanup;
 	}
 
-	socklen_t connected_addr_size;
-	int connectfd;
-	struct sockaddr_un connected_addr;
-
 	connected_addr_size = sizeof(struct sockaddr_un);
 	connectfd = accept(socketfd, (struct sockaddr *)&connected_addr, &connected_addr_size);
 	if (connectfd < 0) {
-		close(socketfd);
-		return EXIT_FAILURE;
+		goto cleanup;
 	}
 
-	int writedata;
 	writedata = write(connectfd, buffer, buffer_len);
 
-	close(socketfd);
-	close(connectfd);
+cleanup:
+	if (socketfd != 0) {
+		close(socketfd);
+	}
+	if (connectfd != 0) {
+		close(connectfd);
+	}
+
+	if (buffer != NULL) {
+		memset(buffer, 0, buffer_len);
+		munlock(buffer, buffer_len);
+		free(buffer);
+		buffer = NULL;
+	}
 
+	/* This should be only true on the write, so we can use this to check
+	   out as writedata is init to 0 */
 	if (writedata == buffer_len) {
 		return 0;
 	}
-- 
cgit v1.2.3


From d40d11a475fa08d52dc8a66f34ca2ce28ab39b7e Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 11:14:46 -0500
Subject: Checking the return for mlock and snprintf

---
 src/pam-freerdp.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 33105d4..5696bbd 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -264,6 +264,7 @@ session_socket_handler (struct passwd * pwdent, const char * ruser, const char *
 	/* Our buffer */
 	char * buffer = NULL;
 	int buffer_len = 0;
+	int buffer_fill = 0;
 
 	/* Track write out */
 	int writedata = 0;
@@ -294,8 +295,18 @@ session_socket_handler (struct passwd * pwdent, const char * ruser, const char *
 	}
 
 	/* Lock the buffer before writing */
-	mlock(buffer, buffer_len);
-	snprintf(buffer, buffer_len, "%s %s %s %s", ruser, password, rdomain, rhost);
+	if (mlock(buffer, buffer_len) != 0) {
+		/* We can't lock, we go home */
+		goto cleanup;
+	}
+
+	buffer_fill = snprintf(buffer, buffer_len, "%s %s %s %s", ruser, password, rdomain, rhost);
+	if (buffer_fill > buffer_len) {
+		/* This really shouldn't happen, but if for some reason we have an
+		   difference between they way that the lengths are calculated we want
+		   to catch that. */
+		goto cleanup;
+	}
 
 	/* Make our socket and bind it */
 	socketfd = socket(AF_UNIX, SOCK_STREAM, 0);
-- 
cgit v1.2.3


From 49131cc81474a9bc7f872f948ea148ca8a602e29 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 11:24:18 -0500
Subject: Setting up a pipe to communicate with the sub process

---
 src/pam-freerdp.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 5696bbd..9007a52 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -250,7 +250,7 @@ done:
 }
 
 static int
-session_socket_handler (struct passwd * pwdent, const char * ruser, const char * rhost, const char * rdomain, const char * password)
+session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruser, const char * rhost, const char * rdomain, const char * password)
 {
 	/* Socket stuff */
 	int socketfd = 0;
@@ -402,16 +402,27 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char ** argv
 		goto done;
 	}
 
+	int sessionready[2];
+	if (pipe(sessionready) != 0) {
+		retval = PAM_SYSTEM_ERR;
+		goto done;
+	}
+
 	pid_t pid = fork();
 	if (pid == 0) {
 		int retval = 0;
 
-		retval = session_socket_handler(pwdent, ruser, rhost, rdomain, password);
+		retval = session_socket_handler(pwdent, sessionready[1], ruser, rhost, rdomain, password);
 
+		close(sessionready[1]);
 		_exit(retval);
 	} else if (pid < 0) {
+		close(sessionready[0]);
+		close(sessionready[1]);
+
 		retval = PAM_SYSTEM_ERR;
 	} else {
+		close(sessionready[0]);
 		session_pid = pid;
 	}
 
-- 
cgit v1.2.3


From 63889954b78abbb9396fce05620f61e834e8a2ed Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 11:34:13 -0500
Subject: Use the pipe to signal when the subprocess has gotten to a point
 where it can opperate.

---
 src/pam-freerdp.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 9007a52..89c5d5d 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -33,6 +33,7 @@
 #include <security/pam_appl.h>
 
 #define PAM_TYPE_DOMAIN  1234
+#define ALL_GOOD_SIGNAL  "Ar, ready to authenticate cap'n"
 
 static char * global_domain = NULL;
 /* FIXME? This is a work around to the fact that PAM seems to be clearing
@@ -269,6 +270,9 @@ session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruse
 	/* Track write out */
 	int writedata = 0;
 
+	/* Track ready writing */
+	int readywrite = 0;
+
 	if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
 			setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
 		/* Don't need to clean up yet */
@@ -337,6 +341,11 @@ session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruse
 		goto cleanup;
 	}
 
+	readywrite = write(readypipe, ALL_GOOD_SIGNAL, strlen(ALL_GOOD_SIGNAL) + 1);
+	if (readywrite != strlen(ALL_GOOD_SIGNAL) + 1) {
+		goto cleanup;
+	}
+
 	connected_addr_size = sizeof(struct sockaddr_un);
 	connectfd = accept(socketfd, (struct sockaddr *)&connected_addr, &connected_addr_size);
 	if (connectfd < 0) {
@@ -422,8 +431,18 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char ** argv
 
 		retval = PAM_SYSTEM_ERR;
 	} else {
+		char readbuffer[strlen(ALL_GOOD_SIGNAL) + 1];
+		int readlen = 0;
+
+		readlen = read(sessionready[0], readbuffer, strlen(ALL_GOOD_SIGNAL) + 1);
+
 		close(sessionready[0]);
-		session_pid = pid;
+
+		if (readlen == strlen(ALL_GOOD_SIGNAL) + 1) {
+			session_pid = pid;
+		} else {
+			retval = PAM_SYSTEM_ERR;
+		}
 	}
 
 done:
-- 
cgit v1.2.3


From bda98f787c631e65e347e414b2bb1a3e0c10423a Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 11:49:06 -0500
Subject: Checking the return value of the mlock

---
 src/pam-freerdp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 89c5d5d..0e5c3fa 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -150,8 +150,13 @@ get_item (pam_handle_t * pamh, int type)
 				free(global_password);
 			}
 			global_password = strdup(promptval);
-			mlock(global_password, strlen(global_password));
-			retval = global_password;
+			if (mlock(global_password, strlen(global_password)) != 0) {
+				/* Woah, can't lock it.  Can't keep it. */
+				free(global_password);
+				global_password = NULL;
+			} else {
+				retval = global_password;
+			}
 		}
 
 		free(promptval);
-- 
cgit v1.2.3


From 58bca1e60eaccd13b99cfcba93a6ad7ab6d93f75 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 11:53:42 -0500
Subject: Locking memory if we expect the prompt to be returning a password

---
 src/pam-freerdp.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 0e5c3fa..43b16d5 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -110,6 +110,18 @@ get_item (pam_handle_t * pamh, int type)
 	char * promptval = responses->resp;
 	free(responses);
 
+	/* If we didn't get anything, just move on */
+	if (promptval == NULL) {
+		return NULL;
+	}
+
+	if (type == PAM_AUTHTOK) {
+		if (mlock(promptval, strlen(promptval) + 1) != 0) {
+			free(promptval);
+			return NULL;
+		}
+	}
+
 	if (type == PAM_RHOST) {
 		char * subloc = strstr(promptval, "://");
 		if (subloc != NULL) {
@@ -146,11 +158,11 @@ get_item (pam_handle_t * pamh, int type)
 			/* We also save the password globally if we've got one */
 			if (global_password != NULL) {
 				memset(global_password, 0, strlen(global_password));
-				munlock(global_password, strlen(global_password));
+				munlock(global_password, strlen(global_password) + 1);
 				free(global_password);
 			}
 			global_password = strdup(promptval);
-			if (mlock(global_password, strlen(global_password)) != 0) {
+			if (mlock(global_password, strlen(global_password) + 1) != 0) {
 				/* Woah, can't lock it.  Can't keep it. */
 				free(global_password);
 				global_password = NULL;
@@ -159,6 +171,11 @@ get_item (pam_handle_t * pamh, int type)
 			}
 		}
 
+		if (type == PAM_AUTHTOK) {
+			memset(promptval, 0, strlen(promptval) + 1);
+			munlock(promptval, strlen(promptval) + 1);
+		}
+
 		free(promptval);
 	}
 
-- 
cgit v1.2.3


From 3058f050cdb8f65f176281a82def12804ae85d05 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 11:55:41 -0500
Subject: Make sure to clear the environments

---
 src/pam-freerdp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 43b16d5..ed83402 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -238,6 +238,10 @@ pam_sm_authenticate (pam_handle_t *pamh, int flags, int argc, const char **argv)
 			_exit(EXIT_FAILURE);
 		}
 
+		if (clearenv() != 0) {
+			_exit(EXIT_FAILURE);
+		}
+
 		setenv("HOME", pwdent->pw_dir, 1);
 
 		execvp(args[0], args);
@@ -301,6 +305,11 @@ session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruse
 		return EXIT_FAILURE;
 	}
 
+	if (clearenv() != 0) {
+		/* Don't need to clean up yet */
+		return EXIT_FAILURE;
+	}
+
 	/* Build this up as a buffer so we can just write it and see that
 	   very, very clearly */
 	buffer_len += strlen(ruser) + 1;    /* Add one for the space */
-- 
cgit v1.2.3


From f839484b45f89e62a3e635c35402ebd807e78499 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 11:58:02 -0500
Subject: Clear the groups when dropping privs

---
 src/pam-freerdp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index ed83402..90686a9 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -27,6 +27,7 @@
 #include <sys/mman.h>
 #include <sys/un.h>
 #include <pwd.h>
+#include <grp.h>
 
 #include <security/pam_modules.h>
 #include <security/pam_modutil.h>
@@ -238,6 +239,10 @@ pam_sm_authenticate (pam_handle_t *pamh, int flags, int argc, const char **argv)
 			_exit(EXIT_FAILURE);
 		}
 
+		if (setgroups(1, &pwdent->pw_gid) != 0) {
+			_exit(EXIT_FAILURE);
+		}
+
 		if (clearenv() != 0) {
 			_exit(EXIT_FAILURE);
 		}
@@ -305,6 +310,11 @@ session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruse
 		return EXIT_FAILURE;
 	}
 
+	if (setgroups(1, &pwdent->pw_gid) != 0) {
+		/* Don't need to clean up yet */
+		return EXIT_FAILURE;
+	}
+
 	if (clearenv() != 0) {
 		/* Don't need to clean up yet */
 		return EXIT_FAILURE;
-- 
cgit v1.2.3


From d71967fdfecdc1ed309eac4e68b4a72d522d3633 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 12:01:14 -0500
Subject: Make sure to lock the password buffer

---
 src/freerdp-auth-check.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/freerdp-auth-check.c b/src/freerdp-auth-check.c
index d50833b..178db98 100644
--- a/src/freerdp-auth-check.c
+++ b/src/freerdp-auth-check.c
@@ -60,6 +60,10 @@ main (int argc, char * argv[])
 		return -1;
 	}
 
+	if (mlock(password, sizeof(password)) != 0) {
+		return -1;
+	}
+
 	freerdp_channels_global_init();
 
 	freerdp * instance = freerdp_new();
@@ -88,10 +92,14 @@ main (int argc, char * argv[])
 		instance->settings->port = strtoul(colonloc, NULL, 10);
 	}
 
+	int retval = -1;
 	if (freerdp_connect(instance)) {
 		freerdp_disconnect(instance);
-		return 0;
-	} else {
-		return -1;
+		retval = 0;
 	}
+
+	memset(password, 0, sizeof(password));
+	munlock(password, sizeof(password));
+
+	return retval;
 }
-- 
cgit v1.2.3


From 4b95b652e1567fe781be8e91450903169cc45e12 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 12:02:34 -0500
Subject: Dropping the ignoring of the cert

---
 src/freerdp-auth-check.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/freerdp-auth-check.c b/src/freerdp-auth-check.c
index 178db98..e5e9d13 100644
--- a/src/freerdp-auth-check.c
+++ b/src/freerdp-auth-check.c
@@ -81,7 +81,6 @@ main (int argc, char * argv[])
 	instance->settings->username = argv[2];
 	instance->settings->domain = argv[3];
 	instance->settings->password = password;
-	instance->settings->ignore_certificate = true;
 
 	char * colonloc = strstr(argv[1], ":");
 	if (colonloc != NULL) {
-- 
cgit v1.2.3


From 716fa94e881301ddbc923fbde787d32b70a7243c Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 12:06:11 -0500
Subject: Make sure to change the working directory for the subprocesses to the
 guest user's home directory

---
 src/pam-freerdp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 90686a9..f097ae2 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -247,6 +247,10 @@ pam_sm_authenticate (pam_handle_t *pamh, int flags, int argc, const char **argv)
 			_exit(EXIT_FAILURE);
 		}
 
+		if (chdir(pwdent->pw_dir) != 0) {
+			_exit(EXIT_FAILURE);
+		}
+
 		setenv("HOME", pwdent->pw_dir, 1);
 
 		execvp(args[0], args);
@@ -320,6 +324,11 @@ session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruse
 		return EXIT_FAILURE;
 	}
 
+	if (chdir(pwdent->pw_dir) != 0) {
+		/* Don't need to clean up yet */
+		return EXIT_FAILURE;
+	}
+
 	/* Build this up as a buffer so we can just write it and see that
 	   very, very clearly */
 	buffer_len += strlen(ruser) + 1;    /* Add one for the space */
-- 
cgit v1.2.3


From 0501a4375073d9e098446cb3b4c1515b08704a23 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 12:16:31 -0500
Subject: Making sure to kill as the user so that if there is PID wrap or
 something else we won't kill the wrong thing

---
 src/pam-freerdp.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index f097ae2..4b97bb8 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -504,12 +504,60 @@ done:
 PAM_EXTERN int
 pam_sm_close_session (pam_handle_t *pamh, int flags, int argc, const char **argv)
 {
-	if (session_pid != 0) {
-		kill(session_pid, SIGKILL);
+	if (session_pid == 0) {
+		return PAM_IGNORE;
+	}
+
+	char * username = NULL;
+	int retval = PAM_SUCCESS;
+
+	GET_ITEM(username, PAM_USER);
+
+	struct passwd * pwdent = getpwnam(username);
+	if (pwdent == NULL) {
+		retval = PAM_SYSTEM_ERR;
+		goto done;
+	}
+
+	pid_t pid = fork();
+	if (pid == 0) {
+		if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
+				setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
+			_exit(EXIT_FAILURE);
+		}
+
+		if (setgroups(1, &pwdent->pw_gid) != 0) {
+			_exit(EXIT_FAILURE);
+		}
+
+		if (clearenv() != 0) {
+			_exit(EXIT_FAILURE);
+		}
+
+		int killval = kill(session_pid, SIGKILL);
 		session_pid = 0;
+
+		if (killval != 0) {
+			printf("Unable to kill\n");
+		}
+
+		/* NOTE: We're ignoring whether we could kill it or not.  It'd be nice to
+		   track that but there are a lot of reason that we could fail there and
+		   it's not a bad thing.  Really we're attempting a best effort to clean up
+		   we won't be able to gaurantee it. */
+		_exit(EXIT_SUCCESS);	
+	} else if (pid < 0) {
+		retval = PAM_SYSTEM_ERR;
+	} else {
+		int forkret = 0;
+
+		if (waitpid(pid, &forkret, 0) < 0) {
+			retval = PAM_SYSTEM_ERR;
+		}
 	}
 
-	return PAM_IGNORE;
+done:
+	return retval;
 }
 
 /* LightDM likes to have this function around, but we don't need it as we
-- 
cgit v1.2.3


From 73858946b83ff5fe24aea484af32bb8ed4c1a1c2 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 13:49:41 -0500
Subject: Clear the session_pid after trying to kill it.

---
 src/pam-freerdp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 4b97bb8..88286c9 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -556,6 +556,10 @@ pam_sm_close_session (pam_handle_t *pamh, int flags, int argc, const char **argv
 		}
 	}
 
+	/* We reset this no matter.  If we error'd trying to do it, we don't
+	   want to try again.  We'll just return the error for this time. */
+	session_pid = 0;
+
 done:
 	return retval;
 }
-- 
cgit v1.2.3


From 85bd7adb19764abedb6ef072c17a16ebd53a2b1c Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 14:52:01 -0500
Subject: Removing setgroups as it doesn't seem to be working

---
 src/pam-freerdp.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 88286c9..24a55d0 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -239,10 +239,6 @@ pam_sm_authenticate (pam_handle_t *pamh, int flags, int argc, const char **argv)
 			_exit(EXIT_FAILURE);
 		}
 
-		if (setgroups(1, &pwdent->pw_gid) != 0) {
-			_exit(EXIT_FAILURE);
-		}
-
 		if (clearenv() != 0) {
 			_exit(EXIT_FAILURE);
 		}
@@ -314,11 +310,6 @@ session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruse
 		return EXIT_FAILURE;
 	}
 
-	if (setgroups(1, &pwdent->pw_gid) != 0) {
-		/* Don't need to clean up yet */
-		return EXIT_FAILURE;
-	}
-
 	if (clearenv() != 0) {
 		/* Don't need to clean up yet */
 		return EXIT_FAILURE;
@@ -526,10 +517,6 @@ pam_sm_close_session (pam_handle_t *pamh, int flags, int argc, const char **argv
 			_exit(EXIT_FAILURE);
 		}
 
-		if (setgroups(1, &pwdent->pw_gid) != 0) {
-			_exit(EXIT_FAILURE);
-		}
-
 		if (clearenv() != 0) {
 			_exit(EXIT_FAILURE);
 		}
-- 
cgit v1.2.3

-- 
cgit v1.2.3


From dbef2c50ce25e1fbb6a517fa1103d953c5c50dd8 Mon Sep 17 00:00:00 2001
From: Ted Gould <ted@gould.cx>
Date: Thu, 30 Aug 2012 22:35:28 -0500
Subject: Clearing the groups, but handling the EPERM issue with not being root

---
 src/pam-freerdp.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/pam-freerdp.c b/src/pam-freerdp.c
index 24a55d0..b271834 100644
--- a/src/pam-freerdp.c
+++ b/src/pam-freerdp.c
@@ -28,6 +28,7 @@
 #include <sys/un.h>
 #include <pwd.h>
 #include <grp.h>
+#include <errno.h>
 
 #include <security/pam_modules.h>
 #include <security/pam_modutil.h>
@@ -234,6 +235,12 @@ pam_sm_authenticate (pam_handle_t *pamh, int flags, int argc, const char **argv)
 			_exit(EXIT_FAILURE);
 		}
 
+		/* Setting groups, but allowing EPERM as if we're not 100% root
+		   we might not be able to do this */
+		if (setgroups(1, &pwdent->pw_gid) != 0 && errno != EPERM) {
+			_exit(EXIT_FAILURE);
+		}
+
 		if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
 				setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
 			_exit(EXIT_FAILURE);
@@ -304,6 +311,12 @@ session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruse
 	/* Track ready writing */
 	int readywrite = 0;
 
+	/* Setting groups, but allowing EPERM as if we're not 100% root
+	   we might not be able to do this */
+	if (setgroups(1, &pwdent->pw_gid) != 0 && errno != EPERM) {
+		_exit(EXIT_FAILURE);
+	}
+
 	if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
 			setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
 		/* Don't need to clean up yet */
@@ -512,6 +525,12 @@ pam_sm_close_session (pam_handle_t *pamh, int flags, int argc, const char **argv
 
 	pid_t pid = fork();
 	if (pid == 0) {
+		/* Setting groups, but allowing EPERM as if we're not 100% root
+		   we might not be able to do this */
+		if (setgroups(1, &pwdent->pw_gid) != 0 && errno != EPERM) {
+			_exit(EXIT_FAILURE);
+		}
+
 		if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
 				setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
 			_exit(EXIT_FAILURE);
-- 
cgit v1.2.3