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.


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

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

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

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