On Fri, Jul 19, 2024 at 08:14:02PM +0200, Mickaël Salaün wrote:

On Wed, Jul 17, 2024 at 10:15:19PM -0600, Tahera Fahimi wrote:

quoted

The patch introduces a new "scoped" attribute to the
landlock_ruleset_attr that can specify "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET"
to scope abstract unix sockets from connecting to a process outside of
the same landlock domain.

This patch implement two hooks, "unix_stream_connect" and "unix_may_send" to
enforce this restriction.

Signed-off-by: Tahera Fahimi <redacted>

-------

Hello Mickaël, 
Thanks for the feedback.

Only "---"

quoted

v7:

Thanks for the detailed changelog, it helps!

quoted

 - Using socket's file credentials for both connected(STREAM) and
   non-connected(DGRAM) sockets.
 - Adding "domain_sock_scope" instead of the domain scoping mechanism used in
   ptrace ensures that if a server's domain is accessible from the client's
   domain (where the client is more privileged than the server), the client
   can connect to the server in all edge cases.
 - Removing debug codes.
v6:
 - Removing curr_ruleset from landlock_hierarchy, and switching back to use
   the same domain scoping as ptrace.
 - code clean up.
v5:
 - Renaming "LANDLOCK_*_ACCESS_SCOPE" to "LANDLOCK_*_SCOPE"
 - Adding curr_ruleset to hierarachy_ruleset structure to have access from
   landlock_hierarchy to its respective landlock_ruleset.
 - Using curr_ruleset to check if a domain is scoped while walking in the
   hierarchy of domains.
 - Modifying inline comments.
V4:
 - Rebased on Günther's Patch:
   https://lore.kernel.org/all/20240610082115.1693267-1-gnoack@google.com/ (local)
   so there is no need for "LANDLOCK_SHIFT_ACCESS_SCOPE", then it is removed.
 - Adding get_scope_accesses function to check all scoped access masks in a ruleset.
 - Using file's FD credentials instead of credentials stored in peer_cred
   for datagram sockets. (see discussion in [1])
 - Modifying inline comments.
V3:
 - Improving commit description.
 - Introducing "scoped" attribute to landlock_ruleset_attr for IPC scoping
   purpose, and adding related functions.
 - Changing structure of ruleset based on "scoped".
 - Removing rcu lock and using unix_sk lock instead.
 - Introducing scoping for datagram sockets in unix_may_send.
V2:
 - Removing wrapper functions

[1]https://lore.kernel.org/outreachy/Zmi8Ydz4Z6tYtpY1@tahera-OptiPlex-5000/T/#m8cdf33180d86c7ec22932e2eb4ef7dd4fc94c792 (local)

quoted

-------

Signed-off-by: Tahera Fahimi <redacted>

No need for this hunk.

quoted

---
 include/uapi/linux/landlock.h |  29 +++++++++
 security/landlock/limits.h    |   3 +
 security/landlock/ruleset.c   |   7 ++-
 security/landlock/ruleset.h   |  23 ++++++-
 security/landlock/syscalls.c  |  14 +++--
 security/landlock/task.c      | 112 ++++++++++++++++++++++++++++++++++
 6 files changed, 181 insertions(+), 7 deletions(-)

quoted

diff --git a/security/landlock/task.c b/security/landlock/task.c
index 849f5123610b..597d89e54aae 100644
--- a/security/landlock/task.c
+++ b/security/landlock/task.c

@@ -13,6 +13,8 @@
 #include <linux/lsm_hooks.h>
 #include <linux/rcupdate.h>
 #include <linux/sched.h>
+#include <net/sock.h>
+#include <net/af_unix.h>
 
 #include "common.h"
 #include "cred.h"

@@ -108,9 +110,119 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
 	return task_ptrace(parent, current);
 }
 
