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

[Paul Moore]

On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <casey at schaufler-ca.com> 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 at schaufler-ca.com>
> > 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.

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.

> >> ---
> >>  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);
> >>                      }

-- 
paul moore
www.paul-moore.com