Re: [PATCH v3 21/24] Audit: Store LSM audit information in an lsmblob

On Mon, Jun 24, 2019 at 10:15 PM John Johansen <john.johansen(a)canonical.com&gt; wrote: > On 6/24/19 6:46 PM, Paul Moore wrote: >> On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <casey(a)schaufler-ca.com&gt; wrote: >>> On 6/24/2019 2:33 PM, John Johansen wrote: >>>> On 6/21/19 11:52 AM, Casey Schaufler wrote: >>>>> Change the audit code to store full lsmblob data instead of >>>>> a single u32 secid. This allows for multiple security modules >>>>> to use the audit system at the same time. It also allows the >>>>> removal of scaffolding code that was included during the >>>>> revision of LSM interfaces. >>>>> >>>>> Signed-off-by: Casey Schaufler <casey(a)schaufler-ca.com&gt; >>>> I know Kees raised this too, but I haven't seen a reply >>>> >>>> Eric (Paul is already CCed): I have directly added you because of >>>> the question below. >>>> >>>> In summary there isn't necessarily a single secid any more, and >>>> we need to know whether dropping the logging of the secid or >>>> logging all secids is the correct action. >>> It is to be considered that this is an error case. If >>> everything is working normally you should have produced >>> a secctx previously, which you'll have included in the >>> audit record. Including the secid in the record ought to >>> be pointless, as the secid is strictly an internal token >>> with no meaning outside the running kernel. You are providing >>> no security relevant information by providing the secid. >>> I will grant the possibility that the secid might be useful >>> in debugging, but for that a pr_warn is more appropriate >>> than a field in the audit record. >> FWIW, this probably should have been CC'd to the audit list. >> > hrmm indeed, sorry > >> I agree that this is an error case (security_secid_to_secctx() failed >> to resolve the secid) and further that logging the secid, or a >> collection of secids, has little value the way things currently work. >> Since secids are a private kernel implementation detail, we don't >> really display them outside the context of the kernel, including in >> the audit logs. Recording a secid in this case doesn't provide >> anything meaningful since secids aren't recorded in the audit record >> stream, only the secctxs, and there is no "magic decoder ring" to go >> between the two in the audit logs, or anywhere else in userspace for >> that matter. > Okay, thanks. Casey I am good with just a pr_warn here. I just didn't > have context of why it was going to the audit_log and didn't want > to change that without some more input. Hmm. Actually, let me change my comments slightly ... perhaps what we should do here is keep the audit_log_format(), but change it from audit_log_format("osid=%u",...) to audit_log_format("obj=?"). The "?" is used in audit when we can't determine a piece of information, but we normally log it. It wasn't used very widely originally, which is probably why it isn't in this piece of code.