Message-ID: <20241127.oophah4Ueboo@digikod.net>
Date: Wed, 27 Nov 2024 19:43:53 +0100
From: Mickaël Salaün <mic@...ikod.net>
To: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
Cc: Günther Noack <gnoack@...gle.com>, 
	willemdebruijn.kernel@...il.com, gnoack3000@...il.com, linux-security-module@...r.kernel.org, 
	netdev@...r.kernel.org, netfilter-devel@...r.kernel.org, yusongping@...wei.com, 
	artem.kuzin@...wei.com, konstantin.meskhidze@...wei.com
Subject: Re: [RFC PATCH v3 01/19] landlock: Support socket access-control

On Mon, Nov 25, 2024 at 02:04:09PM +0300, Mikhail Ivanov wrote:
> On 11/22/2024 8:45 PM, Günther Noack wrote:
> > Hello Mikhail,
> > 
> > sorry for the delayed response;
> > I am very happy to see activity on this patch set! :)
> 
> Hello Günther,
> No problem, thanks a lot for your feedback!
> 
> > 
> > On Mon, Nov 11, 2024 at 07:29:49PM +0300, Mikhail Ivanov wrote:
> > > On 9/4/2024 1:48 PM, Mikhail Ivanov wrote:
> > > > Landlock implements the `LANDLOCK_RULE_NET_PORT` rule type, which provides
> > > > fine-grained control of actions for a specific protocol. Any action or
> > > > protocol that is not supported by this rule can not be controlled. As a
> > > > result, protocols for which fine-grained control is not supported can be
> > > > used in a sandboxed system and lead to vulnerabilities or unexpected
> > > > behavior.
> > > > 
> > > > Controlling the protocols used will allow to use only those that are
> > > > necessary for the system and/or which have fine-grained Landlock control
> > > > through others types of rules (e.g. TCP bind/connect control with
> > > > `LANDLOCK_RULE_NET_PORT`, UNIX bind control with
> > > > `LANDLOCK_RULE_PATH_BENEATH`). Consider following examples:
> > > > 
> > > > * Server may want to use only TCP sockets for which there is fine-grained
> > > >     control of bind(2) and connect(2) actions [1].
> > > > * System that does not need a network or that may want to disable network
> > > >     for security reasons (e.g. [2]) can achieve this by restricting the use
> > > >     of all possible protocols.
> > > > 
> > > > This patch implements such control by restricting socket creation in a
> > > > sandboxed process.
> > > > 
> > > > Add `LANDLOCK_RULE_SOCKET` rule type that restricts actions on sockets.
> > > > This rule uses values of address family and socket type (Cf. socket(2))
> > > > to determine sockets that should be restricted. This is represented in a
> > > > landlock_socket_attr struct:
> > > > 
> > > >     struct landlock_socket_attr {
> > > >       __u64 allowed_access;
> > > >       int family; /* same as domain in socket(2) */
> > > >       int type; /* see socket(2) */
> > > >     };
> > > 
> > > Hello! I'd like to consider another approach to define this structure
> > > before sending the next version of this patchset.
> > > 
> > > Currently, it has following possible issues:
> > > 
> > > First of all, there is a lack of protocol granularity. It's impossible
> > > to (for example) deny creation of ICMP and SCTP sockets and allow TCP
> > > and UDP. Since the values of address family and socket type do not
> > > completely define the protocol for the restriction, we may gain
> > > incomplete control of the network actions. AFAICS, this is limited to
> > > only a couple of IP protocol cases (e.g. it's impossible to deny SCTP
> > > and SMC sockets to only allow TCP, deny ICMP and allow UDP).
> > > 
> > > But one of the main advantages of socket access rights is the ability to
> > > allow only those protocols for which there is a fine-grained control
> > > over their actions (TCP bind/connect). It can be inconvenient
> > > (and unsafe) for SCTP to be unrestricted, while sandboxed process only
> > > needs TCP sockets.
> > 
> > That is a good observation which I had missed.
> > 
> > I agree with your analysis, I also see the main use case of socket()
> > restrictions in:
> > 
> >   (a) restricting socket creating altogether
> >   (b) only permitting socket types for which there is fine grained control
> > 
> > and I also agree that it would be very surprising when the same socket types
> > that provide fine grained control would also open the door for unrestricted
> > access to SMC, SCTP or other protocols.  We should instead strive for a
> > socket() access control with which these additional protocols weren't
> > accessible.
> > 
> > 
> > > Adding protocol (Cf. socket(2)) field was considered a bit during the
> > > initial discussion:
> > > https://lore.kernel.org/all/CABi2SkVWU=Wxb2y3fP702twyHBD3kVoySPGSz2X22VckvcHeXw@mail.gmail.com/
> > 
> > So adding "protocol" to the rule attributes would suffice to restrict the use of
> > SMC and SCTP then?  (Sorry, I lost context on these protocols a bit in the
> > meantime, I was so far under the impression that these were using different
> > values for family and type than TCP and UDP do.)
> 
> Yeap. Following rule will be enough to allow TCP sockets only:
> 
> const struct landlock_socket_attr create_socket_attr = {
> 	.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> 	.family = AF_INET{,6},
> 	.type = SOCK_STREAM,
> 	.protocol = 0
> };

