On Mon, Jun 24, 2019 at 10:15 PM John Johansen
<john.johan...@canonical.com> wrote:
> On 6/24/19 6:46 PM, Paul Moore wrote:
> > On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <ca...@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 <ca...@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.
> >
> 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.
> >>>> ---
> >>>> 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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
