Bug 237876

+++ This bug was initially created as a clone of Bug #237249 +++

Description of problem:
Despite configuring a directory to be polyinstantiated at level sometimes it
gets polyinstantiated by user. So I looked at the code and
I see the reason for the behavior that had confused me partly because it
isn't documented and partly because I don't think it is desired. The code as
described in the following comment is overriding my specified method
if getexeccon fails.
/*
 * This function checks if the calling program has requested context
 * change by calling setexeccon(). If context change is not requested
 * then it does not make sense to polyinstantiate based on context.
 * The return value from this function is used when selecting the
 * polyinstantiation method. If context change is not requested then
 * the polyinstantiation method is set to USER, even if the configuration
 * file lists the method as "context" or "both".
 */
static int ctxt_based_inst_needed(void)

Why if getexeccon fails doesn't it make sense to polyinstantiate based
on context/level? Why not call getcon if getexeccon fails and use that
context instead of switching the method? 

At a minimum this needs to be documented so that users will understand the
behavior. 


Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:

-- Additional comment from tmraz on 2007-04-20 11:25 EST --
It's really questionable what is the best behaviour in the case getexeccon
fails. It would perhaps also make sense to completely fail out.

But I completely agree that the behavior should be documented.

-- Additional comment from sgrubb on 2007-04-20 16:26 EST --
I'll go for man page updates.

-- Additional comment from tmraz on 2007-04-24 05:51 EST --
Behavior documented in pam-0.99.6.2-3.21.el5.

-- Additional comment from txtoth on 2007-04-24 16:23 EST --
Created an attachment (id=153379)
patch


-- Additional comment from txtoth on 2007-04-24 16:33 EST --
I think this needs more than a doc change so I created a patch. The patch calls
getcon in the event that getexeccon fails and it doesn't change the user
specified method.

-- Additional comment from sgrubb on 2007-04-24 16:43 EST --
But if getexeccon fails, wouldn't that polyinstantiate based on the daemon's
context instead of the user's intended context? IOW,
system_u:system_r:xdm_t:SystemLow-SystemHigh. That would not be good either.

-- Additional comment from tmraz on 2007-04-24 18:06 EST --
I'm curious if there is a valid application calling pam which wouldn't call
setexeccon and have the proper user's context assigned by other means?

If some app like this exists I think the best solution would be to add an option
which would make pam_namespace to simply call getcon instead of getexeccon. I'm
not really sure we want to change the current behavior unconditionally.


