[PATCH 0/4] newrole suid functionality (take 2)

[PATCH 0/4] newrole suid functionality (take 2)

[Michael C Thompson]

This is the intro to a set of four patches.

These patches are an attempt to make newrole be an acceptably secure 
suid root program, to provide it with the capabilities to generate audit 
records (existing) and handle polyinstatiation (new).

The 4 patches are as follows:
1) New functions introduced to newrole.c, new and existing functionality
2) Changes to existing functions in newrole.c
3) Updates to main in newrole.c to use the aforementioned changes
4) Changes to the Makefile to allow building of newrole with the
    changes and introduction of newrole-lspp.pamd

Note: This is an atomically applicable patch set. Applying a subset of 
these patches will break the build.

The comments from the previous send of these patches have been integrated.

Thanks,
Mike


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

[PATCH 1/4] newrole suid functionality (take 2)

[Michael C Thompson]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

Michael C Thompson wrote:
> This is the intro to a set of four patches.
> 
> These patches are an attempt to make newrole be an acceptably secure 
> suid root program, to provide it with the capabilities to generate audit 
> records (existing) and handle polyinstatiation (new).
> 
> The 4 patches are as follows:
> 1) New functions introduced to newrole.c, new and existing functionality

This is the 1st of 4 patches.
This patch applies against policycoreutils-1.30.30-1.

Changes:
  * This patch breaks up the operations performed by newrole
    into functions and adds new functionality to support suid
    operations. They are called in patch 3/4.
  * The following functions do not new functionality:
    - extract_pw_data
    - relabel_tty
    - restore_tty_label
    - parse_command_line_arguments
  * The following functions do add new functionality:
    - sanitize_environment
    - transition_to_caller_uid
    - set_signal_handles

The new (and existing) functionality should be adequately documented in 
the code. If it is not clear, I consider that to be a failure of the 
code, and please let me know.



Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>


[-- Attachment #2: 01-new_functions.patch --]
[-- Type: text/x-diff, Size: 12123 bytes --]

diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
--- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:11:41.000000000 -0500
+++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:12:29.000000000 -0500
@@ -87,6 +87,7 @@
 /* USAGE_STRING describes the command-line args of this program. */
 #define USAGE_STRING "USAGE: newrole [ -r role ] [ -t type ] [ -l level ] [ -V ] [ -- args ]"
 
+#define DEFAULT_PATH "/bin:/usr/bin:/usr/local/bin"
 #define DEFAULT_CONTEXT_SIZE 255	/* first guess at context size */
 
 char *xstrdup(const char *s)
@@ -332,6 +333,117 @@
 	return found;
 }
 
+/**
+ * Determine the Linux user identity to re-authenticate.
+ * If supported and set, use the login uid, as this should be more stable.
+ * Otherwise, use the real uid.
+ *
+ * This function assigns malloc'd memory into the pw_copy struct.
+ * Returns zero on success, non-zero otherwise
+ */
+int extract_pw_data(struct passwd *pw_copy)
+{
+	uid_t uid;
+	struct passwd *pw;
+
+#ifdef USE_AUDIT
+	uid = audit_getloginuid();
+	if (uid == (uid_t) - 1)
+		uid = getuid();
+#else
+	uid = getuid();
+#endif
+
+	setpwent();
+	pw = getpwuid(uid);
+	endpwent();
+	if (!(pw && pw->pw_name && pw->pw_name[0] && pw->pw_shell
+	      && pw->pw_shell[0] && pw->pw_dir && pw->pw_dir[0])) {
+		fprintf(stderr,
+			_("cannot find valid entry in the passwd file.\n"));
+		return -1;
+	}
+
+	*pw_copy = *pw;
+	pw = pw_copy;
+	pw->pw_name = strdup(pw->pw_name);
+	pw->pw_dir = strdup(pw->pw_dir);
+	pw->pw_shell = strdup(pw->pw_shell);
+
+	if (! (pw->pw_name && pw->pw_dir && pw->pw_shell)) {
+		fprintf(stderr, _("Out of memory!\n"));
+		goto out_free;
+	}
+
+	if (verify_shell(pw->pw_shell) == 0) {
+		fprintf(stderr, _("Error!  Shell is not valid.\n"));
+		goto out_free;
+	}
+	return 0;
+
+out_free:
+	free(pw->pw_name);
+	free(pw->pw_dir);
+	free(pw->pw_shell);
+	return -1;
+}
+
+/**
+ * Unset all environment variables except:
+ * TERM, DISPLAY and XAUTHORITY - if they are set, preserve values
+ * HOME, SHELL, USER and LOGNAME - set to contents of /etc/passwd
+ * PATH - set to default value DEFAULT_PATH
+ *
+ * Returns zero on success, non-zero otherwise
+ */
+static int sanitize_environment(const struct passwd *pw)
+{
+	char const *term_env = getenv("TERM");
+	char const *display_env = getenv("DISPLAY");
+	char const *xauthority_env = getenv("XAUTHORITY");
+	char *term = NULL;		/* temporary container */
+	char *display = NULL;		/* temporary container */
+	char *xauthority = NULL;	/* temporary container */
+	int rc;
+
+	/* Save the variable values we want */
+	if (term_env)
+		term = strdup(term_env);
+	if (display_env)
+		display = strdup(display_env);
+	if (xauthority_env)
+		xauthority = strdup(xauthority_env);
+	if ((term_env && !term) || (display_env && !display) ||
+	     (xauthority_env && !xauthority)) {
+		rc = -1;
+		goto out;
+	}
+
+	/* Clear everything from the env */
+	if ((rc = clearenv())) {
+		fprintf(stderr, _("Unable to clear environment\n"));
+		goto out;
+	}
+
+	/* Restore that which we saved */
+	if (term)
+		rc |= setenv("TERM", term, 1);
+	if (display)
+		rc |= setenv("DISPLAY", display, 1);
+	if (xauthority)
+		rc |= setenv("XAUTHORITY", xauthority, 1);
+	rc |= setenv("HOME", pw->pw_dir, 1);
+	rc |= setenv("SHELL", pw->pw_shell, 1);
+	rc |= setenv("USER", pw->pw_name, 1);
+	rc |= setenv("LOGNAME", pw->pw_name, 1);
+	rc |= setenv("PATH", DEFAULT_PATH, 1);
+out:
+	free(term);
+	free(display);
+	free(xauthority);
+	return rc;
+}
+
 /*
  * This function will drop the capabilities so that we are left
  * only with access to the audit system. If the user is root, we leave
@@ -396,6 +508,28 @@
 }
 #endif
 
+#ifdef NAMESPACE_PRIV
+/**
+ * This function will set the uid values to be that of caller's uid, and
+ * will drop any privilages which maybe have been raised.
+ */
+static int transition_to_caller_uid()
+{
+	uid_t uid = getuid();
+
+	if (prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0) < 0) {
+		fprintf(stderr, _("Error resetting KEEPCAPS, aborting\n"));
+		return -1;
+	}
+
+	if (setresuid(uid, uid, uid)) {
+		fprintf(stderr, _("Error changing uid, aborting.\n"));
+		return -1;
+	}
+	return 0;
+}
+#endif
+
 #ifdef LOG_AUDIT_PRIV
 /* Send audit message */
 static
@@ -443,6 +577,297 @@
 }
 #endif
 