+static int walk_and_check(const struct landlock_ruleset *const child,
+			  struct landlock_hierarchy **walker, int i, int j,

We don't know what are "i" and "j" are while reading this function's
signature.  They need a better name.

Also, they are ingegers (signed), whereas l1 and l2 are size_t (unsigned).

quoted

+			  bool check)
+{
+	if (!child || i < 0)
+		return -1;
+
+	while (i < j && *walker) {

This would be more readable with a for() loop.

quoted

+		if (check && landlock_get_scope_mask(child, j))

This is correct now but it will be a bug when we'll have other scope.
Instead, you can replace the "check" boolean with a variable containing
LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET.

quoted

+			return -1;
+		*walker = (*walker)->parent;
+		j--;
+	}
+	if (!*walker)
+		pr_warn_once("inconsistency in landlock hierarchy and layers");

This must indeed never happen, but WARN_ON_ONCE(!*walker) would be
better than this check+pr_warn.

Anyway, if this happen this pointer will still be dereferenced in
domain_sock_scope() right?  This must not be possible.

quoted

+	return j;

Because j is now equal to i, no need to return it.  This function can
return a boolean instead, or a struct landlock_ruleset pointer/NULL to
avoid the pointer of pointer?

corret, in the next version, this function will return a boolean that
shows chcking the hierarchy of domain is successful or not. 

quoted

+}
+
+/**
+ * domain_sock_scope - Checks if client domain is scoped in the same
+ *			domain as server.
+ *
+ * @client: Connecting socket domain.
+ * @server: Listening socket domain.
+ *
+ * Checks if the @client domain is scoped, then the server should be
+ * in the same domain to connect. If not, @client can connect to @server.
+ */
+static bool domain_sock_scope(const struct landlock_ruleset *const client,

This function can have a more generic name if
LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET is passed as argument.  This could
be reused as-is for other kind of scope.

quoted

+			      const struct landlock_ruleset *const server)
+{
+	size_t l1, l2;
+	int scope_layer;
+	struct landlock_hierarchy *cli_walker, *srv_walker;

We have some room for a bit more characters ;)
client_walker, server_walker;

quoted

+
+	if (!client)
+		return true;
+
+	l1 = client->num_layers - 1;

Please rename variables in a consistent way, in this case something like
client_layer?

done 

quoted

+	cli_walker = client->hierarchy;
+	if (server) {
+		l2 = server->num_layers - 1;
+		srv_walker = server->hierarchy;
+	} else
+		l2 = 0;
+
+	if (l1 > l2)
+		scope_layer = walk_and_check(client, &cli_walker, l2, l1, true);

Instead of mixing the layer number with an error code, walk_and_check()
can return a boolean, take as argument &scope_layer, and update it.

quoted

+	else if (l2 > l1)
+		scope_layer =
+			walk_and_check(server, &srv_walker, l1, l2, false);
+	else
+		scope_layer = l1;
+
+	if (scope_layer == -1)
+		return false;

All these domains and layers checks are difficult to review. It needs at
least some comments, and preferably also some code refactoring to avoid
potential inconsistencies (checks).

quoted

+
+	while (scope_layer >= 0 && cli_walker) {

Why srv_walker is not checked?  Could this happen?  What would be the
result?

This is the same scenario as "walk_and_check". If the loop breaks
because of cli_walker is null, then there is an inconsistency between
num_layers and landlock_hierarchy. In normal scenario, we expect the
loop breaks with condition(scope_layer>=0).

Please also use a for() loop here.

quoted

+		if (landlock_get_scope_mask(client, scope_layer) &
+		    LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET) {

The logic needs to be explained.

quoted

+			if (!server)
+				return false;
+
+			if (srv_walker == cli_walker)
+				return true;
+
+			return false;
+		}
+		cli_walker = cli_walker->parent;
+		srv_walker = srv_walker->parent;
+		scope_layer--;
+	}
+	return true;
+}
+
+static bool sock_is_scoped(struct sock *const other)
+{
+	const struct landlock_ruleset *dom_other;
+	const struct landlock_ruleset *const dom =
+		landlock_get_current_domain();
+
+	/* the credentials will not change */
+	lockdep_assert_held(&unix_sk(other)->lock);
+	dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;
+
+	/* other is scoped, they connect if they are in the same domain */
+	return domain_sock_scope(dom, dom_other);
+}
+
+static int hook_unix_stream_connect(struct sock *const sock,
+				    struct sock *const other,
+				    struct sock *const newsk)
+{
+	if (sock_is_scoped(other))
+		return 0;
+
+	return -EPERM;
+}
+
+static int hook_unix_may_send(struct socket *const sock,
+			      struct socket *const other)
+{
+	if (sock_is_scoped(other->sk))
+		return 0;
+
+	return -EPERM;
+}
+
 static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme),
+	LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect),
+	LSM_HOOK_INIT(unix_may_send, hook_unix_may_send),
 };
 
 __init void landlock_add_task_hooks(void)
-- 
2.34.1