Re: [RFC PATCH v1 0/7] Landlock audit support

["Günther Noack" <gnoack@google.com>]

Hi Mickaël!

On Thu, Sep 21, 2023 at 08:16:34AM +0200, Mickaël Salaün wrote:
> This patch series adds basic audit support to Landlock for most actions.
> Logging denied requests is useful for different use cases:
> * app developers: to ease and speed up sandboxing support
> * power users: to understand denials
> * sysadmins: to look for users' issues
> * tailored distro maintainers: to get usage metrics from their fleet
> * security experts: to detect attack attempts
> 
> To make logs useful, they need to contain the most relevant Landlock
> domain that denied an action, and the reason. This translates to the
> latest nested domain and the related missing access rights.

Is "domain" always the latest nested domain, or is that the domain which caused
the check to fail because it denied the requested access right?  (If it is just
the counter of how many domains are stacked, this could maybe also be queried
through proc instead?)


> Two "Landlock permissions" are used to describe mandatory restrictions
> enforced on all domains:
> * fs_layout: change the view of filesystem with mount operations.
> * ptrace: tamper with a process.

I find the term "access" already a bit overloaded, and the term "permission"
also already appears in other contexts.  Maybe we can avoid the additional
terminology by grouping these two together in the log format, and calling them
the "cause" or "reason" for the deny decision?  In a sense, the access rights
and the other permissions can already be told apart by their names, so they
might also both appear under the same key without causing additional confusion?


> Here is an example of logs, result of the sandboxer activity:
> tid=267 comm="sandboxer" op=create-ruleset ruleset=1 handled_access_fs=execute,write_file,read_file,read_dir,remove_dir,remove_file,make_char,make_dir,make_reg,make_sock,make_fifo,make_block,make_sym,refer,truncate
> tid=267 comm="sandboxer" op=restrict-self domain=2 ruleset=1 parent=0
> op=release-ruleset ruleset=1
> tid=267 comm="bash" domain=2 op=open errno=13 missing-fs-accesses=write_file,read_file missing-permission= path="/dev/tty" dev="devtmpfs" ino=9
> tid=268 comm="ls" domain=2 op=open errno=13 missing-fs-accesses=read_dir missing-permission= path="/" dev="vda2" ino=256
> tid=269 comm="touch" domain=2 op=mknod errno=13 missing-fs-accesses=make_reg missing-permission= path="/" dev="vda2" ino=256
> tid=270 comm="umount" domain=2 op=umount errno=1 missing-fs-accesses= missing-permission=fs_layout name="/" dev="tmpfs" ino=1
> tid=271 comm="strace" domain=2 op=ptrace errno=1 missing-fs-accesses= missing-permission=ptrace opid=1 ocomm="systemd"

In more complicated cases like "refer" and "open", it is possible that more than
one access right is missing, and presumably they'll both be listed in
missing-fs-accesses=.  In this case, it is not clear to me whether the domain=
number is referring to the first or the second of these missing rights.
(Assuming that the domain= is about the domain which caused the denial.)


> As highlighted in comments, support for audit is not complete yet with
> this series: some actions are not logged (e.g. file reparenting), and
> rule additions are not logged neither.

When ftruncate(2) gets denied, it is also not possible to tell which of the
nested domains is responsible, without additional changes to what we carry
around in the file's security blob.  (Right now, we calculate the overall
truncation right in advance at open(2) time, and just store that bit with the
newly opened file.)


> I'm also not sure if we need to have seccomp-like features such as
> SECCOMP_FILTER_FLAG_LOG, SECCOMP_RET_LOG, and
> /proc/sys/kernel/seccomp/actions_logged
> 
> I'd like to get some early feedback on this proposal.

If you want to have the full feature set as proposed above for other operations
as well, like file reparenting and truncation, it'll complicate the Landlock
logic and increase the amount of data that needs to be kept around just for
logging.  I'm not convinced that this is worth it.  After all, the simpler the
Landlock implementation is, the easier it'll be to reason about its logic and
its security guarantees.

A possible simplification would be to omit the domain number which is
responsible for a "deny" decision.  I feel that for debugging, knowing the fact
that Landlock denied an operation might already be a big step forward, and the
exact domain responsible for it might not be that important?

—Günther

-- 
Sent using Mutt 🐕 Woof Woof