On Thu, Jun 9, 2011 at 8:28 AM, Patrick McHardy <ka...@trash.net> wrote:
> On 08.06.2011 21:39, Eric Paris wrote:
>> On Wed, Jun 8, 2011 at 3:28 PM, Steve Grubb <sgr...@redhat.com> wrote:
>>> On Wednesday, June 08, 2011 03:08:38 PM Eric Paris wrote:
>>>> On Wed, Jun 8, 2011 at 3:00 PM, Mr Dash Four
>>>>
>>>> <mr.dash.f...@googlemail.com> wrote:
>>>>>> int audit_log_secctx(struct auditbuffer *ab, u32 secid)
>>>>>> {
>>>>>> int len, rc;
>>>>>> char *ctx;
>>>>>>
>>>>>> rc = security_secid_to_secctx(sid, &ctx, &len);
>>>>>> if (rc) {
>>>>>> audit_panic("Cannot convert secid to context");
>>>>>> } else {
>>>>>> audit_log_format(ab, " subj=%s", ctx);
>>>>>> security_release_secctx(ctx, len);
>>>>>> }
>>>>>> return rc;
>>>>>> }
>>>>>>
>>>>>> Such a function could be used a couple of places in the audit code
>>>>>> itself.
>>>>>
>>>>> My view on this is that LSM error-handling should be part of LSM.
>>>>>
>>>>> I presume security_secid_to_secctx is going to be called from quite a few
>>>>> places (well, I know of at least two now and they have nothing to do with
>>>>> the LSM) and in my opinion it would be better if that error handling, if
>>>>> adopted, is implemented within the function itself - whether by calling
>>>>> another function, like the one you proposed above, or as part of the
>>>>> secctx retrieval - this could be open to interpretation, but the point I
>>>>> am trying to make is that whichever code security_secid_to_secctx is
>>>>> invoked from shouldn't be involved in reporting/handling (internal LSM)
>>>>> errors at all.
>>>>>
>>>>> I think I made that point in my previous post, but just wanted to make
>>>>> sure that is the case.
>>>>
>>>> The LSM might report and error. It's up to the caller to figure out
>>>> how to deal with that error. In this case we want to use the audit
>>>> system so it's up to the audit system how to handle that error.
>>>
>>> We are happy recording the failed number. Its the LSM people that say nuke
>>> the system.
>>> So, I would put that in security_secid_to_secctx() so that everyone knows
>>> whose
>>> requirements it was to do the nuclear option.
>>
>> If the number meets your requirements then the requirements are total
>> shit. The number has NO relation to the label on the object as
>> understood by the system. If audit has a requirement to always log
>> the label or call audit_panic(), its only option is to call
>> audit_panic().
>>
>> Exposing secids and internal representations of information to
>> userspace is always wrong. Full stop.
>>
>> I'd be willing to take a patch which caused security_secid_to_secctx()
>> to BUG() if it got an invalid secid. But on ENOMEM I'm going to just
>> push the error back up the stack. In that case audit has to decide
>> how to handle the situation. That secid is NOT the label associated
>> with the object and printing it to userspace is meaningless garbage.
>>
>> Just because audit did it wrong yesterday doesn't mean I'm going to
>> ACK more patches that do it wrong tomorrow. I don't care what some
>> arbitrary and obviously poorly thought out requirement document says.
>
> Just to make sure, so the conclusion is that the patch is fine as
> it is and anything related to unconvertible secids will be handled
> by SELinux internally?
>
No. This patch does not get my ACK. Steve is right that silently
dropping information is a big big no no for the audit system and
that's what this patch does. This cannot be wholly handled properly
inside the LSM either. I don't see any patch meeting everyone's
requirements outside of a new one that includes the audit helper I
suggested.
-Eric
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
