On 10/28/2013 03:09 PM, Stephen Smalley wrote:
> On 10/28/2013 03:00 PM, Stephen Smalley wrote:
>> On 10/28/2013 02:24 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.
>>>
>>> I forget the original bugzilla that caused us to do this check but it was
>>> something to do with dbus running witht he wrong context, I believe.
>>
>> I think the only error case of interest is when
>> security_compute_av_flags_raw() returns -1 with errno EINVAL. That can
>> mean that the source or target security context are no longer valid,
>> e.g. due to a policy reload since the time the SIDs were allocated, or
>> that the target security class is invalid, e.g. a userspace security
>> class unknown to the loaded policy. (Or it could just represent random
>> memory corruption in the application, of course).
>>
>> Returning 0 from avc_has_perm_noaudit() could be a source of subtle bugs
>> without ensuring that the avd has been initialized sanely.
>> avc_has_perm() initializes it before calling avc_has_perm_noaudit(), but
>> maybe that should be moved inside of avc_has_perm_noaudit() if avd is set?
>>
>> Then the subsequent avc_audit() call by avc_has_perm() should set denied
>> to 0 (as avd->allowed will be initialized to 0), but audited will be set
>> to 0 because avd->auditdeny will be initialized to 0 too. So we'll
>> audit nothing and get no indication of the error at that point.
>>
>> We could instead initialize avd in a similar way as the kernel does:
>> static void avd_init(struct av_decision *avd)
>> {
>> avd->allowed = 0;
>> avd->auditallow = 0;
>> avd->auditdeny = 0xffffffff;
>> avd->seqno = avc_cache.latest_notif;
>> avd->flags = 0;
>> }
>>
>> And call that on entry to avc_has_perm_noaudit() if avd is non-NULL.
>> Then we'll get an audit message as well, although the security contexts
>> may be invalid (but will be shown as their string form) or the class may
>> be invalid (in which case we'll get a null string).
>
> Like this:
Alternatively, we could go with this one to ensure that in the enforcing
case, we get EACCES rather than EINVAL back in the original caller.
>From 03d3e844340087e772fd4348abe98105abb10b94 Mon Sep 17 00:00:00 2001
From: Stephen Smalley <sds@xxxxxxxxxxxxx>
Date: Mon, 28 Oct 2013 15:15:38 -0400
Subject: [PATCH] Fix avc_has_perm() returns -1 even when SELinux is in
permissive mode.
If we get an EINVAL from security_compute_av* (indicates an invalid
source or target security context, likely due to a policy reload that
removed one or the other), then treat it like any other permission denial.
Reported-by: Laurent Bigonville <bigon@xxxxxxxxxx>
Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
---
libselinux/src/avc.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/libselinux/src/avc.c b/libselinux/src/avc.c
index 802a07f..122c6fc 100644
--- a/libselinux/src/avc.c
+++ b/libselinux/src/avc.c
@@ -739,6 +739,16 @@ void avc_audit(security_id_t ssid, security_id_t tsid,
hidden_def(avc_audit)
+
+static void avd_init(struct av_decision *avd)
+{
+ avd->allowed = 0;
+ avd->auditallow = 0;
+ avd->auditdeny = 0xffffffff;
+ avd->seqno = avc_cache.latest_notif;
+ avd->flags = 0;
+}
+
int avc_has_perm_noaudit(security_id_t ssid,
security_id_t tsid,
security_class_t tclass,
@@ -751,6 +761,9 @@ int avc_has_perm_noaudit(security_id_t ssid,
access_vector_t denied;
struct avc_entry_ref ref;
+ if (avd)
+ avd_init(avd);
+
if (!avc_using_threads && !avc_app_main_loop) {
(void)avc_netlink_check_nb();
}
@@ -783,6 +796,13 @@ int avc_has_perm_noaudit(security_id_t ssid,
rc = security_compute_av_flags_raw(ssid->ctx, tsid->ctx,
tclass, requested,
&entry.avd);
+ if (rc && errno == EINVAL) {
+ if (avc_enforcing)
+ errno = EACCES;
+ else
+ rc = errno = 0;
+ goto out;
+ }
if (rc)
goto out;
rc = avc_insert(ssid, tsid, tclass, &entry, aeref);
@@ -821,8 +841,6 @@ int avc_has_perm(security_id_t ssid, security_id_t tsid,
struct av_decision avd;
int errsave, rc;
- memset(&avd, 0, sizeof(avd));
-
rc = avc_has_perm_noaudit(ssid, tsid, tclass, requested, aeref, &avd);
errsave = errno;
avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
--
1.7.11.7