+/**
+ * This function attempts to relabel the tty. If this function fails, then
+ * the fd is closed, the contexts are free'd and -1 is returned. On success,
+ * a valid fd is returned and tty_context and new_tty_context are set.
+ *
+ * This function will not fail if it can not relabel the tty when selinux is
+ * in permissive mode.
+ */
+static int relabel_tty(const char *ttyn, security_context_t new_context,
+			security_context_t *tty_context,
+			security_context_t *new_tty_context)
+{
+	int fd;
+	int enforcing = security_getenforce();
+	security_context_t tty_con = NULL;
+	security_context_t new_tty_con = NULL;
+
+	if (enforcing < 0) {
+		fprintf(stderr, _("Could not determine enforcing mode.\n"));
+		return -1;
+	}
+
+	/* Re-open TTY descriptor */
+	fd = open(ttyn, O_RDWR);
+	if (fd < 0) {
+		fprintf(stderr, _("Error!  Could not open %s.\n"), ttyn);
+		return fd;
+	}
+
+	if (fgetfilecon(fd, &tty_con) < 0) {
+		fprintf(stderr, _("%s!  Could not get current context "
+			"for %s, not relabeling tty.\n"),
+			enforcing ? "Error" : "Warning", ttyn);
+		if (enforcing)
+			goto close_fd;
+	}
+
+	if (tty_con &&
+	    (security_compute_relabel(new_context, tty_con,
+				      SECCLASS_CHR_FILE, &new_tty_con) < 0)) {
+		fprintf(stderr,	_("%s!  Could not get new context for %s, "
+			"not relabeling tty.\n"),
+			enforcing ? "Error" : "Warning", ttyn);
+		if (enforcing)
+			goto close_fd;
+	}
+
+	if (new_tty_con)
+		if (fsetfilecon(fd, new_tty_con) < 0) {
+			fprintf(stderr,
+				_("%s!  Could not set new context for %s\n"),
+				enforcing ? "Error" : "Warning", ttyn);
+			freecon(new_tty_con);
+			new_tty_con = NULL;
+			if (enforcing)
+				goto close_fd;
+		}
+
+	*tty_context = tty_con;
+	*new_tty_context = new_tty_con;
+	return fd;
+
+close_fd:
+	freecon(tty_con);
+	close(fd);
+	return -1;
+}
+
+/**
+ * This function attempts to revert the relabeling done to the tty.
+ * fd   - referencing the opened ttyn
+ * ttyn - name of tty to restore
+ * tty_context     - original context of the tty
+ * new_tty_context - context tty was relabeled to
+ *
+ * Returns zero on success, non-zero otherwise
+ */
+static int restore_tty_label(int fd, const char *ttyn,
+			     security_context_t tty_context,
+			     security_context_t new_tty_context)
+{
+	int rc = 0;
+	security_context_t chk_tty_context = NULL;
+
+	if (!new_tty_context)
+		goto skip_relabel;
+
+	/* Verify that the tty still has the context set by newrole. */
+	if ((rc = fgetfilecon(fd, &chk_tty_context)) < 0) {
+		fprintf(stderr, "Could not fgetfilecon %s.\n", ttyn);
+		goto skip_relabel;
+	}
+
+	if ((rc = strcmp(chk_tty_context, new_tty_context))) {
+		fprintf(stderr, _("%s changed labels.\n"), ttyn);
+		goto skip_relabel;
+	}
+
+	if ((rc = fsetfilecon(fd, tty_context)) < 0)
+		fprintf(stderr,
+			_("Warning! Could not restore context for %s\n"), ttyn);
+skip_relabel:
+	freecon(chk_tty_context);
+	return rc;
+}
+
+/**
+ * Parses and validates the provided command line options and
+ * constructs a new context based on our old context and the
+ * arguments specified on the command line. On success
+ * new_context will be set to valid values, otherwise its value
+ * is left unchanged.
+ *
+ * Returns zero on success, non-zero otherwise.
+ */
+static int parse_command_line_arguments(int argc, char **argv, char *ttyn,
+					security_context_t old_context,
+					security_context_t *new_context)
+{
+	int flag_index;		/* flag index in argv[] */
+	int clflag;		/* holds codes for command line flags */
+	char *role_s = NULL;	/* role spec'd by user in argv[] */
+	char *type_s = NULL;	/* type spec'd by user in argv[] */
+	char *type_ptr = NULL;	/* stores malloc'd data from get_default_type */
+	char *level_s = NULL;	/* level spec'd by user in argv[] */
+	char *range_ptr = NULL;
+	security_context_t new_con = NULL;
+	context_t context = NULL; /* manipulatable form of new_context */
+	const struct option long_options[] = {
+		{"role", 1, 0, 'r'},
+		{"type", 1, 0, 't'},
+		{"level", 1, 0, 'l'},
+		{"version", 0, 0, 'V'},
+		{NULL, 0, 0, 0}
+	};
+
+	while (1) {
+		clflag = getopt_long(argc, argv, "r:t:l:V", long_options,
+				     &flag_index);
+		if (clflag == -1)
+			break;
+
+		switch (clflag) {
+		case 'V':
+			printf("newrole: %s version %s\n", PACKAGE, VERSION);
+			exit(0);
+			break;
+		case 'r':
+			if (role_s) {
+				fprintf(stderr,
+					_("Error: multiple roles specified\n"));
+				return -1;
+			}
+			role_s = optarg;
+			break;
+		case 't':
+			if (type_s) {
+				fprintf(stderr,
+					_("Error: multiple types specified\n"));
+				return -1;
+			}
+			type_s = optarg;
+			break;
+		case 'l':
+			if (!is_selinux_mls_enabled()) {
+				fprintf(stderr, _("Sorry, -l may be used with "
+					"SELinux MLS support.\n"));
+				return -1;
+			}
+			if (level_s) {
+				fprintf(stderr, _("Error: multiple levels "
+					"specified\n"));
+				return -1;
+			}
+			level_s = optarg;
+			break;
+		default:
+			fprintf(stderr, "%s\n", USAGE_STRING);
+			return -1;
+		}
+	}
+
+	/* Verify that the combination of command-line arguments are viable */
+	if (!(role_s || type_s || level_s)) {
+		fprintf(stderr, "%s\n", USAGE_STRING);
+		return -1;
+	}
+
+	/* Fill in a default type if one hasn't been specified. */
+	if (role_s && !type_s) {
+	 	/* get_default_type() returns malloc'd memory */
+		if (get_default_type(role_s, &type_ptr)) {
+			fprintf(stderr, _("Couldn't get default type.\n"));
+			send_audit_message(0, old_context, new_con, ttyn);
+			return -1;
+		}
+		type_s = type_ptr;
+	}
+
+	/* Create a temporary new context structure we extract and modify */
+	context = context_new(old_context);
+	if (!context) {
+		fprintf(stderr, _("failed to get new context.\n"));
+		goto err_free;
+	}
+
+	/* Modify the temporary new context */
+	if (role_s)
+		if (context_role_set(context, role_s)) {
+			fprintf(stderr, _("failed to set new role %s\n"),
+				role_s);
+			goto err_free;
+		}
+
+	if (type_s)
+		if (context_type_set(context, type_s)) {
+			fprintf(stderr, _("failed to set new type %s\n"),
+				type_s);
+			goto err_free;
+		}
+
+	if (level_s) {
+		range_ptr = build_new_range(level_s,context_range_get(context));
+		if (!range_ptr) {
+			fprintf(stderr,
+				_("failed to build new range with level %s\n"),
+				level_s);
+			goto err_free;
+		}
+		if (context_range_set(context, range_ptr)) {
+			fprintf(stderr, _("failed to set new range %s\n"),
+				range_ptr);
+			goto err_free;
+		}
+	}
+
+	/* Construct the final new context */
+	if (!(new_con = context_str(context))) {
+		fprintf(stderr, _("failed to convert new context to string\n"));
+		goto err_free;
+	}
+
+	if (security_check_context(new_con) < 0) {
+		fprintf(stderr, _("%s is not a valid context\n"), new_con);
+		send_audit_message(0, old_context, new_con, ttyn);
+		goto err_free;
+	}
+
+	*new_context = strdup(new_con);
+	if (!*new_context) {
+		fprintf(stderr, _("Unable to allocate memory for new_context"));
+		goto err_free;
+	}
+
+	free(type_ptr);
+	free(range_ptr);
+	context_free(context);
+	return 0;
+
+err_free:
+	free(type_ptr);
+	free(range_ptr);
+	/* Don't free new_con, context_free(context) handles this */
+	context_free(context);
+	return -1;
+}
+
+/**
+ * Take care of any signal setup
+ */
+static int set_signal_handles()
+{
+	sigset_t empty;
+
+	/* Empty the signal mask in case someone is blocking a signal */
+	if (sigemptyset(&empty)) {
+		fprintf(stderr, _("Unable to obtain empty signal set\n"));
+		return -1;
+	}
+
+	(void)sigprocmask(SIG_SETMASK, &empty, NULL);
+
+	/* Terminate on SIGHUP. */
+	if (signal(SIGHUP, SIG_DFL) == SIG_ERR) {
+		fprintf(stderr, _("Unable to set SIGHUP handler\n"));
+		return -1;
+	}
+
+	return 0;
+}
+
 /************************************************************************
  *
  * All code used for both PAM and shadow passwd goes in this section.

Re: [PATCH 0/4] newrole suid functionality (take 2)

[Michael C Thompson]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

Michael C Thompson wrote:
> This is the intro to a set of four patches.
> 
> These patches are an attempt to make newrole be an acceptably secure 
> suid root program, to provide it with the capabilities to generate audit 
> records (existing) and handle polyinstatiation (new).
> 
> The 4 patches are as follows:
> 1) New functions introduced to newrole.c, new and existing functionality
> 2) Changes to existing functions in newrole.c

This is the 2nd of 4 patches.
This patch applies against policycoreutils-1.30.30-1.

Changes:
  * Updated existing functions and added documentation where deemed
    appropriate.
  * Removed xstrdup() as it is no longer used
  * authenticate_via_pam() updated to accept pam_handle as argument
  * Drop capabilities is split into three versions
    (AUDIT, NAMESPACE and neither)
  * These changes will be used in patch 3/4.

Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>


[-- Attachment #2: 02-update-existing-functions.patch --]
[-- Type: text/x-diff, Size: 13154 bytes --]

diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
--- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:13:44.000000000 -0500
+++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:14:13.000000000 -0500
@@ -90,18 +90,17 @@
 #define DEFAULT_PATH "/bin:/usr/bin:/usr/local/bin"
 #define DEFAULT_CONTEXT_SIZE 255	/* first guess at context size */
 
-char *xstrdup(const char *s)
-{
-	char *s2;
-
-	s2 = strdup(s);
-	if (!s2) {
-		fprintf(stderr, _("Out of memory!\n"));
-		exit(1);
-	}
-	return s2;
-}
-
+/**
+ * Construct from the current range and specified desired level a resulting
+ * range. If the specified level is a range, return that. If it is not, then
+ * construct a range with level as the sensitivity and clearance of the current
+ * context.
+ *
+ * newlevel - the level specified on the command line
+ * range    - the range in the current context
+ *
+ * Returns malloc'd memory
+ */
 static char *build_new_range(char *newlevel, const char *range)
 {
 	char *newrangep = NULL;
@@ -118,9 +117,8 @@
 		return newrangep;
 	}
 
-	/* look for MLS range */
+	/* look for MLS range in current context */
 	tmpptr = strchr(range, '-');
