[RFC PATCH] newrole suid breakdown

[RFC PATCH] newrole suid breakdown

[Michael C Thompson]

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

Hey all,

So, in light of the concerns regarding making newrole suid, I have taken 
some steps to break up the functionality of newrole. I am sending this 
out for some feedback on this approach, I think it is a step in the 
right direction. This note has become rather lengthy, but seeing as 
we're working on avoiding a "root hole", I hope people can take the time 
to look at it :)

This patch applies against the policycoreutils-1.30.30-1 package.

Changes from 1.30.30-1:
Makefile
  * Took Janak's patch, and broke LSPP_PRIV into two
    subparts, AUDIT_LOG_PRIV and NAMESPACE_PRIV. newrole
    can be built with none, either or both sets of functionality.
  * Included the newrole-lspp.pamd install step from Janak's patch

newrole-lspp.pamd
  * Taken straight from Janak's patch

newrole.c
  * NAMESPACE_PRIV requires USE_PAM
  * Updated authenticate_via_pam as per Janak's patch
  * drop_capabilities has three flavors: AUDIT_LOG_PRIV, NAMESPACE_PRIV
    and neither. AUDIT_LOG_PRIV is a subset of the capabilities of
    NAMESPACE_PRIV
  * drop_capabilities has been restructured to return an int
  * New function transition_to_caller_uid()
  * pam usage changes as per Janak's patch
  * change_pns_effective_caps() (from Janak's patches) is removed in
    favor of retaining euid of 0 until shell exec

A few comments:
Most of this work is based off of Janak's patches, so there will be a 
decent amount of familiar code. The most notable difference is the 
existence of two separate drop_capabilities() functions and 
change_pns_effective_caps() has been removed. I added a new 
drop_capabilities so that the capabilities needed to do only auditing 
and handling namespaces are clearly differentiated.

The new drop_capabilities retains in the EFFECTIVE set all of the 
capabilities it will need to affect namespaces. I don't really see the 
value gained from raising and lowering the capabilities mid-execution, 
if someone is able to attack newrole and execute arbitrary code, they 
can simply raise those capabilities from the PERMITTED set. If you would 
like this changed to be more paranoid, let me know.

The removal of change_pns_effective_caps() was done in favor of 
maintaining the euid of 0 for nearly the lifetime of the application, 
only switching to the caller uid before the shell exec. This is 
functionally permissible since we retain CAP_DAC_OVERRIDE (otherwise 
being euid 0 without this cap breaks opening the tty). For the same 
reasons as above, I don't see the reason to be non-euid 0 for the 
lifetime of the app since it is desired to be able to change euid to 0 
for the pam_namespace actions, and it should be changed to the caller's 
uid prior to the exec.


 From what I have seen, the points of possible attack (i.e. input) to 
newrole include:
* command line options
* reading the password from stdin
* reading the contents of /etc/passwd and /etc/shell

I doubt this is an exhaustive list. Since /etc/passwd and /etc/shell are 
restricted to root-only write, I've discounted these. Since we require 
PAM to do namespaces, which will cause PAM to handle the password check, 
we should be able to defer any vulnerabilities to PAM, and I would hope 
that PAM is secure.

This leaves command line options. These are parsed with getopt_long, and 
are currently passed into libselinux without any scrubbing. Since 
SELinux doesn't apply a limit on the length of its context strings, is 
there any checks that need to be done before handing these strings into 
libselinux?


The cleanup process for errors within newrole currently does not make a 
call to pam_end (after pam_start). Is this an issue? There are also 
other cases where cleanup isn't really handled as nicely as it could. 
Since this is a user-space app, a lot of cleanup is handled by the 
kernel, but I do have some patches that go a ways to begin cleaning this 
sort of thing up. Are they desirable?


I have tested all 4 ways of compiling newrole (no priv, AUDIT_LOG_PRIV, 
NAMESPACE_PRIV, LSPP_PRIV (both)), and it functions as expected. As for 
code quality and security, if this is to be suid, I would like to see it 
be as clean and tight as possible :)

Thanks,
Mike


[-- Attachment #2: newrole_suid_breakdown.patch --]
[-- Type: text/x-diff, Size: 13783 bytes --]

diff -Naur policycoreutils-1.30.30/newrole/Makefile policycoreutils-1.30.30.dev/newrole/Makefile
--- policycoreutils-1.30.30/newrole/Makefile	2006-09-29 10:50:27.000000000 -0500
+++ policycoreutils-1.30.30.dev/newrole/Makefile	2006-10-04 16:38:33.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 ?= n
 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/newrole/newrole.c policycoreutils-1.30.30.dev/newrole/newrole.c
--- policycoreutils-1.30.30/newrole/newrole.c	2006-09-29 10:50:27.000000000 -0500
+++ policycoreutils-1.30.30.dev/newrole/newrole.c	2006-10-04 16:37:04.000000000 -0500
@@ -48,6 +48,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() */
@@ -66,10 +74,7 @@
 #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
@@ -158,8 +163,6 @@
 
 #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 
@@ -176,31 +179,13 @@
  *
  */
 
-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 */
 	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
@@ -224,10 +209,7 @@
 		result = 1;	/* user authenticated OK! */
 	}
 
-	/* We're done with PAM.  Free `pam_handle'. */
       out:
-	pam_end(pam_handle, rc);
-
 	return (result);
 
 }				/* authenticate_via_pam() */
@@ -246,8 +228,6 @@
 
 #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 
@@ -338,65 +318,153 @@
  * the capabilities alone since they already should have access to the
  * audit netlink socket.
  */
-#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 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 ((rc = prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0))) {
+		fprintf(stderr, _("Error setting KEEPCAPS, aborting\n"));
+		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.
+ */
+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 };
+	uid_t uid = getuid();
+
+	if (!uid)
+		return 0;
+
+	/* Non-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
+
+#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()
+{
+	int rc;
+	uid_t uid = getuid();
+
+	/* Change uid */
+	if ((rc = setresuid(uid, uid, uid))) {
+		fprintf(stderr, _("Error changing uid, aborting.\n"));
+		goto out;
 	}
+out:
+	return rc;
 }
 #endif
 
-#ifdef LOG_AUDIT_PRIV
+#ifdef AUDIT_LOG_PRIV
 /* Send audit message */
 static
 int send_audit_message(int success, security_context_t old_context,
@@ -481,11 +549,22 @@
 	int fd;
 	int enforcing;
 	sigset_t empty;
+#ifdef USE_PAM
+	int rc;			/* pam return code */
+	pam_handle_t *pam_handle;  /* opaque handle used by all PAM functions */
 
-#ifdef LOG_AUDIT_PRIV
-	drop_capabilities();
+	/* 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
+	};
 #endif
 
+	if (drop_capabilities())
+		return -1;
+
 	/* Empty the signal mask in case someone is blocking a signal */
 	sigemptyset(&empty);
 	(void)sigprocmask(SIG_SETMASK, &empty, NULL);
@@ -673,7 +752,16 @@
 	 * by policy).  Trusted path mechanism would be preferred.
 	 */
 #ifdef USE_PAM
-	if (!authenticate_via_pam(pw, ttyn))
+	/* 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 (!authenticate_via_pam(ttyn, pam_handle))
 #else				/* !USE_PAM */
 	if (!authenticate_via_shadow_passwd(pw))
 #endif				/* if/else USE_PAM */
@@ -870,6 +958,21 @@
 		fsetfilecon(fd, tty_context);
 		freecon(tty_context);
 
+#ifdef USE_PAM
+#ifdef NAMESPACE_PRIV
+		rc = pam_close_session(pam_handle,0);
+		if(rc != PAM_SUCCESS) {
+			fprintf(stderr, "pam_close_session failed with %s\n",
+				pam_strerror(pam_handle, rc));
+			pam_end(pam_handle, rc);
+			exit(-1);
+		}
+#endif
+
+		/* We're done with PAM.  Free `pam_handle'. */
+		pam_end(pam_handle, rc);
+#endif
+
 		/* Done! */
 		exit(0);
 	}
@@ -922,9 +1025,23 @@
 			new_context);
 		exit(-1);
 	}
+#ifdef NAMESPACE_PRIV
+	/* Ask PAM to setup session for user running this program */
+	rc = pam_open_session(pam_handle,0);
+	if(rc != PAM_SUCCESS) {
+		fprintf(stderr, "pam_open_session failed with %s\n",
+			pam_strerror(pam_handle, rc));
+		exit(-1);
+	}
+#endif
+
 	if (send_audit_message(1, old_context, new_context, ttyn))
 		exit(-1);
 	freecon(old_context);
+#ifdef NAMESPACE_PRIV
+	if (transition_to_caller_uid())
+		exit(-1);
+#endif
 	execv(pw->pw_shell, argv + optind - 1);
 
 	/* If we reach here, then we failed to exec the new shell. */
diff -Naur policycoreutils-1.30.30/newrole/newrole-lspp.pamd policycoreutils-1.30.30.dev/newrole/newrole-lspp.pamd
--- policycoreutils-1.30.30/newrole/newrole-lspp.pamd	1969-12-31 18:00:00.000000000 -0600
+++ policycoreutils-1.30.30.dev/newrole/newrole-lspp.pamd	2006-10-04 16:11:21.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: [RFC PATCH] newrole suid breakdown

[Daniel J Walsh]

