Re: Security issues with local filesystem caching

[David Howells <dhowells@redhat.com>]

Stephen Smalley <sds@tycho.nsa.gov> wrote:

> It doesn't look simpler, but if you take this approach, then:
> 1) fssid needs to be renamed to reflect its use as the subject/actor
> label (e.g. => "subjsid"),

Okay.

> 2) Use "secid" in the core code and LSM interfaces rather than just
> "sid" to avoid confusion with session ids (e.g. => "subjsecid"),

Okay.

> 3) Define and use a SECID_NULL or similar in the LSM interface
> (SECSID_NULL is SELinux private),

Defining a reset function would be better.

> 4) Be careful about overuse of this id (see below for some examples).

Yeah.

> > The flag approach is a bit more work to implement and will slow most ops
> > down by virtue of including an extra check, though not all ops can be
> > bypassed (for instance the op that assigns a security label to an inode
> > can't be bypassed).
> 
> In most cases, you could just generalize the existing IS_PRIVATE(inode)
> tests in the security static inlines in security.h to also incorporate a
> task flag test.  security_inode_init_security() would be an exception.

So you were thinking of doing it in security.h?

> > There are a couple of things I'm not sure about with the ->fssid approach:
> > 
> >  (1) What will auditing do?  Is it possible to have an SID that isn't
> >      audited?
> > 
> >  (2) How do I come up with a security label for the module to use?
> 
> Note that these issues don't exist in the task flag approach.

Yes.

> I'm not entirely sure what you mean by (1); the existing syscall audit
> support would never look at your fssid, just the ->sid, and only at syscall
> entry and exit.

Which is irrelevant since the CacheFiles module doesn't make syscalls.

> SELinux avc audit messages from permission checks would include the fssid's
> context if that was the basis of the check.

That'd probably be sufficient since at least you could tell them apart.

> For (2), you have to make up a SID.

I've done this and got it working with Karl's help.

In cachefilesd.te, I have:

	...
	type cachefilesd_t;
	type cachefilesd_exec_t;
	domain_type(cachefilesd_t)
	init_daemon_domain(cachefilesd_t, cachefilesd_exec_t)

	require { type kernel_t; }
	type cachefiles_kernel_t;
	domain_type(cachefiles_kernel_t)
	type_transition cachefilesd_t kernel_t : process cachefiles_kernel_t;
	...

Then I added a CacheFiles hook to do:

	static int selinux_cachefiles_getsid(u32 secid, u32 *modsecid)
	{
		return security_transition_sid(secid, SECINITSID_KERNEL,
					       SECCLASS_PROCESS, modsecid);
	}

It takes the daemon's security ID and translates it to the module's security
ID.

The way I was thinking about it is that a CacheFiles cache has a presence in
the kernel that lasts as long as the cache is in active service.  This could
be considered the equivalent of a process, although it doesn't have a
task_struct of its own; but rather makes use of the task_struct of processes
that want to use the service.

> This differs from how NFS would use such a facility, since it could just use
> the client process' context (if that were available),

Indeed; but once NFS had a SID, the two would then be the same.

> But this requires policy configuration to make it work properly, and the
> absence of the necessary type transition and allow rules could yield a
> broken cache.

If the rule isn't there then the cache would refuse to start if SELinux is in
enforcing mode, assuming security_transition_sid() returns an error in that
case.  Otherwise it'll run in cachefilesd's context, I think, which it will
cause it to give an error and refuse to cache in enforcing mode because that
context doesn't permit it to do certain things it checks for (like creating
files and directories).

> > +		rc = avc_has_perm(secid, tsec->fssid, SECCLASS_PROCESS, perm, NULL);
> ...
> The task is the target of a check here, so you don't want to use the
> overriding SID for it.  Otherwise, you may have a false denial or a
> false allow of the signal.

Yep...  I got that one wrong.

> > -	isec->sid = tsec->sid;
> > +	isec->sid = tsec->fssid;
> 
> Here we are labeling a /proc/pid inode with the task SID, when it is
> created or revalidated, so you don't want to use the overriding SID here
> either.  

And again.


Anyway, I'll expand what I've done and give it a go.  I may find compelling
reasons[*] not to do it this way.  I can always ditch the patches after all...

[*] Overzealous auditing most probably.

Thanks,
David
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/