-
 	if (tmpptr) {
 		/* we are inserting into a ranged MLS context */
 		len = strlen(newlevel) + 1 + strlen(tmpptr + 1) + 1;
@@ -151,16 +149,11 @@
  * All PAM code goes in this section.
  *
  ************************************************************************/
-
-#include <unistd.h>		/* for getuid(), exit(), getopt() */
-
 #include <security/pam_appl.h>	/* for PAM functions */
 #include <security/pam_misc.h>	/* for misc_conv PAM utility function */
 
 #define SERVICE_NAME "newrole"	/* the name of this program for PAM */
 
-int authenticate_via_pam(const struct passwd *, const char *);
-
 /* authenticate_via_pam()
  *
  * in:     pw - struct containing data from our user's line in 
@@ -174,63 +167,39 @@
  * This function uses PAM to authenticate the user running this
  * program.  This is the only function in this program that makes PAM
  * calls.
- *
  */
-
-int authenticate_via_pam(const struct passwd *pw, const char *ttyn)
+int authenticate_via_pam(const char *ttyn, pam_handle_t *pam_handle)
 {
 
-	int result = 0;		/* our result, set to 0 (not authenticated) by default */
-	int rc;			/* pam return code */
-	pam_handle_t *pam_handle;	/* opaque handle used by all PAM functions */
+	int result = 0;		/* set to 0 (not authenticated) by default */
+	int pam_rc;		/* pam return code */
 	const char *tty_name;
 
-	/* This is a jump table of functions for PAM to use when it wants to *
-	 * communicate with the user.  We'll be using misc_conv(), which is  *
-	 * provided for us via pam_misc.h.                                   */
-	struct pam_conv pam_conversation = {
-		misc_conv,
-		NULL
-	};
-
-	/* Make `p_pam_handle' a valid PAM handle so we can use it when *
-	 * calling PAM functions.                                       */
-	rc = pam_start(SERVICE_NAME,
-		       pw->pw_name, &pam_conversation, &pam_handle);
-	if (rc != PAM_SUCCESS) {
-		fprintf(stderr, _("failed to initialize PAM\n"));
-		exit(-1);
-	}
-
 	if (strncmp(ttyn, "/dev/", 5) == 0)
 		tty_name = ttyn + 5;
 	else
 		tty_name = ttyn;
 
-	rc = pam_set_item(pam_handle, PAM_TTY, tty_name);
-	if (rc != PAM_SUCCESS) {
+	pam_rc = pam_set_item(pam_handle, PAM_TTY, tty_name);
+	if (pam_rc != PAM_SUCCESS) {
 		fprintf(stderr, _("failed to set PAM_TTY\n"));
 		goto out;
 	}
 
 	/* Ask PAM to authenticate the user running this program */
-	rc = pam_authenticate(pam_handle, 0);
-	if (rc != PAM_SUCCESS) {
+	pam_rc = pam_authenticate(pam_handle, 0);
+	if (pam_rc != PAM_SUCCESS) {
 		goto out;
 	}
 
 	/* Ask PAM to verify acct_mgmt */
-	rc = pam_acct_mgmt(pam_handle, 0);
-	if (rc == PAM_SUCCESS) {
+	pam_rc = pam_acct_mgmt(pam_handle, 0);
+	if (pam_rc == PAM_SUCCESS) {
 		result = 1;	/* user authenticated OK! */
 	}
 
-	/* We're done with PAM.  Free `pam_handle'. */
       out:
-	pam_end(pam_handle, rc);
-
-	return (result);
-
+	return result;
 }				/* authenticate_via_pam() */
 
 #else				/* else !USE_PAM */
@@ -240,19 +209,14 @@
  * All shadow passwd code goes in this section.
  *
  ************************************************************************/
-
-#include <unistd.h>		/* for getuid(), exit(), crypt() */
 #include <shadow.h>		/* for shadow passwd functions */
 #include <string.h>		/* for strlen(), memset() */
 
 #define PASSWORD_PROMPT _("Password:")	/* prompt for getpass() */
 
-int authenticate_via_shadow_passwd(const struct passwd *);
-
 /* authenticate_via_shadow_passwd()
  *
- * in:     pw - struct containing data from our user's line in 
- *                         the passwd file.
+ * in:     uname - the calling user's user name
  * out:    nothing
  * return: value   condition
  *         -----   ---------
@@ -262,51 +226,37 @@
  *
  * This function uses the shadow passwd file to thenticate the user running
  * this program.
- *
  */
-
-int authenticate_via_shadow_passwd(const struct passwd *pw)
+int authenticate_via_shadow_passwd(const char *uname)
 {
-
-	struct spwd *p_shadow_line;	/* struct derived from shadow passwd file line */
-	char *unencrypted_password_s;	/* unencrypted password input by user */
-	char *encrypted_password_s;	/* user's password input after being crypt()ed */
-
-	/* Make `p_shadow_line' point to the data from the current user's *
-	 * line in the shadow passwd file.                                */
-	setspent();		/* Begin access to the shadow passwd file. */
-	p_shadow_line = getspnam(pw->pw_name);
-	endspent();		/* End access to the shadow passwd file. */
+	struct spwd *p_shadow_line;
+	char *unencrypted_password_s;
+	char *encrypted_password_s;
+
+	setspent();
+	p_shadow_line = getspnam(uname);
+	endspent();
 	if (!(p_shadow_line)) {
-		fprintf(stderr,
-			_
-			("Cannot find your entry in the shadow passwd file.\n"));
-		exit(-1);
+		fprintf(stderr, _("Cannot find your entry in the shadow "
+			"passwd file.\n"));
+		return 0;
 	}
 
 	/* Ask user to input unencrypted password */
 	if (!(unencrypted_password_s = getpass(PASSWORD_PROMPT))) {
 		fprintf(stderr, _("getpass cannot open /dev/tty\n"));
-		exit(-1);
+		return 0;
 	}
 
-	/* Use crypt() to encrypt user's input password.  Clear the *
-	 * unencrypted password as soon as we're done, so it is not * 
-	 * visible to memory snoopers.                              */
+	/* Use crypt() to encrypt user's input password. */
 	encrypted_password_s = crypt(unencrypted_password_s,
 				     p_shadow_line->sp_pwdp);
 	memset(unencrypted_password_s, 0, strlen(unencrypted_password_s));
-
-	/* Return 1 (authenticated) iff the encrypted version of the user's *
-	 * input password matches the encrypted password stored in the      *
-	 * shadow password file.                                            */
 	return (!strcmp(encrypted_password_s, p_shadow_line->sp_pwdp));
-
-}				/* authenticate_via_shadow_passwd() */
-
+}
 #endif				/* if/else USE_PAM */
 
-/*
+/**
  * This function checks to see if the shell is known in /etc/shells.
  * If so, it returns 1. On error or illegal shell, it returns 0.
  */
@@ -315,7 +265,7 @@
 	int found = 0;
 	const char *buf;
 
-	if (!shell_name)
+	if (! (shell_name && shell_name[0]))
 		return found;
 
 	while ((buf = getusershell()) != NULL) {
@@ -444,67 +394,138 @@
 	return rc;
 }
 
-/*
+/**
  * This function will drop the capabilities so that we are left
  * only with access to the audit system. If the user is root, we leave
  * the capabilities alone since they already should have access to the
  * audit netlink socket.
+ *
+ * Returns zero on success, non-zero otherwise
  */
-#ifdef LOG_AUDIT_PRIV
-static void drop_capabilities(void)
+#if defined(AUDIT_LOG_PRIV) && !defined(NAMESPACE_PRIV)
+static int drop_capabilities(void)
 {
+	int rc = 0;
+	cap_t new_caps, tmp_caps;
+	cap_value_t cap_list[] = { CAP_AUDIT_WRITE };
+	cap_value_t tmp_cap_list[] = { CAP_AUDIT_WRITE, CAP_SETUID };
 	uid_t uid = getuid();
 
-	if (uid) {		/* Non-root path */
-		cap_t new_caps, tmp_caps;
-		cap_value_t cap_list[] = { CAP_AUDIT_WRITE };
-		cap_value_t tmp_cap_list[] = { CAP_AUDIT_WRITE, CAP_SETUID };
-
-		new_caps = cap_init();
-		tmp_caps = cap_init();
-		if (!new_caps || !tmp_caps) {
-			fprintf(stderr,
-				_("Error initing capabilities, aborting.\n"));
-			exit(-1);
-		}
-		cap_set_flag(new_caps, CAP_PERMITTED, 1, cap_list, CAP_SET);
-		cap_set_flag(new_caps, CAP_EFFECTIVE, 1, cap_list, CAP_SET);
-		cap_set_flag(tmp_caps, CAP_PERMITTED, 2, tmp_cap_list, CAP_SET);
-		cap_set_flag(tmp_caps, CAP_EFFECTIVE, 2, tmp_cap_list, CAP_SET);
+	if (!uid)
+		return 0;
 
-		/* Keep capabilities across uid change */
-		prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0);
+	/* Non-root caller, suid root path */
+	new_caps = cap_init();
+	tmp_caps = cap_init();
+	if (!new_caps || !tmp_caps) {
+		fprintf(stderr, _("Error initing capabilities, aborting.\n"));
+		return -1;
+	}
+	rc |= cap_set_flag(new_caps, CAP_PERMITTED, 1, cap_list, CAP_SET);
+	rc |= cap_set_flag(new_caps, CAP_EFFECTIVE, 1, cap_list, CAP_SET);
+	rc |= cap_set_flag(tmp_caps, CAP_PERMITTED, 2, tmp_cap_list, CAP_SET);
+	rc |= cap_set_flag(tmp_caps, CAP_EFFECTIVE, 2, tmp_cap_list, CAP_SET);
+	if (rc) {
+		fprintf(stderr, _("Error setting capabilities, aborting\n"));
+		goto out;
+	}
 
-		/* We should still have root's caps, so drop most capabilities now */
-		if (cap_set_proc(tmp_caps)) {
-			fprintf(stderr,
-				_("Error dropping capabilities, aborting\n"));
-			exit(-1);
-		}
-		cap_free(tmp_caps);
+	/* Keep capabilities across uid change */
+	if (prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0)) {
+		fprintf(stderr, _("Error setting KEEPCAPS, aborting\n"));
+		rc = -1;
+		goto out;
+	}
 
-		/* Change uid */
-		if (setresuid(uid, uid, uid)) {
-			fprintf(stderr, _("Error changing uid, aborting.\n"));
-			exit(-1);
-		}
+	/* Does this temporary change really buy us much? */
+	/* We should still have root's caps, so drop most capabilities now */
+	if ((rc = cap_set_proc(tmp_caps))) {
+		fprintf(stderr, _("Error dropping capabilities, aborting\n"));
+		goto out;
+	}
 
-		/* Now get rid of this ability */
-		if (prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0) < 0) {
-			fprintf(stderr,
-				_("Error resetting KEEPCAPS, aborting\n"));
-			exit(-1);
-		}
+	/* Change uid */
+	if ((rc = setresuid(uid, uid, uid))) {
+		fprintf(stderr, _("Error changing uid, aborting.\n"));
+		goto out;
+	}
 
-		/* Finish dropping capabilities. */
-		if (cap_set_proc(new_caps)) {
-			fprintf(stderr,
-				_
-				("Error dropping SETUID capability, aborting\n"));
-			exit(-1);
-		}
-		cap_free(new_caps);
+	/* Now get rid of this ability */
+	if ((rc = prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0) < 0)) {
+		fprintf(stderr, _("Error resetting KEEPCAPS, aborting\n"));
+		goto out;
 	}
+
+	/* Finish dropping capabilities. */
+	if ((rc = cap_set_proc(new_caps))) {
+		fprintf(stderr,
+			_("Error dropping SETUID capability, aborting\n"));
+		goto out;
+	}
+out:
+	if (cap_free(tmp_caps) || cap_free(new_caps))
+		fprintf(stderr, _("Error freeing caps\n"));
+	return rc;
+}
+#elif defined(NAMESPACE_PRIV)
+/**
+ * This function will drop the capabilities so that we are left
+ * only with access to the audit system and the ability to raise
+ * CAP_SYS_ADMIN, CAP_DAC_OVERRIDE, CAP_FOWNER and CAP_CHOWN,
+ * before invoking pam_namespace.  These capabilities are needed
+ * for performing bind mounts/unmounts and to create potential new
+ * instance directories with appropriate DAC attributes. If the
+ * user is root, we leave the capabilities alone since they already
+ * should have access to the audit netlink socket and should have
+ * the ability to create/mount/unmount instance directories.
+ *
+ * Returns zero on success, non-zero otherwise
+ */
+static int drop_capabilities(void)
+{
+	int rc = 0;
+	cap_t new_caps;
+	cap_value_t cap_list[] = { CAP_AUDIT_WRITE, CAP_SETUID,
+				   CAP_SYS_ADMIN, CAP_FOWNER, CAP_CHOWN,
+				   CAP_DAC_OVERRIDE };
+
+	if (!getuid())
+		return 0;
+
+	/* Non-root caller, suid root path */
+	new_caps = cap_init();
+	if (!new_caps) {
+		fprintf(stderr, _("Error initing capabilities, aborting.\n"));
+		return -1;
+	}
+	rc |= cap_set_flag(new_caps, CAP_PERMITTED, 6, cap_list, CAP_SET);
+	rc |= cap_set_flag(new_caps, CAP_EFFECTIVE, 6, cap_list, CAP_SET);
+	if (rc) {
+		fprintf(stderr, _("Error setting capabilities, aborting\n"));
+		goto out;
+	}
+
+	/* Ensure that caps are dropped after setuid call */
+	if ((rc = prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0) < 0)) {
+		fprintf(stderr, _("Error resetting KEEPCAPS, aborting\n"));
+		goto out;
+	}
+
+	/* We should still have root's caps, so drop most capabilities now */
+	if ((rc = cap_set_proc(new_caps))) {
+		fprintf(stderr, _("Error dropping capabilities, aborting\n"));
+		goto out;
+	}
+out:
+	if (cap_free(new_caps))
+		fprintf(stderr, _("Error freeing caps\n"));
+	return rc;
+}
+
+#else
+static inline int drop_capabilities(void)
+{
+	return 0;
 }
 #endif
 
@@ -530,7 +551,7 @@
 }
 #endif
 
-#ifdef LOG_AUDIT_PRIV
+#ifdef AUDIT_LOG_PRIV
 /* Send audit message */
 static
 int send_audit_message(int success, security_context_t old_context,

[PATCH 3/4] newrole suid functionality (take 2)

[Michael C Thompson]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

Michael C Thompson wrote:
> This is the intro to a set of four patches.
> 
> These patches are an attempt to make newrole be an acceptably secure 
> suid root program, to provide it with the capabilities to generate audit 
> records (existing) and handle polyinstatiation (new).
> 
> The 4 patches are as follows:
> 1) New functions introduced to newrole.c, new and existing functionality
> 2) Changes to existing functions in newrole.c
> 3) Updates to main in newrole.c to use the aforementioned changes

This is the 3rd of 4 patches.
This patch applies against policycoreutils-1.30.30-1.

Changes:
  * main is changed in the following ways:
    - remove the duplicated functionality from patch 1/4
    - set to call the new functions from patch 1/4
    - set to use changes to functions in patch 2/4
    - introduces better error handling and cleanup paths

Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>


