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

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.

>>>> --- >>>> kernel/audit.h | 6 +++--- >>>> kernel/auditsc.c | 38 +++++++++++--------------------------- >>>> 2 files changed, 14 insertions(+), 30 deletions(-) > > ... > >>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c >>>> index 0478680cd0a8..d3ad13f11788 100644 >>>> --- a/kernel/auditsc.c >>>> +++ b/kernel/auditsc.c >>>> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic) >>>> context->socketcall.args[i]); >>>> break; } >>>> case AUDIT_IPC: { >>>> - u32 osid = context->ipc.osid; >>>> + struct lsmblob *olsm = &context->ipc.olsm; >>>> >>>> audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho", >>>> from_kuid(&init_user_ns, context->ipc.uid), >>>> from_kgid(&init_user_ns, context->ipc.gid), >>>> context->ipc.mode); >>>> - if (osid) { >>>> + if (lsmblob_is_set(olsm)) { >>>> struct lsmcontext lsmcxt; >>>> - struct lsmblob blob; >>>> >>>> - lsmblob_init(&blob, osid); >>>> - if (security_secid_to_secctx(&blob, &lsmcxt)) { >>>> - audit_log_format(ab, " osid=%u", osid); >>> I am not comfortable just dropping this I would think logging all secids is the >>> correct action here. >>> >>> >>>> + if (security_secid_to_secctx(olsm, &lsmcxt)) >>>> *call_panic = 1; >>>> - } else { >>>> + else { >>>> audit_log_format(ab, " obj=%s", lsmcxt.context); >>>> security_release_secctx(&lsmcxt); >>>> }