[PATCH v4 1/4] landlock: Fix handling of disconnected directories

[Tingmao Wang]

Hi Mickaël,

I think this implementation makes sense - to me this feels better than
ignoring rules between the leaf and the mount when disconnected, given the
interaction with domain checks.  This approach is also simpler in code.

However, there is one caveat which, while requiring a slightly problematic
policy to happen in the first place, might still be a bit surprising: if,
for some reason, there are rules "hidden" in the "real" parent of a (bind)
mounted dir, a sandboxed program that is able to cause directories to be
disconnected (for example, because there are more bind mounts within the
bind mount, and the program has enough rename access (but not read/write))
may be able to "surface" those rules and "gain access" (requires the
existance of the already questionable "hidden" rule):

  root at g3ef6e4434e3a-dirty /# mkdir -p /hidden/bind1_src /bind1_dst
  /# cd hidden
  /hidden# mount --bind bind1_src /bind1_dst
  /hidden# mkdir -p bind1_src/bind2_src/dir bind1_src/bind2_dst
  /hidden# mount --bind /bind1_dst/bind2_src /bind1_dst/bind2_dst
  /hidden# echo secret > bind1_src/bind2_src/dir/secret
  /hidden# ls -la /bind1_dst/bind2_dst/dir/secret 
  -rw-r--r-- 1 root root 7 Nov 28 00:49 /bind1_dst/bind2_dst/dir/secret
  /hidden# mount -t tmpfs none /hidden
  /hidden# ls .
  bind1_src/
  /hidden# ls /hidden
  /hidden# LL_FS_RO=/usr:/bin:/lib:/etc:. LL_FS_RW= LL_FS_CREATE_DELETE_REFER=./bind1_src /sandboxer bash
                                        ^ this attaches a read rule to a "invisible" dir
  Executing the sandboxed command...
  /hidden# cd /
  /# ls /hidden
  ls: cannot open directory '/hidden': Permission denied
  /# cd /bind1_dst/bind2_dst/dir       
  /bind1_dst/bind2_dst/dir# cat secret
  cat: secret: Permission denied
  /bind1_dst/bind2_dst/dir# mv -v /bind1_dst/bind2_src/dir /bind1_dst/outside
  renamed '/bind1_dst/bind2_src/dir' -> '/bind1_dst/outside'
  /bind1_dst/bind2_dst/dir# ls ..
  ls: cannot access '..': No such file or directory
  /bind1_dst/bind2_dst/dir# cat secret
  secret

Earlier I was thinking we could make domain check for rename/links
stricter, in that it would make sure there are no rules granting more
access on the destination than what's granted by the "visible" rules on
the source even if those rules are "hidden" within the fs above the
mountpoint.  This way, the application would not be able to move the
source's parent to cause a disconnection in the first place.  However, I'm
not sure if this is worth the complication (e.g. in the case of exchange
rename, source is also the destination, and so this check needs to also
check that there are no "hidden" rules on the source that grants more access
than the "visible" rules on the destination).

I see another approach to mitigate this - we can disallow (return with
-EXDEV probably) rename/links altogether when the destination (and also
source if exchange) contains "hidden" rules that grants more access than
the "visible" rules.  However this approach would break backward
compatibility if a sandboxer or Landlock-enlightened application creates
such problematic policies (most likely unknowingly).

Stepping back a bit, I also think it is reasonable to leave this issue as
is and not mitigate it (maybe warn about it in some way in the docs),
given that this can only happen if the policy is already weird (if the
intention is to protect some file, setting an allow access rule on its
parent, even if that parent is "hidden", is questionable).

Not sure which is best, but even with this issue this patch is probably
still an improvement over the existing behavior (i.e. the one currently in
mainline, where if the path is disconnected, the "hidden" rules are used
and any "normal" rules from mnt_parent and above are ignored).

Reviewed-by: Tingmao Wang <m at maowtm.org>

Kind regards,
Tingmao