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/
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.
We could then reduce the hook codes to just:
return current_check_access_socket(sock, address, LANDLOCK_ACCESS_NET_*);
.