Michael C Thompson wrote:
> Hey all,
>
> So, in light of the concerns regarding making newrole suid, I have 
> taken some steps to break up the functionality of newrole. I am 
> sending this out for some feedback on this approach, I think it is a 
> step in the right direction. This note has become rather lengthy, but 
> seeing as we're working on avoiding a "root hole", I hope people can 
> take the time to look at it :)
>
> This patch applies against the policycoreutils-1.30.30-1 package.
>
> Changes from 1.30.30-1:
> Makefile
>  * Took Janak's patch, and broke LSPP_PRIV into two
>    subparts, AUDIT_LOG_PRIV and NAMESPACE_PRIV. newrole
>    can be built with none, either or both sets of functionality.
>  * Included the newrole-lspp.pamd install step from Janak's patch
>
> newrole-lspp.pamd
>  * Taken straight from Janak's patch
>
> newrole.c
>  * NAMESPACE_PRIV requires USE_PAM
>  * Updated authenticate_via_pam as per Janak's patch
>  * drop_capabilities has three flavors: AUDIT_LOG_PRIV, NAMESPACE_PRIV
>    and neither. AUDIT_LOG_PRIV is a subset of the capabilities of
>    NAMESPACE_PRIV
>  * drop_capabilities has been restructured to return an int
>  * New function transition_to_caller_uid()
>  * pam usage changes as per Janak's patch
>  * change_pns_effective_caps() (from Janak's patches) is removed in
>    favor of retaining euid of 0 until shell exec
>
> A few comments:
> Most of this work is based off of Janak's patches, so there will be a 
> decent amount of familiar code. The most notable difference is the 
> existence of two separate drop_capabilities() functions and 
> change_pns_effective_caps() has been removed. I added a new 
> drop_capabilities so that the capabilities needed to do only auditing 
> and handling namespaces are clearly differentiated.
>
> The new drop_capabilities retains in the EFFECTIVE set all of the 
> capabilities it will need to affect namespaces. I don't really see the 
> value gained from raising and lowering the capabilities mid-execution, 
> if someone is able to attack newrole and execute arbitrary code, they 
> can simply raise those capabilities from the PERMITTED set. If you 
> would like this changed to be more paranoid, let me know.
>
> The removal of change_pns_effective_caps() was done in favor of 
> maintaining the euid of 0 for nearly the lifetime of the application, 
> only switching to the caller uid before the shell exec. This is 
> functionally permissible since we retain CAP_DAC_OVERRIDE (otherwise 
> being euid 0 without this cap breaks opening the tty). For the same 
> reasons as above, I don't see the reason to be non-euid 0 for the 
> lifetime of the app since it is desired to be able to change euid to 0 
> for the pam_namespace actions, and it should be changed to the 
> caller's uid prior to the exec.
>
>
> From what I have seen, the points of possible attack (i.e. input) to 
> newrole include:
> * command line options
> * reading the password from stdin
> * reading the contents of /etc/passwd and /etc/shell
>
> I doubt this is an exhaustive list. Since /etc/passwd and /etc/shell 
> are restricted to root-only write, I've discounted these. Since we 
> require PAM to do namespaces, which will cause PAM to handle the 
> password check, we should be able to defer any vulnerabilities to PAM, 
> and I would hope that PAM is secure.
>
> This leaves command line options. These are parsed with getopt_long, 
> and are currently passed into libselinux without any scrubbing. Since 
> SELinux doesn't apply a limit on the length of its context strings, is 
> there any checks that need to be done before handing these strings 
> into libselinux?
>
>
> The cleanup process for errors within newrole currently does not make 
> a call to pam_end (after pam_start). Is this an issue? There are also 
> other cases where cleanup isn't really handled as nicely as it could. 
> Since this is a user-space app, a lot of cleanup is handled by the 
> kernel, but I do have some patches that go a ways to begin cleaning 
> this sort of thing up. Are they desirable?
>
>
> I have tested all 4 ways of compiling newrole (no priv, 
> AUDIT_LOG_PRIV, NAMESPACE_PRIV, LSPP_PRIV (both)), and it functions as 
> expected. As for code quality and security, if this is to be suid, I 
> would like to see it be as clean and tight as possible :)
>
Does the code continue to work correctly if I compile in AUDIT_LOG_PRIV 
and NAMESPACE_PRIV but run it without the setuid bit and as a normal 
user.  IE, We want the option to only set this setuid when in an MLS 
environment.  This is not required for targeted or strict policy machines.
> Thanks,
> Mike
>
> ------------------------------------------------------------------------
>
> diff -Naur policycoreutils-1.30.30/newrole/Makefile policycoreutils-1.30.30.dev/newrole/Makefile
> --- policycoreutils-1.30.30/newrole/Makefile	2006-09-29 10:50:27.000000000 -0500
> +++ policycoreutils-1.30.30.dev/newrole/Makefile	2006-10-04 16:38:33.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 ?= n
>  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/newrole/newrole.c policycoreutils-1.30.30.dev/newrole/newrole.c
> --- policycoreutils-1.30.30/newrole/newrole.c	2006-09-29 10:50:27.000000000 -0500
> +++ policycoreutils-1.30.30.dev/newrole/newrole.c	2006-10-04 16:37:04.000000000 -0500
> @@ -48,6 +48,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() */
> @@ -66,10 +74,7 @@
>  #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
> @@ -158,8 +163,6 @@
>  
>  #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 
> @@ -176,31 +179,13 @@
>   *
>   */
>  
> -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 */
>  	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
> @@ -224,10 +209,7 @@
>  		result = 1;	/* user authenticated OK! */
>  	}
>  
> -	/* We're done with PAM.  Free `pam_handle'. */
>        out:
> -	pam_end(pam_handle, rc);
> -
>  	return (result);
>  
>  }				/* authenticate_via_pam() */
> @@ -246,8 +228,6 @@
>  
>  #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 
> @@ -338,65 +318,153 @@
>   * the capabilities alone since they already should have access to the
>   * audit netlink socket.
>   */
> -#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 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 ((rc = prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0))) {
> +		fprintf(stderr, _("Error setting KEEPCAPS, aborting\n"));
> +		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.
> + */
> +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 };
> +	uid_t uid = getuid();
> +
> +	if (!uid)
> +		return 0;
> +
> +	/* Non-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
> +
> +#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()
> +{
> +	int rc;
> +	uid_t uid = getuid();
> +
> +	/* Change uid */
> +	if ((rc = setresuid(uid, uid, uid))) {
> +		fprintf(stderr, _("Error changing uid, aborting.\n"));
> +		goto out;
>  	}
> +out:
> +	return rc;
>  }
>  #endif
>  
> -#ifdef LOG_AUDIT_PRIV
> +#ifdef AUDIT_LOG_PRIV
>  /* Send audit message */
>  static
>  int send_audit_message(int success, security_context_t old_context,
> @@ -481,11 +549,22 @@
>  	int fd;
>  	int enforcing;
>  	sigset_t empty;
> +#ifdef USE_PAM
> +	int rc;			/* pam return code */
> +	pam_handle_t *pam_handle;  /* opaque handle used by all PAM functions */
>  
> -#ifdef LOG_AUDIT_PRIV
> -	drop_capabilities();
> +	/* 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
> +	};
>  #endif
>  
> +	if (drop_capabilities())
> +		return -1;
> +
>  	/* Empty the signal mask in case someone is blocking a signal */
>  	sigemptyset(&empty);
>  	(void)sigprocmask(SIG_SETMASK, &empty, NULL);
> @@ -673,7 +752,16 @@
>  	 * by policy).  Trusted path mechanism would be preferred.
>  	 */
>  #ifdef USE_PAM
> -	if (!authenticate_via_pam(pw, ttyn))
> +	/* 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 (!authenticate_via_pam(ttyn, pam_handle))
>  #else				/* !USE_PAM */
>  	if (!authenticate_via_shadow_passwd(pw))
>  #endif				/* if/else USE_PAM */
> @@ -870,6 +958,21 @@
>  		fsetfilecon(fd, tty_context);
>  		freecon(tty_context);
>  
> +#ifdef USE_PAM
> +#ifdef NAMESPACE_PRIV
> +		rc = pam_close_session(pam_handle,0);
> +		if(rc != PAM_SUCCESS) {
> +			fprintf(stderr, "pam_close_session failed with %s\n",
> +				pam_strerror(pam_handle, rc));
> +			pam_end(pam_handle, rc);
> +			exit(-1);
> +		}
> +#endif
> +
> +		/* We're done with PAM.  Free `pam_handle'. */
> +		pam_end(pam_handle, rc);
> +#endif
> +
>  		/* Done! */
>  		exit(0);
>  	}
> @@ -922,9 +1025,23 @@
>  			new_context);
>  		exit(-1);
>  	}
> +#ifdef NAMESPACE_PRIV
> +	/* Ask PAM to setup session for user running this program */
> +	rc = pam_open_session(pam_handle,0);
> +	if(rc != PAM_SUCCESS) {
> +		fprintf(stderr, "pam_open_session failed with %s\n",
> +			pam_strerror(pam_handle, rc));
> +		exit(-1);
> +	}
> +#endif
> +
>  	if (send_audit_message(1, old_context, new_context, ttyn))
>  		exit(-1);
>  	freecon(old_context);
> +#ifdef NAMESPACE_PRIV
> +	if (transition_to_caller_uid())
> +		exit(-1);
> +#endif
>  	execv(pw->pw_shell, argv + optind - 1);
>  
>  	/* If we reach here, then we failed to exec the new shell. */
> diff -Naur policycoreutils-1.30.30/newrole/newrole-lspp.pamd policycoreutils-1.30.30.dev/newrole/newrole-lspp.pamd
> --- policycoreutils-1.30.30/newrole/newrole-lspp.pamd	1969-12-31 18:00:00.000000000 -0600
> +++ policycoreutils-1.30.30.dev/newrole/newrole-lspp.pamd	2006-10-04 16:11:21.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
>   


--
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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Wed, 2006-10-04 at 17:17 -0500, Michael C Thompson wrote:
>  From what I have seen, the points of possible attack (i.e. input) to 
> newrole include:
> * command line options
> * reading the password from stdin
> * reading the contents of /etc/passwd and /etc/shell
> 
> I doubt this is an exhaustive list. Since /etc/passwd and /etc/shell are 
> restricted to root-only write, I've discounted these. Since we require 
> PAM to do namespaces, which will cause PAM to handle the password check, 
> we should be able to defer any vulnerabilities to PAM, and I would hope 
> that PAM is secure.
> 
> This leaves command line options. These are parsed with getopt_long, and 
> are currently passed into libselinux without any scrubbing. Since 
> SELinux doesn't apply a limit on the length of its context strings, is 
> there any checks that need to be done before handing these strings into 
> libselinux?

I don't think so.  The larger concern is not the command line arguments
but the environment.  glibc does some automatic sanitization of the
environment for setuid programs, but is limited in what it can do.
You likely want to save the environment or portions of it for
propagation to the newrole'd shell, reset the environment to a clean
state for newrole itself, and then invoke the user shell with the
previously saved environment.  Look at other setuid programs, like su
and sudo, as possible examples.   Also see:
http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/environment-variables.html

Since pam_namespace further executes a script (namespace.init), the
environment is especially critical, and you need to consider the
implications of how the shell handles setuid execution.  For example,
unless namespace.init invokes /bin/sh with the -p option (privileged
mode), bash will reset the effective identities to the real identities
during startup, such that the script itself will run in the caller's
identity (and thus likely fail if it tries to do its job, like bind
mounting the .X11-unix directory into the member directory, as well as
creating any files or directories in the caller's uid).  If you change
namespace.init to use the -p option, then bash will leave the effective
identities alone, and will ignore certain aspects of the environment
that would normally affect it.  One might argue that pam_namespace
itself should do some environment cleansing, since it executes a script
and this won't be immediately obvious to all potential users of
pam_namespace.

> The cleanup process for errors within newrole currently does not make a 
> call to pam_end (after pam_start). Is this an issue? There are also 
> other cases where cleanup isn't really handled as nicely as it could. 
> Since this is a user-space app, a lot of cleanup is handled by the 
> kernel, but I do have some patches that go a ways to begin cleaning this 
> sort of thing up. Are they desirable?

Yes.

> I have tested all 4 ways of compiling newrole (no priv, AUDIT_LOG_PRIV, 
> NAMESPACE_PRIV, LSPP_PRIV (both)), and it functions as expected.

Did you check the effective identity and capability sets at each stage
of execution to see if they were what you expected?  Within newrole
after each change?  On entry to pam_namespace?  On entry to
namespace.init?  In the newrole'd shell?

Also, I'm not clear on how this resolves the issue of being able to have
a single newrole binary in a distribution, and of not requiring any
additional exposure for systems that do not require the privileged
functionality, which Dan also noted.

-- 
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: [RFC PATCH] newrole suid breakdown

[Michael C Thompson]

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

Daniel J Walsh wrote:
> Michael C Thompson wrote:
>> Hey all,
>>
>> So, in light of the concerns regarding making newrole suid, I have 
>> taken some steps to break up the functionality of newrole. I am 
>> sending this out for some feedback on this approach, I think it is a 
>> step in the right direction. This note has become rather lengthy, but 
>> seeing as we're working on avoiding a "root hole", I hope people can 
>> take the time to look at it :)
>>
>> This patch applies against the policycoreutils-1.30.30-1 package.
>>
>> Changes from 1.30.30-1:
>> Makefile
>>  * Took Janak's patch, and broke LSPP_PRIV into two
>>    subparts, AUDIT_LOG_PRIV and NAMESPACE_PRIV. newrole
>>    can be built with none, either or both sets of functionality.
>>  * Included the newrole-lspp.pamd install step from Janak's patch
>>
>> newrole-lspp.pamd
>>  * Taken straight from Janak's patch
>>
>> newrole.c
>>  * NAMESPACE_PRIV requires USE_PAM
>>  * Updated authenticate_via_pam as per Janak's patch
>>  * drop_capabilities has three flavors: AUDIT_LOG_PRIV, NAMESPACE_PRIV
>>    and neither. AUDIT_LOG_PRIV is a subset of the capabilities of
>>    NAMESPACE_PRIV
>>  * drop_capabilities has been restructured to return an int
>>  * New function transition_to_caller_uid()
>>  * pam usage changes as per Janak's patch
>>  * change_pns_effective_caps() (from Janak's patches) is removed in
>>    favor of retaining euid of 0 until shell exec
>>
>> A few comments:
>> Most of this work is based off of Janak's patches, so there will be a 
>> decent amount of familiar code. The most notable difference is the 
>> existence of two separate drop_capabilities() functions and 
>> change_pns_effective_caps() has been removed. I added a new 
>> drop_capabilities so that the capabilities needed to do only auditing 
>> and handling namespaces are clearly differentiated.
>>
>> The new drop_capabilities retains in the EFFECTIVE set all of the 
>> capabilities it will need to affect namespaces. I don't really see the 
>> value gained from raising and lowering the capabilities mid-execution, 
>> if someone is able to attack newrole and execute arbitrary code, they 
>> can simply raise those capabilities from the PERMITTED set. If you 
>> would like this changed to be more paranoid, let me know.
>>
>> The removal of change_pns_effective_caps() was done in favor of 
>> maintaining the euid of 0 for nearly the lifetime of the application, 
>> only switching to the caller uid before the shell exec. This is 
>> functionally permissible since we retain CAP_DAC_OVERRIDE (otherwise 
>> being euid 0 without this cap breaks opening the tty). For the same 
>> reasons as above, I don't see the reason to be non-euid 0 for the 
>> lifetime of the app since it is desired to be able to change euid to 0 
>> for the pam_namespace actions, and it should be changed to the 
>> caller's uid prior to the exec.
>>
>>
>> From what I have seen, the points of possible attack (i.e. input) to 
>> newrole include:
>> * command line options
>> * reading the password from stdin
>> * reading the contents of /etc/passwd and /etc/shell
>>
>> I doubt this is an exhaustive list. Since /etc/passwd and /etc/shell 
>> are restricted to root-only write, I've discounted these. Since we 
>> require PAM to do namespaces, which will cause PAM to handle the 
>> password check, we should be able to defer any vulnerabilities to PAM, 
>> and I would hope that PAM is secure.
>>
>> This leaves command line options. These are parsed with getopt_long, 
>> and are currently passed into libselinux without any scrubbing. Since 
>> SELinux doesn't apply a limit on the length of its context strings, is 
>> there any checks that need to be done before handing these strings 
>> into libselinux?
>>
>>
>> The cleanup process for errors within newrole currently does not make 
>> a call to pam_end (after pam_start). Is this an issue? There are also 
>> other cases where cleanup isn't really handled as nicely as it could. 
>> Since this is a user-space app, a lot of cleanup is handled by the 
>> kernel, but I do have some patches that go a ways to begin cleaning 
>> this sort of thing up. Are they desirable?
>>
>>
>> I have tested all 4 ways of compiling newrole (no priv, 
>> AUDIT_LOG_PRIV, NAMESPACE_PRIV, LSPP_PRIV (both)), and it functions as 
>> expected. As for code quality and security, if this is to be suid, I 
>> would like to see it be as clean and tight as possible :)
>>
> Does the code continue to work correctly if I compile in AUDIT_LOG_PRIV 
> and NAMESPACE_PRIV but run it without the setuid bit and as a normal 
> user.  IE, We want the option to only set this setuid when in an MLS 
> environment.  This is not required for targeted or strict policy machines.

