On Monday, October 28, 2013 03:14:48 PM Stephen Smalley wrote: > On 10/28/2013 03:03 PM, Paul Moore wrote: > > On Monday, October 28, 2013 02:24:35 PM Daniel J Walsh wrote: > >> On 10/28/2013 02:10 PM, Paul Moore wrote: > >>> On Monday, October 28, 2013 12:58:55 PM Stephen Smalley wrote: > >>>> On 10/28/2013 11:56 AM, Eric Paris wrote: > >>>>> On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote: > >>>>>> Maybe the solution here is to add logging messages to the function. > >>>>>> > >>>>>> My opionion is that if something is wrong with SELinux, IE The labels > >>>>>> are wrong, the policy is wrong or the app is wrong, we should not > >>>>>> block in permissive mode. > >>>>>> > >>>>>> Having the tool write "foobar_t is not a valid source context" would > >>>>>> be better then what we have now, which is a silent denial even in > >>>>>> permissive mode.> > >>>>> > >>>>> I understand Stephen's argument. But agree with dwalsh/bigon that > >>>>> hiding this in the library is a lot better than moving the logic to > >>>>> userspace programs. So this might not be so super simple to do. How > >>>>> about the idea of a new interface which always returns 0 in > >>>>> permissive? But it does a couple of extra things. These are just > >>>>> rough > >>>>> early thoughts.... > >>>>> > >>>>> 0) new interface just like avc_has_perm() but which always returns 0 > >>>>> in permissive. > >>>>> > >>>>> 1) a new SELINX_USER_ERR audit message. On EINVAL we check if the > >>>>> scontext/tcontext are valid and print the equivalent of a SELINUX_ERR > >>>>> message into the audit log if not. > >>>>> > >>>>> 2) a new /sys/fs/selinux/context like mechanism, which will both > >>>>> validate the context and will force it into the sid cache. So > >>>>> subsequent broken calls to avc_has_perm() will not generate a second > >>>>> SELINX_USER_ERR message, since the second call to 'access' will find a > >>>>> valid type and will give a denial for that unlabeled_t type? > >>>>> > >>>>> maybe /sys/fs/selinux/access should be changed/new interface added to > >>>>> do all of this in kernel? generating a real SELINUX_ERR in kernel and > >>>>> forcing the invalid label into the sid cache? > >>>>> > >>>>> I really do think that userspace object managers should be allowed to > >>>>> call avc_has_perm() and either get an error that should be handled as > >>>>> a hard failure or a 0... checking permissive in userspace object > >>>>> managers just seems prone to breakage... > >>>> > >>>> I'm ok with changing avc_has_perm as long as: a) Something gets > >>>> logged/audited so you'll see that something went wrong in permissive > >>>> mode > >>>> and not just get silent failures in enforcing mode, > >>>> > >>>> b) We are careful about what error conditions are remapped to 0 in > >>>> permissive mode. If we just hit a memory allocation failure, we > >>>> shouldn't hide that from the caller. It should only affect things > >>>> relating to policy. > >>> > >>> I'm far from an expert on the SELinux userland libraries, but as far as > >>> my > >>> two cents are concerned I like the idea of changing avc_has_perm() to > >>> incorporate the permissive/enforcing logic. I think asking applications > >>> to worry about things like that is a step in the wrong direction. > >>> > >>> I also agree with Stephen's comments above: logging is important and the > >>> only failures that should be ignored by avc_has_perm() are those > >>> relating > >>> to access denials from policy, error conditions should propagate to the > >>> caller. > >> > >> The only functions that can fail are the following: > >> rc = avc_lookup(ssid, tsid, tclass, requested, aeref); > >> if (rc) { > >> > >> rc = security_compute_av_flags_raw(ssid->ctx, tsid->ctx, > >> > >> tclass, requested, > >> &entry.avd); > >> > >> if (rc) > >> > >> goto out; > >> > >> rc = avc_insert(ssid, tsid, tclass, &entry, aeref); > >> > >> Are we stating that we should check for EINVAL and return 0 if > >> permissive, > >> or is there some more complicated thing we should look up. > > > > Perhaps I'm mistaken, but I believe the concern has to do with what the > > kernel returns via security_compute_av_flags_raw(). I think the idea is > > to ensure that non-policy related error codes returned from the kernel > > are not papered over when running in permissive mode. > > I think the kernel side is fine as is; it will only return EINVAL if one > of the security contexts is invalid. My mistake, I should have been more clear ... Yes, I agree the kernel is fine, what I meant to get across was what you already said (far better than I): when in permissive mode the library should return 0 when performing access control checks and a policy related denial is returned from the kernel. > Looking again, I see that invalid class is just handled like any other > denial in the kernel these days, so EINVAL from writing to > /sys/fs/selinux/access is unambiguously an invalid security context string > (could be the source or target). > > I think we just need the userspace AVC to handle it cleanly and we'll be > fine. I think my patch will work, but don't have a test case offhand; > I think we'd essentially need to launch dbusd, go permissive, remove its > domain from policy, and then trigger a dbus check? Dan is probably the most knowledgeable here ... if it was left to me I'd probably just cobble up some little test program, but that's probably overkill. -- paul moore www.paul-moore.com -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with the words "unsubscribe selinux" without quotes as the message.