Merge lp:~freerdp-remote-team/libpam-freerdp/ubuntu into lp:~ubuntu-desktop/libpam-freerdp/ubuntu

[Ted Gould]

0.4.0

=== modified file 'ChangeLog'
--- ChangeLog	2012-08-29 15:31:18 +0000
+++ ChangeLog	2012-09-05 20:59:30 +0000
@@ -1,5 +1,101 @@
 # Generated by Makefile. Do not edit.
 
[phone]  Ted Gould  <[e-mail]>
+
+	0.4.0
+
[phone]  Ted Gould  <[e-mail]>
+
+	Making the open_session kill also unpriv. Approved by Albert Astals Cid, jenkins.
+
[phone]  Ted Gould  <[e-mail]>
+
+	Using the new function in the open_session function instead of killing directly.
+
[phone]  Ted Gould  <[e-mail]>
+
+	Moving the kill code into a function
+
[phone]  Ted Gould  <[e-mail]>
+
+	Resolving concerns of the security team. Fixes: https://bugs.launchpad.net/bugs/1039634. Approved by Albert Astals Cid, jenkins.
+
[phone]  Ted Gould  <[e-mail]>
+
+	Clearing the groups, but handling the EPERM issue with not being root
+
[phone]  Ted Gould  <[e-mail]>
+
+	Attaching bug
+
[phone]  Ted Gould  <[e-mail]>
+
+	Removing setgroups as it doesn't seem to be working
+
[phone]  Ted Gould  <[e-mail]>
+
+	Clear the session_pid after trying to kill it.
+
[phone]  Ted Gould  <[e-mail]>
+
+	Making sure to kill as the user so that if there is PID wrap or something else we won't kill the wrong thing
+
[phone]  Ted Gould  <[e-mail]>
+
+	Make sure to change the working directory for the subprocesses to the guest user's home directory
+
[phone]  Ted Gould  <[e-mail]>
+
+	Dropping the ignoring of the cert
+
[phone]  Ted Gould  <[e-mail]>
+
+	Make sure to lock the password buffer
+
[phone]  Ted Gould  <[e-mail]>
+
+	Clear the groups when dropping privs
+
[phone]  Ted Gould  <[e-mail]>
+
+	Make sure to clear the environments
+
[phone]  Ted Gould  <[e-mail]>
+
+	Locking memory if we expect the prompt to be returning a password
+
[phone]  Ted Gould  <[e-mail]>
+
+	Checking the return value of the mlock
+
[phone]  Ted Gould  <[e-mail]>
+
+	Use the pipe to signal when the subprocess has gotten to a point where it can opperate.
+
[phone]  Ted Gould  <[e-mail]>
+
+	Setting up a pipe to communicate with the sub process
+
[phone]  Ted Gould  <[e-mail]>
+
+	Checking the return for mlock and snprintf
+
[phone]  Ted Gould  <[e-mail]>
+
+	Restructure so that clean up is all at the end of the function
+
[phone]  Ted Gould  <[e-mail]>
+
+	Moving buffer allocation into the function
+
[phone]  Ted Gould  <[e-mail]>
+
+	Move the socket creation into the fork'd function
+
[phone]  Ted Gould  <[e-mail]>
+
+	Refactor to pull the long running stuff out of the if statement and into a function
+
 2012-08-29  Ted Gould  <[e-mail]>
 
 	0.3.0

=== modified file 'configure'
--- configure	2012-08-29 15:31:18 +0000
+++ configure	2012-09-05 20:59:30 +0000
@@ -1,6 +1,6 @@
 #! /bin/sh
 # Guess values for system-dependent variables and create Makefiles.
-# Generated by GNU Autoconf 2.69 for libpam-freerdp 0.3.0.
+# Generated by GNU Autoconf 2.69 for libpam-freerdp 0.4.0.
 #
 #
 # Copyright (C) 1992-1996, 1998-2012 Free Software Foundation, Inc.
@@ -587,8 +587,8 @@
 # Identity of this package.
 PACKAGE_NAME='libpam-freerdp'
 PACKAGE_TARNAME='libpam-freerdp'