What happens (currently), is it attempts to drop capabilities and if it 
can't do that, newrole fails and exists. The reason I chose this 
behavior is, as I figured, if you have compiled newrole with 
AUDIT_LOG_PRIV or NAMESPACE_PRIV, you requiring that behaviour.

It should be easy to change this behavior, as I expect RH will want a 
single compiled binary that can do it all, right? :)

Suggested patch attached. If you have a more elegant solution, feel free 
to let me know.

Thanks,
Mike



[-- Attachment #2: rh.newrole.patch --]
[-- Type: text/x-diff, Size: 2914 bytes --]

diff -Naur policycoreutils-1.30.30.dev/newrole/newrole.c policycoreutils-1.30.30.dev.rh/newrole/newrole.c
--- policycoreutils-1.30.30.dev/newrole/newrole.c	2006-10-04 17:01:17.000000000 -0500
+++ policycoreutils-1.30.30.dev.rh/newrole/newrole.c	2006-10-05 09:39:14.000000000 -0500
@@ -548,6 +548,7 @@
 	uid_t uid;
 	int fd;
 	int enforcing;
+	int ignore_privilage_actions = 0;
 	sigset_t empty;
 #ifdef USE_PAM
 	int rc;			/* pam return code */
@@ -562,8 +563,11 @@
 	};
 #endif
 
-	if (drop_capabilities())
-		return -1;
+	if (geteuid())
+		ignore_privilage_actions = 1;
+	else
+		if (drop_capabilities())
+			return -1;
 
 	/* Empty the signal mask in case someone is blocking a signal */
 	sigemptyset(&empty);
@@ -662,7 +666,9 @@
 	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);
+			if (!ignore_privilage_actions)
+				send_audit_message(0, old_context, new_context,
+						   ttyn);
 			exit(-1);
 		}
 #ifdef CANTSPELLGDB
@@ -851,7 +857,8 @@
 
 	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);
+		if (!ignore_privilage_actions)
+			send_audit_message(0, old_context, new_context, ttyn);
 		exit(-1);
 	}
 
@@ -960,12 +967,15 @@
 
 #ifdef USE_PAM
 #ifdef NAMESPACE_PRIV
-		rc = pam_close_session(pam_handle,0);
-		if(rc != PAM_SUCCESS) {
-			fprintf(stderr, "pam_close_session failed with %s\n",
-				pam_strerror(pam_handle, rc));
-			pam_end(pam_handle, rc);
-			exit(-1);
+		if (!ignore_privilage_actions) {
+			rc = pam_close_session(pam_handle,0);
+			if(rc != PAM_SUCCESS) {
+				fprintf(stderr,
+					"pam_close_session failed with %s\n",
+					pam_strerror(pam_handle, rc));
+				pam_end(pam_handle, rc);
+				exit(-1);
+			}
 		}
 #endif
 
@@ -1026,21 +1036,24 @@
 		exit(-1);
 	}
 #ifdef NAMESPACE_PRIV
-	/* Ask PAM to setup session for user running this program */
-	rc = pam_open_session(pam_handle,0);
-	if(rc != PAM_SUCCESS) {
-		fprintf(stderr, "pam_open_session failed with %s\n",
-			pam_strerror(pam_handle, rc));
-		exit(-1);
+	if (!ignore_privilage_actions) {
+		/* Ask PAM to setup session for user running this program */
+		rc = pam_open_session(pam_handle,0);
+		if(rc != PAM_SUCCESS) {
+			fprintf(stderr, "pam_open_session failed with %s\n",
+				pam_strerror(pam_handle, rc));
+			exit(-1);
+		}
 	}
 #endif
-
-	if (send_audit_message(1, old_context, new_context, ttyn))
-		exit(-1);
+	if (!ignore_privilage_actions)
+		if (send_audit_message(1, old_context, new_context, ttyn))
+			exit(-1);
 	freecon(old_context);
 #ifdef NAMESPACE_PRIV
-	if (transition_to_caller_uid())
-		exit(-1);
+	if (!ignore_privilage_actions)
+		if (transition_to_caller_uid())
+			exit(-1);
 #endif
 	execv(pw->pw_shell, argv + optind - 1);
 

Re: [RFC PATCH] newrole suid breakdown

[Daniel J Walsh]

Michael C Thompson wrote:
> Daniel J Walsh wrote:
>> Michael C Thompson wrote:
>>> Hey all,
>>>
>>> So, in light of the concerns regarding making newrole suid, I have 
>>> taken some steps to break up the functionality of newrole. I am 
>>> sending this out for some feedback on this approach, I think it is a 
>>> step in the right direction. This note has become rather lengthy, 
>>> but seeing as we're working on avoiding a "root hole", I hope people 
>>> can take the time to look at it :)
>>>
>>> This patch applies against the policycoreutils-1.30.30-1 package.
>>>
>>> Changes from 1.30.30-1:
>>> Makefile
>>>  * Took Janak's patch, and broke LSPP_PRIV into two
>>>    subparts, AUDIT_LOG_PRIV and NAMESPACE_PRIV. newrole
>>>    can be built with none, either or both sets of functionality.
>>>  * Included the newrole-lspp.pamd install step from Janak's patch
>>>
>>> newrole-lspp.pamd
>>>  * Taken straight from Janak's patch
>>>
>>> newrole.c
>>>  * NAMESPACE_PRIV requires USE_PAM
>>>  * Updated authenticate_via_pam as per Janak's patch
>>>  * drop_capabilities has three flavors: AUDIT_LOG_PRIV, NAMESPACE_PRIV
>>>    and neither. AUDIT_LOG_PRIV is a subset of the capabilities of
>>>    NAMESPACE_PRIV
>>>  * drop_capabilities has been restructured to return an int
>>>  * New function transition_to_caller_uid()
>>>  * pam usage changes as per Janak's patch
>>>  * change_pns_effective_caps() (from Janak's patches) is removed in
>>>    favor of retaining euid of 0 until shell exec
>>>
>>> A few comments:
>>> Most of this work is based off of Janak's patches, so there will be 
>>> a decent amount of familiar code. The most notable difference is the 
>>> existence of two separate drop_capabilities() functions and 
>>> change_pns_effective_caps() has been removed. I added a new 
>>> drop_capabilities so that the capabilities needed to do only 
>>> auditing and handling namespaces are clearly differentiated.
>>>
>>> The new drop_capabilities retains in the EFFECTIVE set all of the 
>>> capabilities it will need to affect namespaces. I don't really see 
>>> the value gained from raising and lowering the capabilities 
>>> mid-execution, if someone is able to attack newrole and execute 
>>> arbitrary code, they can simply raise those capabilities from the 
>>> PERMITTED set. If you would like this changed to be more paranoid, 
>>> let me know.
>>>
>>> The removal of change_pns_effective_caps() was done in favor of 
>>> maintaining the euid of 0 for nearly the lifetime of the 
>>> application, only switching to the caller uid before the shell exec. 
>>> This is functionally permissible since we retain CAP_DAC_OVERRIDE 
>>> (otherwise being euid 0 without this cap breaks opening the tty). 
>>> For the same reasons as above, I don't see the reason to be non-euid 
>>> 0 for the lifetime of the app since it is desired to be able to 
>>> change euid to 0 for the pam_namespace actions, and it should be 
>>> changed to the caller's uid prior to the exec.
>>>
>>>
>>> From what I have seen, the points of possible attack (i.e. input) to 
>>> newrole include:
>>> * command line options
>>> * reading the password from stdin
>>> * reading the contents of /etc/passwd and /etc/shell
>>>
>>> I doubt this is an exhaustive list. Since /etc/passwd and /etc/shell 
>>> are restricted to root-only write, I've discounted these. Since we 
>>> require PAM to do namespaces, which will cause PAM to handle the 
>>> password check, we should be able to defer any vulnerabilities to 
>>> PAM, and I would hope that PAM is secure.
>>>
>>> This leaves command line options. These are parsed with getopt_long, 
>>> and are currently passed into libselinux without any scrubbing. 
>>> Since SELinux doesn't apply a limit on the length of its context 
>>> strings, is there any checks that need to be done before handing 
>>> these strings into libselinux?
>>>
>>>
>>> The cleanup process for errors within newrole currently does not 
>>> make a call to pam_end (after pam_start). Is this an issue? There 
>>> are also other cases where cleanup isn't really handled as nicely as 
>>> it could. Since this is a user-space app, a lot of cleanup is 
>>> handled by the kernel, but I do have some patches that go a ways to 
>>> begin cleaning this sort of thing up. Are they desirable?
>>>
>>>
>>> I have tested all 4 ways of compiling newrole (no priv, 
>>> AUDIT_LOG_PRIV, NAMESPACE_PRIV, LSPP_PRIV (both)), and it functions 
>>> as expected. As for code quality and security, if this is to be 
>>> suid, I would like to see it be as clean and tight as possible :)
>>>
>> Does the code continue to work correctly if I compile in 
>> AUDIT_LOG_PRIV and NAMESPACE_PRIV but run it without the setuid bit 
>> and as a normal user.  IE, We want the option to only set this setuid 
>> when in an MLS environment.  This is not required for targeted or 
>> strict policy machines.
>
> What happens (currently), is it attempts to drop capabilities and if 
> it can't do that, newrole fails and exists. The reason I chose this 
> behavior is, as I figured, if you have compiled newrole with 
> AUDIT_LOG_PRIV or NAMESPACE_PRIV, you requiring that behaviour.
>
> It should be easy to change this behavior, as I expect RH will want a 
> single compiled binary that can do it all, right? :)
>
> Suggested patch attached. If you have a more elegant solution, feel 
> free to let me know.
>
> Thanks,
> Mike
>
>
> ------------------------------------------------------------------------
>
> diff -Naur policycoreutils-1.30.30.dev/newrole/newrole.c policycoreutils-1.30.30.dev.rh/newrole/newrole.c
> --- policycoreutils-1.30.30.dev/newrole/newrole.c	2006-10-04 17:01:17.000000000 -0500
> +++ policycoreutils-1.30.30.dev.rh/newrole/newrole.c	2006-10-05 09:39:14.000000000 -0500
> @@ -548,6 +548,7 @@
>  	uid_t uid;
>  	int fd;
>  	int enforcing;
> +	int ignore_privilage_actions = 0;
>  	sigset_t empty;
>  #ifdef USE_PAM
>  	int rc;			/* pam return code */
> @@ -562,8 +563,11 @@
>  	};
>  #endif
>  
> -	if (drop_capabilities())
> -		return -1;
> +	if (geteuid())
> +		ignore_privilage_actions = 1;
> +	else
> +		if (drop_capabilities())
> +			return -1;
>  
>   
I think this is all you need.  Audit can handle the fact that it can't 
send a message by sending it to /var/log/messages.  And if You use 
pam_namespace.so you should fail if you are not setuid and you are not root.
>  	/* Empty the signal mask in case someone is blocking a signal */
>  	sigemptyset(&empty);
> @@ -662,7 +666,9 @@
>  	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);
> +			if (!ignore_privilage_actions)
> +				send_audit_message(0, old_context, new_context,
> +						   ttyn);
>  			exit(-1);
>  		}
>  #ifdef CANTSPELLGDB
> @@ -851,7 +857,8 @@
>  
>  	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);
> +		if (!ignore_privilage_actions)
> +			send_audit_message(0, old_context, new_context, ttyn);
>  		exit(-1);
>  	}
>  
> @@ -960,12 +967,15 @@
>  
>  #ifdef USE_PAM
>  #ifdef NAMESPACE_PRIV
> -		rc = pam_close_session(pam_handle,0);
> -		if(rc != PAM_SUCCESS) {
> -			fprintf(stderr, "pam_close_session failed with %s\n",
> -				pam_strerror(pam_handle, rc));
> -			pam_end(pam_handle, rc);
> -			exit(-1);
> +		if (!ignore_privilage_actions) {
> +			rc = pam_close_session(pam_handle,0);
> +			if(rc != PAM_SUCCESS) {
> +				fprintf(stderr,
> +					"pam_close_session failed with %s\n",
> +					pam_strerror(pam_handle, rc));
> +				pam_end(pam_handle, rc);
> +				exit(-1);
> +			}
>  		}
>  #endif
>  
> @@ -1026,21 +1036,24 @@
>  		exit(-1);
>  	}
>  #ifdef NAMESPACE_PRIV
> -	/* Ask PAM to setup session for user running this program */
> -	rc = pam_open_session(pam_handle,0);
> -	if(rc != PAM_SUCCESS) {
> -		fprintf(stderr, "pam_open_session failed with %s\n",
> -			pam_strerror(pam_handle, rc));
> -		exit(-1);
> +	if (!ignore_privilage_actions) {
> +		/* Ask PAM to setup session for user running this program */
> +		rc = pam_open_session(pam_handle,0);
> +		if(rc != PAM_SUCCESS) {
> +			fprintf(stderr, "pam_open_session failed with %s\n",
> +				pam_strerror(pam_handle, rc));
> +			exit(-1);
> +		}
>  	}
>  #endif
> -
> -	if (send_audit_message(1, old_context, new_context, ttyn))
> -		exit(-1);
> +	if (!ignore_privilage_actions)
> +		if (send_audit_message(1, old_context, new_context, ttyn))
> +			exit(-1);
>  	freecon(old_context);
>  #ifdef NAMESPACE_PRIV
> -	if (transition_to_caller_uid())
> -		exit(-1);
> +	if (!ignore_privilage_actions)
> +		if (transition_to_caller_uid())
> +			exit(-1);
>  #endif
>  	execv(pw->pw_shell, argv + optind - 1);
>  
>   


--
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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Thu, 2006-10-05 at 09:42 -0500, Michael C Thompson wrote:
> Daniel J Walsh wrote:
> > Does the code continue to work correctly if I compile in AUDIT_LOG_PRIV 
> > and NAMESPACE_PRIV but run it without the setuid bit and as a normal 
> > user.  IE, We want the option to only set this setuid when in an MLS 
> > environment.  This is not required for targeted or strict policy machines.
> 
> What happens (currently), is it attempts to drop capabilities and if it 
> can't do that, newrole fails and exists. The reason I chose this 
> behavior is, as I figured, if you have compiled newrole with 
> AUDIT_LOG_PRIV or NAMESPACE_PRIV, you requiring that behaviour.
> 
> It should be easy to change this behavior, as I expect RH will want a 
> single compiled binary that can do it all, right? :)
> 
> Suggested patch attached. If you have a more elegant solution, feel free 
> to let me know.