-- Additional comment from txtoth on 2007-04-25 12:16 EST --
(In reply to comment #8)
> But if getexeccon fails, wouldn't that polyinstantiate based on the daemon's
> context instead of the user's intended context? IOW,
> system_u:system_r:xdm_t:SystemLow-SystemHigh. That would not be good either.

You're right so I took a crack at the patch. Instead of a straight getcon if
getexeccon fails I'm getting the users default context and using that instead. 

--- Linux-PAM-0.99.7.0/modules/pam_namespace/pam_namespace.c	2007-04-25
10:45:52.000000000 -0500
+++ Linux-PAM-0.99.7.0.new/modules/pam_namespace/pam_namespace.c	2007-04-25
10:45:37.000000000 -0500
@@ -251,21 +251,15 @@
 
     poly.method = NONE;
     if (strcmp(method, "user") == 0) 
-	    poly.method = USER;
+        poly.method = USER;
 
 #ifdef WITH_SELINUX
     if (strcmp(method, "level") == 0) {
-        if (idata->flags & PAMNS_CTXT_BASED_INST)
-            poly.method = LEVEL;
-	else
-            poly.method = USER;
+        poly.method = LEVEL;
     }
 
     if (strcmp(method, "context") == 0) {
-        if (idata->flags & PAMNS_CTXT_BASED_INST)
-            poly.method = CONTEXT;
-	else
-            poly.method = USER;
+        poly.method = CONTEXT;
     }
 
 #endif
@@ -447,6 +441,7 @@
 	int rc = PAM_SUCCESS;
 	security_context_t scon = NULL;
 	security_class_t tclass;
+	char *selinuxuser, *level;
 
 	/*
 	 * Get the security context of the directory to polyinstantiate.
@@ -462,9 +457,29 @@
 
 	rc = getexeccon(&scon);
 	if (rc < 0 || scon == NULL) {
+	    
+	  rc = getseuserbyname(idata->user, &selinuxuser, &level);
+	  if (rc < 0) {
 		pam_syslog(idata->pamh, LOG_ERR, 
-			   "Error getting exec context, %m");
+			   "Error getting selinux user, %m");
 		return PAM_SESSION_ERR;
+	  }
+	  if (idata->flags & PAMNS_DEBUG)
+	    pam_syslog(idata->pamh, LOG_DEBUG,
+		       "selinux user %s level %s", selinuxuser, level);
+	  
+	  rc = get_default_context_with_level(selinuxuser, level, NULL, &scon);
+	  free(selinuxuser);
+	  free(level);
+	  if (rc < 0) {
+		pam_syslog(idata->pamh, LOG_ERR, 
+			   "Error getting selinux user default context, %m");
+		return PAM_SESSION_ERR;
+	  }
+	  if (idata->flags & PAMNS_DEBUG)
+	    pam_syslog(idata->pamh, LOG_DEBUG,
+		       "selinux user default context %s", scon);
+	  
 	}
 
 	/*
@@ -515,6 +530,10 @@
 			pam_syslog(idata->pamh, LOG_ERR, "Unable to set MLS Componant of context");
 			goto fail;
 		}
+		if (idata->flags & PAMNS_DEBUG)
+		  pam_syslog(idata->pamh, LOG_DEBUG,
+			     "context_range_set %s %s", context_str(fcontext), context_str(scontext));
+
 		*i_context=strdup(context_str(fcontext));
 		if (! *i_context) {
 			pam_syslog(idata->pamh, LOG_ERR, "out of memory");


-- Additional comment from tmraz on 2007-04-25 12:53 EST --
That's debatable as well. I'd just like to know why the getexeccon could fail
and if there is any real application using pam_namespace where the getexeccon
fails and it is reasonable to continue. Because in the standard login/sshd/su...
cases the getexeccon simply shouldn't fail.



-- Additional comment from txtoth on 2007-04-25 13:09 EST --
gdm which I've configured to use pam_namespace doesn't ever calls setexeccon.
Also I don't believe that su calls setexeccon but I could be wrong.

-- Additional comment from tmraz on 2007-04-25 13:25 EST --
They call it through pam_selinux.


-- Additional comment from tmraz on 2007-04-25 13:27 EST --
Oops... I should have looked at the pam configs first. You're right that they
don't  have pam_selinux in configuration. But probably that should be changed?


-- Additional comment from tmraz on 2007-04-25 13:42 EST --
And looking at the gdm sources it calls setexeccon on its own but after
pam_open_session is called so that's why pam_namespace doesn't see it.

Well then the patch from comment 10 could make sense, however I'd prefer it
being optional.

Thee su case is different because it doesn't change the context at all. So in
its case it would make more sense to get the context using getcon. Again that
could be a different option.



-- Additional comment from txtoth on 2007-04-25 15:18 EST --
(In reply to comment #15)
> And looking at the gdm sources it calls setexeccon on its own but after
> pam_open_session is called so that's why pam_namespace doesn't see it.
> 
> Well then the patch from comment 10 could make sense, however I'd prefer it
> being optional.
> 
> Thee su case is different because it doesn't change the context at all. So in
> its case it would make more sense to get the context using getcon. Again that
> could be a different option.
> 
> 
What are you suggesting be the behavior if the option to use the users default
context is not set and in the case of su if the option to use getcon is not set?


-- Additional comment from txtoth on 2007-04-25 15:28 EST --
(In reply to comment #15)
> And looking at the gdm sources it calls setexeccon on its own but after
> pam_open_session is called so that's why pam_namespace doesn't see it.
> 
> Well then the patch from comment 10 could make sense, however I'd prefer it
> being optional.
> 
> Thee su case is different because it doesn't change the context at all. So in
> its case it would make more sense to get the context using getcon. Again that
> could be a different option.
> 
> 
What are you suggesting be the behavior if the option to use the users default
context is not set and in the case of su if the option to use getcon is not set?

-- Additional comment from tmraz on 2007-04-25 15:57 EST --
Either the current fallback to just username based polyinst. or returning
PAM_SESSION_ERROR.