Message-ID: <Y/fl5iEbkL5Pj5cJ@galopp>
Date:   Thu, 23 Feb 2023 23:17:10 +0100
From:   Günther Noack <gnoack3000@...il.com>
To:     Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Cc:     mic@...ikod.net, willemdebruijn.kernel@...il.com,
        linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org, yusongping@...wei.com,
        artem.kuzin@...wei.com
Subject: Re: [PATCH v9 00/12] Network support for Landlock

Hello Konstantin!

Sorry for asking such fundamental questions again so late in the review.

After playing with patch V9 with the Go-Landlock library, I'm still
having trouble understanding these questions -- they probably have
good answers, but I also did not see them explained in the
documentation. Maybe it would help to clarify it there?

* What is the use case for permitting processes to connect to a given
  TCP port, but leaving unspecified what the IP address is?

  Example: If a Landlock ruleset permits connecting to TCP port 53,
  that makes it possible to talk to any IP address on the internet (at
  least if the process runs on a normal Linux desktop machine), and we
  can't really control whether that is the system's proper (TCP-)DNS
  server or whether that is an attacker-controlled service for
  accepting leaked secrets from the process...?

  Is the plan that IP address support should be added in a follow-up
  patch?  Will it become part of the landlock_net_service_attr struct?

* Given the list of obscure network protocols listed in the socket(2)
  man page, I find it slightly weird to have rules for the use of TCP,
  but to leave less prominent protocols unrestricted.

  For example, a process with an enabled Landlock network ruleset may
  connect only to certain TCP ports, but at the same time it can
  happily use Bluetooth/CAN bus/DECnet/IPX or other protocols?

  I'm mentioning these more obscure protocols, because I doubt that
  Landlock will grow more sophisticated support for them anytime soon,
  so maybe the best option would be to just make it possible to
  disable these?  Is that also part of the plan?

  (I think there would be a lot of value in restricting network
  access, even when it's done very broadly.  There are many programs
  that don't need network at all, and among those that do need
  network, most only require IP networking.

  Btw, the argument for more broad disabling of network access was
  already made at https://cr.yp.to/unix/disablenetwork.html in the
  past.)

* This one is more of an implementation question: I don't understand
  why we are storing the networking rules in the same RB tree as the
  file system rules. - It looks a bit like "YAGNI" to me...?

  Would it be more efficient to keep the file system rules in the
  existing RB tree, and store the networking rules *separately* next
  to it in a different RB tree, or even in a more optimized data
  structure? In pseudocode:

    struct fast_lookup_int_set bind_tcp_ports;
    struct fast_lookup_int_set connect_tcp_ports;
    struct landlock_rb_tree fs_rules;

  It seems that there should be a data structure that supports this
  well and which uses the fact that we only need to store small
  integers?

Thanks,
–Günther

P.S.: Apologies if some of it was discussed previously. I did my best
to catch up on previous threads, but it's long, and it's possible that
I missed parts of the discussion.

On Mon, Jan 16, 2023 at 04:58:06PM +0800, Konstantin Meskhidze wrote:
> Hi,
> This is a new V9 patch related to Landlock LSM network confinement.
> It is based on the landlock's -next branch on top of v6.2-rc3 kernel version:
> https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
> 
> It brings refactoring of previous patch version V8.
> Mostly there are fixes of logic and typos, adding new tests.
> 
> All test were run in QEMU evironment and compiled with
>  -static flag.
>  1. network_test: 32/32 tests passed.
>  2. base_test: 7/7 tests passed.
>  3. fs_test: 78/78 tests passed.
>  4. ptrace_test: 8/8 tests passed.
> 
> Previous versions:
> v8: https://lore.kernel.org/linux-security-module/20221021152644.155136-1-konstantin.meskhidze@huawei.com/
> v7: https://lore.kernel.org/linux-security-module/20220829170401.834298-1-konstantin.meskhidze@huawei.com/
> v6: https://lore.kernel.org/linux-security-module/20220621082313.3330667-1-konstantin.meskhidze@huawei.com/
> v5: https://lore.kernel.org/linux-security-module/20220516152038.39594-1-konstantin.meskhidze@huawei.com
> v4: https://lore.kernel.org/linux-security-module/20220309134459.6448-1-konstantin.meskhidze@huawei.com/
> v3: https://lore.kernel.org/linux-security-module/20220124080215.265538-1-konstantin.meskhidze@huawei.com/
> v2: https://lore.kernel.org/linux-security-module/20211228115212.703084-1-konstantin.meskhidze@huawei.com/
> v1: https://lore.kernel.org/linux-security-module/20211210072123.386713-1-konstantin.meskhidze@huawei.com/
> 
> Konstantin Meskhidze (11):
>   landlock: Make ruleset's access masks more generic
>   landlock: Refactor landlock_find_rule/insert_rule
>   landlock: Refactor merge/inherit_ruleset functions
>   landlock: Move and rename umask_layers() and init_layer_masks()
>   landlock: Refactor _unmask_layers() and _init_layer_masks()
>   landlock: Refactor landlock_add_rule() syscall
>   landlock: Add network rules and TCP hooks support
>   selftests/landlock: Share enforce_ruleset()
>   selftests/landlock: Add 10 new test suites dedicated to network
>   samples/landlock: Add network demo
>   landlock: Document Landlock's network support
> 
> Mickaël Salaün (1):
>   landlock: Allow filesystem layout changes for domains without such
>     rule type
> 
>  Documentation/userspace-api/landlock.rst     |   72 +-
>  include/uapi/linux/landlock.h                |   49 +
>  samples/landlock/sandboxer.c                 |  131 +-
>  security/landlock/Kconfig                    |    1 +
>  security/landlock/Makefile                   |    2 +
>  security/landlock/fs.c                       |  255 ++--
>  security/landlock/limits.h                   |    7 +-
>  security/landlock/net.c                      |  200 +++
>  security/landlock/net.h                      |   26 +
>  security/landlock/ruleset.c                  |  409 +++++--
>  security/landlock/ruleset.h                  |  185 ++-
>  security/landlock/setup.c                    |    2 +
>  security/landlock/syscalls.c                 |  165 ++-
>  tools/testing/selftests/landlock/base_test.c |    2 +-
>  tools/testing/selftests/landlock/common.h    |   10 +
>  tools/testing/selftests/landlock/config      |    4 +
>  tools/testing/selftests/landlock/fs_test.c   |   75 +-
>  tools/testing/selftests/landlock/net_test.c  | 1157 ++++++++++++++++++
>  18 files changed, 2398 insertions(+), 354 deletions(-)
>  create mode 100644 security/landlock/net.c
>  create mode 100644 security/landlock/net.h
>  create mode 100644 tools/testing/selftests/landlock/net_test.c
> 
> -- 
> 2.25.1
> 

Powered by blists - more mailing lists