Concerns with this approach:
1) The geteuid() test is ambiguous in the case where the caller was
already uid 0 (i.e. root runs newrole).  For dropping capabilities and
switching uids, this has no real effect since that processing is skipped
when the caller had uid 0.  But it does have an impact on the audit and
namespace functionality.  Note btw that the
pam_open_session/pam_close_session calls don't strictly require such a
test, since we can always put pam_permit as a session module in the
default pam config.

2) I'm not sure how RH plans to set and maintain the suid bit on newrole
in the LSPP case, since they want to have it off by default.  If another
package, like the lspp package, modifies the suid bit on newrole, then a
rpm -V policycoreutils will report a change in mode from the package
defaults and any update of policycoreutils could reset the mode to the
original one.

-- 
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: [RFC PATCH] newrole suid breakdown

[Michael C Thompson]

Daniel J Walsh wrote:
> Michael C Thompson wrote:
>> Daniel J Walsh wrote:
>>> Michael C Thompson wrote:
<snip>
>> diff -Naur policycoreutils-1.30.30.dev/newrole/newrole.c 
>> policycoreutils-1.30.30.dev.rh/newrole/newrole.c
>> --- policycoreutils-1.30.30.dev/newrole/newrole.c    2006-10-04 
>> 17:01:17.000000000 -0500
>> +++ policycoreutils-1.30.30.dev.rh/newrole/newrole.c    2006-10-05 
>> 09:39:14.000000000 -0500
>> @@ -548,6 +548,7 @@
>>      uid_t uid;
>>      int fd;
>>      int enforcing;
>> +    int ignore_privilage_actions = 0;
>>      sigset_t empty;
>>  #ifdef USE_PAM
>>      int rc;            /* pam return code */
>> @@ -562,8 +563,11 @@
>>      };
>>  #endif
>>  
>> -    if (drop_capabilities())
>> -        return -1;
>> +    if (geteuid())
>> +        ignore_privilage_actions = 1;
>> +    else
>> +        if (drop_capabilities())
>> +            return -1;
>>  
>>   
> I think this is all you need.  Audit can handle the fact that it can't 
> send a message by sending it to /var/log/messages.  And if You use 
> pam_namespace.so you should fail if you are not setuid and you are not 
> root.

True, but right now we are set to abort newrole if those actions are 
expected to succeed (we have compiled with the respective _PRIV). Should 
that behavior be changed?

>>      /* Empty the signal mask in case someone is blocking a signal */
>>      sigemptyset(&empty);
>> @@ -662,7 +666,9 @@
>>      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);
>> +            if (!ignore_privilage_actions)
>> +                send_audit_message(0, old_context, new_context,
>> +                           ttyn);
>>              exit(-1);
>>          }
>>  #ifdef CANTSPELLGDB
>> @@ -851,7 +857,8 @@
>>  
>>      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);
>> +        if (!ignore_privilage_actions)
>> +            send_audit_message(0, old_context, new_context, ttyn);
>>          exit(-1);
>>      }
>>  
>> @@ -960,12 +967,15 @@
>>  
>>  #ifdef USE_PAM
>>  #ifdef NAMESPACE_PRIV
>> -        rc = pam_close_session(pam_handle,0);
>> -        if(rc != PAM_SUCCESS) {
>> -            fprintf(stderr, "pam_close_session failed with %s\n",
>> -                pam_strerror(pam_handle, rc));
>> -            pam_end(pam_handle, rc);
>> -            exit(-1);
>> +        if (!ignore_privilage_actions) {
>> +            rc = pam_close_session(pam_handle,0);
>> +            if(rc != PAM_SUCCESS) {
>> +                fprintf(stderr,
>> +                    "pam_close_session failed with %s\n",
>> +                    pam_strerror(pam_handle, rc));
>> +                pam_end(pam_handle, rc);
>> +                exit(-1);
>> +            }
>>          }
>>  #endif
>>  
>> @@ -1026,21 +1036,24 @@
>>          exit(-1);
>>      }
>>  #ifdef NAMESPACE_PRIV
>> -    /* Ask PAM to setup session for user running this program */
>> -    rc = pam_open_session(pam_handle,0);
>> -    if(rc != PAM_SUCCESS) {
>> -        fprintf(stderr, "pam_open_session failed with %s\n",
>> -            pam_strerror(pam_handle, rc));
>> -        exit(-1);
>> +    if (!ignore_privilage_actions) {
>> +        /* Ask PAM to setup session for user running this program */
>> +        rc = pam_open_session(pam_handle,0);
>> +        if(rc != PAM_SUCCESS) {
>> +            fprintf(stderr, "pam_open_session failed with %s\n",
>> +                pam_strerror(pam_handle, rc));
>> +            exit(-1);
>> +        }
>>      }
>>  #endif
>> -
>> -    if (send_audit_message(1, old_context, new_context, ttyn))
>> -        exit(-1);
>> +    if (!ignore_privilage_actions)
>> +        if (send_audit_message(1, old_context, new_context, ttyn))
>> +            exit(-1);
>>      freecon(old_context);
>>  #ifdef NAMESPACE_PRIV
>> -    if (transition_to_caller_uid())
>> -        exit(-1);
>> +    if (!ignore_privilage_actions)
>> +        if (transition_to_caller_uid())
>> +            exit(-1);
>>  #endif
>>      execv(pw->pw_shell, argv + optind - 1);
>>  
>>   
> 
> 
> -- 
> 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.



--
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: [RFC PATCH] newrole suid breakdown

[Michael C Thompson]

Stephen Smalley wrote:
> On Thu, 2006-10-05 at 09:42 -0500, Michael C Thompson wrote:
>> Daniel J Walsh wrote:
>>> Does the code continue to work correctly if I compile in AUDIT_LOG_PRIV 
>>> and NAMESPACE_PRIV but run it without the setuid bit and as a normal 
>>> user.  IE, We want the option to only set this setuid when in an MLS 
>>> environment.  This is not required for targeted or strict policy machines.
>> What happens (currently), is it attempts to drop capabilities and if it 
>> can't do that, newrole fails and exists. The reason I chose this 
>> behavior is, as I figured, if you have compiled newrole with 
>> AUDIT_LOG_PRIV or NAMESPACE_PRIV, you requiring that behaviour.
>>
>> It should be easy to change this behavior, as I expect RH will want a 
>> single compiled binary that can do it all, right? :)
>>
>> Suggested patch attached. If you have a more elegant solution, feel free 
>> to let me know.
> 
> Concerns with this approach:
> 1) The geteuid() test is ambiguous in the case where the caller was
> already uid 0 (i.e. root runs newrole).

Well, this is a really hackish patch. I could integrate this check into 
drop_capabilties, and have it return 0 on success, >0 for ignore 
privileged actions, and <0 on real failure. This way its more consolidated.

 > For dropping capabilities and
> switching uids, this has no real effect since that processing is skipped
> when the caller had uid 0.  But it does have an impact on the audit and
> namespace functionality.

In what way? Either you have the capability to do these actions, or you 
don't, right? I can see 4 cases:
uid == 0 && euid == 0 : you're root
uid == 0 && euid != 0 : you're root with euid non-0
                         (not sure why you would have this)
uid != 0 && euid == 0 : non-root running suid newrole
uid == 0 && euid != 0 : non-root running non-suid newrole

In the case you are root with non-0 euid, this would ignore the 
privileged actions, although I suspect you would still have the 
capabilities to do them (being root and all)?

 > Note btw that the
> pam_open_session/pam_close_session calls don't strictly require such a
> test, since we can always put pam_permit as a session module in the
> default pam config.

Is that something we would want to rely on though?

> 2) I'm not sure how RH plans to set and maintain the suid bit on newrole
> in the LSPP case, since they want to have it off by default.  If another
> package, like the lspp package, modifies the suid bit on newrole, then a
> rpm -V policycoreutils will report a change in mode from the package
> defaults and any update of policycoreutils could reset the mode to the
> original one.
> 



--
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: [RFC PATCH] newrole suid breakdown

[Michael C Thompson]

Stephen Smalley wrote:
> On Wed, 2006-10-04 at 17:17 -0500, Michael C Thompson wrote:
>>  From what I have seen, the points of possible attack (i.e. input) to 
>> newrole include:
>> * command line options
>> * reading the password from stdin
>> * reading the contents of /etc/passwd and /etc/shell
>>
>> I doubt this is an exhaustive list. Since /etc/passwd and /etc/shell are 
>> restricted to root-only write, I've discounted these. Since we require 
>> PAM to do namespaces, which will cause PAM to handle the password check, 
>> we should be able to defer any vulnerabilities to PAM, and I would hope 
>> that PAM is secure.
>>
>> This leaves command line options. These are parsed with getopt_long, and 
>> are currently passed into libselinux without any scrubbing. Since 
>> SELinux doesn't apply a limit on the length of its context strings, is 
>> there any checks that need to be done before handing these strings into 
>> libselinux?
> 
> I don't think so.  The larger concern is not the command line arguments
> but the environment.  glibc does some automatic sanitization of the
> environment for setuid programs, but is limited in what it can do.
> You likely want to save the environment or portions of it for
> propagation to the newrole'd shell, reset the environment to a clean
> state for newrole itself, and then invoke the user shell with the
> previously saved environment.  Look at other setuid programs, like su
> and sudo, as possible examples.   Also see:
> http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/environment-variables.html
> 
> Since pam_namespace further executes a script (namespace.init), the
> environment is especially critical, and you need to consider the
> implications of how the shell handles setuid execution.  For example,
> unless namespace.init invokes /bin/sh with the -p option (privileged
> mode), bash will reset the effective identities to the real identities
> during startup, such that the script itself will run in the caller's
> identity (and thus likely fail if it tries to do its job, like bind
> mounting the .X11-unix directory into the member directory, as well as
> creating any files or directories in the caller's uid).  If you change
> namespace.init to use the -p option, then bash will leave the effective
> identities alone, and will ignore certain aspects of the environment
> that would normally affect it.  One might argue that pam_namespace
> itself should do some environment cleansing, since it executes a script
> and this won't be immediately obvious to all potential users of
> pam_namespace.
> 
>> The cleanup process for errors within newrole currently does not make a 
>> call to pam_end (after pam_start). Is this an issue? There are also 
>> other cases where cleanup isn't really handled as nicely as it could. 
>> Since this is a user-space app, a lot of cleanup is handled by the 
>> kernel, but I do have some patches that go a ways to begin cleaning this 
>> sort of thing up. Are they desirable?
> 
> Yes.

OK, shall I merge these changes into 1 large patch? or how would you 
like this delivered? I can do some seperate smaller patches, which take 
care of cleanup, and then those that take care of adding suid related 
changes.

>> I have tested all 4 ways of compiling newrole (no priv, AUDIT_LOG_PRIV, 
>> NAMESPACE_PRIV, LSPP_PRIV (both)), and it functions as expected.
> 
> Did you check the effective identity and capability sets at each stage
> of execution to see if they were what you expected?  Within newrole
> after each change?  On entry to pam_namespace?  On entry to
> namespace.init?  In the newrole'd shell?

I have run this with debug output (omitted from my patches) and yes the 
capabilities, uid and euid are all as I expected them to be. I did not 
check the capabilities on entrance to namespace.init since I would 
require rebuilding pam_namespace.so, I can do this if desired. From 
looking at the code, it didn't look like pam_namespace did anything to 
the capabilities or uids, but I'm only human, so I could be wrong.

> Also, I'm not clear on how this resolves the issue of being able to have
> a single newrole binary in a distribution, and of not requiring any
> additional exposure for systems that do not require the privileged
> functionality, which Dan also noted.

Yes, I basically glossed over that requirement in my mind.



--
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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Thu, 2006-10-05 at 11:07 -0500, Michael C Thompson wrote:
> >> The cleanup process for errors within newrole currently does not make a 
> >> call to pam_end (after pam_start). Is this an issue? There are also 
> >> other cases where cleanup isn't really handled as nicely as it could. 
> >> Since this is a user-space app, a lot of cleanup is handled by the 
> >> kernel, but I do have some patches that go a ways to begin cleaning this 
> >> sort of thing up. Are they desirable?
> > 
> > Yes.
> 
> OK, shall I merge these changes into 1 large patch? or how would you 
> like this delivered? I can do some seperate smaller patches, which take 
> care of cleanup, and then those that take care of adding suid related 
> changes.

Separated patches are nice, but I suspect it depends on the specifics,
e.g. in some cases you are completely reworking the code path, and thus
cleaning up the old error paths becomes fairly useless when you go to
apply your other changes.

> >> I have tested all 4 ways of compiling newrole (no priv, AUDIT_LOG_PRIV, 
> >> NAMESPACE_PRIV, LSPP_PRIV (both)), and it functions as expected.
> > 
> > Did you check the effective identity and capability sets at each stage
> > of execution to see if they were what you expected?  Within newrole
> > after each change?  On entry to pam_namespace?  On entry to
> > namespace.init?  In the newrole'd shell?
> 
> I have run this with debug output (omitted from my patches) and yes the 
> capabilities, uid and euid are all as I expected them to be. I did not 
> check the capabilities on entrance to namespace.init since I would 
> require rebuilding pam_namespace.so, I can do this if desired. From 
> looking at the code, it didn't look like pam_namespace did anything to 
> the capabilities or uids, but I'm only human, so I could be wrong.

Ok, but it is still the case that bash explicitly resets the effective
uid to the real uid unless invoked with -p as a safety precaution, so
namespace.init runs in the caller's uid.  You don't need to rebuild
pam_namespace to see that - just run 'id' in namespace.init.  You would
need to modify the header line of namespace.init to include -p to avoid
that.

-- 
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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Thu, 2006-10-05 at 10:46 -0500, Michael C Thompson wrote:
> Daniel J Walsh wrote:
> > I think this is all you need.  Audit can handle the fact that it can't 
> > send a message by sending it to /var/log/messages.  And if You use 
> > pam_namespace.so you should fail if you are not setuid and you are not 
> > root.
> 
> True, but right now we are set to abort newrole if those actions are 
> expected to succeed (we have compiled with the respective _PRIV). Should 
> that behavior be changed?

No.

-- 
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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Thu, 2006-10-05 at 10:55 -0500, Michael C Thompson wrote:
> Stephen Smalley wrote:
> > On Thu, 2006-10-05 at 09:42 -0500, Michael C Thompson wrote:
> >> Daniel J Walsh wrote:
> >>> Does the code continue to work correctly if I compile in AUDIT_LOG_PRIV 
> >>> and NAMESPACE_PRIV but run it without the setuid bit and as a normal 
> >>> user.  IE, We want the option to only set this setuid when in an MLS 
> >>> environment.  This is not required for targeted or strict policy machines.
> >> What happens (currently), is it attempts to drop capabilities and if it 
> >> can't do that, newrole fails and exists. The reason I chose this 
> >> behavior is, as I figured, if you have compiled newrole with 
> >> AUDIT_LOG_PRIV or NAMESPACE_PRIV, you requiring that behaviour.
> >>
> >> It should be easy to change this behavior, as I expect RH will want a 
> >> single compiled binary that can do it all, right? :)
> >>
> >> Suggested patch attached. If you have a more elegant solution, feel free 
> >> to let me know.
> > 
> > Concerns with this approach:
> > 1) The geteuid() test is ambiguous in the case where the caller was
> > already uid 0 (i.e. root runs newrole).
> 
> Well, this is a really hackish patch. I could integrate this check into 
> drop_capabilties, and have it return 0 on success, >0 for ignore 
> privileged actions, and <0 on real failure. This way its more consolidated.

