Re: [PATCH v4 9/30] landlock: Add AUDIT_LANDLOCK_DOM_{INFO,DROP} and log domain properties

[Paul Moore <paul@paul-moore.com>]

On Thu, Jan 16, 2025 at 5:51 AM Mickaël Salaün <mic@digikod.net> wrote:
> On Wed, Jan 15, 2025 at 06:53:07PM -0500, Paul Moore wrote:
> > On Jan  8, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:

...

> > The audit subsystem is for security releveant events, not diagnostic,
> > debugging, or other "nice to know" messages.
>
> I agree, my goal was to only log denials with the minimal required
> information to make sense of it.  "minimal" may be subjective though :)

As a data point, it may be worth mentioning that it is not uncommon
for heavy audit users to generate more than 1TB of audit logs in a
day.  So yes, the scale of audit can vary quite a bit.

> > > Audit event sample for a deletion of a domain that denied something:
> > >
> > >   type=LANDLOCK_DOM_DROP msg=audit(1732186800.393:46): domain=195ba459b denials=2
> >
> > As mentioned earlier, I don't like the number of different Landlock
> > specific audit record types that are being created.  I'm going to
> > suggest combining the LANDLOCK_DOM_INFO and LANDLOCK_DOM_DROP
> > records into one (LANDLOCK_DOM?) and using an "op=" field to indicate
> > creation/registration or destruction/unregistration of the domain ID.
>
> I can squash them but they're just not about the same semantic at all.
> One is an asynchronous event that describe a domain, and the other is a
> synchronous event that informs about the end of a domain.

From my perspective, both messages report the status/lifecycle of a
Landlock domain ID and are thus well suited for a single message type.
Personally I question the value of this information, it seems overly
verbose when the access control decisions are the important things,
but you seem to believe this information is important so fine, we'll
add a message type for the domain information, but you only get one.

> If we use an "op" field to differentiate these two types of information,
> it would probably be "op=information" instead of "op=creation" because
> the audit's timestamp will not identify the creation time of this
> domain.

Call it whatever you like, I personally don't care so long as you
don't reuse a single "op=" value for multiple "operations".

> > > +static void log_node(struct landlock_hierarchy *const node)
> > > +{
> > > +   struct audit_buffer *ab;
> > > +
> > > +   if (WARN_ON_ONCE(!node))
> > > +           return;
> > > +
> > > +   /* Ignores already logged domains.  */
> > > +   if (READ_ONCE(node->log_status) == LANDLOCK_LOG_RECORDED)
> > > +           return;
> > > +
> > > +   ab = audit_log_start(audit_context(), GFP_ATOMIC,
> > > +                        AUDIT_LANDLOCK_DOM_INFO);
> > > +   if (!ab)
> > > +           return;
> > > +
> > > +   WARN_ON_ONCE(node->id == 0);
> > > +   audit_log_format(
> > > +           ab,
> > > +           "domain=%llx creation=%llu.%03lu pid=%d uid=%u exe=", node->id,
> > > +           /* See audit_log_start() */
> > > +           (unsigned long long)node->details->creation.tv_sec,
> > > +           node->details->creation.tv_nsec / 1000000,
> > > +           pid_nr(node->details->pid),
> > > +           from_kuid(&init_user_ns, node->details->cred->uid));
> > > +   audit_log_untrustedstring(ab, node->details->exe_path);
> > > +   audit_log_format(ab, " comm=");
> > > +   audit_log_untrustedstring(ab, node->details->comm);
> > > +   audit_log_end(ab);
> >
> > I'm still struggling to understand why you need to log the domain's
> > creation time if you are connecting various Landlock audit events for a
> > single domain by the domain ID.  To be clear, I'm not opposed if you
> > want to include it, it just seems like there is a disconnect between
> > how audit is typically used and what you are proposing.
>
> For the reasons explained in this commit message, domain's creation cannot
> be logged synchronously as other audit events.

Yeah, I've read that, and I disagree.  You *could* log a task's domain
creation synchronously as creation of a Landlock sandbox is a
process/syscall triggered event, you're *choosing* not to do so until
a denial occurs.  That's okay, but I'm tired of hearing that used as
an excuse to do other silly things.

> However, timestamps are
> useful to place them in the logs and order them according to other log
> messages (i.e. to enrich log with more metadata).  Without this domain's
> creation timestamp, we cannot know when it was created.

From my perspective this is because you are choosing not to record a
Landlock domain creation event.  I still have not read any reason why
this is not possible, only a design decision which is now causing you
to do some other unusual things from an audit perspective.

> This
> information is not strictly required but I think it can help get back to
> the creation/creator of a domain.  I'll remove it if you think it
> doesn't make sense to have such information with audit or if it falls
> into the "nice to know" category.

Ignoring my comments above for a moment, it does really seem to fall
into the "nice to know" category to me, but if you feel strongly about
it, it's okay to leave in ... it just isn't really consistent with how
things are generally audited.

-- 
paul-moore.com