[redhat-lspp] Re: [PATCH] cron changes needed for MLS range checking (requires at least the libselinux patches)

[Stephen Smalley]

On Wed, 2006-11-08 at 16:57 -0500, James Antill wrote:
> On Wed, 2006-11-08 at 15:53 -0500, Stephen Smalley wrote:
> 
> > The scontext is supposed to be a process context in which to run the
> > cron job, not a file context.  You are presently replacing the default
> > scontext (extracted from u->scontext that was previously computed) with
> > a strange mixture of the crontab file context and the user-specified
> > range.  What you want to do is to take the default scontext value,
> > create a new context that is identical except for its range (from the
> > environment), and apply a check between those two contexts (and the
> > check is only needed when using a user-supplied range).
> 
>  Ok, I've used u->scontext instead of the file context now. I've also
> renamed the variables. And the check should only happen if they specify
> a different level.
> 
> >   BTW, you cannot
> > continue to refer to the string returned by context_str() after
> > performing a context_free() on the structure; you'd have to dup it
> > first.
> 
>  Right, stupid mistake. Fixed that too.

Looks better.  A few nits:

+static int 
+cron_authorize_range
+( 
+	security_context_t scontext,
+	security_context_t file_context

s/file_context/ucontext/

+)	
+{
+#ifdef WITH_SELINUX
+	struct av_decision avd;
+	int retval;
+        unsigned int bit = CONTEXT__CONTAINS;
+	/*
+	 * Since crontab files are not directly executed,
+	 * crond must ensure that the crontab range has
+	 * a context that is appropriate for the context of
+	 * the user cron job.  It performs an entrypoint
+	 * permission check for this purpose.

cut-and-paste

+	 */
+	retval = security_compute_av(scontext, file_context,

s/file_context/ucontext/

+static int cron_get_job_range( user *u, security_context_t *ucontextp,
+                               char **jobenv )
+{
+	char *sroletype;

s/sroletype/range/

+	if ( (sroletype = env_get("MLS_LEVEL",jobenv)) != 0L )
+	{
+		char crontab[MAX_FNAME];
+                context_t ccon;
+
+		if ( strcmp(u->name,"*system*") == 0 )
+			strncpy(crontab, u->tabname, MAX_FNAME);
+		else
+			snprintf(crontab, MAX_FNAME, "%s/%s", CRONDIR, u->tabname);
+                
+                if (!(ccon = context_new(u->scontext)))
+                {
+			if ( security_getenforce() > 0 ) 
+			{
+				log_it(u->name, 
+				       getpid(), "context_new FAILED for SELINUX_ROLE_TYPE",

s/SELINUX_ROLE_TYPE/MLS_LEVEL/
 
+				       sroletype
+				      );
+				return -1;
+			} else
+                        {
+				log_it(u->name,
+				       getpid(), 
+				       "context_new FAILED but SELinux in permissive mode, continuing "
+				       "- SELINUX_ROLE_TYPE=", sroletype
+				       );
+				return 0;
+                        }

I wouldn't put tests of security_getenforce() on anything other than
permission denials (which avc_has_perm would do internally for you if
you were to use it instead of security_compute_av).  If context_new()
fails, it means you are out of memory, so permissive mode or not, you
are going to die.

+
+	        *ucontextp = strdup(context_str(ccon));

Needs checking of both the intermediate result (context_str return
value) and strdup to avoid seg faulting on NULL.

-- 
Stephen Smalley
National Security Agency