That doesn't change the ambiguity in the geteuid() test.  The point is
that if we want to make certain logic in newrole conditional on whether
or not newrole is suid, we can't distinguish in the case where the
caller was already uid 0.  See below.

>  > For dropping capabilities and
> > switching uids, this has no real effect since that processing is skipped
> > when the caller had uid 0.  But it does have an impact on the audit and
> > namespace functionality.
> 
> In what way? Either you have the capability to do these actions, or you 
> don't, right? I can see 4 cases:
> uid == 0 && euid == 0 : you're root

Right.  And per your logic, this means that newrole would always try to
send an audit message and call pam_open/close_session, right?  Even if
newrole was _not_ marked suid, just by virtue of the fact that the
caller was uid 0.  Suppose that I have a non-LSPP system and I run
newrole as root - it will still end up trying to perform those actions.

>  > Note btw that the
> > pam_open_session/pam_close_session calls don't strictly require such a
> > test, since we can always put pam_permit as a session module in the
> > default pam config.
> 
> Is that something we would want to rely on though?

It is cleaner to make the calls unconditional and just let the
configuration vary.  It would be nice if we could do the same thing with
the audit support, just letting libaudit do different things in the
non-LSPP vs. LSPP configurations.

> > 2) I'm not sure how RH plans to set and maintain the suid bit on newrole
> > in the LSPP case, since they want to have it off by default.  If another
> > package, like the lspp package, modifies the suid bit on newrole, then a
> > rpm -V policycoreutils will report a change in mode from the package
> > defaults and any update of policycoreutils could reset the mode to the
> > original one.

This remains a problem AFAICS.

-- 
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: [RFC PATCH] newrole suid breakdown

[Michael C Thompson]

Stephen Smalley wrote:
> On Thu, 2006-10-05 at 10:55 -0500, Michael C Thompson wrote:
>> Stephen Smalley wrote:
>>> On Thu, 2006-10-05 at 09:42 -0500, Michael C Thompson wrote:
>>>> Daniel J Walsh wrote:
>>>>> Does the code continue to work correctly if I compile in AUDIT_LOG_PRIV 
>>>>> and NAMESPACE_PRIV but run it without the setuid bit and as a normal 
>>>>> user.  IE, We want the option to only set this setuid when in an MLS 
>>>>> environment.  This is not required for targeted or strict policy machines.
>>>> What happens (currently), is it attempts to drop capabilities and if it 
>>>> can't do that, newrole fails and exists. The reason I chose this 
>>>> behavior is, as I figured, if you have compiled newrole with 
>>>> AUDIT_LOG_PRIV or NAMESPACE_PRIV, you requiring that behaviour.
>>>>
>>>> It should be easy to change this behavior, as I expect RH will want a 
>>>> single compiled binary that can do it all, right? :)
>>>>
>>>> Suggested patch attached. If you have a more elegant solution, feel free 
>>>> to let me know.
>>> Concerns with this approach:
>>> 1) The geteuid() test is ambiguous in the case where the caller was
>>> already uid 0 (i.e. root runs newrole).
>> Well, this is a really hackish patch. I could integrate this check into 
>> drop_capabilties, and have it return 0 on success, >0 for ignore 
>> privileged actions, and <0 on real failure. This way its more consolidated.
> 
> That doesn't change the ambiguity in the geteuid() test.  The point is
> that if we want to make certain logic in newrole conditional on whether
> or not newrole is suid, we can't distinguish in the case where the
> caller was already uid 0.  See below.
> 
>>  > For dropping capabilities and
>>> switching uids, this has no real effect since that processing is skipped
>>> when the caller had uid 0.  But it does have an impact on the audit and
>>> namespace functionality.
>> In what way? Either you have the capability to do these actions, or you 
>> don't, right? I can see 4 cases:
>> uid == 0 && euid == 0 : you're root
> 
> Right.  And per your logic, this means that newrole would always try to
> send an audit message and call pam_open/close_session, right?  Even if
> newrole was _not_ marked suid, just by virtue of the fact that the
> caller was uid 0.  Suppose that I have a non-LSPP system and I run
> newrole as root - it will still end up trying to perform those actions.
> 
>>  > Note btw that the
>>> pam_open_session/pam_close_session calls don't strictly require such a
>>> test, since we can always put pam_permit as a session module in the
>>> default pam config.
>> Is that something we would want to rely on though?
> 
> It is cleaner to make the calls unconditional and just let the
> configuration vary.  It would be nice if we could do the same thing with
> the audit support, just letting libaudit do different things in the
> non-LSPP vs. LSPP configurations.

What I didn't understand that the desire was to have those actions be 
dependent on the suid state of newrole, and not the ability to perform 
the actions.

It would be possible to stat the newrole binary's permissions to see if 
newrole is suid, but we then need to watch out for the possibility that 
argv[0] isn't correct, so we'd need to address that (could hardcode into 
newrole where it expects to be, for example).

With this approach, we would either attempt to perform the privileged 
actions we've been compiled with, or not. Audit would always be 
performed, and pam_open_session would always be called, but we could 
disable the namespace actions through the pam.d file. Is this approach 
acceptable?

>>> 2) I'm not sure how RH plans to set and maintain the suid bit on newrole
>>> in the LSPP case, since they want to have it off by default.  If another
>>> package, like the lspp package, modifies the suid bit on newrole, then a
>>> rpm -V policycoreutils will report a change in mode from the package
>>> defaults and any update of policycoreutils could reset the mode to the
>>> original one.
> 
> This remains a problem AFAICS.

Yup, can't really weigh in on that though.



--
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: [RFC PATCH] newrole suid breakdown

[Michael C Thompson]

Stephen Smalley wrote:
> On Wed, 2006-10-04 at 17:17 -0500, Michael C Thompson wrote:
>>  From what I have seen, the points of possible attack (i.e. input) to 
>> newrole include:
>> * command line options
>> * reading the password from stdin
>> * reading the contents of /etc/passwd and /etc/shell
>>
>> I doubt this is an exhaustive list. Since /etc/passwd and /etc/shell are 
>> restricted to root-only write, I've discounted these. Since we require 
>> PAM to do namespaces, which will cause PAM to handle the password check, 
>> we should be able to defer any vulnerabilities to PAM, and I would hope 
>> that PAM is secure.
>>
>> This leaves command line options. These are parsed with getopt_long, and 
>> are currently passed into libselinux without any scrubbing. Since 
>> SELinux doesn't apply a limit on the length of its context strings, is 
>> there any checks that need to be done before handing these strings into 
>> libselinux?
> 
> I don't think so.  The larger concern is not the command line arguments
> but the environment.  glibc does some automatic sanitization of the
> environment for setuid programs, but is limited in what it can do.
> You likely want to save the environment or portions of it for
> propagation to the newrole'd shell, reset the environment to a clean
> state for newrole itself, and then invoke the user shell with the
> previously saved environment.  Look at other setuid programs, like su
> and sudo, as possible examples.   Also see:
> http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/environment-variables.html

Working towards this now. I am mimicing the behavior of su. By the time 
we invoke pam_namespace, the environment will be scrubbed clean and will 
contain a limited set of values (stardard shell related HOME, PATH, 
TERM, etc). PATH will be set to a set of trusted paths like 
/bin:/usr/bin, etc.

> Since pam_namespace further executes a script (namespace.init), the
> environment is especially critical, and you need to consider the
> implications of how the shell handles setuid execution.  For example,
> unless namespace.init invokes /bin/sh with the -p option (privileged
> mode), bash will reset the effective identities to the real identities
> during startup, such that the script itself will run in the caller's
> identity (and thus likely fail if it tries to do its job, like bind
> mounting the .X11-unix directory into the member directory, as well as
> creating any files or directories in the caller's uid).  If you change
> namespace.init to use the -p option, then bash will leave the effective
> identities alone, and will ignore certain aspects of the environment
> that would normally affect it.  One might argue that pam_namespace
> itself should do some environment cleansing, since it executes a script
> and this won't be immediately obvious to all potential users of
> pam_namespace.

So I take it changing namespace.init is an acceptable solution?

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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Thu, 2006-10-05 at 14:53 -0500, Michael C Thompson wrote:
> Stephen Smalley wrote:
> > It is cleaner to make the calls unconditional and just let the
> > configuration vary.  It would be nice if we could do the same thing with
> > the audit support, just letting libaudit do different things in the
> > non-LSPP vs. LSPP configurations.
> 
> What I didn't understand that the desire was to have those actions be 
> dependent on the suid state of newrole, and not the ability to perform 
> the actions.
> 
> It would be possible to stat the newrole binary's permissions to see if 
> newrole is suid, but we then need to watch out for the possibility that 
> argv[0] isn't correct, so we'd need to address that (could hardcode into 
> newrole where it expects to be, for example).

Not a good idea - too susceptible to caller subversion or a simple
mistake leading to newrole failing to drop capabilities and/or skipping
security-relevant processing.

The best option is to make the calls to audit and to pam be
unconditional and provide a way in audit and pam configuration to
effectively make them no-ops (already possible for pam via pam_permit).
Then only the capability dropping and uid switching has to be
conditional, and that processing can be based on whether newrole is able
to perform those actions.

If that isn't possible for audit, then I guess we just live with audit
always being generated when the caller has euid 0, regardless of whether
newrole was suid.  That is better than the alternative.

> >>> 2) I'm not sure how RH plans to set and maintain the suid bit on newrole
> >>> in the LSPP case, since they want to have it off by default.  If another
> >>> package, like the lspp package, modifies the suid bit on newrole, then a
> >>> rpm -V policycoreutils will report a change in mode from the package
> >>> defaults and any update of policycoreutils could reset the mode to the
> >>> original one.
> > 
> > This remains a problem AFAICS.
> 
> Yup, can't really weigh in on that though.

Right.  Dan?  Steve?  