[-- Attachment #2: 03-update_main.patch --]
[-- Type: text/x-diff, Size: 20120 bytes --]

diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
--- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:15:20.000000000 -0500
+++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:15:03.000000000 -0500
@@ -36,11 +36,6 @@
  * setuid root, so that it can read the shadow passwd file.
  * 
  *
- * option CANTSPELLGDB:
- *
- * If you set CANTSPELLGDB you will turn on some debugging printfs.
- *
- *
  * Authors:  Tim Fraser , 
  *           Anthony Colatrella <amcolat@epoch.ncsc.mil>
  * Various bug fixes by Stephen Smalley <sds@epoch.ncsc.mil>
@@ -48,6 +43,14 @@
  *************************************************************************/
 
 #define _GNU_SOURCE
+
+#if defined(AUDIT_LOG_PRIV) && !defined(USE_AUDIT)
+#error AUDIT_LOG_PRIV needs the USE_AUDIT option
+#endif
+#if defined(NAMESPACE_PRIV) && !defined(USE_PAM)
+#error NAMESPACE_PRIV needs the USE_PAM option
+#endif
+
 #include <stdio.h>
 #include <stdlib.h>		/* for malloc(), realloc(), free() */
 #include <pwd.h>		/* for getpwuid() */
@@ -63,13 +66,11 @@
 #include <selinux/get_default_type.h>
 #include <selinux/get_context_list.h>	/* for SELINUX_DEFAULTUSER */
 #include <signal.h>
+#include <unistd.h>		/* for getuid(), exit(), getopt() */
 #ifdef USE_AUDIT
 #include <libaudit.h>
 #endif
-#ifdef LOG_AUDIT_PRIV
-#ifndef USE_AUDIT
-#error LOG_AUDIT_PRIV needs the USE_AUDIT option
-#endif
+#if defined(AUDIT_LOG_PRIV) || (NAMESPACE_PRIV)
 #include <sys/prctl.h>
 #include <sys/capability.h>
 #endif
@@ -897,47 +898,43 @@
 
 int main(int argc, char *argv[])
 {
+	security_context_t new_context = NULL;	/* target security context */
+	security_context_t old_context = NULL;	/* original securiy context */
+	security_context_t tty_context = NULL;	/* current context of tty */
+	security_context_t new_tty_context = NULL; /* new context of tty */
 
-	security_context_t new_context = NULL;	/* our target security context */
-	security_context_t old_context = NULL;	/* our original securiy context */
-	security_context_t tty_context = NULL;	/* The current context of tty file */
-	security_context_t new_tty_context = NULL;	/* The new context of tty file */
-	security_context_t chk_tty_context = NULL;
+	struct passwd pw;	/* struct derived from passwd file line */
+	char *ttyn = NULL;	/* tty path */
 
-	context_t context;	/* manipulatable form of new_context */
+	int fd;
+	int rc;
+	pid_t childPid = 0;
+	char *shell_argv0 = NULL;
 
-	struct passwd *pw;	/* struct derived from passwd file line */
-	struct passwd pw_copy;
+#ifdef USE_PAM
+	int pam_status;		   /* pam return code */
+	pam_handle_t *pam_handle;  /* opaque handle used by all PAM functions */
 
-	int clflag;		/* holds codes for command line flags */
-	int flag_index;		/* flag index in argv[] */
-	const struct option long_options[] = {	/* long option flags for getopt() */
-		{"role", 1, 0, 'r'},
-		{"type", 1, 0, 't'},
-		{"level", 1, 0, 'l'},
-		{"version", 0, 0, 'V'},
-		{NULL, 0, 0, 0}
+	/* This is a jump table of functions for PAM to use when it wants to *
+	 * communicate with the user.  We'll be using misc_conv(), which is  *
+	 * provided for us via pam_misc.h.                                   */
+	struct pam_conv pam_conversation = {
+		misc_conv,
+		NULL
 	};
-	char *role_s = NULL;	/* role spec'd by user in argv[] */
-	char *type_s = NULL;	/* type spec'd by user in argv[] */
-	char *level_s = NULL;	/* level spec'd by user in argv[] */
-	char *ttyn = NULL;	/* tty path */
-	pid_t childPid = 0;
-	uid_t uid;
-	int fd;
-	int enforcing;
-	sigset_t empty;
-
-#ifdef LOG_AUDIT_PRIV
-	drop_capabilities();
 #endif
 
-	/* Empty the signal mask in case someone is blocking a signal */
-	sigemptyset(&empty);
-	(void)sigprocmask(SIG_SETMASK, &empty, NULL);
-
-	/* Terminate on SIGHUP. */
-	signal(SIGHUP, SIG_DFL);
+	/*
+	 * Step 0: Setup
+	 *
+	 * Do some intial setup, including dropping capabilities, checking
+	 * if it makes sense to continue to run newrole, and setting up
+	 * a scrubbed environment.
+	 */
+	if (set_signal_handles())
+		return -1;
+	if (drop_capabilities())
+		return -1;
 
 #ifdef USE_NLS
 	setlocale(LC_ALL, "");
@@ -945,435 +942,235 @@
 	textdomain(PACKAGE);
 #endif
 
-	/*
-	 *
-	 * Step 1:  Handle command-line arguments.
-	 *
-	 */
-
 	if (!is_selinux_enabled()) {
-		fprintf(stderr,
-			_
-			("Sorry, newrole may be used only on a SELinux kernel.\n"));
-		exit(-1);
+		fprintf(stderr, _("Sorry, newrole may be used only on "
+			"a SELinux kernel.\n"));
+		return -1;
 	}
-	enforcing = security_getenforce();
-	if (enforcing < 0) {
+
+	if (security_getenforce() < 0) {
 		fprintf(stderr, _("Could not determine enforcing mode.\n"));
-		exit(-1);
+		return -1;
 	}
 
-	while (1) {
-		clflag =
-		    getopt_long(argc, argv, "r:t:l:V", long_options,
-				&flag_index);
-		if (clflag == -1)
-			break;
-
-		switch (clflag) {
-		case 'V':
-			printf("newrole: %s version %s\n", PACKAGE, VERSION);
-			exit(0);
-			break;
-		case 'r':
-			/* If role_s is already set, the user spec'd multiple roles - bad. */
-			if (role_s) {
-				fprintf(stderr,
-					_("Error: multiple roles specified\n"));
-				exit(-1);
-			}
-			role_s = optarg;	/* save the role string spec'd by user */
-			break;
-
-		case 't':
-			/* If type_s is already set, the user spec'd multiple types - bad. */
-			if (type_s) {
-				fprintf(stderr,
-					_("Error: multiple types specified\n"));
-				exit(-1);
-			}
-			type_s = optarg;	/* save the type string spec'd by user */
-			break;
-
-		case 'l':
-			if (!is_selinux_mls_enabled()) {
-				fprintf(stderr,
-					_
-					("Sorry, -l may be used with SELinux MLS support.\n"));
-				exit(-1);
-			}
-			/* If level_s is already set, the user spec'd multiple levels - bad. */
-			if (level_s) {
-				fprintf(stderr,
-					_
-					("Error: multiple levels specified\n"));
-				exit(-1);
-			}
-			level_s = optarg;	/* save the level string spec'd by user */
-			break;
-
-		default:
-			fprintf(stderr, "%s\n", USAGE_STRING);
-			exit(-1);
-		}		/* switch( clflag ) */
-	}			/* while command-line flags remain for newrole */
-
-	/* Verify that the combination of command-line arguments we were *
-	 * given is a viable one.                                        */
-	if (!(role_s || type_s || level_s)) {
-		fprintf(stderr, "%s\n", USAGE_STRING);
-		exit(-1);
-	}
+	if (extract_pw_data(&pw))
+		return -1;
 
-	/* Fill in a default type if one hasn't been specified */
-	if (role_s && !type_s) {
-		if (get_default_type(role_s, &type_s)) {
-			fprintf(stderr, _("Couldn't get default type.\n"));
-			send_audit_message(0, old_context, new_context, ttyn);
-			exit(-1);
-		}
-#ifdef CANTSPELLGDB
-		printf("Your type will be %s.\n", type_s);
-#endif
+	if (sanitize_environment(&pw)) {
+		fprintf(stderr, _("Unable to sanitize the environment, "
+			"aborting\n"));
+		goto err_free;
 	}
 
 	/*
+	 * Step 1: Parse command line and valid arguments
 	 *
-	 * Step 2:  Authenticate the user.
-	 *
-	 */
-
-	/*
-	 * Get the context of the caller, and extract
-	 * the username from the context.  Don't rely on the Linux
-	 * uid information - it isn't trustworthy.
+	 * old_context and ttyn are required for audit logging,
+	 * context validation and pam
 	 */
-
-	/* Put the caller's context into `old_context'. */
-	if (0 != (getprevcon(&old_context))) {
+	if (getprevcon(&old_context)) {
 		fprintf(stderr, _("failed to get old_context.\n"));
-		exit(-1);
-	}
-#ifdef CANTSPELLGDB
-	printf("Your old context was %s\n", old_context);
-#endif
-
-	/* 
-	 * Create a context structure so that we extract and modify 
-	 * components easily. 
-	 */
-	context = context_new(old_context);
-	if (context == 0) {
-		fprintf(stderr, _("failed to get new context.\n"));
-		exit(-1);
-	}
-
-	/*
-	 * Determine the Linux user identity to re-authenticate.
-	 * If supported and set, use the login uid, as this should be more stable.
-	 * Otherwise, use the real uid.
-	 * The SELinux user identity is no longer used, as Linux users are now
-	 * mapped to SELinux users via seusers and the SELinux user identity space
-	 * is separate.
-	 */
-#ifdef USE_AUDIT
-	uid = audit_getloginuid();
-	if (uid == (uid_t) - 1)
-		uid = getuid();
-#else
-	uid = getuid();
-#endif
-
-	/* Get the passwd info for the Linux user identity. */
-	pw = getpwuid(uid);
-	if (!pw) {
-		fprintf(stderr,
-			_("cannot find your entry in the passwd file.\n"));
-		exit(-1);
-	}
-	pw_copy = *pw;
-	pw = &pw_copy;
-	pw->pw_name = xstrdup(pw->pw_name);
-	pw->pw_dir = xstrdup(pw->pw_dir);
-	pw->pw_shell = xstrdup(pw->pw_shell);
-
-	if (verify_shell(pw->pw_shell) == 0) {
-		fprintf(stderr, _("Error!  Shell is not valid.\n"));
-		exit(-1);
+		goto err_free;
 	}
 
-	/* Get the tty name. Pam will need it. */
 	ttyn = ttyname(0);
 	if (!ttyn || *ttyn == '\0') {
 		fprintf(stderr,
 			_("Error!  Could not retrieve tty information.\n"));
-		exit(-1);
+		goto err_free;
 	}
 
-	printf(_("Authenticating %s.\n"), pw->pw_name);
+	if (parse_command_line_arguments(argc, argv, ttyn, old_context,
+					 &new_context))
+		goto err_free;
 
-	/* 
+	/*
+	 * Step 2:  Authenticate the user.
+	 *
 	 * Re-authenticate the user running this program.
 	 * This is just to help confirm user intent (vs. invocation by
 	 * malicious software), not to authorize the operation (which is covered
 	 * by policy).  Trusted path mechanism would be preferred.
 	 */
+	printf(_("Authenticating %s.\n"), pw.pw_name);
 #ifdef USE_PAM
-	if (!authenticate_via_pam(pw, ttyn))
-#else				/* !USE_PAM */
-	if (!authenticate_via_shadow_passwd(pw))
-#endif				/* if/else USE_PAM */
+	pam_status = pam_start(SERVICE_NAME, pw.pw_name, &pam_conversation,
+			       &pam_handle);
+	if (pam_status != PAM_SUCCESS) {
+		fprintf(stderr, _("failed to initialize PAM\n"));
+		goto err_free;
+	}
+
+	if (!authenticate_via_pam(ttyn, pam_handle))
+#else
+	if (!authenticate_via_shadow_passwd(pw.pw_name))
+#endif
 	{
 		fprintf(stderr, _("newrole: incorrect password for %s\n"),
-			pw->pw_name);
-		return (-1);
+			pw.pw_name);
+		goto err_close_pam;
 	}
-	/* If we reach here, then we have authenticated the user. */
-#ifdef CANTSPELLGDB
-	printf("You are authenticated!\n");
-#endif
 
 	/*
+	 * Step 3:  Handle relabeling of the tty.
 	 *
-	 * Step 3:  Construct a new context based on our old context and the
-	 *          arguments specified on the command line.
-	 *
+	 * Once we authenticate the user, we know that we want to proceed with
+	 * the action. Prior to this point, no changes are made the to system.
 	 */
-
-	/* The first step in constructing a new context for the new shell we  *
-	 * plan to exec is to take our old context in `context' as a   *
-	 * starting point, and modify it according to the options the user *
-	 * specified on the command line.                                  */
-
-	/* If the user specified a new role on the command line (if `role_s'   *
-	 * is set), then replace the old role in `context' with this new role. */
-	if (role_s) {
-		if (context_role_set(context, role_s)) {
-			fprintf(stderr, _("failed to set new role %s\n"),
-				role_s);
-			exit(-1);
-		}
-#ifdef CANTSPELLGDB
-		printf("Your new role is %s\n", context_role_get(context));
-#endif
-	}
-
-	/* if user specified new role */
-	/* If the user specified a new type on the command line (if `type_s'   *
-	 * is set), then replace the old type in `context' with this new type. */
-	if (type_s) {
-		if (context_type_set(context, type_s)) {
-			fprintf(stderr, _("failed to set new type %s\n"),
-				type_s);
-			exit(-1);
-		}
-#ifdef CANTSPELLGDB
-		printf("Your new type is %s\n", context_type_get(context));
-#endif
-	}
-
-	/* if user specified new type */
-	/* If the user specified a new level on the command line (if `level_s'   *
-	 * is set), then replace the old level in `context' with this new level. */
-	if (level_s) {
-		char *range_s =
-		    build_new_range(level_s, context_range_get(context));
-		if (!range_s) {
-			fprintf(stderr,
-				_("failed to build new range with level %s\n"),
-				level_s);
-			exit(-1);
-		}
-		if (context_range_set(context, range_s)) {
-			fprintf(stderr, _("failed to set new range %s\n"),
-				range_s);
-			free(range_s);
-			exit(-1);
-		}
-		free(range_s);
-#ifdef CANTSPELLGDB
-		printf("Your new range is %s\n", context_range_get(context));
-#endif
-	}
-
-	/* if user specified new level */
-	/* The second step in creating the new context is to convert our modified *
-	 * `context' structure back to a context string and then to a Context.    */
-	if (!(new_context = context_str(context))) {
-		fprintf(stderr, _("failed to convert new context to string\n"));
-		exit(-1);
-	}
-#ifdef CANTSPELLGDB
-	printf("Your new context is %s\n", new_context);
-#endif
-
-	if (security_check_context(new_context) < 0) {
-		fprintf(stderr, _("%s is not a valid context\n"), new_context);
-		send_audit_message(0, old_context, new_context, ttyn);
-		exit(-1);
-	}
+	fd = relabel_tty(ttyn, new_context, &tty_context, &new_tty_context);
+	if (fd < 0)
+		goto err_close_pam;
 
 	/*
+	 * Step 4: Fork
 	 *
-	 * Step 4:  Handle relabeling of the tty.
-	 *
+	 * Fork, allowing parent to clean up after shell has executed.
+	 * Child: reopen stdin, stdout, stderr and exec shell
+	 * Parnet: wait for child to die and restore tty's context
 	 */
-
-	/* Re-open TTY descriptor */
-	fd = open(ttyn, O_RDWR);
-	if (fd < 0) {
-		fprintf(stderr, _("Error!  Could not open %s.\n"), ttyn);
-		exit(-1);
-	}
-
-	tty_context = NULL;
-	if (fgetfilecon(fd, &tty_context) < 0) {
-		fprintf(stderr,
-			_
-			("%s!  Could not get current context for %s, not relabeling tty.\n"),
-			enforcing ? "Error" : "Warning", ttyn);
-		if (enforcing)
-			exit(-1);
-	}
-#ifdef CANTSPELLGDB
-	if (tty_context)
-		printf("Your tty %s was labeled with context %s\n", ttyn,
-		       tty_context);
-#endif
-
-	new_tty_context = NULL;
-	if (tty_context
-	    &&
-	    (security_compute_relabel
-	     (new_context, tty_context, SECCLASS_CHR_FILE,
-	      &new_tty_context) < 0)) {
-		fprintf(stderr,
-			_
-			("%s!  Could not get new context for %s, not relabeling tty.\n"),
-			enforcing ? "Error" : "Warning", ttyn);
-		if (enforcing)
-			exit(-1);
-	}
-#ifdef CANTSPELLGDB
-	if (new_tty_context)
-		printf("Relabeling tty %s to context %s\n", ttyn,
-		       new_tty_context);
-#endif
-
-	if (new_tty_context) {
-		if (fsetfilecon(fd, new_tty_context) < 0) {
-			fprintf(stderr,
-				_("%s!  Could not set new context for %s\n"),
-				enforcing ? "Error" : "Warning", ttyn);
-			freecon(new_tty_context);
-			new_tty_context = NULL;
-			if (enforcing)
-				exit(-1);
-		}
-	}
-
-	/* Fork, allowing parent to clean up after shell has executed */
 	childPid = fork();
 	if (childPid < 0) {
+		/* fork failed, no child to worry about */
 		int errsv = errno;
 		fprintf(stderr, _("newrole: failure forking: %s"),
 			strerror(errsv));
-		if (fsetfilecon(fd, tty_context) < 0)
-			fprintf(stderr,
-				_
-				("Warning!  Could not restore context for %s\n"),
-				ttyn);
-		freecon(tty_context);
-		exit(-1);
+		if (restore_tty_label(fd, ttyn, tty_context, new_tty_context))
+			fprintf(stderr, _("Unable to restore tty label...\n"));
+		if (close(fd))
+			fprintf(stderr, _("Failed to close tty properly\n"));
+		goto err_close_pam;
 	} else if (childPid) {
-		/* PARENT */
+		/* PARENT
+		 * It doesn't make senes to exit early on errors at this point,
+		 * since we are doing cleanup which needs to be done.
+		 * We can exit with a bad rc though
+		 */
 		int rc;
+		int exit_code = 0;
+
 		do {
 			rc = wait(NULL);
 		} while (rc < 0 && errno == EINTR);
 
-		if (!new_tty_context || !tty_context)
-			exit(0);
-
-		/* Verify that the tty still has the context set by newrole. */
-		if (fgetfilecon(fd, &chk_tty_context) < 0) {
-			fprintf(stderr, "Could not fgetfilecon %s.\n", ttyn);
-			exit(-1);
-		}
-
-		if (strcmp(chk_tty_context, new_tty_context)) {
-			fprintf(stderr, _("%s changed labels.\n"), ttyn);
-			exit(-1);
+		if (restore_tty_label(fd, ttyn, tty_context, new_tty_context)) {
+			fprintf(stderr, _("Unable to restore tty label...\n"));
+			exit_code = -1;
 		}
-
+		freecon(tty_context);
 		freecon(new_tty_context);
-
-#ifdef CANTSPELLGDB
-		printf("Restoring tty %s back to context %s\n", ttyn,
-		       tty_context);
+		if (close(fd)) {
+			fprintf(stderr, _("Failed to close tty properly\n"));
+			exit_code = -1;
+		}
+#ifdef USE_PAM
+#ifdef NAMESPACE_PRIV
+		pam_status = pam_close_session(pam_handle,0);
+		if (pam_status != PAM_SUCCESS) {
+			fprintf(stderr, "pam_close_session failed with %s\n",
+				pam_strerror(pam_handle, pam_status));
+			exit_code = -1;
+		}
 #endif
-
-		fsetfilecon(fd, tty_context);
-		freecon(tty_context);
-
-		/* Done! */
-		exit(0);
+		rc = pam_end(pam_handle, pam_status);
+		if (rc != PAM_SUCCESS) {
+			fprintf(stderr, "pam_end failed with %s\n",
+				pam_strerror(pam_handle, rc));
+			exit_code = -1;
+		}
+#endif
+		free(pw.pw_name);
+		free(pw.pw_dir);
+		free(pw.pw_shell);
+		free(shell_argv0);
+		return exit_code;
 	}
 
 	/* CHILD */
-
-	close(fd);
-
-	/* Close and reopen descriptors 0 through 2 */
-	if (close(0) || close(1) || close(2)) {
+	/* Close the tty and reopen descriptors 0 through 2 */
+	if (close(fd) || close(0) || close(1) || close(2)) {
 		fprintf(stderr, _("Could not close descriptors.\n"));
-		exit(-1);
+		goto err_close_pam;
 	}
 	fd = open(ttyn, O_RDONLY);
-	if (fd != 0) {
-		exit(-1);
-	}
+	if (fd != 0)
+		goto err_close_pam;
 	fd = open(ttyn, O_WRONLY);
-	if (fd != 1) {
-		exit(-1);
-	}
+	if (fd != 1)
+		goto err_close_pam;
 	fd = open(ttyn, O_WRONLY);
-	if (fd != 2) {
-		exit(-1);
-	}
+	if (fd != 2)
+		goto err_close_pam;
 
 	/*
-	 *
 	 * Step 5:  Execute a new shell with the new context in `new_context'. 
 	 *
+	 * Establish context, namesapce and any options for the new shell
 	 */
-
 	if (optind < 1)
 		optind = 1;
-	if (asprintf(&argv[optind - 1], "-%s", pw->pw_shell) < 0) {
-		fprintf(stderr, _("Error allocating shell.\n"));
-		exit(-1);
-	}
-#ifdef CANTSPELLGDB
-	{
-		int i;
-		printf("Executing ");
-		for (i = optind - 1; i < argc; i++)
-			printf("%s ", argv[i]);
-		printf("with context %s\n", new_context);
+
+	/* This is ugly, but use newrole's argv for the exec'd shells argv */
+	if (asprintf(&shell_argv0, "-%s", pw.pw_shell) < 0) {
+		fprintf(stderr, _("Error allocating shell's argv0.\n"));
+		shell_argv0 = NULL;
+		goto err_close_pam;
 	}
-#endif
-	if (setexeccon(new_context) < 0) {
+	argv[optind-1] = shell_argv0;
+
+	if (setexeccon(new_context)) {
 		fprintf(stderr, _("Could not set exec context to %s.\n"),
 			new_context);
-		exit(-1);
+		goto err_close_pam;
+	}
+
+#ifdef NAMESPACE_PRIV
+	/* Ask PAM to setup session for user running this program */
+	pam_status = pam_open_session(pam_handle,0);
+	if (pam_status != PAM_SUCCESS) {
+		fprintf(stderr, "pam_open_session failed with %s\n",
+			pam_strerror(pam_handle, pam_status));
+		goto err_close_pam;
 	}
+#endif
+
 	if (send_audit_message(1, old_context, new_context, ttyn))
-		exit(-1);
+		goto err_close_pam_session;
+#ifdef NAMESPACE_PRIV
+	if (transition_to_caller_uid())
+		goto err_close_pam_session;
+#endif
 	freecon(old_context);
-	execv(pw->pw_shell, argv + optind - 1);
+	freecon(new_context);
+	execv(pw.pw_shell, argv + optind - 1);
 
-	/* If we reach here, then we failed to exec the new shell. */
+	/*
+	 * Error path cleanup
+	 *
+	 * If we reach here, then we failed to exec the new shell.
+	 */
 	perror(_("failed to exec shell\n"));
-	return (-1);
+err_close_pam_session:
+#ifdef NAMESPACE_PRIV
+	pam_status = pam_close_session(pam_handle,0);
+	if(pam_status != PAM_SUCCESS)
+		fprintf(stderr, "pam_close_session failed with %s\n",
+			pam_strerror(pam_handle, pam_status));
+#endif
+err_close_pam:
+#ifdef USE_PAM
+	rc = pam_end(pam_handle, pam_status);
+	if (rc != PAM_SUCCESS)
+		fprintf(stderr, "pam_end failed with %s\n",
+			pam_strerror(pam_handle, rc));
+#endif
+err_free:
+	freecon(tty_context);
+	freecon(new_tty_context);
+	freecon(old_context);
+	freecon(new_context);
+	free(pw.pw_name);
+	free(pw.pw_dir);
+	free(pw.pw_shell);
+	free(shell_argv0);
+	return -1;
 }				/* main() */

Re: [PATCH 3/4] newrole suid functionality (take 2)

[Stephen Smalley]

On Tue, 2006-10-17 at 13:42 -0500, Michael C Thompson wrote:
> Michael C Thompson wrote:
> > This is the intro to a set of four patches.
> > 
> > These patches are an attempt to make newrole be an acceptably secure 
> > suid root program, to provide it with the capabilities to generate audit 
> > records (existing) and handle polyinstatiation (new).
> > 
> > The 4 patches are as follows:
> > 1) New functions introduced to newrole.c, new and existing functionality
> > 2) Changes to existing functions in newrole.c
> > 3) Updates to main in newrole.c to use the aforementioned changes
> 
> This is the 3rd of 4 patches.
> This patch applies against policycoreutils-1.30.30-1.
> 
> Changes:
>   * main is changed in the following ways:
>     - remove the duplicated functionality from patch 1/4
>     - set to call the new functions from patch 1/4
>     - set to use changes to functions in patch 2/4
>     - introduces better error handling and cleanup paths
> 
> Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>

diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
--- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:15:20.000000000 -0500
+++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:15:03.000000000 -0500
@@ -897,47 +898,43 @@
 
 int main(int argc, char *argv[])
<snip>
+	/*
+	 * Step 0: Setup
+	 *
+	 * Do some intial setup, including dropping capabilities, checking
+	 * if it makes sense to continue to run newrole, and setting up
+	 * a scrubbed environment.
+	 */
+	if (set_signal_handles())
+		return -1;
+	if (drop_capabilities())
+		return -1;

I'd keep drop_capabilities() first, as it was before this patch.
It would also make sense to move up sanitize_environment() as soon as
possible, even if that means splitting it into two phases (in particular
considering the locale support).

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 3/4] newrole suid functionality (take 2)

[Michael C Thompson]

Stephen Smalley wrote:
> On Tue, 2006-10-17 at 13:42 -0500, Michael C Thompson wrote:
>> Michael C Thompson wrote:
>>> This is the intro to a set of four patches.
>>>
>>> These patches are an attempt to make newrole be an acceptably secure 
>>> suid root program, to provide it with the capabilities to generate audit 
>>> records (existing) and handle polyinstatiation (new).
>>>
>>> The 4 patches are as follows:
>>> 1) New functions introduced to newrole.c, new and existing functionality
>>> 2) Changes to existing functions in newrole.c
>>> 3) Updates to main in newrole.c to use the aforementioned changes
>> This is the 3rd of 4 patches.
>> This patch applies against policycoreutils-1.30.30-1.
>>
>> Changes:
>>   * main is changed in the following ways:
>>     - remove the duplicated functionality from patch 1/4
>>     - set to call the new functions from patch 1/4
>>     - set to use changes to functions in patch 2/4
>>     - introduces better error handling and cleanup paths
>>
>> Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>
> 
> diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
> --- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:15:20.000000000 -0500
> +++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:15:03.000000000 -0500
> @@ -897,47 +898,43 @@
>  
>  int main(int argc, char *argv[])
> <snip>
> +	/*
> +	 * Step 0: Setup
> +	 *
> +	 * Do some intial setup, including dropping capabilities, checking
> +	 * if it makes sense to continue to run newrole, and setting up
> +	 * a scrubbed environment.
> +	 */
> +	if (set_signal_handles())
> +		return -1;
> +	if (drop_capabilities())
> +		return -1;
> 
> I'd keep drop_capabilities() first, as it was before this patch.
> It would also make sense to move up sanitize_environment() as soon as
> possible, even if that means splitting it into two phases (in particular
> considering the locale support).

Alright, that can be done. If we want to preserve the environment 
variables when we exec the new shell, this could be done at the same 
time support this change.

Thanks,
Mike



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 3/4] newrole suid functionality (take 2)

