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@xxxxxxxxxxxxxx/
> > > >
> > > > 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@xxxxxxxxxxxxxxxxx/
> > >
> > > >
> > > >
> > > > > 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
> > >
>
