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