[RFC PATCH v3 01/19] landlock: Support socket access-control

[Mickaël Salaün]

On Thu, Nov 28, 2024 at 03:01:52PM +0300, Mikhail Ivanov wrote:
> On 11/27/2024 9:43 PM, Mickaël Salaün wrote:
> > 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.
> 
> Agreed
> 
> > 
> > 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.
> 
> Do you mean that Landlock should not make any assumptions about this
> values during a build time? Currently, patchset provides boundary checks
> for domain (< AF_MAX) and type (< SOCK_MAX) in landlock_add_rule().

The *running kernel* may not support some socket's domains or types,
which may be confusing for users if the rule was tested on a kernel
supporting such domains/types.

For the bitmask of domains or types, the issues to keep boundary checks
would be when a subset of them is not supported.  Landlock would reject
such rule and it would be difficult for users to identify the cause.

I'm still wondering if the landlock_append_net_rule()'s -EAFNOSUPPORT
return value for kernels without CONFIG_INET was a good idea.  We should
probably return 0 in this case, which would be similar to not checking
socket's domains nor types.

> 
> > 
> > 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
> >    };
> 
> Looks good! I think it's a nice approach which will provide a sufficient
> level of flexibility to define a single rule for a specific protocol (or
> for related protocols).
> 
> But, this adds possibility to define a single rule for the set of
> unrelated protocols:
> 
> /* Allows TCP, UDP and UNIX sockets. */
> const struct landlock_socket_attr create_socket_attr = {
> 	.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> 	.families = 1 << AF_INET | 1 << AF_INET6 | 1 << AF_UNIX,
> 	.types = 1 << SOCK_STREAM | 1 << SOCK_DGRAM,
> 	.protocol = 0
> };
> 
> Perhaps limiting the addition of one rule to only one address family
> would be more clear in terms of rule semantics?:
> 
> /* Allows TCP, UDP, UNIX STREAM, UNIX DGRAM sockets. */
> const struct landlock_socket_attr create_socket_attrs[] = {
> 	{
> 		/* Allows IPv4 TCP and UDP sockets. */
> 		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> 		.family = AF_INET,
> 		.types = 1 << SOCK_STREAM | 1 << SOCK_DGRAM,
> 		.protocol = 0
> 	},
> 	{
> 		/* Allows IPv6 TCP and UDP sockets. */
> 		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> 		.family = AF_INET6,
> 		.types = 1 << SOCK_STREAM | 1 << SOCK_DGRAM,
> 		.protocol = 0
> 	},
> 	{
> 		/* Allows UNIX sockets. */
> 		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> 		.family = AF_UNIX,
> 		.types = 1 << SOCK_STREAM | 1 << SOCK_DGRAM,
> 		.protocol = 0
> 	},
> };

Because we are already mixing bitmasks and (protocol) value, I'm not
sure it will help much.  I think in most cases the "families" bitmask
would handle IPv4 and IPv6 the same (e.g. to only allow TCP with one
rule).  I think this is also required to be able to have a 1:1 mapping
with SELinux's socket_type_to_security_class().

> 
> > 
> > 
> > > > 
> > > > 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.
> 
> I agree, implementing this in a library is a better approach. Thanks for
> the catch!
> 
> > 
> > > 
> > > > 
> > > > 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
> > > 
>