On 05/12/2022 03:55, Konstantin Meskhidze (A) wrote:
12/2/2022 4:01 PM, Mickaël Salaün пишет:
On 02/12/2022 04:13, Konstantin Meskhidze (A) wrote:
11/29/2022 12:00 AM, Mickaël Salaün пишет:
The previous commit provides an interface to theoretically restrict
network access (i.e. ruleset handled network accesses), but in fact this
is not enforced until this commit. I like this split but to avoid any
inconsistency, please squash this commit into the previous one: "7/12
landlock: Add network rules support"
You should keep all the commit messages but maybe tweak them a bit.
Ok. Will be squashed.
On 28/11/2022 09:21, Konstantin Meskhidze (A) wrote:
11/17/2022 9:43 PM, Mickaël Salaün пишет:
On 21/10/2022 17:26, Konstantin Meskhidze wrote:
This patch adds support of socket_bind() and socket_connect() hooks.
It's possible to restrict binding and connecting of TCP sockets to
particular ports.
Implement socket_bind() and socket_connect LSM hooks, which enable to
restrict TCP socket binding and connection to specific ports.
Ok. Thanks.
Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@xxxxxxxxxx>
---
[...]
+static int hook_socket_connect(struct socket *sock, struct sockaddr *address,
+ int addrlen)
+{
+ const struct landlock_ruleset *const dom =
+ landlock_get_current_domain();
+
+ if (!dom)
+ return 0;
+
+ /* Check if it's a TCP socket. */
+ if (sock->type != SOCK_STREAM)
+ return 0;
+
+ /* Check if the hook is AF_INET* socket's action. */
+ switch (address->sa_family) {
+ case AF_INET:
+#if IS_ENABLED(CONFIG_IPV6)
+ case AF_INET6:
+#endif
+ return check_socket_access(dom, get_port(address),
+ LANDLOCK_ACCESS_NET_CONNECT_TCP);
+ case AF_UNSPEC: {
+ u16 i;
You can move "i" after the "dom" declaration to remove the extra braces.
Ok. Thanks.
+
+ /*
+ * If just in a layer a mask supports connect access,
+ * the socket_connect() hook with AF_UNSPEC family flag
+ * must be banned. This prevents from disconnecting already
+ * connected sockets.
+ */
+ for (i = 0; i < dom->num_layers; i++) {
+ if (landlock_get_net_access_mask(dom, i) &
+ LANDLOCK_ACCESS_NET_CONNECT_TCP)
+ return -EACCES;
I'm wondering if this is the right error code for this case. EPERM may
be more appropriate.
Ok. Will be refactored.
Thinking more about this case, I don't understand what is the rationale
to deny such action. What would be the consequence to always allow
connection with AF_UNSPEC (i.e. to disconnect a socket)?
I thought we have come to a conclusion about connect(...AF_UNSPEC..)
behaviour in the patchset V3:
https://lore.kernel.org/linux-security-module/19ad3a01-d76e-0e73-7833-99acd4afd97e@xxxxxxxxxx/
The conclusion was that AF_UNSPEC disconnects a socket, but I'm asking
if this is a security issue. I don't think it is more dangerous than a
new (unconnected) socket. Am I missing something? Which kind of rule
could be bypassed? What are we protecting against by restricting AF_UNSPEC?
I just follow Willem de Bruijn concerns about this issue:
quote: "It is valid to pass an address with AF_UNSPEC to a PF_INET(6)
socket. And there are legitimate reasons to want to deny this. Such as
passing a connection to a unprivileged process and disallow it from
disconnect and opening a different new connection."
https://lore.kernel.org/linux-security-module/CA+FuTSf4EjgjBCCOiu-PHJcTMia41UkTh8QJ0+qdxL_J8445EA@xxxxxxxxxxxxxx/
I agree with the fact that we want to deny this, but in this example the
new connection should still be restricted by the Landlock domain. Using
AF_UNSPEC on a connected socket should not make this socket allowed to
create any connection if the process is restricted with TCP_CONNECT.
Being allowed to close a connection should not be an issue, and any new
connection must be vetted by Landlock.
You are right. This makes sense. Thanks for the comment.
quote: "The intended use-case is for a privileged process to open a
connection (i.e., bound and connected socket) and pass that to a
restricted process. The intent is for that process to only be allowed to
communicate over this pre-established channel.
In practice, it is able to disconnect (while staying bound) and
elevate its privileges to that of a listening server: ..."
https://lore.kernel.org/linux-security-module/CA+FuTScaoby-=xRKf_Dz3koSYHqrMN0cauCg4jMmy_nDxwPADA@xxxxxxxxxxxxxx/
Looks like it's a security issue here.
It the provided example, if child_process() is restricted with
TCP_CONNECT and TCP_BIND, any call to connect() or bind() will return an
access error. listen() and accept() would work if the socket is bound,
which is the case here, and then implicitly allowed by the parent
process. I don' see any security issue. Am I missing something?
In fact, connect with AF_UNSPEC should always be allowed to be
consistent with close(2), which is a way to drop privileges.
It should be allowed with checking:
"return check_socket_access(dom, get_port(address),
LANDLOCK_ACCESS_NET_CONNECT_TCP);
What Willem said:
It would be good to also
ensure that a now-bound socket cannot call listen.
This is not relevant for Landlock because the security model is to check
process's requests to get new accesses (e.g. create a new file
descriptor), but not to check passed accesses (e.g. inherited from a
parent process, or pass through a unix socket) which are delegated to
the sender/parent. The goal of a sandbox is to limit the set of new
access requested (to the kernel) from within this sandbox. All already
opened file descriptors were previously vetted by Landlock (and other
access control systems).
I got your point. Thanks.
We could then reduce the hook codes to just:
return current_check_access_socket(sock, address, LANDLOCK_ACCESS_NET_*);
This current_check_access_socket() helper should contain all the access
control code.
.
As for SELinux, the connect hook should first do this check (with an
appropriate comment):
if (address->sa_family == AF_UNSPEC)
return 0;
In case of Landlock it looks like a landlocked process could connnect
to the ports it's not allowed to connect to.
So we need just to return check_socket_access(dom, get_port(address),
LANDLOCK_ACCESS_NET_CONNECT_TCP);
I'm I correct? Did I miss something?
Using AF_UNSPEC with connect(2) doesn't connect the socket to a port,
and in fact completely ignore the port. We can move the AF_UNSPEC check
to the current_check_access_socket() helper:
switch (address->sa_family) {
case AF_UNSPEC:
+ /*
+ * Connecting to an address with AF_UNSPEC dissolves the TCP
+ * association, which have the same effect as closing the
+ * connection while retaining the socket object (i.e., the file
+ * descriptor). As for dropping privileges, closing
+ * connections is always allowed.
+ */
+ if (access_request == LANDLOCK_ACCESS_NET_CONNECT_TCP)
+ return 0;
+
+ /*
+ * For compatibility reason, accept AF_UNSPEC for bind
+ * accesses (mapped to AF_INET) only if the address is
+ * INADDR_ANY (cf. __inet_bind). Checking the address is
+ * required to not wrongfully return -EACCES instead of
+ * -EAFNOSUPPORT.
+ */
+ if (access_request == LANDLOCK_ACCESS_NET_BIND_TCP) {
+ const struct sockaddr_in *const sockaddr =
+ (struct sockaddr_in *)address;
+
+ if (sockaddr->sin_addr.s_addr != htonl(INADDR_ANY))
+ return -EAFNOSUPPORT;
+ }
+
+ fallthrough;
case AF_INET:
#if IS_ENABLED(CONFIG_IPV6)
case AF_INET6:
I also added another check (copied from SELinux) with the appropriate
explanation. All this needs dedicated tests to make sure everything is
covered.
We also need to add extra checks (and related tests) for addrlen as do
other LSMs.