We should indeed include the protocol type in the rule definition.

> 
> Btw, creation of SMC sockets via IP stack was added quite recently.
> So far, creation has been possible only with AF_SMC family.
> 
> https://lore.kernel.org/all/1718301630-63692-1-git-send-email-alibuda@linux.alibaba.com/
> 
> > 
> > 
> > > Secondly, I'm not really sure if socket type granularity is required
> > > for most of the protocols. It may be more convenient for the end user
> > > to be able to completely restrict the address family without specifying
> > > whether restriction is dedicated to stream or dgram sockets (e.g. for
> > > BLUETOOTH, VSOCK sockets). However, this is not a big issue for the
> > > current design, since address family can be restricted by specifying
> > > type = SOCK_TYPE_MASK.

It looks like SOCK_TYPE_MASK is not part of UAPI, which means it could
change with kernel versions (even while being in UAPI in fact).  This
new socket creation control should allow to deny any socket creation
known or unknow at the time of the user space program build, and
whatever the available C headers.

This also means that Landlock should accept any domain, type, and
protocols defined in rules.  Indeed, we don't want to reject rules for
which some protocols are not allowed.

What about using bitmasks for the domain and type fields (renamed to
"domains" and "types")?  The last protocol is currently 45/MCTP so a
64-bit field is enough, and 10/SOCK_PACKET also fits for the last socket
type.

We cannot do the same with the protocol because the higher one is
262/MPTCP though.  But it looks like a value of 0 (default protocol)
should be enough for most use cases, and users could specify a protocol
(but this time as a number, not a bitmask).

To sum up, we could have something like this:

  const struct landlock_socket_attr create_socket_attr = {
  	.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
  	.families = 1 << AF_INET | 1 << AF_INET6,
  	.types = 1 << SOCK_STREAM,
  	.protocol = IPPROTO_SCTP
  };


> > 
> > Whether the user is adding one rule to permit AF_INET+*, or whether the user is
> > adding two rules to permit (1) AF_INET+SOCK_STREAM and (2) AF_INET+SOCK_DGRAM,
> > that does not seem like a big deal to me as long as the list of such
> > combinations is so low?
> 
> Agreed

I also agree, but this might change if users have to set a combination
of families, types, and protocols.  This should be OK with the bitmask
approach though.

> 
> > 
> > 
> > > I suggest implementing something close to selinux socket classes for the
> > > struct landlock_socket_attr (Cf. socket_type_to_security_class()). This
> > > will provide protocol granularity and may be simpler and more convenient
> > > in the terms of determining access rights. WDYT?
> > 
> > I see that this is a longer switch statement that maps to this enum, it would be
> > an additional data table that would have to be documented separately for users.
> 
> This table is the general drawback, since it makes API a bit more
> complex.
> 
> > 
> > Do you have an example for how such a "security class enum" would map to the
> > combinations of family, type and socket for the protocols discussed above?
> 
> I think the socket_type_to_security_class() has a pretty good mapping
> for UNIX and IP families.

The mapping looks good indeed, and it has been tested for a long time
with many applications.  However, this would make the kernel
implementation more complex, and I think this mapping could easily be
implemented in user space libraries with the bitmask approach, if really
needed, which I'm not sure.

> 
> > 
> > If this is just a matter of actually mapping (family, type, protocol)
> > combinations in a more flexible way, could we get away by allowing a special
> > "wildcard" value for the "protocol" field, when it is used within a ruleset?
> > Then the LSM would have to look up whether there is a rule for (family, type,
> > protocol) and the only change would be that it now needs to also check whether
> > there is a rule for (family, type, *)?
> 
> Something like this?
> 
> const struct landlock_socket_attr create_socket_attr = {
> 	.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> 	.family = AF_INET6,
> 	.type = SOCK_DGRAM,
> 	.protocol = LANDLOCK_SOCKET_PROTO_ALL
> };
> 
> > 
> > —Günther
> 

Powered by blists - more mailing lists