-- 
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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Thu, 2006-10-05 at 15:10 -0500, Michael C Thompson wrote:
> Stephen Smalley wrote:
> > On Wed, 2006-10-04 at 17:17 -0500, Michael C Thompson wrote:
> >>  From what I have seen, the points of possible attack (i.e. input) to 
> >> newrole include:
> >> * command line options
> >> * reading the password from stdin
> >> * reading the contents of /etc/passwd and /etc/shell
> >>
> >> I doubt this is an exhaustive list. Since /etc/passwd and /etc/shell are 
> >> restricted to root-only write, I've discounted these. Since we require 
> >> PAM to do namespaces, which will cause PAM to handle the password check, 
> >> we should be able to defer any vulnerabilities to PAM, and I would hope 
> >> that PAM is secure.
> >>
> >> This leaves command line options. These are parsed with getopt_long, and 
> >> are currently passed into libselinux without any scrubbing. Since 
> >> SELinux doesn't apply a limit on the length of its context strings, is 
> >> there any checks that need to be done before handing these strings into 
> >> libselinux?
> > 
> > I don't think so.  The larger concern is not the command line arguments
> > but the environment.  glibc does some automatic sanitization of the
> > environment for setuid programs, but is limited in what it can do.
> > You likely want to save the environment or portions of it for
> > propagation to the newrole'd shell, reset the environment to a clean
> > state for newrole itself, and then invoke the user shell with the
> > previously saved environment.  Look at other setuid programs, like su
> > and sudo, as possible examples.   Also see:
> > http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/environment-variables.html
> 
> Working towards this now. I am mimicing the behavior of su. By the time 
> we invoke pam_namespace, the environment will be scrubbed clean and will 
> contain a limited set of values (stardard shell related HOME, PATH, 
> TERM, etc). PATH will be set to a set of trusted paths like 
> /bin:/usr/bin, etc.

Ok.  Are you restoring the original environment or some portion of it
for the newrole'd shell?  su has different options there for handling
environment depending on how it is called.

> > Since pam_namespace further executes a script (namespace.init), the
> > environment is especially critical, and you need to consider the
> > implications of how the shell handles setuid execution.  For example,
> > unless namespace.init invokes /bin/sh with the -p option (privileged
> > mode), bash will reset the effective identities to the real identities
> > during startup, such that the script itself will run in the caller's
> > identity (and thus likely fail if it tries to do its job, like bind
> > mounting the .X11-unix directory into the member directory, as well as
> > creating any files or directories in the caller's uid).  If you change
> > namespace.init to use the -p option, then bash will leave the effective
> > identities alone, and will ignore certain aspects of the environment
> > that would normally affect it.  One might argue that pam_namespace
> > itself should do some environment cleansing, since it executes a script
> > and this won't be immediately obvious to all potential users of
> > pam_namespace.
> 
> So I take it changing namespace.init is an acceptable solution?

Yes, I think so.

-- 
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: [RFC PATCH] newrole suid breakdown

[Michael C Thompson]

Stephen Smalley wrote:
> On Thu, 2006-10-05 at 15:10 -0500, Michael C Thompson wrote:
>> Stephen Smalley wrote:
>>> On Wed, 2006-10-04 at 17:17 -0500, Michael C Thompson wrote:
>>>>  From what I have seen, the points of possible attack (i.e. input) to 
>>>> newrole include:
>>>> * command line options
>>>> * reading the password from stdin
>>>> * reading the contents of /etc/passwd and /etc/shell
>>>>
>>>> I doubt this is an exhaustive list. Since /etc/passwd and /etc/shell are 
>>>> restricted to root-only write, I've discounted these. Since we require 
>>>> PAM to do namespaces, which will cause PAM to handle the password check, 
>>>> we should be able to defer any vulnerabilities to PAM, and I would hope 
>>>> that PAM is secure.
>>>>
>>>> This leaves command line options. These are parsed with getopt_long, and 
>>>> are currently passed into libselinux without any scrubbing. Since 
>>>> SELinux doesn't apply a limit on the length of its context strings, is 
>>>> there any checks that need to be done before handing these strings into 
>>>> libselinux?
>>> I don't think so.  The larger concern is not the command line arguments
>>> but the environment.  glibc does some automatic sanitization of the
>>> environment for setuid programs, but is limited in what it can do.
>>> You likely want to save the environment or portions of it for
>>> propagation to the newrole'd shell, reset the environment to a clean
>>> state for newrole itself, and then invoke the user shell with the
>>> previously saved environment.  Look at other setuid programs, like su
>>> and sudo, as possible examples.   Also see:
>>> http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/environment-variables.html
>> Working towards this now. I am mimicing the behavior of su. By the time 
>> we invoke pam_namespace, the environment will be scrubbed clean and will 
>> contain a limited set of values (stardard shell related HOME, PATH, 
>> TERM, etc). PATH will be set to a set of trusted paths like 
>> /bin:/usr/bin, etc.
> 
> Ok.  Are you restoring the original environment or some portion of it
> for the newrole'd shell?  su has different options there for handling
> environment depending on how it is called.

Just a portion. I can implement the su functionality to handle these 
scenarios different as well if you like.

>>> Since pam_namespace further executes a script (namespace.init), the
>>> environment is especially critical, and you need to consider the
>>> implications of how the shell handles setuid execution.  For example,
>>> unless namespace.init invokes /bin/sh with the -p option (privileged
>>> mode), bash will reset the effective identities to the real identities
>>> during startup, such that the script itself will run in the caller's
>>> identity (and thus likely fail if it tries to do its job, like bind
>>> mounting the .X11-unix directory into the member directory, as well as
>>> creating any files or directories in the caller's uid).  If you change
>>> namespace.init to use the -p option, then bash will leave the effective
>>> identities alone, and will ignore certain aspects of the environment
>>> that would normally affect it.  One might argue that pam_namespace
>>> itself should do some environment cleansing, since it executes a script
>>> and this won't be immediately obvious to all potential users of
>>> pam_namespace.
>> So I take it changing namespace.init is an acceptable solution?
> 
> Yes, I think so.

Will send a patch to the list.



--
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: [RFC PATCH] newrole suid breakdown

[Michael C Thompson]

Stephen Smalley wrote:
> On Thu, 2006-10-05 at 14:53 -0500, Michael C Thompson wrote:
>> Stephen Smalley wrote:
>>> It is cleaner to make the calls unconditional and just let the
>>> configuration vary.  It would be nice if we could do the same thing with
>>> the audit support, just letting libaudit do different things in the
>>> non-LSPP vs. LSPP configurations.
>> What I didn't understand that the desire was to have those actions be 
>> dependent on the suid state of newrole, and not the ability to perform 
>> the actions.
>>
>> It would be possible to stat the newrole binary's permissions to see if 
>> newrole is suid, but we then need to watch out for the possibility that 
>> argv[0] isn't correct, so we'd need to address that (could hardcode into 
>> newrole where it expects to be, for example).
> 
> Not a good idea - too susceptible to caller subversion or a simple
> mistake leading to newrole failing to drop capabilities and/or skipping
> security-relevant processing.
> 
> The best option is to make the calls to audit and to pam be
> unconditional and provide a way in audit and pam configuration to
> effectively make them no-ops (already possible for pam via pam_permit).
> Then only the capability dropping and uid switching has to be
> conditional, and that processing can be based on whether newrole is able
> to perform those actions.
> 
> If that isn't possible for audit, then I guess we just live with audit
> always being generated when the caller has euid 0, regardless of whether
> newrole was suid.  That is better than the alternative.

OK, so this is rather ugly in general, and I don't really like where 
this is heading. However, I do understand the desire to have the 
multi-purpose binary. So here's my thoughts:

Making calls to pam_open_session can work with minor modifications to 
the config file, so there is not a big concern there.

AFAIK, we can't call audit without getting a failure, and I would really 
rather not suppress those errors. It would be possible to add a check to 
make sure that either we have CAP_AUDIT_WRITE or euid=0 or something, 
but I'm not really fond of that.

RedHat: is there going to be a scenario where you are sending out this 
package on a system which doesn't have an audit-aware kernel?

If so, we can probably do the euid check and if euid is non-zero, we 
skip calling to audit. The fallout of that is you would see audit 
records when root, and only root, uses newrole. Again, I am not fond of 
this solution.

Is there no sane way to check if an app is suid? Because this would 
relieve some of the headaches from this.

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: [RFC PATCH] newrole suid breakdown

[Steve Grubb]

On Thursday 05 October 2006 16:47, Michael C Thompson wrote:
> AFAIK, we can't call audit without getting a failure, and I would really
> rather not suppress those errors.

There is a library function get_auditfail_action where admins can say what the 
expected behavior should be. There is a man page for it.

However, why would sending an audit message fail? newrole is setuid, that's 
why I did a code review last winter...and we can do another code review if 
people still aren't sure. pam is already used in several setuid programs, so 
I hope that is not the issue.

> It would be possible to add a check to make sure that either we have
> CAP_AUDIT_WRITE 

This is something simple to do and would solve your problem. 

> or euid=0 or something, but I'm not really fond of that.

By checking euid, you are really hoping that 0 has CAP_AUDIT_WRITE, so why not 
check the capability since that's what matters.

> RedHat: is there going to be a scenario where you are sending out this
> package on a system which doesn't have an audit-aware kernel?

No.

> If so, we can probably do the euid check and if euid is non-zero, we
> skip calling to audit. The fallout of that is you would see audit
> records when root, and only root, uses newrole. Again, I am not fond of
> this solution.

Me neither.

> Is there no sane way to check if an app is suid? Because this would
> relieve some of the headaches from this.

I don't think checking suid is the right thing. Checking the capability is.

-Steve

--
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: [RFC PATCH] newrole suid breakdown

[Russell Coker]

On Thursday 05 October 2006 23:57, Daniel J Walsh <dwalsh@redhat.com> wrote:
> Does the code continue to work correctly if I compile in AUDIT_LOG_PRIV
> and NAMESPACE_PRIV but run it without the setuid bit and as a normal
> user.  IE, We want the option to only set this setuid when in an MLS
> environment.  This is not required for targeted or strict policy machines.

Who does "we" mean in this context?

I would like to have newrole work with namespaces in a strict policy 
environment!

-- 
russell@coker.com.au
http://etbe.blogspot.com/          My Blog

http://www.coker.com.au/sponsorship.html Sponsoring Free Software development


--
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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Thu, 2006-10-05 at 17:48 -0400, Steve Grubb wrote:
> On Thursday 05 October 2006 16:47, Michael C Thompson wrote:
> > AFAIK, we can't call audit without getting a failure, and I would really
> > rather not suppress those errors.
> 
> There is a library function get_auditfail_action where admins can say what the 
> expected behavior should be. There is a man page for it.

Ok, so newrole can call get_auditfail_action() upon an audit_open()
failure, and return 0 from send_audit_message() if the failmode is
FAIL_IGNORE.  Then a stock RHEL 5 system can have 'failure_action =
ignore' in /etc/libaudit.conf and 'session required pam_permit.so'
in /etc/pam.d/newrole, and newrole can unconditionally invoke the audit
calls and pam_open/close_session calls.  And the LSPP package can
replace /etc/libaudit.conf to change the failure_action and can
replace /etc/pam.d/newrole to change the session module to
pam_namespace.

> However, why would sending an audit message fail? newrole is setuid, that's 
> why I did a code review last winter...and we can do another code review if 
> people still aren't sure. pam is already used in several setuid programs, so 
> I hope that is not the issue.

Dan had suggested only making newrole suid under the LSPP configuration,
leaving it non-suid otherwise, and having newrole dynamically test
whether it is suid (which it cannot actually do precisely) to decide
whether or not to perform privileged operations.  This was motivated by
the desire to not expose non-LSPP systems (particularly a stock system
with targeted policy) to greater risk, since the changes to newrole for
pam_namespace leave it much more privileged than your earlier changes
for audit.  Unlike the audit support, pam_namespace requires retaining
several powerful capabilities (CAP_SYS_ADMIN, CAP_DAC_OVERRIDE, etc) and
euid 0 for most of the lifetime of newrole (until after pam_namespace
runs, which occurs late).

No one has yet commented on the concern I raised about whether setting
the suid bit on newrole in this manner is workable from a packaging and
maintenance point of view (if policycoreutils installs a non-suid
newrole and the lspp package makes it suid from a scriptlet, then rpm -V
policycoreutils will report variance in the file mode, and an update of
policycoreutils will reset the newrole mode back to non-suid).  That
seems a bit problematic to me.

Russell had alternatively suggested moving newrole into its own package
and only installing it (always suid) if installing a -strict or -mls
policy (i.e. make it a dependency of those policy packages), so that a
default install does not include it at all (as -targeted has no need for
newrole functionality).   Concerns there might include: a) mere
installation of -strict or -mls on a system does not mean that they are
the active policy, so one could still end up with the suid newrole on a
machine running targeted, b) not all users of -strict necessarily want
pam_namespace functionality.   Also not sure about whether that poses
any packaging or dependency issues.

I had earlier suggested having newrole somehow perform a runtime check
of whether pam_namespace is in its pam config, and dropping all
capabilities other than CAP_AUDIT_WRITE if it is not present.  But I
don't see any PAM interface for getting such information, and it seems
to violate pam encapsulation.

> > Is there no sane way to check if an app is suid? Because this would
> > relieve some of the headaches from this.
> 
> I don't think checking suid is the right thing. Checking the capability is.

