Re: [PATCH v3 10/23] landlock: Log domain properties and release

["Mickaël Salaün" <mic@digikod.net>]

On Mon, Jan 06, 2025 at 04:56:50PM -0500, Paul Moore wrote:
> On Mon, Jan 6, 2025 at 9:51 AM Mickaël Salaün <mic@digikod.net> wrote:
> > On Sat, Jan 04, 2025 at 08:23:51PM -0500, Paul Moore wrote:
> > > On Nov 22, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> 
> ...
> 
> > > > Audit event sample for a first denial:
> > > >
> > > >   type=LL_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
> > > >   type=LL_DOM_INFO msg=audit(1732186800.349:44): domain=195ba459b creation=1732186800.345 pid=300 uid=0 exe="/root/sandboxer" comm="sandboxer"
> > > >   type=SYSCALL msg=audit(1732186800.349:44): arch=c000003e syscall=101 success=no [...] pid=300 auid=0
> > >
> > > As mentioned in patch 9/23, I don't want subsystems external to audit
> > > to access the audit timestamp information, so the "creation=" field
> > > in the audit event would need to be removed.  Assuming that the timestamp
> > > was used either to reference the original domain creation and/or simply
> > > provide some additional information for analysis, all of that information
> > > should already be in the audit log, assuming of course that you are
> > > logging domain creation (which you should, at least as an option).
> >
> > As explained in this patch, we don't want to (and cannot realistically)
> > log domain creations.  That would make the audit support for Landlock
> > unusable.  Moreover, these information is useless and only add noise
> > unless there is a denial, hence this asynchronous approach.
> 
> That's fine, just know that it doesn't change my thoughts on exposing
> the audit timestamp.
> 
> > However,
> > users may want to log some syscalls, including landlock_restrict_self(),
> > and it would make audit logs more consistent using the same timestamp as
> > the Landlock domain creation time.  I'm wondering why exposing this
> > timestamp to the kernel would be an issue whereas it is already exposed
> > to user space.
> 
> Currently there are no other users of the audit timestamp besides
> audit.  Making the audit timestamp available to other subsystems makes
> the timestamp less flexible over the long term as it would become, in
> a way, part of the API that audit provides to other in-kernel users.
> 
> I still have hopes to rework a large chunk of the audit subsystem, and
> keeping the interfaces between audit and the other in-kernel
> subsystems makes that easier.

OK

> 
> > If you're really opposed to it I can create a new unrelated timestamp
> > specific to Landlock.
> 
> Yes, at this point in time I don't want to support exporting the audit
> timestamp outside of audit.  My guess is that you probably want to use
> some identifier, other than a timestamp, when trying to link Landlock
> events (presumably the domain ID would do this?), but I don't pretend
> to know the details of Landlock very well right now.

Correct, Landlock domain IDs are used to tie domain creation, denial,
and destruction/drop events (and their use will be extended to user
space in the future).

> 
> > > Also, is there a good reason why the LL_DOM_INFO information can't be
> > > recorded in the LL_DENY (or LL_ACCESS) record?  I think that would be
> > > preferable.
> >
> > The goal of the standalone LL_DOM_INFO record type is to limit useless
> > log verbosity.  Including this information in LL_DENY would have two
> > downsides:
> > - it would increases the length of *all* LL_DENY messages
> 
> Are you ever going to emit a LL_ACCESS/LL_DENY record without a
> LL_DOM_INFO record?

Yes, only the first LL_DENY (for a domain) emits an LL_DOM_INFO (for
this same domain), which is why this design is interesting: creation of
domains can happen at a high frequency (e.g. script executing a
sandboxed program in a loop, or just build a kernel with sandbox
compilers), and logging every domain creation would make 99% of these
events useless.  See log_status's LANDLOCK_LOG_RECORDED in log_node() in
this patch.

> 
> > - it would make it more difficult to extend this new mixed messages with
> >   access-related informations (e.g. file property) and domain-related
> >   informations (and associate them with either the object or the
> >   domain).
> 
> How?  Please elaborate on this.

I mean that appending intertwined (i.e. some might be related to define
a domain whereas others might be related to define an object) and
optional (e.g. a file object and a socket object are not defined the
same way) new fields to one message type makes the message less
predictable and more difficult to parse.

> 
> > > > Audit event sample for logged domains deletion:
> > > >
> > > >   type=LL_DOM_DROP msg=audit(1732186800.393:45): domain=195ba459b
> > > >
> > > > Cc: Günther Noack <gnoack@google.com>
> > > > Cc: Paul Moore <paul@paul-moore.com>
> > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > Link: https://lore.kernel.org/r/20241122143353.59367-11-mic@digikod.net
> > > > ---
> > > > Questions:
> > > > - Should we also log the creator's loginuid?
> > > > - Should we also log the creator's sessionid?
> > >
> > > Creation of a Landlock domain can only happen through the Landlock
> > > syscalls, yes?  If so, that information should already be logged in
> > > the associated syscall record (see the "auid=" and "ses=" fields )and
> > > we generally try to avoid duplicating information across records in
> > > the same audit event.
> >
> > The specificity of Landlock compared to existing supported systems is
> > that we cannot log domain creation for the reason I explain before.
> 
> Can you provide a link to that explanation?  I'm sure you explained it
> well, but I missed it when going over the patchset with a focus on
> audit.

That wasn't clear enough, I'll include the previous description in the
next series, but the basic design idea is defined in the cover letter:
https://lore.kernel.org/all/20241122143353.59367-1-mic@digikod.net/

> 
> If the Landlock domain is created independent from any user/process
> action, it likely doesn't make sense to log either the loginuid or
> sessionid since the domain creation is happening independently from a
> user session.

Landlock domain creations are a process action.  What we want to log are
the denials and a minimal context (e.g. which task created the related
domain).  I was wondering if we should (right now) include loginuid or
sessionid in this (asynchronous) context.