-PACKAGE_VERSION='0.3.0'
-PACKAGE_STRING='libpam-freerdp 0.3.0'
+PACKAGE_VERSION='0.4.0'
+PACKAGE_STRING='libpam-freerdp 0.4.0'
 PACKAGE_BUGREPORT=''
 PACKAGE_URL=''
 
@@ -1318,7 +1318,7 @@
   # Omit some internal or obsolete options to make the list less imposing.
   # This message is too long to be a string in the A/UX 3.1 sh.
   cat <<_ACEOF
-\`configure' configures libpam-freerdp 0.3.0 to adapt to many kinds of systems.
+\`configure' configures libpam-freerdp 0.4.0 to adapt to many kinds of systems.
 
 Usage: $0 [OPTION]... [VAR=VALUE]...
 
@@ -1388,7 +1388,7 @@
 
 if test -n "$ac_init_help"; then
   case $ac_init_help in
-     short | recursive ) echo "Configuration of libpam-freerdp 0.3.0:";;
+     short | recursive ) echo "Configuration of libpam-freerdp 0.4.0:";;
    esac
   cat <<\_ACEOF
 
@@ -1501,7 +1501,7 @@
 test -n "$ac_init_help" && exit $ac_status
 if $ac_init_version; then
   cat <<\_ACEOF
-libpam-freerdp configure 0.3.0
+libpam-freerdp configure 0.4.0
 generated by GNU Autoconf 2.69
 
 Copyright (C) 2012 Free Software Foundation, Inc.
@@ -1779,7 +1779,7 @@
 This file contains any messages produced by compilers while
 running configure, to aid debugging if configure makes a mistake.
 
-It was created by libpam-freerdp $as_me 0.3.0, which was
+It was created by libpam-freerdp $as_me 0.4.0, which was
 generated by GNU Autoconf 2.69.  Invocation command line was
 
   $ $0 $@
@@ -2597,7 +2597,7 @@
 
 # Define the identity of the package.
  PACKAGE='libpam-freerdp'
- VERSION='0.3.0'
+ VERSION='0.4.0'
 
 
 cat >>confdefs.h <<_ACEOF
@@ -12068,7 +12068,7 @@
 # report actual input values of CONFIG_FILES etc. instead of their
 # values after options handling.
 ac_log="
-This file was extended by libpam-freerdp $as_me 0.3.0, which was
+This file was extended by libpam-freerdp $as_me 0.4.0, which was
 generated by GNU Autoconf 2.69.  Invocation command line was
 
   CONFIG_FILES    = $CONFIG_FILES
@@ -12134,7 +12134,7 @@
 cat >>$CONFIG_STATUS <<_ACEOF || ac_write_fail=1
 ac_cs_config="`$as_echo "$ac_configure_args" | sed 's/^ //; s/[\\""\`\$]/\\\\&/g'`"
 ac_cs_version="\\
-libpam-freerdp config.status 0.3.0
+libpam-freerdp config.status 0.4.0
 configured by $0, generated by GNU Autoconf 2.69,
   with options \\"\$ac_cs_config\\"
 

=== modified file 'configure.ac'
--- configure.ac	2012-08-29 15:28:35 +0000
+++ configure.ac	2012-09-05 20:59:30 +0000
@@ -1,4 +1,4 @@
-AC_INIT([libpam-freerdp], [0.3.0])
+AC_INIT([libpam-freerdp], [0.4.0])
 AC_CONFIG_HEADERS([config.h])
 
 AM_INIT_AUTOMAKE([1.11 -Wno-portability])

=== modified file 'debian/changelog'
--- debian/changelog	2012-08-29 17:37:03 +0000
+++ debian/changelog	2012-09-05 20:59:30 +0000
@@ -1,3 +1,13 @@
+libpam-freerdp (0.4.0-0ubuntu1~ppa1) quantal; urgency=low
+
+  * New upstream release.
+    * Security fixes for priviledged kill of PID
+    * Ensuring return values are trapped
+    * Binding port after dropping permissions
+    * Handling more corner cases
+
+ -- Ted Gould <[e-mail]>  Wed, 05 Sep 2012 15:57:55 -0500
+
 libpam-freerdp (0.3.0-0ubuntu1) quantal; urgency=low
 
   * New upstream release.

=== removed directory 'debian/source'
=== removed file 'debian/source/format'
--- debian/source/format	2012-08-21 15:48:21 +0000
+++ debian/source/format	1970-01-01 00:00:00 +0000
@@ -1,1 +0,0 @@
-3.0 (quilt)

=== modified file 'src/freerdp-auth-check.c'
--- src/freerdp-auth-check.c	2012-08-27 20:07:36 +0000
+++ src/freerdp-auth-check.c	2012-09-05 20:59:30 +0000
@@ -60,6 +60,10 @@
 		return -1;
 	}
 
+	if (mlock(password, sizeof(password)) != 0) {
+		return -1;
+	}
+
 	freerdp_channels_global_init();
 
 	freerdp * instance = freerdp_new();
@@ -77,7 +81,6 @@
 	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) {
@@ -88,10 +91,14 @@
 		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;
 }

=== modified file 'src/pam-freerdp.c'
--- src/pam-freerdp.c	2012-08-29 15:34:27 +0000
+++ src/pam-freerdp.c	2012-09-05 20:59:30 +0000
@@ -27,12 +27,17 @@
 #include <sys/mman.h>
 #include <sys/un.h>
 #include <pwd.h>
+#include <grp.h>
+#include <errno.h>
 
 #include <security/pam_modules.h>
 #include <security/pam_modutil.h>
 #include <security/pam_appl.h>
 
 #define PAM_TYPE_DOMAIN  1234
+#define ALL_GOOD_SIGNAL  "Ar, ready to authenticate cap'n"
+
+static int unpriveleged_kill (struct passwd * pwdent);
 
 static char * global_domain = NULL;
 /* FIXME? This is a work around to the fact that PAM seems to be clearing
@@ -109,6 +114,18 @@
 	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) {
@@ -145,12 +162,22 @@
 			/* 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);
-			mlock(global_password, strlen(global_password));
-			retval = global_password;
+			if (mlock(global_password, strlen(global_password) + 1) != 0) {
+				/* Woah, can't lock it.  Can't keep it. */
+				free(global_password);
+				global_password = NULL;
+			} else {
+				retval = global_password;
+			}
+		}
+
+		if (type == PAM_AUTHTOK) {
+			memset(promptval, 0, strlen(promptval) + 1);
+			munlock(promptval, strlen(promptval) + 1);
 		}
 
 		free(promptval);
@@ -210,11 +237,25 @@
 			_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);
 		}
 
+		if (clearenv() != 0) {
+			_exit(EXIT_FAILURE);
+		}
+
+		if (chdir(pwdent->pw_dir) != 0) {
+			_exit(EXIT_FAILURE);
+		}
+
 		setenv("HOME", pwdent->pw_dir, 1);
 
 		execvp(args[0], args);
@@ -249,47 +290,88 @@
 	return retval;
 }
 
-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
-   xfreerdp viewer for the login */
-PAM_EXTERN int
-pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char ** argv)
+static int
+session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruser, const char * rhost, const char * rdomain, const char * password)
 {
-	if (session_pid != 0) {
-		kill(session_pid, SIGKILL);
-		session_pid = 0;
-	}
-
-	char * username = NULL;
-	char * password = NULL;
-	char * ruser = NULL;
-	char * rhost = NULL;
-	char * rdomain = NULL;
-	int retval = PAM_SUCCESS;
-
-	/* Get all the values, or prompt for them, or return with
-	   an auth error */
-	GET_ITEM(username, PAM_USER);
-	GET_ITEM(ruser,    PAM_RUSER);
-	GET_ITEM(rhost,    PAM_RHOST);
-	GET_ITEM(rdomain,  PAM_TYPE_DOMAIN);
-	GET_ITEM(password, PAM_AUTHTOK);
-
-	struct passwd * pwdent = getpwnam(username);
-	if (pwdent == NULL) {
-		retval = PAM_SYSTEM_ERR;
-		goto done;
-	}
-	
+	/* 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;
+	int buffer_fill = 0;
+
+	/* Track write out */
+	int writedata = 0;
+
+	/* 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 */
+		return EXIT_FAILURE;
+	}
+
+	if (clearenv() != 0) {
+		/* Don't need to clean up yet */
+		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 */
+	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 */
+
+	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 */
+	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 */
-	int socketfd;
-	struct sockaddr_un socket_addr;
-
 	socketfd = socket(AF_UNIX, SOCK_STREAM, 0);
 	if (socketfd < 0) {
-		retval = PAM_SYSTEM_ERR;
-		goto done;
+		goto cleanup;
 	}
 
 	memset(&socket_addr, 0, sizeof(struct sockaddr_un));
@@ -301,80 +383,123 @@
 	   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;
+		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) {
+		goto cleanup;
+	}
+
+	if (listen(socketfd, 1) < 0) {
+		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) {
+		goto cleanup;
+	}
+
+	writedata = write(connectfd, buffer, buffer_len);
+
+cleanup:
+	if (socketfd != 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;
-	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) {
-		/* 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) {
-			_exit(EXIT_FAILURE);
-		}
-
-		if (listen(socketfd, 1) < 0) {
-			_exit(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) {
-			_exit(EXIT_FAILURE);
-		}
-
-		int writedata;
-		writedata = write(connectfd, buffer, buffer_len);
-
+	if (connectfd != 0) {
 		close(connectfd);
-		close(socketfd);
+	}
+
+	if (buffer != NULL) {
+		memset(buffer, 0, buffer_len);
+		munlock(buffer, buffer_len);
 		free(buffer);
-
-		if (writedata == buffer_len) {
-			_exit(0);
+		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;
+	}
+
+	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
+   xfreerdp viewer for the login */
+PAM_EXTERN int
+pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char ** argv)
+{
+	char * username = NULL;
+	char * password = NULL;
+	char * ruser = NULL;
+	char * rhost = NULL;
+	char * rdomain = NULL;
+	int retval = PAM_SUCCESS;
+
+	/* Get all the values, or prompt for them, or return with
+	   an auth error */
+	GET_ITEM(username, PAM_USER);
+	GET_ITEM(ruser,    PAM_RUSER);
+	GET_ITEM(rhost,    PAM_RHOST);
+	GET_ITEM(rdomain,  PAM_TYPE_DOMAIN);
+	GET_ITEM(password, PAM_AUTHTOK);
+
+	struct passwd * pwdent = getpwnam(username);
+	if (pwdent == NULL) {
+		retval = PAM_SYSTEM_ERR;
+		goto done;
+	}
+
+	if (session_pid != 0) {
+		unpriveleged_kill(pwdent);
+	}
+
+	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, 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 {
+		char readbuffer[strlen(ALL_GOOD_SIGNAL) + 1];
+		int readlen = 0;
+
+		readlen = read(sessionready[0], readbuffer, strlen(ALL_GOOD_SIGNAL) + 1);
+
+		close(sessionready[0]);
+
+		if (readlen == strlen(ALL_GOOD_SIGNAL) + 1) {
+			session_pid = pid;
 		} else {
-			_exit(EXIT_FAILURE);
+			retval = PAM_SYSTEM_ERR;
 		}
-	} else if (pid < 0) {
-		retval = PAM_SYSTEM_ERR;
-		close(socketfd);
-	} else {
-		session_pid = pid;
 	}
 
-	memset(buffer, 0, buffer_len);
-	munlock(buffer, buffer_len);
-	free(buffer);
-
 done:
     return retval;
 }
@@ -384,12 +509,79 @@
 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;
+	}
+
+	retval = unpriveleged_kill(pwdent);
+
+done:
+	return retval;
+}
+
+/* Drop privs and try to kill the process with the PID of session_pid.  
+   This ensures that we don't kill something important if there is PID wrap
+   around.  */
+static int
+unpriveleged_kill (struct passwd * pwdent)
+{
+	int retval = PAM_SUCCESS;
+
+	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);
+		}
+
+		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;
+	/* 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;
+
+	return retval;
 }
 
 /* LightDM likes to have this function around, but we don't need it as we