Re: [PATCH v24 02/12] landlock: Add ruleset and domain management
On 21/11/2020 08:00, Jann Horn wrote: > On Thu, Nov 12, 2020 at 9:51 PM Mickaël Salaün wrote: >> A Landlock ruleset is mainly a red-black tree with Landlock rules as >> nodes. This enables quick update and lookup to match a requested >> access, e.g. to a file. A ruleset is usable through a dedicated file >> descriptor (cf. following commit implementing syscalls) which enables a >> process to create and populate a ruleset with new rules. >> >> A domain is a ruleset tied to a set of processes. This group of rules >> defines the security policy enforced on these processes and their future >> children. A domain can transition to a new domain which is the >> intersection of all its constraints and those of a ruleset provided by >> the current process. This modification only impact the current process. >> This means that a process can only gain more constraints (i.e. lose >> accesses) over time. >> >> Cc: James Morris >> Cc: Jann Horn >> Cc: Kees Cook >> Cc: Serge E. Hallyn >> Signed-off-by: Mickaël Salaün >> --- >> >> Changes since v23: >> * Always intersect access rights. Following the filesystem change >> logic, make ruleset updates more consistent by always intersecting >> access rights (boolean AND) instead of combining them (boolean OR) for >> the same layer. > > This seems wrong to me. If some software e.g. builds a policy that > allows it to execute specific libraries and to open input files > specified on the command line, and the user then specifies a library > as an input file, this change will make that fail unless the software > explicitly deduplicates the rules. > Userspace will be forced to add extra complexity to work around this. That's a valid use case I didn't think about. Reverting this change is not an issue. > >> This defensive approach could also help avoid user >> space to inadvertently allow multiple access rights for the same >> object (e.g. write and execute access on a path hierarchy) instead of >> dealing with such inconsistency. This can happen when there is no >> deduplication of objects (e.g. paths and underlying inodes) whereas >> they get different access rights with landlock_add_rule(2). > > I don't see why that's an issue. If userspace wants to be able to > access the same object in different ways for different purposes, it > should be able to do that, no? > > I liked the semantics from the previous version. > I agree, but the real issue is with the ruleset layers applied to the filesystem, cf. patch 7.
Re: [PATCH v24 02/12] landlock: Add ruleset and domain management
On Thu, Nov 12, 2020 at 9:51 PM Mickaël Salaün wrote: > A Landlock ruleset is mainly a red-black tree with Landlock rules as > nodes. This enables quick update and lookup to match a requested > access, e.g. to a file. A ruleset is usable through a dedicated file > descriptor (cf. following commit implementing syscalls) which enables a > process to create and populate a ruleset with new rules. > > A domain is a ruleset tied to a set of processes. This group of rules > defines the security policy enforced on these processes and their future > children. A domain can transition to a new domain which is the > intersection of all its constraints and those of a ruleset provided by > the current process. This modification only impact the current process. > This means that a process can only gain more constraints (i.e. lose > accesses) over time. > > Cc: James Morris > Cc: Jann Horn > Cc: Kees Cook > Cc: Serge E. Hallyn > Signed-off-by: Mickaël Salaün > --- > > Changes since v23: > * Always intersect access rights. Following the filesystem change > logic, make ruleset updates more consistent by always intersecting > access rights (boolean AND) instead of combining them (boolean OR) for > the same layer. This seems wrong to me. If some software e.g. builds a policy that allows it to execute specific libraries and to open input files specified on the command line, and the user then specifies a library as an input file, this change will make that fail unless the software explicitly deduplicates the rules. Userspace will be forced to add extra complexity to work around this. > This defensive approach could also help avoid user > space to inadvertently allow multiple access rights for the same > object (e.g. write and execute access on a path hierarchy) instead of > dealing with such inconsistency. This can happen when there is no > deduplication of objects (e.g. paths and underlying inodes) whereas > they get different access rights with landlock_add_rule(2). I don't see why that's an issue. If userspace wants to be able to access the same object in different ways for different purposes, it should be able to do that, no? I liked the semantics from the previous version.
Re: [PATCH v24 02/12] landlock: Add ruleset and domain management
On Thu, 12 Nov 2020, Mickaël Salaün wrote: > Cc: James Morris > Cc: Jann Horn > Cc: Kees Cook > Cc: Serge E. Hallyn > Signed-off-by: Mickaël Salaün > --- > > Changes since v23: > * Always intersect access rights. Following the filesystem change > logic, make ruleset updates more consistent by always intersecting > access rights (boolean AND) instead of combining them (boolean OR) for > the same layer. This defensive approach could also help avoid user > space to inadvertently allow multiple access rights for the same > object (e.g. write and execute access on a path hierarchy) instead of > dealing with such inconsistency. This can happen when there is no > deduplication of objects (e.g. paths and underlying inodes) whereas > they get different access rights with landlock_add_rule(2). > * Add extra checks to make sure that: > - there is always an (allocated) object in each used rules; > - when updating a ruleset with a new rule (i.e. not merging two > rulesets), the ruleset doesn't contain multiple layers. > * Hide merge parameter from the public landlock_insert_rule() API. This > helps avoid misuse of this function. > * Replace a remaining hardcoded 1 with SINGLE_DEPTH_NESTING. Jann: any chance you could review this patch again given the changes above? Thanks. -- James Morris
[PATCH v24 02/12] landlock: Add ruleset and domain management
From: Mickaël Salaün
A Landlock ruleset is mainly a red-black tree with Landlock rules as
nodes. This enables quick update and lookup to match a requested
access, e.g. to a file. A ruleset is usable through a dedicated file
descriptor (cf. following commit implementing syscalls) which enables a
process to create and populate a ruleset with new rules.
A domain is a ruleset tied to a set of processes. This group of rules
defines the security policy enforced on these processes and their future
children. A domain can transition to a new domain which is the
intersection of all its constraints and those of a ruleset provided by
the current process. This modification only impact the current process.
This means that a process can only gain more constraints (i.e. lose
accesses) over time.
Cc: James Morris
Cc: Jann Horn
Cc: Kees Cook
Cc: Serge E. Hallyn
Signed-off-by: Mickaël Salaün
---
Changes since v23:
* Always intersect access rights. Following the filesystem change
logic, make ruleset updates more consistent by always intersecting
access rights (boolean AND) instead of combining them (boolean OR) for
the same layer. This defensive approach could also help avoid user
space to inadvertently allow multiple access rights for the same
object (e.g. write and execute access on a path hierarchy) instead of
dealing with such inconsistency. This can happen when there is no
deduplication of objects (e.g. paths and underlying inodes) whereas
they get different access rights with landlock_add_rule(2).
* Add extra checks to make sure that:
- there is always an (allocated) object in each used rules;
- when updating a ruleset with a new rule (i.e. not merging two
rulesets), the ruleset doesn't contain multiple layers.
* Hide merge parameter from the public landlock_insert_rule() API. This
helps avoid misuse of this function.
* Replace a remaining hardcoded 1 with SINGLE_DEPTH_NESTING.
Changes since v22:
* Explicitely use RB_ROOT and SINGLE_DEPTH_NESTING (suggested by Jann
Horn).
* Improve comments and fix spelling (suggested by Jann Horn).
Changes since v21:
* Add and clean up comments.
Changes since v18:
* Account rulesets to kmemcg.
* Remove struct holes.
* Cosmetic changes.
Changes since v17:
* Move include/uapi/linux/landlock.h and _LANDLOCK_ACCESS_FS_* to a
following patch.
Changes since v16:
* Allow enforcement of empty ruleset, which enables deny-all policies.
Changes since v15:
* Replace layer_levels and layer_depth with a bitfield of layers, cf.
filesystem commit.
* Rename the LANDLOCK_ACCESS_FS_{UNLINK,RMDIR} with
LANDLOCK_ACCESS_FS_REMOVE_{FILE,DIR} because it makes sense to use
them for the action of renaming a file or a directory, which may lead
to the removal of the source file or directory. Removes the
LANDLOCK_ACCESS_FS_{LINK_TO,RENAME_FROM,RENAME_TO} which are now
replaced with LANDLOCK_ACCESS_FS_REMOVE_{FILE,DIR} and
LANDLOCK_ACCESS_FS_MAKE_* .
* Update the documentation accordingly and highlight how the access
rights are taken into account.
* Change nb_rules from atomic_t to u32 because it is not use anymore by
show_fdinfo().
* Add safeguard for level variables types.
* Check max number of rules.
* Replace struct landlock_access (self and beneath bitfields) with one
bitfield.
* Remove useless variable.
* Add comments.
Changes since v14:
* Simplify the object, rule and ruleset management at the expense of a
less aggressive memory freeing (contributed by Jann Horn, with
additional modifications):
- Make a domain immutable (remove the opportunistic cleaning).
- Remove RCU pointers.
- Merge struct landlock_ref and struct landlock_ruleset_elem into
landlock_rule: get ride of rule's RCU.
- Adjust union.
- Remove the landlock_insert_rule() check about a new object with the
same address as a previously disabled one, because it is not
possible to disable a rule anymore.
Cf.
https://lore.kernel.org/lkml/cag48ez21ben0wl1bbmtiiu8j9jp5iewthowz4turuj+ki0y...@mail.gmail.com/
* Fix nested domains by implementing a notion of layer level and depth:
- Update landlock_insert_rule() to manage such layers.
- Add an inherit_ruleset() helper to properly create a new domain.
- Rename landlock_find_access() to landlock_find_rule() and return a
full rule reference.
- Add a layer_level and a layer_depth fields to struct landlock_rule.
- Add a top_layer_level field to struct landlock_ruleset.
* Remove access rights that may be required for FD-only requests:
truncate, getattr, lock, chmod, chown, chgrp, ioctl. This will be
handle in a future evolution of Landlock, but right now the goal is to
lighten the code to ease review.
* Remove LANDLOCK_ACCESS_FS_OPEN and rename
LANDLOCK_ACCESS_FS_{READ,WRITE} with a FILE suffix.
* Rename LANDLOCK_ACCESS_FS_READDIR to match the *_FILE pattern.
* Remove LANDLOCK_ACCESS_FS_MAP which was useless.
* Fix memory leak in put_hierarchy() (reported by Jann Horn).
* Fix
