Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Eric Paris <eparis@parisplace.org>]

On Wed, Jun 8, 2011 at 3:28 PM, Steve Grubb <sgrubb@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.four@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.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html