Message-ID: <CAHC9VhRiUOe9enkCOko0mGehxt+2tbJNGoJm=jmauhZSPFvzRg@mail.gmail.com>
Date: Fri, 22 Dec 2023 17:42:35 -0500
From: Paul Moore <paul@...l-moore.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Eric Paris <eparis@...hat.com>, James Morris <jmorris@...ei.org>, 
	"Serge E . Hallyn" <serge@...lyn.com>, Ben Scarlato <akhna@...gle.com>, 
	Günther Noack <gnoack@...gle.com>, 
	Jeff Xu <jeffxu@...gle.com>, Jorge Lucangeli Obes <jorgelo@...gle.com>, 
	Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, Shervin Oloumi <enlightened@...gle.com>, 
	audit@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-security-module@...r.kernel.org
Subject: Re: [RFC PATCH v1 3/7] landlock: Log ruleset creation and release

On Thu, Dec 21, 2023 at 1:45 PM Mickaël Salaün <mic@...ikod.net> wrote:
> On Wed, Dec 20, 2023 at 04:22:15PM -0500, Paul Moore wrote:
> > On Thu, Sep 21, 2023 at 2:17 AM Mickaël Salaün <mic@...ikod.net> wrote:
> > >
> > > Add audit support for ruleset/domain creation and release ...

...

> > > +/* Inspired by dump_common_audit_data(). */
> > > +static void log_task(struct audit_buffer *const ab)
> > > +{
> > > +       /* 16 bytes (TASK_COMM_LEN) */
> > > +       char comm[sizeof(current->comm)];
> > > +
> > > +       /*
> > > +        * Uses task_pid_nr() instead of task_tgid_nr() because of how
> > > +        * credentials and Landlock work.
> > > +        */
> > > +       audit_log_format(ab, "tid=%d comm=", task_pid_nr(current));
> > > +       audit_log_untrustedstring(ab,
> > > +                                 memcpy(comm, current->comm, sizeof(comm)));
> > > +}
> >
> > Depending on how log_task() is used, it may be redundant with respect
> > to the existing SYSCALL record.  Yes, there is already redundancy with
> > the AVC record, but that is a legacy problem and not something we can
> > easily fix, but given that the Landlock records are new we have an
> > opportunity to do things properly :)
>
> Indeed, it would make it simpler too. I wasn't sure how standalone a
> record should be, but I guess there tools should be able to look for a
> set of related records (e.g. event with a SYSCALL record matching a PID
> and a LANDLOCK record).

I believe ausearch will output the entire event when it matches on a
field in one of the event's records.

> > > +void landlock_log_create_ruleset(struct landlock_ruleset *const ruleset)
> > > +{
> > > +       struct audit_buffer *ab;
> > > +
> > > +       WARN_ON_ONCE(ruleset->id);
> > > +
> > > +       ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_LANDLOCK);
> > > +       if (!ab)
> > > +               /* audit_log_lost() call */
> > > +               return;
> > > +
> > > +       ruleset->id = atomic64_inc_return(&ruleset_and_domain_counter);
> > > +       log_task(ab);
> > > +       audit_log_format(ab,
> > > +                        " op=create-ruleset ruleset=%llu handled_access_fs=",
> > > +                        ruleset->id);
> >
> > "handled_access_fs" seems a bit long for a field name, is there any
> > reason why it couldn't simply be "access_fs" or something similar?
>
> "handled_access_fs" is from the landlock_create_ruleset(2) API, so I'd
> like to use the same name.

Okay, that's a reasonable reason.

> However, because the types of handled access
> rights for a ruleset will expand (e.g. we now have a
> handled_access_net), I'm wondering if it would be better to keep this
> (growing) one-line record or if we should use several records for a
> ruleset creation (i.e. one line per type of handled access righs).

I think it would be better to have a single record for rulesets rather
than multiple records all dealing with rulesets.