[Michael C Thompson]

Stephen Smalley wrote:
> On Tue, 2006-10-17 at 13:42 -0500, Michael C Thompson wrote:
>> Michael C Thompson wrote:
>>> This is the intro to a set of four patches.
>>>
>>> These patches are an attempt to make newrole be an acceptably secure 
>>> suid root program, to provide it with the capabilities to generate audit 
>>> records (existing) and handle polyinstatiation (new).
>>>
>>> The 4 patches are as follows:
>>> 1) New functions introduced to newrole.c, new and existing functionality
>>> 2) Changes to existing functions in newrole.c
>>> 3) Updates to main in newrole.c to use the aforementioned changes
>> This is the 3rd of 4 patches.
>> This patch applies against policycoreutils-1.30.30-1.
>>
>> Changes:
>>   * main is changed in the following ways:
>>     - remove the duplicated functionality from patch 1/4
>>     - set to call the new functions from patch 1/4
>>     - set to use changes to functions in patch 2/4
>>     - introduces better error handling and cleanup paths
>>
>> Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>
> 
> diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
> --- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:15:20.000000000 -0500
> +++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:15:03.000000000 -0500
> @@ -897,47 +898,43 @@
>  
>  int main(int argc, char *argv[])
> <snip>
> +	/*
> +	 * Step 0: Setup
> +	 *
> +	 * Do some intial setup, including dropping capabilities, checking
> +	 * if it makes sense to continue to run newrole, and setting up
> +	 * a scrubbed environment.
> +	 */
> +	if (set_signal_handles())
> +		return -1;
> +	if (drop_capabilities())
> +		return -1;
> 
> I'd keep drop_capabilities() first, as it was before this patch.
> It would also make sense to move up sanitize_environment() as soon as
> possible, even if that means splitting it into two phases (in particular
> considering the locale support).