Possibly, but if we can just call audit_open() unconditionally and use
get_auditfail_action() to decide how to handle an error, then I don't
think we need to do any checking of suid or capability first.  At that
point, we only need to bracket the code that tries to drop capabilities
and revert the uid with a check of whether the process is privileged.

On the question of checking whether an app is suid, I don't see any way
to do that in Linux unambiguously, since there is always the case where
the caller was already in the same uid as the file.

-- 
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: [RFC PATCH] newrole suid breakdown

[Russell Coker]

On Saturday 07 October 2006 00:52, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> Russell had alternatively suggested moving newrole into its own package
> and only installing it (always suid) if installing a -strict or -mls
> policy (i.e. make it a dependency of those policy packages), so that a
> default install does not include it at all (as -targeted has no need for
> newrole functionality).   Concerns there might include: a) mere
> installation of -strict or -mls on a system does not mean that they are
> the active policy, so one could still end up with the suid newrole on a
> machine running targeted, b) not all users of -strict necessarily want
> pam_namespace functionality.   Also not sure about whether that poses
> any packaging or dependency issues.

You are correct that if my suggestion is followed then a very small portion of 
the machines running targeted policy or without SE Linux will have newrole 
installed setuid-root.

But let's put things in perspective.  A typical install of Fedora has about 30 
setuid-root programs (mine has 36).  The same typical Fedora install can be 
expected to have some setgid programs that give access to important groups 
and some possible indirect chains of execution that lead from non-root to 
root.

Of those programs many of them are of old implementation or old design, for 
example when writing this message I realised that I had several setuid-root 
programs from the rsh package.  I had never desired rsh to be installed, it 
must have either been dragged in by dependencies or be part of a default 
install.

Also there are a number of daemons that run as root (dhclient and cups are on 
most workstations).

I agree that we don't want to reduce security through making a mistake, but 
there are plenty of better avenues of attack than newrole.  It might be a 
better use of our time to just accept that a few people will have a 
setuid-root copy of newrole installed when they don't need it and save our 
time for other things.  After all most Fedora users get a kerberos version of 
su installed when they don't need it.

-- 
russell@coker.com.au
http://etbe.blogspot.com/          My Blog

http://www.coker.com.au/sponsorship.html Sponsoring Free Software development


--
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: [RFC PATCH] newrole suid breakdown

[Linda Knippers]

Stephen Smalley wrote:
<snip>

> No one has yet commented on the concern I raised about whether setting
> the suid bit on newrole in this manner is workable from a packaging and
> maintenance point of view (if policycoreutils installs a non-suid
> newrole and the lspp package makes it suid from a scriptlet, then rpm -V
> policycoreutils will report variance in the file mode, and an update of
> policycoreutils will reset the newrole mode back to non-suid).  That
> seems a bit problematic to me.

This may be a general problem if we do for LSPP what we did for CAPP,
which was strip suid/sgid bits off some programs and change the mode
bits to only allow the owner (root) to execute others.  The kickstart
script Klaus posted still does this.

I think this was something George was dealing with as part of the
self-test script.  If the self-test uses rpm -V, it would need to
check that the variance is expected.  Now sure what to do about the
update case but with CAPP the configuration script could be re-run at any time
to re-do the original changes so maybe this part of the problem is solved
with documentation.

-- ljk

--
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: [RFC PATCH] newrole suid breakdown

[Michael C Thompson]

Stephen Smalley wrote:
> On the question of checking whether an app is suid, I don't see any way
> to do that in Linux unambiguously, since there is always the case where
> the caller was already in the same uid as the file.

Apparently, there is a way to check. Klaus showed me this yesterday, but 
he pointed out that it might not be accurate due to SELinux transitions. 
I have tried this, it doesn't seem like the context of the executable 
(for transitions) affects this value.

#include <stdio.h>

extern int __libc_enable_secure;

int main() {
	printf("%d\n", __libc_enable_secure);
}

If someone can also confirm this behavior, this could be the solution 
we're looking for. Not sure how acceptable it is though, as it is kinda 
hackish.

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: [RFC PATCH] newrole suid breakdown

[Steve Grubb]

On Friday 06 October 2006 10:52, Stephen Smalley wrote:
> > There is a library function get_auditfail_action where admins can say
> > what the expected behavior should be. There is a man page for it.
>
> Ok, so newrole can call get_auditfail_action() upon an audit_open()
> failure, and return 0 from send_audit_message() if the failmode is
> FAIL_IGNORE.  Then a stock RHEL 5 system can have 'failure_action =
> ignore' in /etc/libaudit.conf and 'session required pam_permit.so'
> in /etc/pam.d/newrole, and newrole can unconditionally invoke the audit
> calls and pam_open/close_session calls. 

Yes, if that's the way we want to do this.

> > However, why would sending an audit message fail? newrole is setuid,
> > that's why I did a code review last winter...and we can do another code
> > review if people still aren't sure. pam is already used in several setuid
> > programs, so I hope that is not the issue.
>
> Dan had suggested only making newrole suid under the LSPP configuration,
> leaving it non-suid otherwise, and having newrole dynamically test
> whether it is suid (which it cannot actually do precisely) to decide
> whether or not to perform privileged operations.

It can easily test for CAP_SYS_ADMIN, CAP_DAC_OVERRIDE.

> This was motivated by the desire to not expose non-LSPP systems
> (particularly a stock system with targeted policy) to greater risk, since
> the changes to newrole for pam_namespace leave it much more privileged than
> your earlier changes for audit.

So, in a higher assurance deployment we will have something more powerful that 
is not being used by the majority of users? If its dangerous for general 
deployment, it would be dangerous for higher assurance, too.

> Unlike the audit support, pam_namespace requires retaining several powerful
> capabilities (CAP_SYS_ADMIN, CAP_DAC_OVERRIDE, etc) and 
> euid 0 for most of the lifetime of newrole (until after pam_namespace
> runs, which occurs late).

You know, it occurs to me that we would not need newrole if we were able to 
chose a role at login. We used to be able to do that a long time ago. Maybe 
that solves all the problems? Need to change roles...log out and log in.

> No one has yet commented on the concern I raised about whether setting
> the suid bit on newrole in this manner is workable from a packaging and
> maintenance point of view (if policycoreutils installs a non-suid
> newrole and the lspp package makes it suid from a scriptlet, then rpm -V
> policycoreutils will report variance in the file mode, and an update of
> policycoreutils will reset the newrole mode back to non-suid).  That
> seems a bit problematic to me.

We already have that issue. The script takes the setuid bit from several apps. 
However, we are pulling AIDE into RHEL5 and we will be using that for 
Integrity checking part of the RBAC self-test.

> > I don't think checking suid is the right thing. Checking the capability
> > is.
>
> Possibly, but if we can just call audit_open() unconditionally and use
> get_auditfail_action() to decide how to handle an error, then I don't
> think we need to do any checking of suid or capability first.  

What if they don't have audit compiled into their kernel but want 
pam_namespace? Weird, but people are like that.

> On the question of checking whether an app is suid, I don't see any way
> to do that in Linux unambiguously, since there is always the case where
> the caller was already in the same uid as the file.

The check for setuid is mistakenly a check for capabilities.

-Steve


--
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: [RFC PATCH] newrole suid breakdown

[Steve Grubb]

On Friday 06 October 2006 11:22, Michael C Thompson wrote:
> If someone can also confirm this behavior, this could be the solution
> we're looking for. 

Have you checked it with and without selinux enabled? Besides, why can't you 
just check the capabilities?

-Steve

--
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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Fri, 2006-10-06 at 10:22 -0500, Michael C Thompson wrote:
> Stephen Smalley wrote:
> > On the question of checking whether an app is suid, I don't see any way
> > to do that in Linux unambiguously, since there is always the case where
> > the caller was already in the same uid as the file.
> 
> Apparently, there is a way to check. Klaus showed me this yesterday, but 
> he pointed out that it might not be accurate due to SELinux transitions. 
> I have tried this, it doesn't seem like the context of the executable 
> (for transitions) affects this value.
> 
> #include <stdio.h>
> 
> extern int __libc_enable_secure;
> 
> int main() {
> 	printf("%d\n", __libc_enable_secure);
> }
> 
> If someone can also confirm this behavior, this could be the solution 
> we're looking for. Not sure how acceptable it is though, as it is kinda 
> hackish.

No, that has the same ambiguity; if the caller already had uid 0, then
__libc_enable_secure will be 0 (it is set based on the AT_SECURE value
provided by the kernel, and the kernel is just comparing the euid
against the uid and the egid against the gid, plus the SELinux check).

Context transitions also cause it to be set unless a particular
permission is allowed between the caller and the new domain (naturally
you have to be in enforcing mode too).

-- 
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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Fri, 2006-10-06 at 11:34 -0400, Steve Grubb wrote:
> On Friday 06 October 2006 10:52, Stephen Smalley wrote:
> > Dan had suggested only making newrole suid under the LSPP configuration,
> > leaving it non-suid otherwise, and having newrole dynamically test
> > whether it is suid (which it cannot actually do precisely) to decide
> > whether or not to perform privileged operations.
> 
> It can easily test for CAP_SYS_ADMIN, CAP_DAC_OVERRIDE.

True.  That still has the same issue of yielding a "false" positive when
the caller was already uid 0.

> > This was motivated by the desire to not expose non-LSPP systems
> > (particularly a stock system with targeted policy) to greater risk, since
> > the changes to newrole for pam_namespace leave it much more privileged than
> > your earlier changes for audit.
> 
> So, in a higher assurance deployment we will have something more powerful that 
> is not being used by the majority of users? If its dangerous for general 
> deployment, it would be dangerous for higher assurance, too.

That's why Michael is working on making newrole more paranoid about its
caller.  But in the "general deployment" there is no functional need for
a privileged newrole, so there is no benefit to making it privileged
there and there is risk.  The risk in fact is _greater_ in the general
deployment if you make newrole privileged because neither newrole nor
its caller are confined in -targeted policy, so you have _only_ DAC
protections and you've just removed almost all DAC restrictions from
newrole.  Whereas under -strict or -mls, you further have SELinux
protection/isolation of newrole's domain, and the caller is much more
limited.  

> You know, it occurs to me that we would not need newrole if we were able to 
> chose a role at login. We used to be able to do that a long time ago. Maybe 
> that solves all the problems? Need to change roles...log out and log in.

Well, I would certainly be glad to see such support restored and
expanded (e.g. to include gdm).  But there was resistance to that from
RH in the past.

> > Possibly, but if we can just call audit_open() unconditionally and use
> > get_auditfail_action() to decide how to handle an error, then I don't
> > think we need to do any checking of suid or capability first.  
> 
> What if they don't have audit compiled into their kernel but want 
> pam_namespace? Weird, but people are like that.

Not sure why that presents a problem if newrole unconditionally calls
audit_open() but uses get_auditfail_action() to decide how to handle an
error (in that case, they presumably have it configured to ignore
failures since they built their kernel without support for it).  

-- 
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: [RFC PATCH] newrole suid breakdown

[Daniel J Walsh]

Russell Coker wrote:
> On Thursday 05 October 2006 23:57, Daniel J Walsh <dwalsh@redhat.com> wrote:
>   
>> Does the code continue to work correctly if I compile in AUDIT_LOG_PRIV
>> and NAMESPACE_PRIV but run it without the setuid bit and as a normal
>> user.  IE, We want the option to only set this setuid when in an MLS
>> environment.  This is not required for targeted or strict policy machines.
>>     
>
> Who does "we" mean in this context?
>
> I would like to have newrole work with namespaces in a strict policy 
> environment!
>
>   
I am not denying you that right.  I am asking for the tool to continue 
working with or without setuid.
IE Don't force a setuid app on the OS, if I don't do pam_namespace or 
care about role auditing.

We means Red Hat/Fedora.



--
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: [RFC PATCH] newrole suid breakdown

[Daniel J Walsh]

Stephen Smalley wrote:
> On Fri, 2006-10-06 at 11:34 -0400, Steve Grubb wrote:
>   
>> On Friday 06 October 2006 10:52, Stephen Smalley wrote:
>>     
>>> Dan had suggested only making newrole suid under the LSPP configuration,
>>> leaving it non-suid otherwise, and having newrole dynamically test
>>> whether it is suid (which it cannot actually do precisely) to decide
>>> whether or not to perform privileged operations.
>>>       
>> It can easily test for CAP_SYS_ADMIN, CAP_DAC_OVERRIDE.
>>     
>
> True.  That still has the same issue of yielding a "false" positive when
> the caller was already uid 0.
>
>   
>>> This was motivated by the desire to not expose non-LSPP systems
>>> (particularly a stock system with targeted policy) to greater risk, since
>>> the changes to newrole for pam_namespace leave it much more privileged than
>>> your earlier changes for audit.
>>>       
>> So, in a higher assurance deployment we will have something more powerful that 
>> is not being used by the majority of users? If its dangerous for general 
>> deployment, it would be dangerous for higher assurance, too.
>>     
>
> That's why Michael is working on making newrole more paranoid about its
> caller.  But in the "general deployment" there is no functional need for
> a privileged newrole, so there is no benefit to making it privileged
> there and there is risk.  The risk in fact is _greater_ in the general
> deployment if you make newrole privileged because neither newrole nor
> its caller are confined in -targeted policy, so you have _only_ DAC
> protections and you've just removed almost all DAC restrictions from
> newrole.  Whereas under -strict or -mls, you further have SELinux
> protection/isolation of newrole's domain, and the caller is much more
> limited.  
>
>   
>> You know, it occurs to me that we would not need newrole if we were able to 
>> chose a role at login. We used to be able to do that a long time ago. Maybe 
>> that solves all the problems? Need to change roles...log out and log in.
>>     
>
> Well, I would certainly be glad to see such support restored and
> expanded (e.g. to include gdm).  But there was resistance to that from
> RH in the past.
>   
This causes problems with Root apps that use su to transition to a 
different uid, which prompted the creation of runuser, and end up with 
the app frozen waiting for the file context to be chosen.    Since su no 
longer uses pam_selinux this is less of a problem.  Also the multiple 
flag sometimes returned login domains that made no sense.  I would not 
have a problem with adding it back, but I would prefer it turned off by 
default.  So MLS/Strict people could turn it on, but targeted would 
leave it off.