> > > +               id = ruleset->hierarchy->id;
> > > +               WARN_ON_ONCE(ruleset->id);
> > > +       } else {
> > > +               name = "ruleset";
> > > +               id = ruleset->id;
> > > +       }
> > > +       WARN_ON_ONCE(!id);
> > > +
> > > +       /*
> > > +        * Because this might be called by kernel threads, logging
> > > +        * related task information with log_task() would be useless.
> > > +        */
> > > +       audit_log_format(ab, "op=release-%s %s=%llu", name, name, id);
> >
> > This starts to get a little tricky.  The general guidance is that for
> > a given audit record type, e.g. AUDIT_LANDLOCK, there should be no
> > change in presence or ordering of fields, yet in
> > landlock_log_create_ruleset() we log the permission information and
> > here in landlock_log_release_ruleset() we do not.  The easy fix is to
> > record the permission information here as well, or simply use a
> > "handled_access_fs=?" placeholder.  Something to keep in mind as you
> > move forward.
>
> OK, I used different "op" to specify the related fields, but I should
> use a dedicated record type when it makes sense instead. My reasoning
> was that it would be easier to filter on one or two record types, but
> I like the fixed set of fields per record type.
>
> I plan to add a few record types, something like that:
>
> For a ruleset creation event, several grouped records:
> - AUDIT_LANDLOCK_RULESET: "id=[new ruleset ID] op=create"
> - AUDIT_LANDLOCK_ACCESS: "type=[fs or net] rights=[bitmask]"

I'm guessing that LANDLOCK_RULESET would be for policy changes, and
LANDLOCK_ACCESS would be for individual access grants or denials?  If
so, that looks reasonable.

> For rule addition, several records per landlock_add_rule(2) call.
> Example with a path_beneath rule:
> - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=add_rule"
> - AUDIT_LANDLOCK_PATH: "scope=beneath path=[file path] dev= ino="
> - AUDIT_LANDLOCK_ACCESS: "type=fs rights=[bitmask]"

I worry that LANDLOCK_PATH is too much of a duplicate for the existing
PATH record.  Assuming the "scope=" field is important, could it live
in the LANDLOCK_ACCESS record and then you could do away with the
dedicated LANDLOCK_PATH record?  Oh, wait ... this is to record the
policy, not a individual access request, gotcha.  If that is the case
and RULESET, PATH, ACCESS are all used simply to record the policy
information I might suggest creation of an AUDIT_LANDLOCK_POLICY
record that captures all of the above.  If you think that is too
cumbersome, then perhaps you can do the object/access-specific record
type, e.g. AUDIT_LANDLOCK_POLICY_FS and AUDIT_LANDLOCK_POLICY_NET.

You also shouldn't reuse the "type=" field.  Steve gets grumpy when
people reuse field names for different things.  You can find a
reasonably complete list of fields here:
https://github.com/linux-audit/audit-documentation/blob/main/specs/fields/field-dictionary.csv

> For domain creation/restriction:
> - AUDIT_LANDLOCK_DOMAIN: "id=[new domain ID] op=create"
> - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=use"

I imagine you could capture this in the policy record type?

> For ruleset release:
> - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=release"
>
> For domain release:
> - AUDIT_LANDLOCK_DOMAIN: "id=[domain ID] op=release"

Same with the above two.

> For denied FS access:
> - AUDIT_LANDLOCK_DENIAL: "id=[domain ID] op=mkdir"
> - AUDIT_LANDLOCK_PATH: "scope=exact path=[file path] dev= ino="

I would use a single record type, i.e. AUDIT_LANDLOCK_ACCESS, to
capture both access granted and denied events.  I'd also omit the
dedicated LANDLOCK_PATH record here in favor of the generic PATH
record (see my comments above).

> For denied net access:
> - AUDIT_LANDLOCK_DENIAL: "id=[domain ID] op=connect"
> - AUDIT_LANDLOCK_PORT: "port="

I would look at the SOCKADDR record type instead of introducing a new
LANDLOCK_PORT type.

> I guess it will still be OK to expand a record type with new appended
> fields right? For instance, could we append a "flags" field to a
> AUDIT_LANDLOCK_RULESET record (because it may not make sense to create a
> dedicated record type for that)?

Of course, one can always add fields to an existing record type with
the understanding that they MUST be added to the end.

-- 
paul-moore.com

Powered by blists - more mailing lists