Would this order be acceptable?

drop_cap
set_signal_handlers
setlocale
sanitize_env

...

I'm not familiar with locale enough to understand your point above, but 
I imagine doing environ = NULL will mess up the locale, but I am not 
sure. If that is right, we need to do it before we sanitize the 
environment, right?

Thanks,
Mike



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 3/4] newrole suid functionality (take 2)

[Stephen Smalley]

On Thu, 2006-11-02 at 11:22 -0600, Michael C Thompson wrote:
> Stephen Smalley wrote:
> > On Tue, 2006-10-17 at 13:42 -0500, Michael C Thompson wrote:
> >> Michael C Thompson wrote:
> >>> This is the intro to a set of four patches.
> >>>
> >>> These patches are an attempt to make newrole be an acceptably secure 
> >>> suid root program, to provide it with the capabilities to generate audit 
> >>> records (existing) and handle polyinstatiation (new).
> >>>
> >>> The 4 patches are as follows:
> >>> 1) New functions introduced to newrole.c, new and existing functionality
> >>> 2) Changes to existing functions in newrole.c
> >>> 3) Updates to main in newrole.c to use the aforementioned changes
> >> This is the 3rd of 4 patches.
> >> This patch applies against policycoreutils-1.30.30-1.
> >>
> >> Changes:
> >>   * main is changed in the following ways:
> >>     - remove the duplicated functionality from patch 1/4
> >>     - set to call the new functions from patch 1/4
> >>     - set to use changes to functions in patch 2/4
> >>     - introduces better error handling and cleanup paths
> >>
> >> Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>
> > 
> > diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
> > --- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:15:20.000000000 -0500
> > +++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:15:03.000000000 -0500
> > @@ -897,47 +898,43 @@
> >  
> >  int main(int argc, char *argv[])
> > <snip>
> > +	/*
> > +	 * Step 0: Setup
> > +	 *
> > +	 * Do some intial setup, including dropping capabilities, checking
> > +	 * if it makes sense to continue to run newrole, and setting up
> > +	 * a scrubbed environment.
> > +	 */
> > +	if (set_signal_handles())
> > +		return -1;
> > +	if (drop_capabilities())
> > +		return -1;
> > 
> > I'd keep drop_capabilities() first, as it was before this patch.
> > It would also make sense to move up sanitize_environment() as soon as
> > possible, even if that means splitting it into two phases (in particular
> > considering the locale support).
> 
> Would this order be acceptable?
> 
> drop_cap
> set_signal_handlers
> setlocale
> sanitize_env
> 
> ...
> 
> I'm not familiar with locale enough to understand your point above, but 
> I imagine doing environ = NULL will mess up the locale, but I am not 
> sure. If that is right, we need to do it before we sanitize the 
> environment, right?

Completely purging the environment could break that functionality, yes.
On the other hand, blindly accepting the locale-related environment
settings can be a risk.  See:
http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/locale.html

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 3/4] newrole suid functionality (take 2)

[Michael C Thompson]

Stephen Smalley wrote:
> On Thu, 2006-11-02 at 11:22 -0600, Michael C Thompson wrote:
>> Stephen Smalley wrote:
>>> On Tue, 2006-10-17 at 13:42 -0500, Michael C Thompson wrote:
>>>> Michael C Thompson wrote:
>>>>> This is the intro to a set of four patches.
>>>>>
>>>>> These patches are an attempt to make newrole be an acceptably secure 
>>>>> suid root program, to provide it with the capabilities to generate audit 
>>>>> records (existing) and handle polyinstatiation (new).
>>>>>
>>>>> The 4 patches are as follows:
>>>>> 1) New functions introduced to newrole.c, new and existing functionality
>>>>> 2) Changes to existing functions in newrole.c
>>>>> 3) Updates to main in newrole.c to use the aforementioned changes
>>>> This is the 3rd of 4 patches.
>>>> This patch applies against policycoreutils-1.30.30-1.
>>>>
>>>> Changes:
>>>>   * main is changed in the following ways:
>>>>     - remove the duplicated functionality from patch 1/4
>>>>     - set to call the new functions from patch 1/4
>>>>     - set to use changes to functions in patch 2/4
>>>>     - introduces better error handling and cleanup paths
>>>>
>>>> Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>
>>> diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
>>> --- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:15:20.000000000 -0500
>>> +++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:15:03.000000000 -0500
>>> @@ -897,47 +898,43 @@
>>>  
>>>  int main(int argc, char *argv[])
>>> <snip>
>>> +	/*
>>> +	 * Step 0: Setup
>>> +	 *
>>> +	 * Do some intial setup, including dropping capabilities, checking
>>> +	 * if it makes sense to continue to run newrole, and setting up
>>> +	 * a scrubbed environment.
>>> +	 */
>>> +	if (set_signal_handles())
>>> +		return -1;
>>> +	if (drop_capabilities())
>>> +		return -1;
>>>
>>> I'd keep drop_capabilities() first, as it was before this patch.
>>> It would also make sense to move up sanitize_environment() as soon as
>>> possible, even if that means splitting it into two phases (in particular
>>> considering the locale support).
>> Would this order be acceptable?
>>
>> drop_cap
>> set_signal_handlers
>> setlocale
>> sanitize_env
>>
>> ...
>>
>> I'm not familiar with locale enough to understand your point above, but 
>> I imagine doing environ = NULL will mess up the locale, but I am not 
>> sure. If that is right, we need to do it before we sanitize the 
>> environment, right?
> 
> Completely purging the environment could break that functionality, yes.
> On the other hand, blindly accepting the locale-related environment
> settings can be a risk.  See:
> http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/locale.html

So, based on reading this, and reviewing code from su and login, what 
seems to be the reasonable approach is the following:

drop_capabilities();
set_signal_handles();
setlocale(LC_ALL, "");
bindtextdomain(PACKAGE, LOCALEDIR);
textdomain(PACKAGE);
sanitize_environment();

Obviously, this isn't a patch, but this is the guts of the change. I'm 
not really sure if bindtextdomain relies on NLSPATH still like "Secure 
Programs HOWTO" states, but apparently when newrole is SUID, it will 
ignore NLSPATH so we need not worry about that.

There is no use of any of the locale-related information before the 
setlocale call, which re-initializes the values (as is my understanding 
from reading the code... I can't stand glibc's formatting).

There isn't any propsed change in this code, just a better understanding 
from the coder. If this is suggested code flow looks OK, I will clean up 
the rest of my code and send out the next round of patches.

Thanks,
Mike


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 3/4] newrole suid functionality (take 2)

[Stephen Smalley]

On Thu, 2006-11-02 at 14:35 -0600, Michael C Thompson wrote:
> Stephen Smalley wrote:
> > On Thu, 2006-11-02 at 11:22 -0600, Michael C Thompson wrote:
> >> Stephen Smalley wrote:
> >>> On Tue, 2006-10-17 at 13:42 -0500, Michael C Thompson wrote:
> >>>> Michael C Thompson wrote:
> >>>>> This is the intro to a set of four patches.
> >>>>>
> >>>>> These patches are an attempt to make newrole be an acceptably secure 
> >>>>> suid root program, to provide it with the capabilities to generate audit 
> >>>>> records (existing) and handle polyinstatiation (new).
> >>>>>
> >>>>> The 4 patches are as follows:
> >>>>> 1) New functions introduced to newrole.c, new and existing functionality
> >>>>> 2) Changes to existing functions in newrole.c
> >>>>> 3) Updates to main in newrole.c to use the aforementioned changes
> >>>> This is the 3rd of 4 patches.
> >>>> This patch applies against policycoreutils-1.30.30-1.
> >>>>
> >>>> Changes:
> >>>>   * main is changed in the following ways:
> >>>>     - remove the duplicated functionality from patch 1/4
> >>>>     - set to call the new functions from patch 1/4
> >>>>     - set to use changes to functions in patch 2/4
> >>>>     - introduces better error handling and cleanup paths
> >>>>
> >>>> Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>
> >>> diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
> >>> --- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:15:20.000000000 -0500
> >>> +++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:15:03.000000000 -0500
> >>> @@ -897,47 +898,43 @@
> >>>  
> >>>  int main(int argc, char *argv[])
> >>> <snip>
> >>> +	/*
> >>> +	 * Step 0: Setup
> >>> +	 *
> >>> +	 * Do some intial setup, including dropping capabilities, checking
> >>> +	 * if it makes sense to continue to run newrole, and setting up
> >>> +	 * a scrubbed environment.
> >>> +	 */
> >>> +	if (set_signal_handles())
> >>> +		return -1;
> >>> +	if (drop_capabilities())
> >>> +		return -1;
> >>>
> >>> I'd keep drop_capabilities() first, as it was before this patch.
> >>> It would also make sense to move up sanitize_environment() as soon as
> >>> possible, even if that means splitting it into two phases (in particular
> >>> considering the locale support).
> >> Would this order be acceptable?
> >>
> >> drop_cap
> >> set_signal_handlers
> >> setlocale
> >> sanitize_env
> >>
> >> ...
> >>
> >> I'm not familiar with locale enough to understand your point above, but 
> >> I imagine doing environ = NULL will mess up the locale, but I am not 
> >> sure. If that is right, we need to do it before we sanitize the 
> >> environment, right?
> > 
> > Completely purging the environment could break that functionality, yes.
> > On the other hand, blindly accepting the locale-related environment
> > settings can be a risk.  See:
> > http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/locale.html
> 
> So, based on reading this, and reviewing code from su and login, what 
> seems to be the reasonable approach is the following:
> 
> drop_capabilities();
> set_signal_handles();
> setlocale(LC_ALL, "");
> bindtextdomain(PACKAGE, LOCALEDIR);
> textdomain(PACKAGE);
> sanitize_environment();
> 
> Obviously, this isn't a patch, but this is the guts of the change. I'm 
> not really sure if bindtextdomain relies on NLSPATH still like "Secure 
> Programs HOWTO" states, but apparently when newrole is SUID, it will 
> ignore NLSPATH so we need not worry about that.
> 
> There is no use of any of the locale-related information before the 
> setlocale call, which re-initializes the values (as is my understanding 
> from reading the code... I can't stand glibc's formatting).