>   
>>> Possibly, but if we can just call audit_open() unconditionally and use
>>> get_auditfail_action() to decide how to handle an error, then I don't
>>> think we need to do any checking of suid or capability first.  
>>>       
>> What if they don't have audit compiled into their kernel but want 
>> pam_namespace? Weird, but people are like that.
>>     
>
> Not sure why that presents a problem if newrole unconditionally calls
> audit_open() but uses get_auditfail_action() to decide how to handle an
> error (in that case, they presumably have it configured to ignore
> failures since they built their kernel without support for it).  
>
>   


--
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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Fri, 2006-10-06 at 13:08 -0400, Daniel J Walsh wrote:
> > Well, I would certainly be glad to see such support restored and
> > expanded (e.g. to include gdm).  But there was resistance to that from
> > RH in the past.
> >   
> This causes problems with Root apps that use su to transition to a 
> different uid, which prompted the creation of runuser, and end up with 
> the app frozen waiting for the file context to be chosen.    Since su no 
> longer uses pam_selinux this is less of a problem.  Also the multiple 
> flag sometimes returned login domains that made no sense.  I would not 
> have a problem with adding it back, but I would prefer it turned off by 
> default.  So MLS/Strict people could turn it on, but targeted would 
> leave it off.

We'd need similar support in sshd, and possibly in gdm (although that is
likely out of scope for LSPP itself).  There was an old gdm patch a long
time ago for selinux that allowed context selection there.

-- 
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: [RFC PATCH] newrole suid breakdown

[Russell Coker]

On Saturday 07 October 2006 03:01, Daniel J Walsh <dwalsh@redhat.com> wrote:
> Russell Coker wrote:
> > On Thursday 05 October 2006 23:57, Daniel J Walsh <dwalsh@redhat.com> 
wrote:
> >> Does the code continue to work correctly if I compile in AUDIT_LOG_PRIV
> >> and NAMESPACE_PRIV but run it without the setuid bit and as a normal
> >> user.  IE, We want the option to only set this setuid when in an MLS
> >> environment.  This is not required for targeted or strict policy
> >> machines.
> >
> > Who does "we" mean in this context?
> >
> > I would like to have newrole work with namespaces in a strict policy
> > environment!
>
> I am not denying you that right.  I am asking for the tool to continue
> working with or without setuid.

Without setuid means without poly-instantiation based on SE Linux context, 
which means that probably most strict policy systems won't be able to 
effectively use poly-instantiation.

> IE Don't force a setuid app on the OS, if I don't do pam_namespace or
> care about role auditing.

/usr/kerberos/bin/ksu is forced on the OS even though the vast majority of 
Fedora users will never use Kerberos.

/usr/sbin/ccreds_validate seems to always get installed even on systems that 
will never use network authentication (again the majority).

/usr/libexec/openssh/ssh-keysign is always installed even though it's 
generally recommended that you don't use host based authentication (and my 
observation is that almost no-one is using it).

The rsh package has three setuid root programs and again is almost never 
needed (in fact it's recommended that you don't have it for several reasons).


Without even trying I've found six setuid-root programs that are included in a 
fairly default install of Fedora and which are never needed by the vast 
majority of users.  I doubt that all six are as well audited as newrole.

It seems that the decision to force setuid programs on the OS has already been 
made.

-- 
russell@coker.com.au
http://etbe.blogspot.com/          My Blog

http://www.coker.com.au/sponsorship.html Sponsoring Free Software development

--
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: [RFC PATCH] newrole suid breakdown

[Daniel J Walsh]

Russell Coker wrote:
> On Saturday 07 October 2006 03:01, Daniel J Walsh <dwalsh@redhat.com> wrote:
>   
>> Russell Coker wrote:
>>     
>>> On Thursday 05 October 2006 23:57, Daniel J Walsh <dwalsh@redhat.com> 
>>>       
> wrote:
>   
>>>> Does the code continue to work correctly if I compile in AUDIT_LOG_PRIV
>>>> and NAMESPACE_PRIV but run it without the setuid bit and as a normal
>>>> user.  IE, We want the option to only set this setuid when in an MLS
>>>> environment.  This is not required for targeted or strict policy
>>>> machines.
>>>>         
>>> Who does "we" mean in this context?
>>>
>>> I would like to have newrole work with namespaces in a strict policy
>>> environment!
>>>       
>> I am not denying you that right.  I am asking for the tool to continue
>> working with or without setuid.
>>     
>
> Without setuid means without poly-instantiation based on SE Linux context, 
> which means that probably most strict policy systems won't be able to 
> effectively use poly-instantiation.
>
>   
>> IE Don't force a setuid app on the OS, if I don't do pam_namespace or
>> care about role auditing.
>>     
>
> /usr/kerberos/bin/ksu is forced on the OS even though the vast majority of 
> Fedora users will never use Kerberos.
>
> /usr/sbin/ccreds_validate seems to always get installed even on systems that 
> will never use network authentication (again the majority).
>
> /usr/libexec/openssh/ssh-keysign is always installed even though it's 
> generally recommended that you don't use host based authentication (and my 
> observation is that almost no-one is using it).
>
> The rsh package has three setuid root programs and again is almost never 
> needed (in fact it's recommended that you don't have it for several reasons).
>
>
> Without even trying I've found six setuid-root programs that are included in a 
> fairly default install of Fedora and which are never needed by the vast 
> majority of users.  I doubt that all six are as well audited as newrole.
>
> It seems that the decision to force setuid programs on the OS has already been 
> made.
>
>   

Ok, After talking to people around here, I want to  allow newrole to be 
setuid, but I want to remove it from policycoreutils and move it to 
policycoreutils-newrole, then I will require policycoreutils-newrole for 
mls and strict policy.

Dan
That seems to be the easiest solution.

--
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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Sat, 2006-10-07 at 03:37 +1000, Russell Coker wrote:
> On Saturday 07 October 2006 03:01, Daniel J Walsh <dwalsh@redhat.com> wrote:
> > Russell Coker wrote:
> > > On Thursday 05 October 2006 23:57, Daniel J Walsh <dwalsh@redhat.com> 
> wrote:
> > >> Does the code continue to work correctly if I compile in AUDIT_LOG_PRIV
> > >> and NAMESPACE_PRIV but run it without the setuid bit and as a normal
> > >> user.  IE, We want the option to only set this setuid when in an MLS
> > >> environment.  This is not required for targeted or strict policy
> > >> machines.
> > >
> > > Who does "we" mean in this context?
> > >
> > > I would like to have newrole work with namespaces in a strict policy
> > > environment!
> >
> > I am not denying you that right.  I am asking for the tool to continue
> > working with or without setuid.
> 
> Without setuid means without poly-instantiation based on SE Linux context, 
> which means that probably most strict policy systems won't be able to 
> effectively use poly-instantiation.

Not sure I follow.  Dan just wants to be able to build a single newrole
binary that correctly handles both cases, and the default install to
leave newrole non-setuid.  That doesn't prevent another package or a
user from making newrole setuid and adjusting the libaudit.conf and/or
newrole pam config to enable/disable the desired auditing and namespace
behavior.  To use polyinstantiation at all, it is already the case that
you need to put pam_namespace into the pam configs of various programs
(not just newrole but login et al),
configure /etc/security/namespace.conf, and set
up /etc/security/namespace.init if necessary.

> Without even trying I've found six setuid-root programs that are included in a 
> fairly default install of Fedora and which are never needed by the vast 
> majority of users.  I doubt that all six are as well audited as newrole.

Keep in mind that newrole didn't start life as a setuid program, so it
wasn't written specifically from that perspective.   It was even fairly
limited wrt SELinux - it couldn't transition you to an arbitrary role
and domain, only one that you were already authorized for in the kernel
policy (vs. su, which can serve as the gateway from any uid to any uid).
The only real power it had was access to the tty/ptys.

-- 
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: [RFC PATCH] newrole suid breakdown

[Russell Coker]

On Saturday 07 October 2006 04:54, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> Not sure I follow.  Dan just wants to be able to build a single newrole
> binary that correctly handles both cases, and the default install to
> leave newrole non-setuid.

However the default install (being targeted policy) doesn't need newrole.

> > Without even trying I've found six setuid-root programs that are included
> > in a fairly default install of Fedora and which are never needed by the
> > vast majority of users.  I doubt that all six are as well audited as
> > newrole.
>
> Keep in mind that newrole didn't start life as a setuid program, so it
> wasn't written specifically from that perspective.   It was even fairly
> limited wrt SELinux - it couldn't transition you to an arbitrary role
> and domain, only one that you were already authorized for in the kernel
> policy (vs. su, which can serve as the gateway from any uid to any uid).
> The only real power it had was access to the tty/ptys.

Reasonable points.  However when you compare it to rsh and rcp...

-- 
russell@coker.com.au
http://etbe.blogspot.com/          My Blog

http://www.coker.com.au/sponsorship.html Sponsoring Free Software development

--
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: [RFC PATCH] newrole suid breakdown

[Michael C Thompson]

Stephen Smalley wrote:
> On Sat, 2006-10-07 at 03:37 +1000, Russell Coker wrote:
>> Without even trying I've found six setuid-root programs that are included in a 
>> fairly default install of Fedora and which are never needed by the vast 
>> majority of users.  I doubt that all six are as well audited as newrole.
> 
> Keep in mind that newrole didn't start life as a setuid program, so it
> wasn't written specifically from that perspective.   It was even fairly
> limited wrt SELinux - it couldn't transition you to an arbitrary role
> and domain, only one that you were already authorized for in the kernel
> policy (vs. su, which can serve as the gateway from any uid to any uid).
> The only real power it had was access to the tty/ptys.

I have a patch (its really big, so I'll try to break it down into 
meaningful chunks) that basically restructures newrole in a more 
maintainable, and paranoid, way. If I can't break it down easily, would 
you (the reader) be ok with reading a ~1600 line patch? Like I said, 
I'll try to break it down, but the changes are very wide sweeping, and 
hopefully a large improvement of what was there.

Based on all of the previous discussion wrt checking the capabilities, 
if this is still desired, I can change the behavior to be:

call_do_priv_action
{
   if !(have_right_capabilities)
     return 0 (flag success, even though its not done anything)
   /* if we do have caps, then do actions and expect them to work */
   ...
}

That acceptable? (And is it even needed anymore due to new package?)

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: [RFC PATCH] newrole suid breakdown

[Stephen Smalley]

On Fri, 2006-10-06 at 16:36 -0500, Michael C Thompson wrote:
> Stephen Smalley wrote:
> > On Sat, 2006-10-07 at 03:37 +1000, Russell Coker wrote:
> >> Without even trying I've found six setuid-root programs that are included in a 
> >> fairly default install of Fedora and which are never needed by the vast 
> >> majority of users.  I doubt that all six are as well audited as newrole.
> > 
> > Keep in mind that newrole didn't start life as a setuid program, so it
> > wasn't written specifically from that perspective.   It was even fairly
> > limited wrt SELinux - it couldn't transition you to an arbitrary role
> > and domain, only one that you were already authorized for in the kernel
> > policy (vs. su, which can serve as the gateway from any uid to any uid).
> > The only real power it had was access to the tty/ptys.
> 
> I have a patch (its really big, so I'll try to break it down into 
> meaningful chunks) that basically restructures newrole in a more 
> maintainable, and paranoid, way. If I can't break it down easily, would 
> you (the reader) be ok with reading a ~1600 line patch? Like I said, 
> I'll try to break it down, but the changes are very wide sweeping, and 
> hopefully a large improvement of what was there.
> 
> Based on all of the previous discussion wrt checking the capabilities, 
> if this is still desired, I can change the behavior to be:
> 
> call_do_priv_action
> {
>    if !(have_right_capabilities)
>      return 0 (flag success, even though its not done anything)
>    /* if we do have caps, then do actions and expect them to work */
>    ...
> }
> 
> That acceptable? (And is it even needed anymore due to new package?)

My understanding was that you could make the audit and pam calls
unconditional (using get_auditfail_action to decide how to handle
audit_open failure), since they can be addressed in the configuration
files (libaudit.conf and /etc/pam.d/newrole).  Only the actual dropping
of capabilities and switching of uids would need to be conditional if we
wanted to a single newrole binary to work both suid and non-suid.  But
it sounds like Dan has decided to just make it suid always in a separate
package, at which point you don't need any runtime conditionals at all.

Naturally, it all stays under build options so that people can still
build it non-suid with no auditing or namespace functionality (and that
remains the default for the upstream Makefile).

-- 
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.