From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <45250F35.6030204@redhat.com> Date: Thu, 05 Oct 2006 09:57:09 -0400 From: Daniel J Walsh MIME-Version: 1.0 To: Michael C Thompson CC: SE Linux , Stephen Smalley , jdesai@us.ibm.com Subject: Re: [RFC PATCH] newrole suid breakdown References: <452432FA.1060009@us.ibm.com> In-Reply-To: <452432FA.1060009@us.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Sender: owner-selinux@tycho.nsa.gov List-Id: selinux@tycho.nsa.gov 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 > #include /* for malloc(), realloc(), free() */ > #include /* for getpwuid() */ > @@ -66,10 +74,7 @@ > #ifdef USE_AUDIT > #include > #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 > #include > #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.