The man page for setlocale() says:
	If locale is "", each part of the locale that should be modified is set
according to the environment variables.

Which doesn't sound like it is sanitizing them.  Possibly it has
different behavior in the libc_enable_secure case, which would be 1 for
newrole (even the non-suid newrole, due to the domain transition), but I
don't know offhand.  NLSPATH is on the unsecvars list, so it would be
ignored.

> There isn't any propsed change in this code, just a better understanding 
> from the coder. If this is suggested code flow looks OK, I will clean up 
> the rest of my code and send out the next round of patches.

I take it that su and friends don't do anything special here prior to
calling the localization functions?
  
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 3/4] newrole suid functionality (take 2)

[Michael C Thompson]

Stephen Smalley wrote:
> The man page for setlocale() says:
> 	If locale is "", each part of the locale that should be modified is set
> according to the environment variables.
> 
> Which doesn't sound like it is sanitizing them.  Possibly it has
> different behavior in the libc_enable_secure case, which would be 1 for
> newrole (even the non-suid newrole, due to the domain transition), but I
> don't know offhand.  NLSPATH is on the unsecvars list, so it would be
> ignored.

I read through the glibc source, and glibc does checking for '/' in the 
values obtained from the environment.

>> There isn't any propsed change in this code, just a better understanding 
>> from the coder. If this is suggested code flow looks OK, I will clean up 
>> the rest of my code and send out the next round of patches.
> 
> I take it that su and friends don't do anything special here prior to
> calling the localization functions?

Right, the calls to setlocale, bindtextdomain and textdomain is 
basically the first thing that they do. They also seem to not worry 
about the environment until much later as well.

I'm find it hard to know where to stop worrying and trust the 
libraries... because understanding absolutely everything that's going on 
will not make for a timely patch. Although I would rather not trust 
anything.

Mike


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

[PATCH 4/4] newrole suid functionality (take 2)

[Michael C Thompson]

[-- Attachment #1: Type: text/plain, Size: 914 bytes --]

Michael C Thompson wrote:
> This is the intro to a set of four patches.
> 
> These patches are an attempt to make newrole be an acceptably secure 
> suid root program, to provide it with the capabilities to generate audit 
> records (existing) and handle polyinstatiation (new).
> 
> The 4 patches are as follows:
> 1) New functions introduced to newrole.c, new and existing functionality
> 2) Changes to existing functions in newrole.c
> 3) Updates to main in newrole.c to use the aforementioned changes
> 4) Changes to the Makefile to allow building of newrole with the
>    changes and introduction of newrole-lspp.pamd

This is the 4th of 4 patches.
This patch applies against policycoreutils-1.30.30-1.

Changes:
  * Makefile now has AUDIT_LOG_PRIV and NAMESPACE_PRIV, as well as
    LSPP_PRIV (causes both previous to be on)
  * Adds newrole-lspp.pamd

Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>


[-- Attachment #2: 04-update_Makefile_add_lspp.patch --]
[-- Type: text/x-diff, Size: 2703 bytes --]

diff -Naur policycoreutils-1.30.30.orig/newrole/Makefile policycoreutils-1.30.30.suid/newrole/Makefile
--- policycoreutils-1.30.30.orig/newrole/Makefile	2006-09-29 10:50:27.000000000 -0500
+++ policycoreutils-1.30.30.suid/newrole/Makefile	2006-10-17 12:58:01.000000000 -0500
@@ -6,10 +6,18 @@
 LOCALEDIR = /usr/share/locale
 PAMH = $(shell ls /usr/include/security/pam_appl.h 2>/dev/null)
 AUDITH = $(shell ls /usr/include/libaudit.h 2>/dev/null)
-# If LOG_AUDIT_PRIV is y, then newrole will be made into setuid root program.
-# This is so that we have the CAP_AUDIT_WRITE capability. newrole will
-# shed all privileges and change to the user's uid.
-LOG_AUDIT_PRIV ?= n
+# Enable capabilities to permit newrole to generate audit records.
+# This will make newrole a setuid root program.
+# The capabilities used are: CAP_AUDIT_WRITE.
+AUDIT_LOG_PRIV ?= n
+# Enable capabilities to permit newrole to utilitize the pam_namespace module.
+# This will make newrole a setuid root program.
+# The capabilities used are: CAP_SYS_ADMIN, CAP_CHOWN, CAP_FOWNER and
+# CAP_DAC_OVERRIDE. 
+NAMESPACE_PRIV ?= n
+# If LSPP_PRIV is y, then newrole will be made into setuid root program.
+# Enabling this option will force AUDIT_LOG_PRIV and NAMESPACE_PRIV to be y.
+LSPP_PRIV ?= y
 VERSION = $(shell cat ../VERSION)
 
 CFLAGS ?= -Werror -Wall -W
@@ -26,10 +34,21 @@
 	override CFLAGS += -DUSE_AUDIT
 	LDLIBS += -laudit
 endif
-ifeq (${LOG_AUDIT_PRIV},y)
-	override CFLAGS += -DLOG_AUDIT_PRIV
-	LDLIBS += -lcap
+ifeq (${LSPP_PRIV},y)
+	override AUDIT_LOG_PRIV=y
+	override NAMESPACE_PRIV=y
+endif
+ifeq (${AUDIT_LOG_PRIV},y)
+	override CFLAGS += -DAUDIT_LOG_PRIV
+	IS_SUID=y
+endif
+ifeq (${NAMESPACE_PRIV},y)
+	override CFLAGS += -DNAMESPACE_PRIV
+	IS_SUID=y
+endif
+ifeq (${IS_SUID},y)
 	MODE := 4555
+	LDLIBS += -lcap
 else
 	MODE := 555
 endif
@@ -46,8 +65,12 @@
 	install -m 644 newrole.1 $(MANDIR)/man1/
 ifeq (${PAMH}, /usr/include/security/pam_appl.h)
 	test -d $(ETCDIR)/pam.d || install -m 755 -d $(ETCDIR)/pam.d
+ifeq (${LSPP_PRIV},y)
+	install -m 644 newrole-lspp.pamd $(ETCDIR)/pam.d/newrole
+else
 	install -m 644 newrole.pamd $(ETCDIR)/pam.d/newrole
 endif
+endif
 
 clean:
 	rm -f $(TARGETS) *.o 
diff -Naur policycoreutils-1.30.30.orig/newrole/newrole-lspp.pamd policycoreutils-1.30.30.suid/newrole/newrole-lspp.pamd
--- policycoreutils-1.30.30.orig/newrole/newrole-lspp.pamd	1969-12-31 18:00:00.000000000 -0600
+++ policycoreutils-1.30.30.suid/newrole/newrole-lspp.pamd	2006-10-17 12:58:01.000000000 -0500
@@ -0,0 +1,5 @@
+#%PAM-1.0
+auth       include	system-auth
+account    include	system-auth
+password   include	system-auth
+session    required	pam_namespace.so unmnt_remnt no_unmount_on_close

Re: [PATCH 4/4] newrole suid functionality (take 2)

[Stephen Smalley]

On Tue, 2006-10-17 at 13:43 -0500, Michael C Thompson wrote:
> Michael C Thompson wrote:
> > This is the intro to a set of four patches.
> > 
> > These patches are an attempt to make newrole be an acceptably secure 
> > suid root program, to provide it with the capabilities to generate audit 
> > records (existing) and handle polyinstatiation (new).
> > 
> > The 4 patches are as follows:
> > 1) New functions introduced to newrole.c, new and existing functionality
> > 2) Changes to existing functions in newrole.c
> > 3) Updates to main in newrole.c to use the aforementioned changes
> > 4) Changes to the Makefile to allow building of newrole with the
> >    changes and introduction of newrole-lspp.pamd
> 
> This is the 4th of 4 patches.
> This patch applies against policycoreutils-1.30.30-1.
> 
> Changes:
>   * Makefile now has AUDIT_LOG_PRIV and NAMESPACE_PRIV, as well as
>     LSPP_PRIV (causes both previous to be on)
>   * Adds newrole-lspp.pamd
> 
> Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>

diff -Naur policycoreutils-1.30.30.orig/newrole/Makefile policycoreutils-1.30.30.suid/newrole/Makefile
--- policycoreutils-1.30.30.orig/newrole/Makefile	2006-09-29 10:50:27.000000000 -0500
+++ policycoreutils-1.30.30.suid/newrole/Makefile	2006-10-17 12:58:01.000000000 -0500
@@ -6,10 +6,18 @@
 LOCALEDIR = /usr/share/locale
 PAMH = $(shell ls /usr/include/security/pam_appl.h 2>/dev/null)
 AUDITH = $(shell ls /usr/include/libaudit.h 2>/dev/null)
-# If LOG_AUDIT_PRIV is y, then newrole will be made into setuid root program.
-# This is so that we have the CAP_AUDIT_WRITE capability. newrole will
-# shed all privileges and change to the user's uid.
-LOG_AUDIT_PRIV ?= n
+# Enable capabilities to permit newrole to generate audit records.
+# This will make newrole a setuid root program.
+# The capabilities used are: CAP_AUDIT_WRITE.
+AUDIT_LOG_PRIV ?= n
+# Enable capabilities to permit newrole to utilitize the pam_namespace module.
+# This will make newrole a setuid root program.
+# The capabilities used are: CAP_SYS_ADMIN, CAP_CHOWN, CAP_FOWNER and
+# CAP_DAC_OVERRIDE. 
+NAMESPACE_PRIV ?= n
+# If LSPP_PRIV is y, then newrole will be made into setuid root program.
+# Enabling this option will force AUDIT_LOG_PRIV and NAMESPACE_PRIV to be y.
+LSPP_PRIV ?= y

The plan would be to make LSPP_PRIV = n by default in the upstream
Makefile, then Red Hat can build with make LSPP_PRIV=y in their .spec
file.  That ensures it is always an explicit choice to enable this.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 4/4] newrole suid functionality (take 2)

[Michael C Thompson]

Stephen Smalley wrote:
> On Tue, 2006-10-17 at 13:43 -0500, Michael C Thompson wrote:
>> Michael C Thompson wrote:
>>> This is the intro to a set of four patches.
>>>
>>> These patches are an attempt to make newrole be an acceptably secure 
>>> suid root program, to provide it with the capabilities to generate audit 
>>> records (existing) and handle polyinstatiation (new).
>>>
>>> The 4 patches are as follows:
>>> 1) New functions introduced to newrole.c, new and existing functionality
>>> 2) Changes to existing functions in newrole.c
>>> 3) Updates to main in newrole.c to use the aforementioned changes
>>> 4) Changes to the Makefile to allow building of newrole with the
>>>    changes and introduction of newrole-lspp.pamd
>> This is the 4th of 4 patches.
>> This patch applies against policycoreutils-1.30.30-1.
>>
>> Changes:
>>   * Makefile now has AUDIT_LOG_PRIV and NAMESPACE_PRIV, as well as
>>     LSPP_PRIV (causes both previous to be on)
>>   * Adds newrole-lspp.pamd
>>
>> Signed-off-by: Michael Thompson <thompsmc@us.ibm.com>
> 
> diff -Naur policycoreutils-1.30.30.orig/newrole/Makefile policycoreutils-1.30.30.suid/newrole/Makefile
> --- policycoreutils-1.30.30.orig/newrole/Makefile	2006-09-29 10:50:27.000000000 -0500
> +++ policycoreutils-1.30.30.suid/newrole/Makefile	2006-10-17 12:58:01.000000000 -0500
> @@ -6,10 +6,18 @@
>  LOCALEDIR = /usr/share/locale
>  PAMH = $(shell ls /usr/include/security/pam_appl.h 2>/dev/null)
>  AUDITH = $(shell ls /usr/include/libaudit.h 2>/dev/null)
> -# If LOG_AUDIT_PRIV is y, then newrole will be made into setuid root program.
> -# This is so that we have the CAP_AUDIT_WRITE capability. newrole will
> -# shed all privileges and change to the user's uid.
> -LOG_AUDIT_PRIV ?= n
> +# Enable capabilities to permit newrole to generate audit records.
> +# This will make newrole a setuid root program.
> +# The capabilities used are: CAP_AUDIT_WRITE.
> +AUDIT_LOG_PRIV ?= n
> +# Enable capabilities to permit newrole to utilitize the pam_namespace module.
> +# This will make newrole a setuid root program.
> +# The capabilities used are: CAP_SYS_ADMIN, CAP_CHOWN, CAP_FOWNER and
> +# CAP_DAC_OVERRIDE. 
> +NAMESPACE_PRIV ?= n
> +# If LSPP_PRIV is y, then newrole will be made into setuid root program.
> +# Enabling this option will force AUDIT_LOG_PRIV and NAMESPACE_PRIV to be y.
> +LSPP_PRIV ?= y
> 
> The plan would be to make LSPP_PRIV = n by default in the upstream
> Makefile, then Red Hat can build with make LSPP_PRIV=y in their .spec
> file.  That ensures it is always an explicit choice to enable this.

