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:

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)


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.

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