12/2/2022 4:01 PM, Mickaël Salaün пишет:

On 02/12/2022 04:13, Konstantin Meskhidze (A) wrote:

quoted

11/29/2022 12:00 AM, Mickaël Salaün пишет:

quoted

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.

quoted

On 28/11/2022 09:21, Konstantin Meskhidze (A) wrote:

quoted

11/17/2022 9:43 PM, Mickaël Salaün пишет:

quoted

On 21/10/2022 17:26, Konstantin Meskhidze wrote:

quoted

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.

quoted

quoted

Signed-off-by: Konstantin Meskhidze <redacted>
---

[...]

quoted

quoted

quoted

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

quoted

quoted

+
+		/*
+		 * 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.

quoted

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@huawei.com/ (local)

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@mail.gmail.com/ (local)

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.

quoted

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@mail.gmail.com/ (local)

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:

quoted

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.

quoted

quoted

We could then reduce the hook codes to just:
return current_check_access_socket(sock, address, LANDLOCK_ACCESS_NET_*);
.

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?

.