Oops. Yes, the intention was supposed to be ?= n by default.



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 0/4] newrole suid functionality (take 2)

[Stephen Smalley]

On Tue, 2006-10-17 at 13:24 -0500, Michael C Thompson wrote:
> This is the intro to a set of four patches.
> 
> These patches are an attempt to make newrole be an acceptably secure 
> suid root program, to provide it with the capabilities to generate audit 
> records (existing) and handle polyinstatiation (new).
> 
> The 4 patches are as follows:
> 1) New functions introduced to newrole.c, new and existing functionality
> 2) Changes to existing functions in newrole.c
> 3) Updates to main in newrole.c to use the aforementioned changes
> 4) Changes to the Makefile to allow building of newrole with the
>     changes and introduction of newrole-lspp.pamd
> 
> Note: This is an atomically applicable patch set. Applying a subset of 
> these patches will break the build.
> 
> The comments from the previous send of these patches have been integrated.

diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
--- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:11:41.000000000 -0500
+++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:12:29.000000000 -0500
@@ -87,6 +87,7 @@
 /* USAGE_STRING describes the command-line args of this program. */
 #define USAGE_STRING "USAGE: newrole [ -r role ] [ -t type ] [ -l level ] [ -V ] [ -- args ]"
 
+#define DEFAULT_PATH "/bin:/usr/bin:/usr/local/bin"

Where does this particular path come from?  Why /usr/local/bin at all?
Why doesn't /usr/bin come before /bin?  

+/**
+ * Unset all environment variables except:
+ * TERM, DISPLAY and XAUTHORITY - if they are set, preserve values
+ * HOME, SHELL, USER and LOGNAME - set to contents of /etc/passwd
+ * PATH - set to default value DEFAULT_PATH

Anyone relying on the ability to propagate other environment settings to
a newrole'd shell (that can't be re-created from the user's dotfiles)?
An alternative would be to save the original environment, reset it in
this manner for the duration of newrole, but call the user shell with
the original environment or some combination.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 0/4] newrole suid functionality (take 2)

[Michael C Thompson]

Stephen Smalley wrote:
> On Tue, 2006-10-17 at 13:24 -0500, Michael C Thompson wrote:
>> This is the intro to a set of four patches.
>>
>> These patches are an attempt to make newrole be an acceptably secure 
>> suid root program, to provide it with the capabilities to generate audit 
>> records (existing) and handle polyinstatiation (new).
>>
>> The 4 patches are as follows:
>> 1) New functions introduced to newrole.c, new and existing functionality
>> 2) Changes to existing functions in newrole.c
>> 3) Updates to main in newrole.c to use the aforementioned changes
>> 4) Changes to the Makefile to allow building of newrole with the
>>     changes and introduction of newrole-lspp.pamd
>>
>> Note: This is an atomically applicable patch set. Applying a subset of 
>> these patches will break the build.
>>
>> The comments from the previous send of these patches have been integrated.
> 
> diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
> --- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:11:41.000000000 -0500
> +++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:12:29.000000000 -0500
> @@ -87,6 +87,7 @@
>  /* USAGE_STRING describes the command-line args of this program. */
>  #define USAGE_STRING "USAGE: newrole [ -r role ] [ -t type ] [ -l level ] [ -V ] [ -- args ]"
>  
> +#define DEFAULT_PATH "/bin:/usr/bin:/usr/local/bin"
> 
> Where does this particular path come from?  Why /usr/local/bin at all?
> Why doesn't /usr/bin come before /bin?  

The concept for this came from the su source. The path basically spawned 
out of my head. It can be changed to anything that people deem appropriate.

> +/**
> + * Unset all environment variables except:
> + * TERM, DISPLAY and XAUTHORITY - if they are set, preserve values
> + * HOME, SHELL, USER and LOGNAME - set to contents of /etc/passwd
> + * PATH - set to default value DEFAULT_PATH
> 
> Anyone relying on the ability to propagate other environment settings to
> a newrole'd shell (that can't be re-created from the user's dotfiles)?

If they are, it would be pretty easy to allow the preservation of 
environment variables like su does.

> An alternative would be to save the original environment, reset it in
> this manner for the duration of newrole, but call the user shell with
> the original environment or some combination.

Again, this would be quite possible, say with a -p option?

Mike


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 0/4] newrole suid functionality (take 2)

[Michael C Thompson]

Stephen Smalley wrote:
> On Tue, 2006-10-17 at 13:24 -0500, Michael C Thompson wrote:
>> This is the intro to a set of four patches.
>>
>> These patches are an attempt to make newrole be an acceptably secure 
>> suid root program, to provide it with the capabilities to generate audit 
>> records (existing) and handle polyinstatiation (new).
>>
>> The 4 patches are as follows:
>> 1) New functions introduced to newrole.c, new and existing functionality
>> 2) Changes to existing functions in newrole.c
>> 3) Updates to main in newrole.c to use the aforementioned changes
>> 4) Changes to the Makefile to allow building of newrole with the
>>     changes and introduction of newrole-lspp.pamd
>>
>> Note: This is an atomically applicable patch set. Applying a subset of 
>> these patches will break the build.
>>
>> The comments from the previous send of these patches have been integrated.
> 
> diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
> --- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:11:41.000000000 -0500
> +++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:12:29.000000000 -0500
> @@ -87,6 +87,7 @@
>  /* USAGE_STRING describes the command-line args of this program. */
>  #define USAGE_STRING "USAGE: newrole [ -r role ] [ -t type ] [ -l level ] [ -V ] [ -- args ]"
>  
> +#define DEFAULT_PATH "/bin:/usr/bin:/usr/local/bin"
> 
> Where does this particular path come from?  Why /usr/local/bin at all?
> Why doesn't /usr/bin come before /bin?  

So would "/usr/bin:/bin" be sufficent? Not sure if it is desirable to 
add sbin paths or not.

Thanks,
Mike



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 0/4] newrole suid functionality (take 2)

[Stephen Smalley]

On Thu, 2006-11-02 at 11:18 -0600, Michael C Thompson wrote:
> Stephen Smalley wrote:
> > On Tue, 2006-10-17 at 13:24 -0500, Michael C Thompson wrote:
> >> This is the intro to a set of four patches.
> >>
> >> These patches are an attempt to make newrole be an acceptably secure 
> >> suid root program, to provide it with the capabilities to generate audit 
> >> records (existing) and handle polyinstatiation (new).
> >>
> >> The 4 patches are as follows:
> >> 1) New functions introduced to newrole.c, new and existing functionality
> >> 2) Changes to existing functions in newrole.c
> >> 3) Updates to main in newrole.c to use the aforementioned changes
> >> 4) Changes to the Makefile to allow building of newrole with the
> >>     changes and introduction of newrole-lspp.pamd
> >>
> >> Note: This is an atomically applicable patch set. Applying a subset of 
> >> these patches will break the build.
> >>
> >> The comments from the previous send of these patches have been integrated.
> > 
> > diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
> > --- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:11:41.000000000 -0500
> > +++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:12:29.000000000 -0500
> > @@ -87,6 +87,7 @@
> >  /* USAGE_STRING describes the command-line args of this program. */
> >  #define USAGE_STRING "USAGE: newrole [ -r role ] [ -t type ] [ -l level ] [ -V ] [ -- args ]"
> >  
> > +#define DEFAULT_PATH "/bin:/usr/bin:/usr/local/bin"
> > 
> > Where does this particular path come from?  Why /usr/local/bin at all?
> > Why doesn't /usr/bin come before /bin?  
> 
> So would "/usr/bin:/bin" be sufficent? Not sure if it is desirable to 
> add sbin paths or not.

I'd expect newrole and any code it executes to use fully specified paths
anyway, so I'm not sure it makes a difference.  Since newrole and pam is
often called by non-root users, that code cannot assume that sbin is in
the path, nor would it want to rely on the path.  And for the newrole'd
shell, the path is going to be customized by the user's dotfiles
typically.  So /usr/bin:/bin is likely fine.  Not sure what value login
uses as the initial state for PATH for login sessions.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 0/4] newrole suid functionality (take 2)

[Michael C Thompson]

Stephen Smalley wrote:
> On Thu, 2006-11-02 at 11:18 -0600, Michael C Thompson wrote:
>> Stephen Smalley wrote:
>>> On Tue, 2006-10-17 at 13:24 -0500, Michael C Thompson wrote:
>>>> This is the intro to a set of four patches.
>>>>
>>>> These patches are an attempt to make newrole be an acceptably secure 
>>>> suid root program, to provide it with the capabilities to generate audit 
>>>> records (existing) and handle polyinstatiation (new).
>>>>
>>>> The 4 patches are as follows:
>>>> 1) New functions introduced to newrole.c, new and existing functionality
>>>> 2) Changes to existing functions in newrole.c
>>>> 3) Updates to main in newrole.c to use the aforementioned changes
>>>> 4) Changes to the Makefile to allow building of newrole with the
>>>>     changes and introduction of newrole-lspp.pamd
>>>>
>>>> Note: This is an atomically applicable patch set. Applying a subset of 
>>>> these patches will break the build.
>>>>
>>>> The comments from the previous send of these patches have been integrated.
>>> diff -Naur policycoreutils-1.30.30.orig/newrole/newrole.c policycoreutils-1.30.30.suid/newrole/newrole.c
>>> --- policycoreutils-1.30.30.orig/newrole/newrole.c	2006-10-17 13:11:41.000000000 -0500
>>> +++ policycoreutils-1.30.30.suid/newrole/newrole.c	2006-10-17 13:12:29.000000000 -0500
>>> @@ -87,6 +87,7 @@
>>>  /* USAGE_STRING describes the command-line args of this program. */
>>>  #define USAGE_STRING "USAGE: newrole [ -r role ] [ -t type ] [ -l level ] [ -V ] [ -- args ]"
>>>  
>>> +#define DEFAULT_PATH "/bin:/usr/bin:/usr/local/bin"
>>>
>>> Where does this particular path come from?  Why /usr/local/bin at all?
>>> Why doesn't /usr/bin come before /bin?  
>> So would "/usr/bin:/bin" be sufficent? Not sure if it is desirable to 
>> add sbin paths or not.
> 
> I'd expect newrole and any code it executes to use fully specified paths
> anyway, so I'm not sure it makes a difference.

I can easily test that I guess by having PATH be empty or invalid :)

 > Since newrole and pam is
> often called by non-root users, that code cannot assume that sbin is in
> the path, nor would it want to rely on the path.  And for the newrole'd
> shell, the path is going to be customized by the user's dotfiles
> typically.  So /usr/bin:/bin is likely fine.  Not sure what value login
> uses as the initial state for PATH for login sessions.

I have mimic'd the behaviour of su in the original approach. login uses 
"/usr/bin:/bin" for non-root and "/usr/bin:/bin:/usr/sbin:/sbin" for 
root. Like I said, I can add that functionality if the calling uid is 0. 
That might be nice on a minimal system.

Thanks,
Mike



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.