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

["Günther Noack" <gnoack@google.com>]

Hello Mikhail,

sorry for the delayed response;
I am very happy to see activity on this patch set! :)

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.)


> 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.

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?


> 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.

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?

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, *)?

—Günther