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

On 08.06.2011 21:39, Eric Paris wrote: > On Wed, Jun 8, 2011 at 3:28 PM, Steve Grubb <sgrubb(a)redhat.com&gt; 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(a)googlemail.com&gt; 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?