* [PATCH v8 0/4] Landlock: Add abstract unix socket connect
@ 2024-08-02 4:02 Tahera Fahimi
2024-08-02 4:02 ` [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction Tahera Fahimi
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: Tahera Fahimi @ 2024-08-02 4:02 UTC (permalink / raw)
To: outreachy
Cc: mic, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev, Tahera Fahimi
This patch series adds scoping mechanism for abstract unix sockets.
Closes: https://github.com/landlock-lsm/linux/issues/7
Problem
=======
Abstract unix sockets are used for local inter-process communications
independent of the filesystem. Currently, a sandboxed process can
connect to a socket outside of the sandboxed environment, since Landlock
has no restriction for connecting to an abstract socket address(see more
details in [1,2]). Access to such sockets for a sandboxed process should
be scoped the same way ptrace is limited.
[1] https://lore.kernel.org/all/20231023.ahphah4Wii4v@digikod.net/
[2] https://lore.kernel.org/all/20231102.MaeWaepav8nu@digikod.net/
Solution
========
To solve this issue, we extend the user space interface by adding a new
"scoped" field to Landlock ruleset attribute structure. This field can
contains different rights to restrict different functionalities. For
abstract unix sockets, we introduce
"LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" field to specify that a ruleset
will deny any connection from within the sandbox domain to its parent
(i.e. any parent sandbox or non-sandbox processes).
Example
=======
Starting a listening socket with socat(1):
socat abstract-listen:mysocket -
Starting a sandboxed shell from $HOME with samples/landlock/sandboxer:
LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash
If we try to connect to the listening socket, the connection would be
refused.
socat - abstract-connect:mysocket --> fails
Notes of Implementation
=======================
* Using the "scoped" field provides enough compatibility and flexibility
to extend the scoping mechanism for other IPCs(e.g. signals).
* To access the domain of a socket, we use its credentials of the file's FD
which point to the credentials of the process that created the socket.
(see more details in [3]). Cases where the process using the socket has
a different domain than the process created it are covered in the
unix_sock_special_cases test.
[3]https://lore.kernel.org/all/20240611.Pi8Iph7ootae@digikod.net/
Previous Versions
=================
v6: https://lore.kernel.org/all/Zn32CYZiu7pY+rdI@tahera-OptiPlex-5000/
and https://lore.kernel.org/all/Zn32KKIJrY7Zi51K@tahera-OptiPlex-5000/
v5: https://lore.kernel.org/all/ZnSZnhGBiprI6FRk@tahera-OptiPlex-5000/
v4: https://lore.kernel.org/all/ZnNcE3ph2SWi1qmd@tahera-OptiPlex-5000/
v3: https://lore.kernel.org/all/ZmJJ7lZdQuQop7e5@tahera-OptiPlex-5000/
v2: https://lore.kernel.org/all/ZgX5TRTrSDPrJFfF@tahera-OptiPlex-5000/
v1: https://lore.kernel.org/all/ZgXN5fi6A1YQKiAQ@tahera-OptiPlex-5000/
Tahera Fahimi (4):
Landlock: Add abstract unix socket connect restriction
selftests/landlock: Abstract unix socket restriction tests
sample/Landlock: Support abstract unix socket restriction
Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI
versioning
Documentation/userspace-api/landlock.rst | 33 +-
include/uapi/linux/landlock.h | 30 +
samples/landlock/sandboxer.c | 56 +-
security/landlock/limits.h | 3 +
security/landlock/ruleset.c | 7 +-
security/landlock/ruleset.h | 23 +-
security/landlock/syscalls.c | 14 +-
security/landlock/task.c | 155 +++
tools/testing/selftests/landlock/base_test.c | 2 +-
tools/testing/selftests/landlock/common.h | 72 ++
tools/testing/selftests/landlock/fs_test.c | 34 -
tools/testing/selftests/landlock/net_test.c | 31 +-
.../landlock/scoped_abstract_unix_test.c | 1136 +++++++++++++++++
13 files changed, 1518 insertions(+), 78 deletions(-)
create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c
--
2.34.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-02 4:02 [PATCH v8 0/4] Landlock: Add abstract unix socket connect Tahera Fahimi
@ 2024-08-02 4:02 ` Tahera Fahimi
2024-08-02 16:47 ` Mickaël Salaün
` (2 more replies)
2024-08-02 4:02 ` [PATCH v8 2/4] selftests/landlock: Abstract unix socket restriction tests Tahera Fahimi
` (2 subsequent siblings)
3 siblings, 3 replies; 23+ messages in thread
From: Tahera Fahimi @ 2024-08-02 4:02 UTC (permalink / raw)
To: outreachy
Cc: mic, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev, Tahera Fahimi
This 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. It implements two hooks, unix_stream_connect
and unix_may_send to enforce this restriction.
Closes: https://github.com/landlock-lsm/linux/issues/7
Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
v8:
- Code refactoring (improve code readability, renaming variable, etc.) based
on reviews by Mickaël Salaün on version 7.
- Adding warn_on_once to check (impossible) inconsistencies.
- Adding inline comments.
- Adding check_unix_address_format to check if the scoping socket is an abstract
unix sockets.
v7:
- 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/
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 socket's file 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/all/20240610.Aifee5ingugh@digikod.net/
----
---
include/uapi/linux/landlock.h | 30 +++++++
security/landlock/limits.h | 3 +
security/landlock/ruleset.c | 7 +-
security/landlock/ruleset.h | 23 ++++-
security/landlock/syscalls.c | 14 ++-
security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
6 files changed, 225 insertions(+), 7 deletions(-)
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 68625e728f43..ab31e9f53e55 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -37,6 +37,12 @@ struct landlock_ruleset_attr {
* rule explicitly allow them.
*/
__u64 handled_access_net;
+ /**
+ * @scoped: Bitmask of scopes (cf. `Scope flags`_)
+ * restricting a Landlock domain from accessing outside
+ * resources(e.g. IPCs).
+ */
+ __u64 scoped;
};
/*
@@ -266,4 +272,28 @@ struct landlock_net_port_attr {
#define LANDLOCK_ACCESS_NET_BIND_TCP (1ULL << 0)
#define LANDLOCK_ACCESS_NET_CONNECT_TCP (1ULL << 1)
/* clang-format on */
+
+/**
+ * DOC: scope
+ *
+ * scoped attribute handles a set of restrictions on kernel IPCs through
+ * the following flags.
+ *
+ * Scope flags
+ * ~~~~~~~~~~~
+ *
+ * These flags enable to restrict a sandboxed process from a set of IPC
+ * actions. Setting a flag for a ruleset will isolate the Landlock domain
+ * to forbid connections to resources outside the domain.
+ *
+ * IPCs with scoped actions:
+ *
+ * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandboxed process
+ * from connecting to an abstract unix socket created by a process
+ * outside the related Landlock domain (e.g. a parent domain or a
+ * non-sandboxed process).
+ */
+/* clang-format off */
+#define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET (1ULL << 0)
+/* clang-format on*/
#endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 4eb643077a2a..eb01d0fb2165 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -26,6 +26,9 @@
#define LANDLOCK_MASK_ACCESS_NET ((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
#define LANDLOCK_NUM_ACCESS_NET __const_hweight64(LANDLOCK_MASK_ACCESS_NET)
+#define LANDLOCK_LAST_SCOPE LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
+#define LANDLOCK_MASK_SCOPE ((LANDLOCK_LAST_SCOPE << 1) - 1)
+#define LANDLOCK_NUM_SCOPE __const_hweight64(LANDLOCK_MASK_SCOPE)
/* clang-format on */
#endif /* _SECURITY_LANDLOCK_LIMITS_H */
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 6ff232f58618..a93bdbf52fff 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -52,12 +52,13 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
struct landlock_ruleset *
landlock_create_ruleset(const access_mask_t fs_access_mask,
- const access_mask_t net_access_mask)
+ const access_mask_t net_access_mask,
+ const access_mask_t scope_mask)
{
struct landlock_ruleset *new_ruleset;
/* Informs about useless ruleset. */
- if (!fs_access_mask && !net_access_mask)
+ if (!fs_access_mask && !net_access_mask && !scope_mask)
return ERR_PTR(-ENOMSG);
new_ruleset = create_ruleset(1);
if (IS_ERR(new_ruleset))
@@ -66,6 +67,8 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
if (net_access_mask)
landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
+ if (scope_mask)
+ landlock_add_scope_mask(new_ruleset, scope_mask, 0);
return new_ruleset;
}
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 0f1b5b4c8f6b..c749fa0b3ecd 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -35,6 +35,8 @@ typedef u16 access_mask_t;
static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
/* Makes sure all network access rights can be stored. */
static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
+/* Makes sure all scoped rights can be stored*/
+static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
/* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
@@ -42,6 +44,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
struct access_masks {
access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
+ access_mask_t scoped : LANDLOCK_NUM_SCOPE;
};
typedef u16 layer_mask_t;
@@ -233,7 +236,8 @@ struct landlock_ruleset {
struct landlock_ruleset *
landlock_create_ruleset(const access_mask_t access_mask_fs,
- const access_mask_t access_mask_net);
+ const access_mask_t access_mask_net,
+ const access_mask_t scope_mask);
void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
@@ -280,6 +284,16 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
ruleset->access_masks[layer_level].net |= net_mask;
}
+static inline void
+landlock_add_scope_mask(struct landlock_ruleset *const ruleset,
+ const access_mask_t scope_mask, const u16 layer_level)
+{
+ access_mask_t scoped_mask = scope_mask & LANDLOCK_MASK_SCOPE;
+
+ WARN_ON_ONCE(scope_mask != scoped_mask);
+ ruleset->access_masks[layer_level].scoped |= scoped_mask;
+}
+
static inline access_mask_t
landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
const u16 layer_level)
@@ -303,6 +317,13 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
return ruleset->access_masks[layer_level].net;
}
+static inline access_mask_t
+landlock_get_scope_mask(const struct landlock_ruleset *const ruleset,
+ const u16 layer_level)
+{
+ return ruleset->access_masks[layer_level].scoped;
+}
+
bool landlock_unmask_layers(const struct landlock_rule *const rule,
const access_mask_t access_request,
layer_mask_t (*const layer_masks)[],
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 03b470f5a85a..f51b29521d6b 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -97,8 +97,9 @@ static void build_check_abi(void)
*/
ruleset_size = sizeof(ruleset_attr.handled_access_fs);
ruleset_size += sizeof(ruleset_attr.handled_access_net);
+ ruleset_size += sizeof(ruleset_attr.scoped);
BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
- BUILD_BUG_ON(sizeof(ruleset_attr) != 16);
+ BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
path_beneath_size = sizeof(path_beneath_attr.allowed_access);
path_beneath_size += sizeof(path_beneath_attr.parent_fd);
@@ -149,7 +150,7 @@ static const struct file_operations ruleset_fops = {
.write = fop_dummy_write,
};
-#define LANDLOCK_ABI_VERSION 5
+#define LANDLOCK_ABI_VERSION 6
/**
* sys_landlock_create_ruleset - Create a new ruleset
@@ -170,7 +171,7 @@ static const struct file_operations ruleset_fops = {
* Possible returned errors are:
*
* - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
- * - %EINVAL: unknown @flags, or unknown access, or too small @size;
+ * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size;
* - %E2BIG or %EFAULT: @attr or @size inconsistencies;
* - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs.
*/
@@ -213,9 +214,14 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
LANDLOCK_MASK_ACCESS_NET)
return -EINVAL;
+ /* Checks IPC scoping content (and 32-bits cast). */
+ if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
+ return -EINVAL;
+
/* Checks arguments and transforms to kernel struct. */
ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
- ruleset_attr.handled_access_net);
+ ruleset_attr.handled_access_net,
+ ruleset_attr.scoped);
if (IS_ERR(ruleset))
return PTR_ERR(ruleset);
diff --git a/security/landlock/task.c b/security/landlock/task.c
index 849f5123610b..7e8579ebae83 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,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
return task_ptrace(parent, current);
}
+static bool walk_and_check(const struct landlock_ruleset *const child,
+ struct landlock_hierarchy **walker,
+ size_t base_layer, size_t deep_layer,
+ access_mask_t check_scoping)
+{
+ if (!child || base_layer < 0 || !(*walker))
+ return false;
+
+ for (deep_layer; base_layer < deep_layer; deep_layer--) {
+ if (check_scoping & landlock_get_scope_mask(child, deep_layer))
+ return false;
+ *walker = (*walker)->parent;
+ if (WARN_ON_ONCE(!*walker))
+ /* there is an inconsistency between num_layers
+ * and landlock_hierarchy in the ruleset
+ */
+ return false;
+ }
+ return true;
+}
+
+/**
+ * domain_IPC_scope - Checks if the client domain is scoped in the same
+ * domain as the server.
+ *
+ * @client: IPC sender domain.
+ * @server: IPC receiver domain.
+ *
+ * Check if the @client domain is scoped to access the @server; the @server
+ * must be scoped in the same domain.
+ */
+static bool domain_IPC_scope(const struct landlock_ruleset *const client,
+ const struct landlock_ruleset *const server,
+ access_mask_t ipc_type)
+{
+ size_t client_layer, server_layer = 0;
+ int base_layer;
+ struct landlock_hierarchy *client_walker, *server_walker;
+ bool is_scoped;
+
+ /* Quick return if client has no domain */
+ if (!client)
+ return true;
+
+ client_layer = client->num_layers - 1;
+ client_walker = client->hierarchy;
+ if (server) {
+ server_layer = server->num_layers - 1;
+ server_walker = server->hierarchy;
+ }
+ base_layer = (client_layer > server_layer) ? server_layer :
+ client_layer;
+
+ /* For client domain, walk_and_check ensures the client domain is
+ * not scoped until gets to base_layer.
+ * For server_domain, it only ensures that the server domain exist.
+ */
+ if (client_layer != server_layer) {
+ if (client_layer > server_layer)
+ is_scoped = walk_and_check(client, &client_walker,
+ server_layer, client_layer,
+ ipc_type);
+ else
+ is_scoped = walk_and_check(server, &server_walker,
+ client_layer, server_layer,
+ ipc_type & 0);
+ if (!is_scoped)
+ return false;
+ }
+ /* client and server are at the same level in hierarchy. If client is
+ * scoped, the server must be scoped in the same domain
+ */
+ for (base_layer; base_layer >= 0; base_layer--) {
+ if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
+ /* This check must be here since access would be denied only if
+ * the client is scoped and the server has no domain, so
+ * if the client has a domain but is not scoped and the server
+ * has no domain, access is guaranteed.
+ */
+ if (!server)
+ return false;
+
+ if (server_walker == client_walker)
+ return true;
+
+ return false;
+ }
+ client_walker = client_walker->parent;
+ server_walker = server_walker->parent;
+ /* Warn if there is an incosistenncy between num_layers and
+ * landlock_hierarchy in each of rulesets
+ */
+ if (WARN_ON_ONCE(base_layer > 0 &&
+ (!server_walker || !client_walker)))
+ return false;
+ }
+ 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;
+ return domain_IPC_scope(dom, dom_other,
+ LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET);
+}
+
+static bool check_unix_address_format(struct sock *const sock)
+{
+ struct unix_address *addr = unix_sk(sock)->addr;
+
+ if (!addr)
+ return true;
+
+ if (addr->len > sizeof(AF_UNIX)) {
+ /* handling unspec sockets */
+ if (!addr->name[0].sun_path)
+ return true;
+
+ if (addr->name[0].sun_path[0] == '\0')
+ if (!sock_is_scoped(sock))
+ return false;
+ }
+
+ return true;
+}
+
+static int hook_unix_stream_connect(struct sock *const sock,
+ struct sock *const other,
+ struct sock *const newsk)
+{
+ if (check_unix_address_format(other))
+ return 0;
+
+ return -EPERM;
+}
+
+static int hook_unix_may_send(struct socket *const sock,
+ struct socket *const other)
+{
+ if (check_unix_address_format(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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v8 2/4] selftests/landlock: Abstract unix socket restriction tests
2024-08-02 4:02 [PATCH v8 0/4] Landlock: Add abstract unix socket connect Tahera Fahimi
2024-08-02 4:02 ` [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction Tahera Fahimi
@ 2024-08-02 4:02 ` Tahera Fahimi
2024-08-07 15:08 ` Mickaël Salaün
2024-08-02 4:02 ` [PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction Tahera Fahimi
2024-08-02 4:02 ` [PATCH v8 4/4] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning Tahera Fahimi
3 siblings, 1 reply; 23+ messages in thread
From: Tahera Fahimi @ 2024-08-02 4:02 UTC (permalink / raw)
To: outreachy
Cc: mic, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev, Tahera Fahimi
The patch introduces Landlock ABI version 6 and has four types of tests:
1) unix_socket: base tests of the abstract socket scoping mechanism for a
landlocked process, same as the ptrace test.
2) optional_scoping: generates three processes with different domains and
tests if a process with a non-scoped domain can connect to other
processes.
3) unix_sock_special_cases: since the socket's creator credentials are used
for scoping sockets, this test examines the cases where the socket's
credentials are different from the process using it.
4) pathname_address_sockets: ensures that Unix sockets bound to a
null-terminated filesystem can still connect to a socket outside of
their scoped domain. This means that even if the domain is scoped with
LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, the socket can connect to a socket
outside the scoped domain.
Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
Changes in versions:
V8:
- Move tests to scoped_abstract_unix_test.c file.
- To avoid potential conflicts among Unix socket names in different tests,
set_unix_address is added to common.h to set different sun_path for Unix sockets.
- protocol_variant and service_fixture structures are also moved to common.h
- Adding pathname_address_sockets to cover all types of address formats
for unix sockets, and moving remove_path() to common.h to reuse in this test.
V7:
- Introducing landlock ABI version 6.
- Adding some edge test cases to optional_scoping test.
- Using `enum` for different domains in optional_scoping tests.
- Extend unix_sock_special_cases test cases for connected(SOCK_STREAM) sockets.
- Modifying inline comments.
V6:
- Introducing optional_scoping test which ensures a sandboxed process with a
non-scoped domain can still connect to another abstract unix socket(either
sandboxed or non-sandboxed).
- Introducing unix_sock_special_cases test which tests examines scenarios where
the connecting sockets have different domain than the process using them.
V4:
- Introducing unix_socket to evaluate the basic scoping mechanism for abstract
unix sockets.
---
tools/testing/selftests/landlock/base_test.c | 2 +-
tools/testing/selftests/landlock/common.h | 72 ++
tools/testing/selftests/landlock/fs_test.c | 34 -
tools/testing/selftests/landlock/net_test.c | 31 +-
.../landlock/scoped_abstract_unix_test.c | 1136 +++++++++++++++++
5 files changed, 1210 insertions(+), 65 deletions(-)
create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 3c1e9f35b531..52b00472a487 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -75,7 +75,7 @@ TEST(abi_version)
const struct landlock_ruleset_attr ruleset_attr = {
.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
};
- ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
+ ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
LANDLOCK_CREATE_RULESET_VERSION));
ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index 7e2b431b9f90..a0c8126d94b4 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -16,8 +16,11 @@
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
+#include <sys/un.h>
+#include <arpa/inet.h>
#include "../kselftest_harness.h"
+#define TMP_DIR "tmp"
#ifndef __maybe_unused
#define __maybe_unused __attribute__((__unused__))
@@ -226,3 +229,72 @@ enforce_ruleset(struct __test_metadata *const _metadata, const int ruleset_fd)
TH_LOG("Failed to enforce ruleset: %s", strerror(errno));
}
}
+
+struct protocol_variant {
+ int domain;
+ int type;
+};
+
+struct service_fixture {
+ struct protocol_variant protocol;
+ /* port is also stored in ipv4_addr.sin_port or ipv6_addr.sin6_port */
+ unsigned short port;
+ union {
+ struct sockaddr_in ipv4_addr;
+ struct sockaddr_in6 ipv6_addr;
+ struct {
+ struct sockaddr_un unix_addr;
+ socklen_t unix_addr_len;
+ };
+ };
+};
+
+static pid_t __maybe_unused sys_gettid(void)
+{
+ return syscall(__NR_gettid);
+}
+
+static void __maybe_unused set_unix_address(struct service_fixture *const srv,
+ const unsigned short index)
+{
+ srv->unix_addr.sun_family = AF_UNIX;
+ sprintf(srv->unix_addr.sun_path,
+ "_selftests-landlock-abstract-unix-tid%d-index%d", sys_gettid(),
+ index);
+ srv->unix_addr_len = SUN_LEN(&srv->unix_addr);
+ srv->unix_addr.sun_path[0] = '\0';
+}
+
+static int __maybe_unused remove_path(const char *const path)
+{
+ char *walker;
+ int i, ret, err = 0;
+
+ walker = strdup(path);
+ if (!walker) {
+ err = ENOMEM;
+ goto out;
+ }
+ if (unlink(path) && rmdir(path)) {
+ if (errno != ENOENT && errno != ENOTDIR)
+ err = errno;
+ goto out;
+ }
+ for (i = strlen(walker); i > 0; i--) {
+ if (walker[i] != '/')
+ continue;
+ walker[i] = '\0';
+ ret = rmdir(walker);
+ if (ret) {
+ if (errno != ENOTEMPTY && errno != EBUSY)
+ err = errno;
+ goto out;
+ }
+ if (strcmp(walker, TMP_DIR) == 0)
+ goto out;
+ }
+
+out:
+ free(walker);
+ return err;
+}
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 7d063c652be1..fc74cab6dfb8 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -221,40 +221,6 @@ static void create_file(struct __test_metadata *const _metadata,
}
}
-static int remove_path(const char *const path)
-{
- char *walker;
- int i, ret, err = 0;
-
- walker = strdup(path);
- if (!walker) {
- err = ENOMEM;
- goto out;
- }
- if (unlink(path) && rmdir(path)) {
- if (errno != ENOENT && errno != ENOTDIR)
- err = errno;
- goto out;
- }
- for (i = strlen(walker); i > 0; i--) {
- if (walker[i] != '/')
- continue;
- walker[i] = '\0';
- ret = rmdir(walker);
- if (ret) {
- if (errno != ENOTEMPTY && errno != EBUSY)
- err = errno;
- goto out;
- }
- if (strcmp(walker, TMP_DIR) == 0)
- goto out;
- }
-
-out:
- free(walker);
- return err;
-}
-
struct mnt_opt {
const char *const source;
const char *const type;
diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index f21cfbbc3638..4e0aeb53b225 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -36,30 +36,6 @@ enum sandbox_type {
TCP_SANDBOX,
};
-struct protocol_variant {
- int domain;
- int type;
-};
-
-struct service_fixture {
- struct protocol_variant protocol;
- /* port is also stored in ipv4_addr.sin_port or ipv6_addr.sin6_port */
- unsigned short port;
- union {
- struct sockaddr_in ipv4_addr;
- struct sockaddr_in6 ipv6_addr;
- struct {
- struct sockaddr_un unix_addr;
- socklen_t unix_addr_len;
- };
- };
-};
-
-static pid_t sys_gettid(void)
-{
- return syscall(__NR_gettid);
-}
-
static int set_service(struct service_fixture *const srv,
const struct protocol_variant prot,
const unsigned short index)
@@ -92,12 +68,7 @@ static int set_service(struct service_fixture *const srv,
return 0;
case AF_UNIX:
- srv->unix_addr.sun_family = prot.domain;
- sprintf(srv->unix_addr.sun_path,
- "_selftests-landlock-net-tid%d-index%d", sys_gettid(),
- index);
- srv->unix_addr_len = SUN_LEN(&srv->unix_addr);
- srv->unix_addr.sun_path[0] = '\0';
+ set_unix_address(srv, index);
return 0;
}
return 1;
diff --git a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
new file mode 100644
index 000000000000..ffa1f01dbbce
--- /dev/null
+++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
@@ -0,0 +1,1136 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Landlock tests - Abstract Unix Socket
+ *
+ * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
+ * Copyright © 2019-2020 ANSSI
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/landlock.h>
+#include <signal.h>
+#include <sys/prctl.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <stddef.h>
+#include <sys/stat.h>
+#include <sched.h>
+
+#include "common.h"
+
+static void create_fs_domain(struct __test_metadata *const _metadata)
+{
+ int ruleset_fd;
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = LANDLOCK_ACCESS_FS_READ_DIR,
+ };
+
+ ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+ EXPECT_LE(0, ruleset_fd)
+ {
+ TH_LOG("Failed to create a ruleset: %s", strerror(errno));
+ }
+ EXPECT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
+ EXPECT_EQ(0, landlock_restrict_self(ruleset_fd, 0));
+ EXPECT_EQ(0, close(ruleset_fd));
+}
+
+static void create_unix_domain(struct __test_metadata *const _metadata)
+{
+ int ruleset_fd;
+ const struct landlock_ruleset_attr ruleset_attr = {
+ .scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
+ };
+
+ ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+ EXPECT_LE(0, ruleset_fd)
+ {
+ TH_LOG("Failed to create a ruleset: %s", strerror(errno));
+ }
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+}
+
+/* clang-format off */
+FIXTURE(unix_socket)
+{
+ struct service_fixture stream_address, dgram_address;
+ int server, client, dgram_server, dgram_client;
+};
+
+/* clang-format on */
+FIXTURE_VARIANT(unix_socket)
+{
+ bool domain_both;
+ bool domain_parent;
+ bool domain_child;
+ bool connect_to_parent;
+};
+
+FIXTURE_SETUP(unix_socket)
+{
+ memset(&self->stream_address, 0, sizeof(self->stream_address));
+ memset(&self->dgram_address, 0, sizeof(self->dgram_address));
+
+ set_unix_address(&self->stream_address, 0);
+ set_unix_address(&self->dgram_address, 1);
+}
+
+FIXTURE_TEARDOWN(unix_socket)
+{
+ close(self->server);
+ close(self->client);
+ close(self->dgram_server);
+ close(self->dgram_client);
+}
+
+/*
+ * No domain
+ *
+ * P1-. P1 -> P2 : allow
+ * \ P2 -> P1 : allow
+ * 'P2
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_without_domain_connect_to_parent) {
+ /* clang-format on */
+ .domain_both = false,
+ .domain_parent = false,
+ .domain_child = false,
+ .connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_without_domain_connect_to_child) {
+ /* clang-format on */
+ .domain_both = false,
+ .domain_parent = false,
+ .domain_child = false,
+ .connect_to_parent = false,
+};
+
+/*
+ * Child domain
+ *
+ * P1--. P1 -> P2 : allow
+ * \ P2 -> P1 : deny
+ * .'-----.
+ * | P2 |
+ * '------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_one_domain_connect_to_parent) {
+ /* clang-format on */
+ .domain_both = false,
+ .domain_parent = false,
+ .domain_child = true,
+ .connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_with_one_domain_connect_to_child) {
+ /* clang-format on */
+ .domain_both = false,
+ .domain_parent = false,
+ .domain_child = true,
+ .connect_to_parent = false,
+};
+
+/*
+ * Parent domain
+ * .------.
+ * | P1 --. P1 -> P2 : deny
+ * '------' \ P2 -> P1 : allow
+ * '
+ * P2
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_with_parent_domain_connect_to_parent) {
+ /* clang-format on */
+ .domain_both = false,
+ .domain_parent = true,
+ .domain_child = false,
+ .connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_parent_domain_connect_to_child) {
+ /* clang-format on */
+ .domain_both = false,
+ .domain_parent = true,
+ .domain_child = false,
+ .connect_to_parent = false,
+};
+
+/*
+ * Parent + child domain (siblings)
+ * .------.
+ * | P1 ---. P1 -> P2 : deny
+ * '------' \ P2 -> P1 : deny
+ * .---'--.
+ * | P2 |
+ * '------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_sibling_domain_connect_to_parent) {
+ /* clang-format on */
+ .domain_both = false,
+ .domain_parent = true,
+ .domain_child = true,
+ .connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_sibling_domain_connect_to_child) {
+ /* clang-format on */
+ .domain_both = false,
+ .domain_parent = true,
+ .domain_child = true,
+ .connect_to_parent = false,
+};
+
+/*
+ * Same domain (inherited)
+ * .-------------.
+ * | P1----. | P1 -> P2 : allow
+ * | \ | P2 -> P1 : allow
+ * | ' |
+ * | P2 |
+ * '-------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_inherited_domain_connect_to_parent) {
+ /* clang-format on */
+ .domain_both = true,
+ .domain_parent = false,
+ .domain_child = false,
+ .connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_inherited_domain_connect_to_child) {
+ /* clang-format on */
+ .domain_both = true,
+ .domain_parent = false,
+ .domain_child = false,
+ .connect_to_parent = false,
+};
+
+/*
+ * Inherited + child domain
+ * .-----------------.
+ * | P1----. | P1 -> P2 : allow
+ * | \ | P2 -> P1 : deny
+ * | .-'----. |
+ * | | P2 | |
+ * | '------' |
+ * '-----------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_nested_domain_connect_to_parent) {
+ /* clang-format on */
+ .domain_both = true,
+ .domain_parent = false,
+ .domain_child = true,
+ .connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_nested_domain_connect_to_child) {
+ /* clang-format on */
+ .domain_both = true,
+ .domain_parent = false,
+ .domain_child = true,
+ .connect_to_parent = false,
+};
+
+/*
+ * Inherited + parent domain
+ * .-----------------.
+ * |.------. | P1 -> P2 : deny
+ * || P1 ----. | P2 -> P1 : allow
+ * |'------' \ |
+ * | ' |
+ * | P2 |
+ * '-----------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_with_nested_and_parent_domain_connect_to_parent) {
+ /* clang-format on */
+ .domain_both = true,
+ .domain_parent = true,
+ .domain_child = false,
+ .connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_nested_and_parent_domain_connect_to_child) {
+ /* clang-format on */
+ .domain_both = true,
+ .domain_parent = true,
+ .domain_child = false,
+ .connect_to_parent = false,
+};
+
+/*
+ * Inherited + parent and child domain (siblings)
+ * .-----------------.
+ * | .------. | P1 -> P2 : deny
+ * | | P1 . | P2 -> P1 : deny
+ * | '------'\ |
+ * | \ |
+ * | .--'---. |
+ * | | P2 | |
+ * | '------' |
+ * '-----------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_forked_domain_connect_to_parent) {
+ /* clang-format on */
+ .domain_both = true,
+ .domain_parent = true,
+ .domain_child = true,
+ .connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_forked_domain_connect_to_child) {
+ /* clang-format on */
+ .domain_both = true,
+ .domain_parent = true,
+ .domain_child = true,
+ .connect_to_parent = false,
+};
+
+/* Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent and child,
+ * when they have scoped domain or no domain.
+ */
+TEST_F(unix_socket, abstract_unix_socket)
+{
+ int status;
+ pid_t child;
+ bool can_connect_to_parent, can_connect_to_child;
+ int err, err_dgram;
+ int pipe_child[2], pipe_parent[2];
+ char buf_parent;
+
+ /*
+ * can_connect_to_child is true if a parent process can connect to its
+ * child process. The parent process is not isolated from the child
+ * with a dedicated Landlock domain.
+ */
+ can_connect_to_child = !variant->domain_parent;
+ /*
+ * can_connect_to_parent is true if a child process can connect to its
+ * parent process. This depends on the child process is not isolated from
+ * the parent with a dedicated Landlock domain.
+ */
+ can_connect_to_parent = !variant->domain_child;
+
+ ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
+ ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
+ if (variant->domain_both) {
+ create_unix_domain(_metadata);
+ if (!__test_passed(_metadata))
+ return;
+ }
+
+ child = fork();
+ ASSERT_LE(0, child);
+ if (child == 0) {
+ char buf_child;
+
+ ASSERT_EQ(0, close(pipe_parent[1]));
+ ASSERT_EQ(0, close(pipe_child[0]));
+ if (variant->domain_child)
+ create_unix_domain(_metadata);
+
+ /* Waits for the parent to be in a domain, if any. */
+ ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+
+ if (variant->connect_to_parent) {
+ self->client = socket(AF_UNIX, SOCK_STREAM, 0);
+ self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
+
+ ASSERT_NE(-1, self->client);
+ ASSERT_NE(-1, self->dgram_client);
+ ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+
+ err = connect(self->client,
+ &self->stream_address.unix_addr,
+ (self->stream_address).unix_addr_len);
+ err_dgram =
+ connect(self->dgram_client,
+ &self->dgram_address.unix_addr,
+ (self->dgram_address).unix_addr_len);
+
+ if (can_connect_to_parent) {
+ EXPECT_EQ(0, err);
+ EXPECT_EQ(0, err_dgram);
+ } else {
+ EXPECT_EQ(-1, err);
+ EXPECT_EQ(-1, err_dgram);
+ EXPECT_EQ(EPERM, errno);
+ }
+ } else {
+ self->server = socket(AF_UNIX, SOCK_STREAM, 0);
+ self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
+ ASSERT_NE(-1, self->server);
+ ASSERT_NE(-1, self->dgram_server);
+
+ ASSERT_EQ(0,
+ bind(self->server,
+ &self->stream_address.unix_addr,
+ (self->stream_address).unix_addr_len));
+ ASSERT_EQ(0, bind(self->dgram_server,
+ &self->dgram_address.unix_addr,
+ (self->dgram_address).unix_addr_len));
+ ASSERT_EQ(0, listen(self->server, 32));
+
+ /* signal to parent that child is listening */
+ ASSERT_EQ(1, write(pipe_child[1], ".", 1));
+ /* wait to connect */
+ ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+ }
+ _exit(_metadata->exit_code);
+ return;
+ }
+
+ ASSERT_EQ(0, close(pipe_child[1]));
+ ASSERT_EQ(0, close(pipe_parent[0]));
+
+ if (variant->domain_parent)
+ create_unix_domain(_metadata);
+
+ /* Signals that the parent is in a domain, if any. */
+ ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+
+ if (!variant->connect_to_parent) {
+ self->client = socket(AF_UNIX, SOCK_STREAM, 0);
+ self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
+
+ ASSERT_NE(-1, self->client);
+ ASSERT_NE(-1, self->dgram_client);
+
+ /* Waits for the child to listen */
+ ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1));
+ err = connect(self->client, &self->stream_address.unix_addr,
+ (self->stream_address).unix_addr_len);
+ err_dgram = connect(self->dgram_client,
+ &self->dgram_address.unix_addr,
+ (self->dgram_address).unix_addr_len);
+
+ if (can_connect_to_child) {
+ EXPECT_EQ(0, err);
+ EXPECT_EQ(0, err_dgram);
+ } else {
+ EXPECT_EQ(-1, err);
+ EXPECT_EQ(-1, err_dgram);
+ EXPECT_EQ(EPERM, errno);
+ }
+ ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+ } else {
+ self->server = socket(AF_UNIX, SOCK_STREAM, 0);
+ self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
+ ASSERT_NE(-1, self->server);
+ ASSERT_NE(-1, self->dgram_server);
+ ASSERT_EQ(0, bind(self->server, &self->stream_address.unix_addr,
+ (self->stream_address).unix_addr_len));
+ ASSERT_EQ(0, bind(self->dgram_server,
+ &self->dgram_address.unix_addr,
+ (self->dgram_address).unix_addr_len));
+ ASSERT_EQ(0, listen(self->server, 32));
+
+ /* signal to child that parent is listening */
+ ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+ }
+
+ ASSERT_EQ(child, waitpid(child, &status, 0));
+
+ if (WIFSIGNALED(status) || !WIFEXITED(status) ||
+ WEXITSTATUS(status) != EXIT_SUCCESS)
+ _metadata->exit_code = KSFT_FAIL;
+}
+
+enum sandbox_type {
+ NO_SANDBOX,
+ SCOPE_SANDBOX,
+ /* Any other type of sandboxing domain */
+ OTHER_SANDBOX,
+};
+
+/* clang-format off */
+FIXTURE(optional_scoping)
+{
+ struct service_fixture parent_address, child_address;
+ int parent_server, child_server, client;
+};
+
+/* clang-format on */
+FIXTURE_VARIANT(optional_scoping)
+{
+ const int domain_all;
+ const int domain_parent;
+ const int domain_children;
+ const int domain_child;
+ const int domain_grand_child;
+ const int type;
+};
+
+FIXTURE_SETUP(optional_scoping)
+{
+ memset(&self->parent_address, 0, sizeof(self->parent_address));
+ memset(&self->child_address, 0, sizeof(self->child_address));
+
+ set_unix_address(&self->parent_address, 0);
+ set_unix_address(&self->child_address, 1);
+}
+
+FIXTURE_TEARDOWN(optional_scoping)
+{
+ close(self->parent_server);
+ close(self->child_server);
+ close(self->client);
+}
+
+/*
+ * .-----------------.
+ * | ####### | P3 -> P2 : allow
+ * | P1----# P2 # | P3 -> P1 : deny
+ * | # | # |
+ * | # P3 # |
+ * | ####### |
+ * '-----------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(optional_scoping, deny_scoped) {
+ .domain_all = OTHER_SANDBOX,
+ .domain_parent = NO_SANDBOX,
+ .domain_children = SCOPE_SANDBOX,
+ .domain_child = NO_SANDBOX,
+ .domain_grand_child = NO_SANDBOX,
+ .type = SOCK_DGRAM,
+ /* clang-format on */
+};
+
+/*
+ * .-----------------.
+ * | .-----. | P3 -> P2 : allow
+ * | P1----| P2 | | P3 -> P1 : allow
+ * | | | |
+ * | | P3 | |
+ * | '-----' |
+ * '-----------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(optional_scoping, allow_with_other_domain) {
+ .domain_all = OTHER_SANDBOX,
+ .domain_parent = NO_SANDBOX,
+ .domain_children = OTHER_SANDBOX,
+ .domain_child = NO_SANDBOX,
+ .domain_grand_child = NO_SANDBOX,
+ .type = SOCK_DGRAM,
+ /* clang-format on */
+};
+
+/*
+ * .----. ###### P3 -> P2 : allow
+ * | P1 |----# P2 # P3 -> P1 : allow
+ * '----' ######
+ * |
+ * P3
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(optional_scoping, allow_with_one_domain) {
+ .domain_all = NO_SANDBOX,
+ .domain_parent = OTHER_SANDBOX,
+ .domain_children = NO_SANDBOX,
+ .domain_child = SCOPE_SANDBOX,
+ .domain_grand_child = NO_SANDBOX,
+ .type = SOCK_DGRAM,
+ /* clang-format on */
+};
+
+/*
+ * ###### .-----. P3 -> P2 : allow
+ * # P1 #----| P2 | P3 -> P1 : allow
+ * ###### '-----'
+ * |
+ * P3
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(optional_scoping, allow_with_grand_parent_scoped) {
+ .domain_all = NO_SANDBOX,
+ .domain_parent = SCOPE_SANDBOX,
+ .domain_children = NO_SANDBOX,
+ .domain_child = OTHER_SANDBOX,
+ .domain_grand_child = NO_SANDBOX,
+ .type = SOCK_STREAM,
+ /* clang-format on */
+};
+
+/*
+ * ###### ###### P3 -> P2 : allow
+ * # P1 #----# P2 # P3 -> P1 : allow
+ * ###### ######
+ * |
+ * .----.
+ * | P3 |
+ * '----'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(optional_scoping, allow_with_parents_domain) {
+ .domain_all = NO_SANDBOX,
+ .domain_parent = SCOPE_SANDBOX,
+ .domain_children = NO_SANDBOX,
+ .domain_child = SCOPE_SANDBOX,
+ .domain_grand_child = NO_SANDBOX,
+ .type = SOCK_STREAM,
+ /* clang-format on */
+};
+
+/* Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent, child
+ * and grand child processes when they can have scoped or non-scoped
+ * domains.
+ **/
+TEST_F(optional_scoping, unix_scoping)
+{
+ pid_t child;
+ int status;
+ bool can_connect_to_parent, can_connect_to_child;
+ int pipe_parent[2];
+
+ if (variant->domain_grand_child == SCOPE_SANDBOX)
+ can_connect_to_child = false;
+ else
+ can_connect_to_child = true;
+
+ if (!can_connect_to_child || variant->domain_children == SCOPE_SANDBOX)
+ can_connect_to_parent = false;
+ else
+ can_connect_to_parent = true;
+
+ ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
+
+ if (variant->domain_all == OTHER_SANDBOX)
+ create_fs_domain(_metadata);
+ else if (variant->domain_all == SCOPE_SANDBOX)
+ create_unix_domain(_metadata);
+
+ child = fork();
+ ASSERT_LE(0, child);
+ if (child == 0) {
+ int pipe_child[2];
+
+ ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
+ pid_t grand_child;
+
+ if (variant->domain_children == OTHER_SANDBOX)
+ create_fs_domain(_metadata);
+ else if (variant->domain_children == SCOPE_SANDBOX)
+ create_unix_domain(_metadata);
+
+ grand_child = fork();
+ ASSERT_LE(0, grand_child);
+ if (grand_child == 0) {
+ ASSERT_EQ(0, close(pipe_parent[1]));
+ ASSERT_EQ(0, close(pipe_child[1]));
+
+ char buf1, buf2;
+ int err;
+
+ if (variant->domain_grand_child == OTHER_SANDBOX)
+ create_fs_domain(_metadata);
+ else if (variant->domain_grand_child == SCOPE_SANDBOX)
+ create_unix_domain(_metadata);
+
+ self->client = socket(AF_UNIX, variant->type, 0);
+ ASSERT_NE(-1, self->client);
+
+ ASSERT_EQ(1, read(pipe_child[0], &buf2, 1));
+ err = connect(self->client,
+ &self->child_address.unix_addr,
+ (self->child_address).unix_addr_len);
+ if (can_connect_to_child) {
+ EXPECT_EQ(0, err);
+ } else {
+ EXPECT_EQ(-1, err);
+ EXPECT_EQ(EPERM, errno);
+ }
+
+ if (variant->type == SOCK_STREAM) {
+ EXPECT_EQ(0, close(self->client));
+ self->client =
+ socket(AF_UNIX, variant->type, 0);
+ ASSERT_NE(-1, self->client);
+ }
+
+ ASSERT_EQ(1, read(pipe_parent[0], &buf1, 1));
+ err = connect(self->client,
+ &self->parent_address.unix_addr,
+ (self->parent_address).unix_addr_len);
+ if (can_connect_to_parent) {
+ EXPECT_EQ(0, err);
+ } else {
+ EXPECT_EQ(-1, err);
+ EXPECT_EQ(EPERM, errno);
+ }
+ EXPECT_EQ(0, close(self->client));
+
+ _exit(_metadata->exit_code);
+ return;
+ }
+
+ ASSERT_EQ(0, close(pipe_child[0]));
+ if (variant->domain_child == OTHER_SANDBOX)
+ create_fs_domain(_metadata);
+ else if (variant->domain_child == SCOPE_SANDBOX)
+ create_unix_domain(_metadata);
+
+ self->child_server = socket(AF_UNIX, variant->type, 0);
+ ASSERT_NE(-1, self->child_server);
+ ASSERT_EQ(0, bind(self->child_server,
+ &self->child_address.unix_addr,
+ (self->child_address).unix_addr_len));
+ if (variant->type == SOCK_STREAM)
+ ASSERT_EQ(0, listen(self->child_server, 32));
+
+ ASSERT_EQ(1, write(pipe_child[1], ".", 1));
+ ASSERT_EQ(grand_child, waitpid(grand_child, &status, 0));
+ return;
+ }
+ ASSERT_EQ(0, close(pipe_parent[0]));
+
+ if (variant->domain_parent == OTHER_SANDBOX)
+ create_fs_domain(_metadata);
+ else if (variant->domain_parent == SCOPE_SANDBOX)
+ create_unix_domain(_metadata);
+
+ self->parent_server = socket(AF_UNIX, variant->type, 0);
+ ASSERT_NE(-1, self->parent_server);
+ ASSERT_EQ(0, bind(self->parent_server, &self->parent_address.unix_addr,
+ (self->parent_address).unix_addr_len));
+
+ if (variant->type == SOCK_STREAM)
+ ASSERT_EQ(0, listen(self->parent_server, 32));
+
+ ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+ ASSERT_EQ(child, waitpid(child, &status, 0));
+ if (WIFSIGNALED(status) || !WIFEXITED(status) ||
+ WEXITSTATUS(status) != EXIT_SUCCESS)
+ _metadata->exit_code = KSFT_FAIL;
+}
+
+/*
+ * Since the special case of scoping only happens when the connecting socket
+ * is scoped, the client's domain is true for all the following test cases.
+ */
+/* clang-format off */
+FIXTURE(unix_sock_special_cases) {
+ int server_socket, client;
+ int stream_server, stream_client;
+ struct service_fixture address, transit_address;
+};
+
+/* clang-format on */
+FIXTURE_VARIANT(unix_sock_special_cases)
+{
+ const bool domain_server;
+ const bool domain_server_socket;
+ const int type;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_sock_special_cases, allow_dgram_server_sock_domain) {
+ /* clang-format on */
+ .domain_server = false,
+ .domain_server_socket = true,
+ .type = SOCK_DGRAM,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_sock_special_cases, deny_dgram_server_domain) {
+ /* clang-format on */
+ .domain_server = true,
+ .domain_server_socket = false,
+ .type = SOCK_DGRAM,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_sock_special_cases, allow_stream_server_sock_domain) {
+ /* clang-format on */
+ .domain_server = false,
+ .domain_server_socket = true,
+ .type = SOCK_STREAM,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_sock_special_cases, deny_stream_server_domain) {
+ /* clang-format on */
+ .domain_server = true,
+ .domain_server_socket = false,
+ .type = SOCK_STREAM,
+};
+
+FIXTURE_SETUP(unix_sock_special_cases)
+{
+ memset(&self->transit_address, 0, sizeof(self->transit_address));
+ memset(&self->address, 0, sizeof(self->address));
+ set_unix_address(&self->transit_address, 0);
+ set_unix_address(&self->address, 1);
+}
+
+FIXTURE_TEARDOWN(unix_sock_special_cases)
+{
+ close(self->client);
+ close(self->server_socket);
+ close(self->stream_server);
+ close(self->stream_client);
+}
+
+/* Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent and
+ * child processes when connecting socket has different domain
+ * than the process using it.
+ **/
+TEST_F(unix_sock_special_cases, socket_with_different_domain)
+{
+ pid_t child;
+ int err, status;
+ int pipe_child[2], pipe_parent[2];
+ char buf_parent;
+
+ ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
+ ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
+
+ child = fork();
+ ASSERT_LE(0, child);
+ if (child == 0) {
+ char buf_child;
+
+ ASSERT_EQ(0, close(pipe_parent[1]));
+ ASSERT_EQ(0, close(pipe_child[0]));
+
+ /* client always has domain */
+ create_unix_domain(_metadata);
+
+ if (variant->domain_server_socket) {
+ int data_socket;
+ int fd_sock = socket(AF_UNIX, variant->type, 0);
+
+ ASSERT_NE(-1, fd_sock);
+
+ self->stream_server = socket(AF_UNIX, SOCK_STREAM, 0);
+
+ ASSERT_NE(-1, self->stream_server);
+ ASSERT_EQ(0,
+ bind(self->stream_server,
+ &self->transit_address.unix_addr,
+ (self->transit_address).unix_addr_len));
+ ASSERT_EQ(0, listen(self->stream_server, 32));
+
+ ASSERT_EQ(1, write(pipe_child[1], ".", 1));
+
+ data_socket = accept(self->stream_server, NULL, NULL);
+
+ ASSERT_EQ(0, send_fd(data_socket, fd_sock));
+ ASSERT_EQ(0, close(fd_sock));
+ }
+
+ self->client = socket(AF_UNIX, variant->type, 0);
+ ASSERT_NE(-1, self->client);
+ /* wait for parent signal for connection */
+ ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+
+ err = connect(self->client, &self->address.unix_addr,
+ (self->address).unix_addr_len);
+ if (!variant->domain_server_socket) {
+ EXPECT_EQ(-1, err);
+ EXPECT_EQ(EPERM, errno);
+ } else {
+ EXPECT_EQ(0, err);
+ }
+ _exit(_metadata->exit_code);
+ return;
+ }
+
+ ASSERT_EQ(0, close(pipe_child[1]));
+ ASSERT_EQ(0, close(pipe_parent[0]));
+
+ if (!variant->domain_server_socket) {
+ self->server_socket = socket(AF_UNIX, variant->type, 0);
+ } else {
+ int cli = socket(AF_UNIX, SOCK_STREAM, 0);
+
+ ASSERT_NE(-1, cli);
+ ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1));
+ ASSERT_EQ(0, connect(cli, &self->transit_address.unix_addr,
+ (self->transit_address).unix_addr_len));
+
+ self->server_socket = recv_fd(cli);
+ ASSERT_LE(0, self->server_socket);
+ ASSERT_EQ(0, close(cli));
+ }
+
+ ASSERT_NE(-1, self->server_socket);
+
+ if (variant->domain_server)
+ create_unix_domain(_metadata);
+
+ ASSERT_EQ(0, bind(self->server_socket, &self->address.unix_addr,
+ (self->address).unix_addr_len));
+ if (variant->type == SOCK_STREAM)
+ ASSERT_EQ(0, listen(self->server_socket, 32));
+ /* signal to child that parent is listening */
+ ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+
+ ASSERT_EQ(child, waitpid(child, &status, 0));
+
+ if (WIFSIGNALED(status) || !WIFEXITED(status) ||
+ WEXITSTATUS(status) != EXIT_SUCCESS)
+ _metadata->exit_code = KSFT_FAIL;
+}
+
+static const char path1_variant1[] = TMP_DIR "/s1_variant1";
+static const char path2_variant1[] = TMP_DIR "/s2_variant1";
+
+/* clang-format off */
+FIXTURE(pathname_address_sockets) {
+ struct service_fixture stream_address, dgram_address;
+};
+
+/* clang-format on */
+FIXTURE_VARIANT(pathname_address_sockets)
+{
+ const int domain;
+ const char *stream_path;
+ const char *dgram_path;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(pathname_address_sockets, pathname_socket_scoped_domain) {
+ /* clang-format on */
+ .domain = SCOPE_SANDBOX,
+ .stream_path = path1_variant1,
+ .dgram_path = path2_variant1,
+};
+
+FIXTURE_SETUP(pathname_address_sockets)
+{
+ /* setup abstract addresses */
+ memset(&self->stream_address, 0, sizeof(self->stream_address));
+ set_unix_address(&self->stream_address, 0);
+
+ memset(&self->dgram_address, 0, sizeof(self->dgram_address));
+ set_unix_address(&self->dgram_address, 0);
+
+ const char *s_path = variant->stream_path;
+ const char *dg_path = variant->dgram_path;
+
+ disable_caps(_metadata);
+ umask(0077);
+ ASSERT_EQ(0, mkdir(TMP_DIR, 0700));
+
+ ASSERT_EQ(0, mknod(s_path, S_IFREG | 0700, 0))
+ {
+ TH_LOG("Failed to create file \"%s\": %s", s_path,
+ strerror(errno));
+ remove_path(TMP_DIR);
+ }
+ ASSERT_EQ(0, mknod(dg_path, S_IFREG | 0700, 0))
+ {
+ TH_LOG("Failed to create file \"%s\": %s", dg_path,
+ strerror(errno));
+ remove_path(TMP_DIR);
+ }
+}
+
+FIXTURE_TEARDOWN(pathname_address_sockets)
+{
+ const char *s_path = variant->stream_path;
+ const char *dg_path = variant->dgram_path;
+
+ ASSERT_EQ(0, remove_path(dg_path));
+ ASSERT_EQ(0, remove_path(s_path));
+ ASSERT_EQ(0, remove_path(TMP_DIR));
+}
+
+TEST_F(pathname_address_sockets, scoped_pathname_sockets)
+{
+ const char *const stream_path = variant->stream_path;
+ const char *const dgram_path = variant->dgram_path;
+ int srv_fd, srv_fd_dg;
+ socklen_t size, size_dg;
+ struct sockaddr_un srv_un, srv_un_dg;
+ int pipe_parent[2];
+ pid_t child;
+ int status;
+ char buf_child;
+ int socket_fds_stream[2];
+ int server, client, dgram_server, dgram_client;
+ int recv_data;
+
+ ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0,
+ socket_fds_stream));
+
+ ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
+
+ child = fork();
+ ASSERT_LE(0, child);
+ if (child == 0) {
+ struct sockaddr_un cli_un, cli_un_dg;
+ int cli_fd, cli_fd_dg;
+ socklen_t size, size_dg;
+ int sample = socket(AF_UNIX, SOCK_STREAM, 0);
+ int err, err_dg;
+
+ ASSERT_LE(0, sample);
+
+ ASSERT_EQ(0, close(pipe_parent[1]));
+
+ /* scope the domain */
+ if (variant->domain == SCOPE_SANDBOX)
+ create_unix_domain(_metadata);
+ else if (variant->domain == OTHER_SANDBOX)
+ create_fs_domain(_metadata);
+ ASSERT_EQ(0, close(socket_fds_stream[1]));
+ ASSERT_EQ(0, send_fd(socket_fds_stream[0], sample));
+ ASSERT_EQ(0, close(sample));
+ ASSERT_EQ(0, close(socket_fds_stream[0]));
+
+ /* wait for server to listen */
+ ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+
+ cli_un.sun_family = AF_UNIX;
+ cli_fd = socket(AF_UNIX, SOCK_STREAM, 0);
+ ASSERT_LE(0, cli_fd);
+
+ size = offsetof(struct sockaddr_un, sun_path) +
+ strlen(cli_un.sun_path);
+ ASSERT_EQ(0, bind(cli_fd, (struct sockaddr *)&cli_un, size));
+
+ bzero(&cli_un, sizeof(cli_un));
+ cli_un.sun_family = AF_UNIX;
+ snprintf(cli_un.sun_path, sizeof(cli_un.sun_path), "%s",
+ stream_path);
+ size = offsetof(struct sockaddr_un, sun_path) +
+ strlen(cli_un.sun_path);
+
+ ASSERT_EQ(0, connect(cli_fd, (struct sockaddr *)&cli_un, size));
+
+ ASSERT_EQ(0, close(cli_fd));
+
+ cli_un_dg.sun_family = AF_UNIX;
+ cli_fd_dg = socket(AF_UNIX, SOCK_DGRAM, 0);
+ ASSERT_LE(0, cli_fd_dg);
+
+ size_dg = offsetof(struct sockaddr_un, sun_path) +
+ strlen(cli_un_dg.sun_path);
+ ASSERT_EQ(0, bind(cli_fd_dg, (struct sockaddr *)&cli_un_dg,
+ size_dg));
+
+ bzero(&cli_un_dg, sizeof(cli_un_dg));
+ cli_un_dg.sun_family = AF_UNIX;
+ snprintf(cli_un_dg.sun_path, sizeof(cli_un_dg.sun_path), "%s",
+ dgram_path);
+ size_dg = offsetof(struct sockaddr_un, sun_path) +
+ strlen(cli_un_dg.sun_path);
+
+ ASSERT_EQ(0, connect(cli_fd_dg, (struct sockaddr *)&cli_un_dg,
+ size_dg));
+
+ ASSERT_EQ(0, close(cli_fd_dg));
+
+ /* check connection with abstract sockets */
+
+ client = socket(AF_UNIX, SOCK_STREAM, 0);
+ dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
+
+ ASSERT_NE(-1, client);
+ ASSERT_NE(-1, dgram_client);
+
+ err = connect(client, &(self->stream_address).unix_addr,
+ (self->stream_address).unix_addr_len);
+ err_dg = connect(dgram_client, &(self->dgram_address).unix_addr,
+ (self->dgram_address).unix_addr_len);
+ if (variant->domain == SCOPE_SANDBOX) {
+ EXPECT_EQ(-1, err);
+ EXPECT_EQ(-1, err_dg);
+ EXPECT_EQ(EPERM, errno);
+ } else {
+ EXPECT_EQ(0, err);
+ EXPECT_EQ(0, err_dg);
+ }
+ ASSERT_EQ(0, close(client));
+ ASSERT_EQ(0, close(dgram_client));
+
+ _exit(_metadata->exit_code);
+ return;
+ }
+
+ ASSERT_EQ(0, close(pipe_parent[0]));
+
+ recv_data = recv_fd(socket_fds_stream[1]);
+ ASSERT_LE(0, recv_data);
+ ASSERT_LE(0, close(socket_fds_stream[1]));
+
+ /* Sets up a server */
+ srv_un.sun_family = AF_UNIX;
+ snprintf(srv_un.sun_path, sizeof(srv_un.sun_path), "%s", stream_path);
+
+ ASSERT_EQ(0, unlink(stream_path));
+ srv_fd = socket(AF_UNIX, SOCK_STREAM, 0);
+ ASSERT_LE(0, srv_fd);
+
+ size = offsetof(struct sockaddr_un, sun_path) + strlen(srv_un.sun_path);
+ ASSERT_EQ(0, bind(srv_fd, (struct sockaddr *)&srv_un, size));
+ ASSERT_EQ(0, listen(srv_fd, 10));
+
+ /* set up a datagram server */
+ srv_un_dg.sun_family = AF_UNIX;
+ snprintf(srv_un_dg.sun_path, sizeof(srv_un_dg.sun_path), "%s",
+ dgram_path);
+
+ ASSERT_EQ(0, unlink(dgram_path));
+ srv_fd_dg = socket(AF_UNIX, SOCK_DGRAM, 0);
+ ASSERT_LE(0, srv_fd_dg);
+
+ size_dg = offsetof(struct sockaddr_un, sun_path) +
+ strlen(srv_un_dg.sun_path);
+ ASSERT_EQ(0, bind(srv_fd_dg, (struct sockaddr *)&srv_un_dg, size_dg));
+
+ /*set up abstract servers */
+
+ server = socket(AF_UNIX, SOCK_STREAM, 0);
+ dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
+ ASSERT_NE(-1, server);
+ ASSERT_NE(-1, dgram_server);
+
+ ASSERT_EQ(0, bind(server, &(self->stream_address).unix_addr,
+ self->stream_address.unix_addr_len));
+ ASSERT_EQ(0, bind(dgram_server, &(self->dgram_address).unix_addr,
+ self->dgram_address.unix_addr_len));
+ ASSERT_EQ(0, listen(server, 32));
+
+ /* signal to child! */
+ ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+
+ ASSERT_EQ(child, waitpid(child, &status, 0));
+ ASSERT_EQ(0, close(srv_fd));
+ ASSERT_EQ(0, close(srv_fd_dg));
+ ASSERT_EQ(0, close(server));
+ ASSERT_EQ(0, close(dgram_server));
+
+ if (WIFSIGNALED(status) || !WIFEXITED(status) ||
+ WEXITSTATUS(status) != EXIT_SUCCESS)
+ _metadata->exit_code = KSFT_FAIL;
+}
+
+TEST_HARNESS_MAIN
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction
2024-08-02 4:02 [PATCH v8 0/4] Landlock: Add abstract unix socket connect Tahera Fahimi
2024-08-02 4:02 ` [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction Tahera Fahimi
2024-08-02 4:02 ` [PATCH v8 2/4] selftests/landlock: Abstract unix socket restriction tests Tahera Fahimi
@ 2024-08-02 4:02 ` Tahera Fahimi
2024-08-09 14:11 ` Mickaël Salaün
2024-08-02 4:02 ` [PATCH v8 4/4] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning Tahera Fahimi
3 siblings, 1 reply; 23+ messages in thread
From: Tahera Fahimi @ 2024-08-02 4:02 UTC (permalink / raw)
To: outreachy
Cc: mic, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev, Tahera Fahimi
A sandboxer can receive the character "a" as input from the environment
variable LL_SCOPE to restrict the abstract unix sockets from connecting
to a process outside its scoped domain.
Example
=======
Create an abstract unix socket to listen with socat(1):
socat abstract-listen:mysocket -
Create a sandboxed shell and pass the character "a" to LL_SCOPED:
LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash
If the sandboxed process tries to connect to the listening socket
with command "socat - abstract-connect:mysocket", the connection
will fail.
Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
v8:
- Adding check_ruleset_scope function to parse the scope environment
variable and update the landlock attribute based on the restriction
provided by the user.
- Adding Mickaël Salaün reviews on version 7.
v7:
- Adding IPC scoping to the sandbox demo by defining a new "LL_SCOPED"
environment variable. "LL_SCOPED" gets value "a" to restrict abstract
unix sockets.
- Change LANDLOCK_ABI_LAST to 6.
---
samples/landlock/sandboxer.c | 56 +++++++++++++++++++++++++++++++++---
1 file changed, 52 insertions(+), 4 deletions(-)
diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index e8223c3e781a..98132fd823ad 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -14,6 +14,7 @@
#include <fcntl.h>
#include <linux/landlock.h>
#include <linux/prctl.h>
+#include <linux/socket.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
@@ -22,6 +23,7 @@
#include <sys/stat.h>
#include <sys/syscall.h>
#include <unistd.h>
+#include <stdbool.h>
#ifndef landlock_create_ruleset
static inline int
@@ -55,6 +57,7 @@ static inline int landlock_restrict_self(const int ruleset_fd,
#define ENV_FS_RW_NAME "LL_FS_RW"
#define ENV_TCP_BIND_NAME "LL_TCP_BIND"
#define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT"
+#define ENV_SCOPED_NAME "LL_SCOPED"
#define ENV_DELIMITER ":"
static int parse_path(char *env_path, const char ***const path_list)
@@ -184,6 +187,38 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
return ret;
}
+static bool check_ruleset_scope(const char *const env_var,
+ struct landlock_ruleset_attr *ruleset_attr)
+{
+ bool ret = true;
+ char *env_type_scope, *env_type_scope_next, *ipc_scoping_name;
+
+ ruleset_attr->scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
+ env_type_scope = getenv(env_var);
+ /* scoping is not supported by the user */
+ if (!env_type_scope)
+ return true;
+ env_type_scope = strdup(env_type_scope);
+ unsetenv(env_var);
+
+ env_type_scope_next = env_type_scope;
+ while ((ipc_scoping_name =
+ strsep(&env_type_scope_next, ENV_DELIMITER))) {
+ if (strcmp("a", ipc_scoping_name) == 0) {
+ ruleset_attr->scoped |=
+ LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
+ } else {
+ fprintf(stderr, "Unsupported scoping \"%s\"\n",
+ ipc_scoping_name);
+ ret = false;
+ goto out_free_name;
+ }
+ }
+out_free_name:
+ free(env_type_scope);
+ return ret;
+}
+
/* clang-format off */
#define ACCESS_FS_ROUGHLY_READ ( \
@@ -208,7 +243,7 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
/* clang-format on */
-#define LANDLOCK_ABI_LAST 5
+#define LANDLOCK_ABI_LAST 6
int main(const int argc, char *const argv[], char *const *const envp)
{
@@ -223,14 +258,15 @@ int main(const int argc, char *const argv[], char *const *const envp)
.handled_access_fs = access_fs_rw,
.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
LANDLOCK_ACCESS_NET_CONNECT_TCP,
+ .scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
};
if (argc < 2) {
fprintf(stderr,
- "usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
+ "usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
"<cmd> [args]...\n\n",
ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
- ENV_TCP_CONNECT_NAME, argv[0]);
+ ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
fprintf(stderr,
"Execute a command in a restricted environment.\n\n");
fprintf(stderr,
@@ -251,15 +287,18 @@ int main(const int argc, char *const argv[], char *const *const envp)
fprintf(stderr,
"* %s: list of ports allowed to connect (client).\n",
ENV_TCP_CONNECT_NAME);
+ fprintf(stderr, "* %s: list of restrictions on IPCs.\n",
+ ENV_SCOPED_NAME);
fprintf(stderr,
"\nexample:\n"
"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
"%s=\"9418\" "
"%s=\"80:443\" "
+ "%s=\"a\" "
"%s bash -i\n\n",
ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
- ENV_TCP_CONNECT_NAME, argv[0]);
+ ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
fprintf(stderr,
"This sandboxer can use Landlock features "
"up to ABI version %d.\n",
@@ -327,6 +366,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
/* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
+ __attribute__((fallthrough));
+ case 5:
+ /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
+ ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
fprintf(stderr,
"Hint: You should update the running kernel "
"to leverage Landlock features "
@@ -358,6 +401,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
~LANDLOCK_ACCESS_NET_CONNECT_TCP;
}
+ if (!check_ruleset_scope(ENV_SCOPED_NAME, &ruleset_attr)) {
+ perror("Unsupported IPC scoping requested");
+ return 1;
+ }
+
ruleset_fd =
landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
if (ruleset_fd < 0) {
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v8 4/4] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning
2024-08-02 4:02 [PATCH v8 0/4] Landlock: Add abstract unix socket connect Tahera Fahimi
` (2 preceding siblings ...)
2024-08-02 4:02 ` [PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction Tahera Fahimi
@ 2024-08-02 4:02 ` Tahera Fahimi
2024-08-07 15:14 ` Mickaël Salaün
3 siblings, 1 reply; 23+ messages in thread
From: Tahera Fahimi @ 2024-08-02 4:02 UTC (permalink / raw)
To: outreachy
Cc: mic, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev, Tahera Fahimi
Introducing LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET as an IPC scoping
mechanism in Landlock ABI version 6, and updating ruleset_attr,
Landlock ABI version, and access rights code blocks based on that.
Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
v8:
- Improving documentation by specifying differences between scoped and
non-scoped domains.
- Adding review notes of version 7.
- Update date
v7:
- Add "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" explanation to IPC scoping
section and updating ABI to version 6.
- Adding "scoped" attribute to the Access rights section.
- In current limitation, unnamed sockets are specified as sockets that
are not restricted.
- Update date
---
Documentation/userspace-api/landlock.rst | 33 ++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 07b63aec56fa..d602567b5139 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -8,7 +8,7 @@ Landlock: unprivileged access control
=====================================
:Author: Mickaël Salaün
-:Date: April 2024
+:Date: August 2024
The goal of Landlock is to enable to restrict ambient rights (e.g. global
filesystem or network access) for a set of processes. Because Landlock
@@ -81,6 +81,8 @@ to be explicit about the denied-by-default access rights.
.handled_access_net =
LANDLOCK_ACCESS_NET_BIND_TCP |
LANDLOCK_ACCESS_NET_CONNECT_TCP,
+ .scoped =
+ LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
};
Because we may not know on which kernel version an application will be
@@ -119,6 +121,9 @@ version, and only use the available subset of access rights:
case 4:
/* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
+ case 5:
+ /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
+ ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
}
This enables to create an inclusive ruleset that will contain our rules.
@@ -306,6 +311,23 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
process, a sandboxed process should have a subset of the target process rules,
which means the tracee must be in a sub-domain of the tracer.
+IPC Scoping
+-----------
+
+Similar to the implicit `Ptrace restrictions`_, we may want to further restrict
+interactions between sandboxes. Each Landlock domain can be explicitly scoped
+for a set of actions by specifying it on a ruleset. For example, if a sandboxed
+process should not be able to :manpage:`connect(2)` to a non-sandboxed process
+through abstract :manpage:`unix(7)` sockets, we can specify such restriction
+with ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``.
+
+A sandboxed process can connect to a non-sandboxed process when its domain is
+not scoped. If a process's domain is scoped, it can only connect to processes in
+the same scoped domain.
+
+IPC scoping does not support Landlock rules, so if a domain is scoped, no rules
+can be added to allow accessing to a resource outside of the scoped domain.
+
Truncating files
----------------
@@ -404,7 +426,7 @@ Access rights
-------------
.. kernel-doc:: include/uapi/linux/landlock.h
- :identifiers: fs_access net_access
+ :identifiers: fs_access net_access scope
Creating a new ruleset
----------------------
@@ -541,6 +563,13 @@ earlier ABI.
Starting with the Landlock ABI version 5, it is possible to restrict the use of
:manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right.
+Abstract Unix sockets Restriction (ABI < 6)
+--------------------------------------------
+
+With ABI version 6, it is possible to restrict connection to an abstract Unix socket
+through ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``, thanks to the ``scoped`` ruleset
+attribute.
+
.. _kernel_support:
Kernel support
--
2.34.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-02 4:02 ` [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction Tahera Fahimi
@ 2024-08-02 16:47 ` Mickaël Salaün
2024-08-03 11:29 ` Mickaël Salaün
2024-08-06 19:36 ` Jann Horn
2 siblings, 0 replies; 23+ messages in thread
From: Mickaël Salaün @ 2024-08-02 16:47 UTC (permalink / raw)
To: Tahera Fahimi
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev
On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> This 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. It implements two hooks, unix_stream_connect
> and unix_may_send to enforce this restriction.
>
> Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
>
> ---
> v8:
> - Code refactoring (improve code readability, renaming variable, etc.) based
> on reviews by Mickaël Salaün on version 7.
> - Adding warn_on_once to check (impossible) inconsistencies.
> - Adding inline comments.
> - Adding check_unix_address_format to check if the scoping socket is an abstract
> unix sockets.
> v7:
> - 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/
> 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 socket's file 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/all/20240610.Aifee5ingugh@digikod.net/
> ----
> ---
> include/uapi/linux/landlock.h | 30 +++++++
> security/landlock/limits.h | 3 +
> security/landlock/ruleset.c | 7 +-
> security/landlock/ruleset.h | 23 ++++-
> security/landlock/syscalls.c | 14 ++-
> security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
> 6 files changed, 225 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..ab31e9f53e55 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -37,6 +37,12 @@ struct landlock_ruleset_attr {
> * rule explicitly allow them.
> */
> __u64 handled_access_net;
> + /**
> + * @scoped: Bitmask of scopes (cf. `Scope flags`_)
> + * restricting a Landlock domain from accessing outside
> + * resources(e.g. IPCs).
> + */
> + __u64 scoped;
> };
>
> /*
> @@ -266,4 +272,28 @@ struct landlock_net_port_attr {
> #define LANDLOCK_ACCESS_NET_BIND_TCP (1ULL << 0)
> #define LANDLOCK_ACCESS_NET_CONNECT_TCP (1ULL << 1)
> /* clang-format on */
> +
> +/**
> + * DOC: scope
> + *
> + * scoped attribute handles a set of restrictions on kernel IPCs through
> + * the following flags.
If present, this previous sentence should be after the "Scope flags"
section, otherwise it shows in the previous section. I don't think this
sentence is useful because of the next one, so you can remove it.
> + *
> + * Scope flags
> + * ~~~~~~~~~~~
> + *
> + * These flags enable to restrict a sandboxed process from a set of IPC
> + * actions. Setting a flag for a ruleset will isolate the Landlock domain
> + * to forbid connections to resources outside the domain.
> + *
> + * IPCs with scoped actions:
> + *
> + * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandboxed process
> + * from connecting to an abstract unix socket created by a process
> + * outside the related Landlock domain (e.g. a parent domain or a
> + * non-sandboxed process).
> + */
> +/* clang-format off */
> +#define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET (1ULL << 0)
> +/* clang-format on*/
> #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 4eb643077a2a..eb01d0fb2165 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -26,6 +26,9 @@
> #define LANDLOCK_MASK_ACCESS_NET ((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
> #define LANDLOCK_NUM_ACCESS_NET __const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>
> +#define LANDLOCK_LAST_SCOPE LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
> +#define LANDLOCK_MASK_SCOPE ((LANDLOCK_LAST_SCOPE << 1) - 1)
> +#define LANDLOCK_NUM_SCOPE __const_hweight64(LANDLOCK_MASK_SCOPE)
> /* clang-format on */
>
> #endif /* _SECURITY_LANDLOCK_LIMITS_H */
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index 6ff232f58618..a93bdbf52fff 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -52,12 +52,13 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>
> struct landlock_ruleset *
> landlock_create_ruleset(const access_mask_t fs_access_mask,
> - const access_mask_t net_access_mask)
> + const access_mask_t net_access_mask,
> + const access_mask_t scope_mask)
> {
> struct landlock_ruleset *new_ruleset;
>
> /* Informs about useless ruleset. */
> - if (!fs_access_mask && !net_access_mask)
> + if (!fs_access_mask && !net_access_mask && !scope_mask)
> return ERR_PTR(-ENOMSG);
> new_ruleset = create_ruleset(1);
> if (IS_ERR(new_ruleset))
> @@ -66,6 +67,8 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
> landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
> if (net_access_mask)
> landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
> + if (scope_mask)
> + landlock_add_scope_mask(new_ruleset, scope_mask, 0);
> return new_ruleset;
> }
>
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 0f1b5b4c8f6b..c749fa0b3ecd 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -35,6 +35,8 @@ typedef u16 access_mask_t;
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
> /* Makes sure all network access rights can be stored. */
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
> +/* Makes sure all scoped rights can be stored*/
> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
> /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
> static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>
> @@ -42,6 +44,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
> struct access_masks {
> access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
> access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
> + access_mask_t scoped : LANDLOCK_NUM_SCOPE;
> };
>
> typedef u16 layer_mask_t;
> @@ -233,7 +236,8 @@ struct landlock_ruleset {
>
> struct landlock_ruleset *
> landlock_create_ruleset(const access_mask_t access_mask_fs,
> - const access_mask_t access_mask_net);
> + const access_mask_t access_mask_net,
> + const access_mask_t scope_mask);
>
> void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
> void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
> @@ -280,6 +284,16 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
> ruleset->access_masks[layer_level].net |= net_mask;
> }
>
> +static inline void
> +landlock_add_scope_mask(struct landlock_ruleset *const ruleset,
> + const access_mask_t scope_mask, const u16 layer_level)
> +{
> + access_mask_t scoped_mask = scope_mask & LANDLOCK_MASK_SCOPE;
> +
> + WARN_ON_ONCE(scope_mask != scoped_mask);
> + ruleset->access_masks[layer_level].scoped |= scoped_mask;
> +}
> +
> static inline access_mask_t
> landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
> const u16 layer_level)
> @@ -303,6 +317,13 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
> return ruleset->access_masks[layer_level].net;
> }
>
> +static inline access_mask_t
> +landlock_get_scope_mask(const struct landlock_ruleset *const ruleset,
> + const u16 layer_level)
> +{
> + return ruleset->access_masks[layer_level].scoped;
> +}
> +
> bool landlock_unmask_layers(const struct landlock_rule *const rule,
> const access_mask_t access_request,
> layer_mask_t (*const layer_masks)[],
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 03b470f5a85a..f51b29521d6b 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -97,8 +97,9 @@ static void build_check_abi(void)
> */
> ruleset_size = sizeof(ruleset_attr.handled_access_fs);
> ruleset_size += sizeof(ruleset_attr.handled_access_net);
> + ruleset_size += sizeof(ruleset_attr.scoped);
> BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
> - BUILD_BUG_ON(sizeof(ruleset_attr) != 16);
> + BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
>
> path_beneath_size = sizeof(path_beneath_attr.allowed_access);
> path_beneath_size += sizeof(path_beneath_attr.parent_fd);
> @@ -149,7 +150,7 @@ static const struct file_operations ruleset_fops = {
> .write = fop_dummy_write,
> };
>
> -#define LANDLOCK_ABI_VERSION 5
> +#define LANDLOCK_ABI_VERSION 6
>
> /**
> * sys_landlock_create_ruleset - Create a new ruleset
> @@ -170,7 +171,7 @@ static const struct file_operations ruleset_fops = {
> * Possible returned errors are:
> *
> * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
> - * - %EINVAL: unknown @flags, or unknown access, or too small @size;
> + * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size;
I think you missed one of my previous review.
> * - %E2BIG or %EFAULT: @attr or @size inconsistencies;
> * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs.
> */
> @@ -213,9 +214,14 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> LANDLOCK_MASK_ACCESS_NET)
> return -EINVAL;
>
> + /* Checks IPC scoping content (and 32-bits cast). */
> + if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
> + return -EINVAL;
> +
> /* Checks arguments and transforms to kernel struct. */
> ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
> - ruleset_attr.handled_access_net);
> + ruleset_attr.handled_access_net,
> + ruleset_attr.scoped);
> if (IS_ERR(ruleset))
> return PTR_ERR(ruleset);
>
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..7e8579ebae83 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>
Please sort the included files.
>
> #include "common.h"
> #include "cred.h"
> @@ -108,9 +110,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> return task_ptrace(parent, current);
> }
>
> +static bool walk_and_check(const struct landlock_ruleset *const child,
> + struct landlock_hierarchy **walker,
> + size_t base_layer, size_t deep_layer,
> + access_mask_t check_scoping)
> +{
> + if (!child || base_layer < 0 || !(*walker))
> + return false;
> +
> + for (deep_layer; base_layer < deep_layer; deep_layer--) {
> + if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> + return false;
> + *walker = (*walker)->parent;
> + if (WARN_ON_ONCE(!*walker))
> + /* there is an inconsistency between num_layers
> + * and landlock_hierarchy in the ruleset
> + */
> + return false;
> + }
> + return true;
> +}
> +
> +/**
> + * domain_IPC_scope - Checks if the client domain is scoped in the same
Please don't use uppercase (IPC) in function or variable's names, it is
reserved to "#define" statements only.
> + * domain as the server.
> + *
> + * @client: IPC sender domain.
> + * @server: IPC receiver domain.
> + *
> + * Check if the @client domain is scoped to access the @server; the @server
> + * must be scoped in the same domain.
> + */
> +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> + const struct landlock_ruleset *const server,
> + access_mask_t ipc_type)
> +{
> + size_t client_layer, server_layer = 0;
> + int base_layer;
> + struct landlock_hierarchy *client_walker, *server_walker;
> + bool is_scoped;
> +
> + /* Quick return if client has no domain */
> + if (!client)
> + return true;
> +
> + client_layer = client->num_layers - 1;
> + client_walker = client->hierarchy;
> + if (server) {
> + server_layer = server->num_layers - 1;
> + server_walker = server->hierarchy;
> + }
> + base_layer = (client_layer > server_layer) ? server_layer :
> + client_layer;
> +
> + /* For client domain, walk_and_check ensures the client domain is
> + * not scoped until gets to base_layer.
> + * For server_domain, it only ensures that the server domain exist.
> + */
> + if (client_layer != server_layer) {
> + if (client_layer > server_layer)
> + is_scoped = walk_and_check(client, &client_walker,
> + server_layer, client_layer,
> + ipc_type);
> + else
> + is_scoped = walk_and_check(server, &server_walker,
> + client_layer, server_layer,
> + ipc_type & 0);
> + if (!is_scoped)
> + return false;
> + }
> + /* client and server are at the same level in hierarchy. If client is
> + * scoped, the server must be scoped in the same domain
> + */
> + for (base_layer; base_layer >= 0; base_layer--) {
> + if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
> + /* This check must be here since access would be denied only if
> + * the client is scoped and the server has no domain, so
> + * if the client has a domain but is not scoped and the server
> + * has no domain, access is guaranteed.
> + */
> + if (!server)
> + return false;
> +
> + if (server_walker == client_walker)
> + return true;
> +
> + return false;
> + }
> + client_walker = client_walker->parent;
> + server_walker = server_walker->parent;
> + /* Warn if there is an incosistenncy between num_layers and
> + * landlock_hierarchy in each of rulesets
> + */
> + if (WARN_ON_ONCE(base_layer > 0 &&
> + (!server_walker || !client_walker)))
> + return false;
> + }
> + 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;
> + return domain_IPC_scope(dom, dom_other,
> + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET);
> +}
> +
> +static bool check_unix_address_format(struct sock *const sock)
> +{
> + struct unix_address *addr = unix_sk(sock)->addr;
> +
> + if (!addr)
> + return true;
> +
> + if (addr->len > sizeof(AF_UNIX)) {
AF_UNIX is the family, not a length.
I'm wondering if we cannot also check the "path" field or something
else.
> + /* handling unspec sockets */
> + if (!addr->name[0].sun_path)
> + return true;
> +
> + if (addr->name[0].sun_path[0] == '\0')
> + if (!sock_is_scoped(sock))
This function should only check the address format. sock_is_scoped()
can be called in each LSM hook.
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static int hook_unix_stream_connect(struct sock *const sock,
> + struct sock *const other,
> + struct sock *const newsk)
> +{
To not impact unsandboxed tasks, we need to first check if current is
sandboxed with Landlock, and then pass this dom to the following
functions.
> + if (check_unix_address_format(other))
> + return 0;
> +
> + return -EPERM;
> +}
> +
> +static int hook_unix_may_send(struct socket *const sock,
> + struct socket *const other)
> +{
> + if (check_unix_address_format(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
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-02 4:02 ` [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction Tahera Fahimi
2024-08-02 16:47 ` Mickaël Salaün
@ 2024-08-03 11:29 ` Mickaël Salaün
2024-08-06 19:35 ` Mickaël Salaün
2024-08-07 15:37 ` Tahera Fahimi
2024-08-06 19:36 ` Jann Horn
2 siblings, 2 replies; 23+ messages in thread
From: Mickaël Salaün @ 2024-08-03 11:29 UTC (permalink / raw)
To: Tahera Fahimi
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev
On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> This 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. It implements two hooks, unix_stream_connect
> and unix_may_send to enforce this restriction.
>
> Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
>
> ---
> v8:
> - Code refactoring (improve code readability, renaming variable, etc.) based
> on reviews by Mickaël Salaün on version 7.
> - Adding warn_on_once to check (impossible) inconsistencies.
> - Adding inline comments.
> - Adding check_unix_address_format to check if the scoping socket is an abstract
> unix sockets.
> v7:
> - 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/
> 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 socket's file 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/all/20240610.Aifee5ingugh@digikod.net/
> ----
> ---
> include/uapi/linux/landlock.h | 30 +++++++
> security/landlock/limits.h | 3 +
> security/landlock/ruleset.c | 7 +-
> security/landlock/ruleset.h | 23 ++++-
> security/landlock/syscalls.c | 14 ++-
> security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
> 6 files changed, 225 insertions(+), 7 deletions(-)
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..7e8579ebae83 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,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> return task_ptrace(parent, current);
> }
>
> +static bool walk_and_check(const struct landlock_ruleset *const child,
> + struct landlock_hierarchy **walker,
> + size_t base_layer, size_t deep_layer,
> + access_mask_t check_scoping)
s/check_scoping/scope/
> +{
> + if (!child || base_layer < 0 || !(*walker))
I guess it should be:
WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
> + return false;
> +
> + for (deep_layer; base_layer < deep_layer; deep_layer--) {
No need to pass deep_layer as argument:
deep_layer = child->num_layers - 1
> + if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> + return false;
> + *walker = (*walker)->parent;
> + if (WARN_ON_ONCE(!*walker))
> + /* there is an inconsistency between num_layers
Please use full sentences starting with a capital letter and ending with
a dot, and in this case start with "/*"
> + * and landlock_hierarchy in the ruleset
> + */
> + return false;
> + }
> + return true;
> +}
> +
> +/**
> + * domain_IPC_scope - Checks if the client domain is scoped in the same
> + * domain as the server.
Actually, you can remove IPC from the function name.
> + *
> + * @client: IPC sender domain.
> + * @server: IPC receiver domain.
> + *
> + * Check if the @client domain is scoped to access the @server; the @server
> + * must be scoped in the same domain.
Returns true if...
> + */
> +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> + const struct landlock_ruleset *const server,
> + access_mask_t ipc_type)
> +{
> + size_t client_layer, server_layer = 0;
> + int base_layer;
> + struct landlock_hierarchy *client_walker, *server_walker;
> + bool is_scoped;
> +
> + /* Quick return if client has no domain */
> + if (!client)
> + return true;
> +
> + client_layer = client->num_layers - 1;
> + client_walker = client->hierarchy;
> + if (server) {
> + server_layer = server->num_layers - 1;
> + server_walker = server->hierarchy;
> + }
} else {
server_layer = 0;
server_walker = NULL;
}
> + base_layer = (client_layer > server_layer) ? server_layer :
> + client_layer;
> +
> + /* For client domain, walk_and_check ensures the client domain is
> + * not scoped until gets to base_layer.
until gets?
> + * For server_domain, it only ensures that the server domain exist.
> + */
> + if (client_layer != server_layer) {
bool is_scoped;
> + if (client_layer > server_layer)
> + is_scoped = walk_and_check(client, &client_walker,
> + server_layer, client_layer,
> + ipc_type);
> + else
server_walker may be uninitialized and still read here, and maybe later
in the for loop. The whole code should maks sure this cannot happen,
and a test case should check this.
> + is_scoped = walk_and_check(server, &server_walker,
> + client_layer, server_layer,
> + ipc_type & 0);
"ipc_type & 0" is the same as "0"
> + if (!is_scoped)
The name doesn't reflect the semantic. walk_and_check() should return
the inverse.
> + return false;
> + }
This code would be simpler:
if (client_layer > server_layer) {
base_layer = server_layer;
// TODO: inverse boolean logic
if (!walk_and_check(client, &client_walker,
base_layer, ipc_type))
return false;
} else (client_layer < server_layer) {
base_layer = client_layer;
// TODO: inverse boolean logic
if (!walk_and_check(server, &server_walker,
base_layer, 0))
return false;
} else {
base_layer = client_layer;
}
I think we can improve more to make sure there is no path/risk of
inconsistent pointers.
> + /* client and server are at the same level in hierarchy. If client is
> + * scoped, the server must be scoped in the same domain
> + */
> + for (base_layer; base_layer >= 0; base_layer--) {
> + if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
With each multi-line comment, the first line should be empty:
/*
* This check must be here since access would be denied only if
> + /* This check must be here since access would be denied only if
> + * the client is scoped and the server has no domain, so
> + * if the client has a domain but is not scoped and the server
> + * has no domain, access is guaranteed.
> + */
> + if (!server)
> + return false;
> +
> + if (server_walker == client_walker)
> + return true;
> +
> + return false;
> + }
> + client_walker = client_walker->parent;
> + server_walker = server_walker->parent;
> + /* Warn if there is an incosistenncy between num_layers and
Makes sure there is no inconsistency between num_layers and
> + * landlock_hierarchy in each of rulesets
> + */
> + if (WARN_ON_ONCE(base_layer > 0 &&
> + (!server_walker || !client_walker)))
> + return false;
> + }
> + return true;
> +}
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-03 11:29 ` Mickaël Salaün
@ 2024-08-06 19:35 ` Mickaël Salaün
2024-08-06 20:46 ` Jann Horn
2024-08-07 15:37 ` Tahera Fahimi
1 sibling, 1 reply; 23+ messages in thread
From: Mickaël Salaün @ 2024-08-06 19:35 UTC (permalink / raw)
To: Tahera Fahimi
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev
On Sat, Aug 03, 2024 at 01:29:09PM +0200, Mickaël Salaün wrote:
> On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> > This 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. It implements two hooks, unix_stream_connect
> > and unix_may_send to enforce this restriction.
> >
> > Closes: https://github.com/landlock-lsm/linux/issues/7
> > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> >
> > ---
> > v8:
> > - Code refactoring (improve code readability, renaming variable, etc.) based
> > on reviews by Mickaël Salaün on version 7.
> > - Adding warn_on_once to check (impossible) inconsistencies.
> > - Adding inline comments.
> > - Adding check_unix_address_format to check if the scoping socket is an abstract
> > unix sockets.
> > v7:
> > - 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/
> > 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 socket's file 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/all/20240610.Aifee5ingugh@digikod.net/
> > ----
> > ---
> > include/uapi/linux/landlock.h | 30 +++++++
> > security/landlock/limits.h | 3 +
> > security/landlock/ruleset.c | 7 +-
> > security/landlock/ruleset.h | 23 ++++-
> > security/landlock/syscalls.c | 14 ++-
> > security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
> > 6 files changed, 225 insertions(+), 7 deletions(-)
>
> > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > index 849f5123610b..7e8579ebae83 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,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> > return task_ptrace(parent, current);
> > }
> >
> > +static bool walk_and_check(const struct landlock_ruleset *const child,
> > + struct landlock_hierarchy **walker,
> > + size_t base_layer, size_t deep_layer,
> > + access_mask_t check_scoping)
>
> s/check_scoping/scope/
>
> > +{
> > + if (!child || base_layer < 0 || !(*walker))
>
> I guess it should be:
> WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
>
> > + return false;
> > +
> > + for (deep_layer; base_layer < deep_layer; deep_layer--) {
>
> No need to pass deep_layer as argument:
> deep_layer = child->num_layers - 1
>
> > + if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> > + return false;
> > + *walker = (*walker)->parent;
> > + if (WARN_ON_ONCE(!*walker))
> > + /* there is an inconsistency between num_layers
>
> Please use full sentences starting with a capital letter and ending with
> a dot, and in this case start with "/*"
>
> > + * and landlock_hierarchy in the ruleset
> > + */
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > +/**
> > + * domain_IPC_scope - Checks if the client domain is scoped in the same
> > + * domain as the server.
>
> Actually, you can remove IPC from the function name.
>
> > + *
> > + * @client: IPC sender domain.
> > + * @server: IPC receiver domain.
> > + *
> > + * Check if the @client domain is scoped to access the @server; the @server
> > + * must be scoped in the same domain.
>
> Returns true if...
>
> > + */
> > +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> > + const struct landlock_ruleset *const server,
> > + access_mask_t ipc_type)
> > +{
> > + size_t client_layer, server_layer = 0;
> > + int base_layer;
> > + struct landlock_hierarchy *client_walker, *server_walker;
> > + bool is_scoped;
> > +
> > + /* Quick return if client has no domain */
> > + if (!client)
> > + return true;
> > +
> > + client_layer = client->num_layers - 1;
> > + client_walker = client->hierarchy;
> > + if (server) {
> > + server_layer = server->num_layers - 1;
> > + server_walker = server->hierarchy;
> > + }
>
> } else {
> server_layer = 0;
> server_walker = NULL;
> }
>
> > + base_layer = (client_layer > server_layer) ? server_layer :
> > + client_layer;
> > +
> > + /* For client domain, walk_and_check ensures the client domain is
> > + * not scoped until gets to base_layer.
>
> until gets?
>
> > + * For server_domain, it only ensures that the server domain exist.
> > + */
> > + if (client_layer != server_layer) {
>
> bool is_scoped;
>
> > + if (client_layer > server_layer)
> > + is_scoped = walk_and_check(client, &client_walker,
> > + server_layer, client_layer,
> > + ipc_type);
> > + else
>
> server_walker may be uninitialized and still read here, and maybe later
> in the for loop. The whole code should maks sure this cannot happen,
> and a test case should check this.
>
> > + is_scoped = walk_and_check(server, &server_walker,
> > + client_layer, server_layer,
> > + ipc_type & 0);
>
> "ipc_type & 0" is the same as "0"
>
> > + if (!is_scoped)
>
> The name doesn't reflect the semantic. walk_and_check() should return
> the inverse.
>
> > + return false;
> > + }
>
> This code would be simpler:
>
> if (client_layer > server_layer) {
> base_layer = server_layer;
> // TODO: inverse boolean logic
> if (!walk_and_check(client, &client_walker,
> base_layer, ipc_type))
> return false;
> } else (client_layer < server_layer) {
> base_layer = client_layer;
> // TODO: inverse boolean logic
> if (!walk_and_check(server, &server_walker,
> base_layer, 0))
> return false;
> } else {
> base_layer = client_layer;
> }
>
>
> I think we can improve more to make sure there is no path/risk of
> inconsistent pointers.
>
>
> > + /* client and server are at the same level in hierarchy. If client is
> > + * scoped, the server must be scoped in the same domain
> > + */
> > + for (base_layer; base_layer >= 0; base_layer--) {
> > + if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
>
> With each multi-line comment, the first line should be empty:
> /*
> * This check must be here since access would be denied only if
>
> > + /* This check must be here since access would be denied only if
> > + * the client is scoped and the server has no domain, so
> > + * if the client has a domain but is not scoped and the server
> > + * has no domain, access is guaranteed.
> > + */
> > + if (!server)
> > + return false;
> > +
> > + if (server_walker == client_walker)
> > + return true;
> > +
> > + return false;
> > + }
> > + client_walker = client_walker->parent;
> > + server_walker = server_walker->parent;
> > + /* Warn if there is an incosistenncy between num_layers and
>
> Makes sure there is no inconsistency between num_layers and
>
>
> > + * landlock_hierarchy in each of rulesets
> > + */
> > + if (WARN_ON_ONCE(base_layer > 0 &&
> > + (!server_walker || !client_walker)))
> > + return false;
> > + }
> > + return true;
> > +}
Here is a refactoring that is easier to read and avoid potential pointer
misuse:
static bool domain_is_scoped(const struct landlock_ruleset *const client,
const struct landlock_ruleset *const server,
access_mask_t scope)
{
int client_layer, server_layer;
struct landlock_hierarchy *client_walker, *server_walker;
if (WARN_ON_ONCE(!client))
return false;
client_layer = client->num_layers - 1;
client_walker = client->hierarchy;
/*
* client_layer must be a signed integer with greater capacity than
* client->num_layers to ensure the following loop stops.
*/
BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
if (!server) {
/*
* Walks client's parent domains and checks that none of these
* domains are scoped.
*/
for (; client_layer >= 0; client_layer--) {
if (landlock_get_scope_mask(client, client_layer) &
scope)
return true;
}
return false;
}
server_layer = server->num_layers - 1;
server_walker = server->hierarchy;
/*
* Walks client's parent domains down to the same hierarchy level as
* the server's domain, and checks that none of these client's parent
* domains are scoped.
*/
for (; client_layer > server_layer; client_layer--) {
if (landlock_get_scope_mask(client, client_layer) & scope)
return true;
client_walker = client_walker->parent;
}
/*
* Walks server's parent domains down to the same hierarchy level as
* the client's domain.
*/
for (; server_layer > client_layer; server_layer--)
server_walker = server_walker->parent;
for (; client_layer >= 0; client_layer--) {
if (landlock_get_scope_mask(client, client_layer) & scope) {
/*
* Client and server are at the same level in the
* hierarchy. If the client is scoped, the request is
* only allowed if this domain is also a server's
* ancestor.
*/
if (server_walker == client_walker)
return false;
return true;
}
client_walker = client_walker->parent;
server_walker = server_walker->parent;
}
return false;
}
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-02 4:02 ` [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction Tahera Fahimi
2024-08-02 16:47 ` Mickaël Salaün
2024-08-03 11:29 ` Mickaël Salaün
@ 2024-08-06 19:36 ` Jann Horn
2 siblings, 0 replies; 23+ messages in thread
From: Jann Horn @ 2024-08-06 19:36 UTC (permalink / raw)
To: Tahera Fahimi
Cc: outreachy, mic, gnoack, paul, jmorris, serge,
linux-security-module, linux-kernel, bjorn3_gh, netdev
On Fri, Aug 2, 2024 at 6:03 AM Tahera Fahimi <fahimitahera@gmail.com> wrote:
> This 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. It implements two hooks, unix_stream_connect
> and unix_may_send to enforce this restriction.
[...]
> +static bool check_unix_address_format(struct sock *const sock)
> +{
> + struct unix_address *addr = unix_sk(sock)->addr;
> +
> + if (!addr)
> + return true;
> +
> + if (addr->len > sizeof(AF_UNIX)) {
> + /* handling unspec sockets */
> + if (!addr->name[0].sun_path)
> + return true;
addr->name[0] is a "struct sockaddr_un", whose member "sun_path" is an
array member, not a pointer. If "addr" is a valid pointer,
"addr->name[0].sun_path" can't be NULL.
> + if (addr->name[0].sun_path[0] == '\0')
> + if (!sock_is_scoped(sock))
> + return false;
> + }
> +
> + return true;
> +}
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-06 19:35 ` Mickaël Salaün
@ 2024-08-06 20:46 ` Jann Horn
2024-08-07 7:21 ` Mickaël Salaün
0 siblings, 1 reply; 23+ messages in thread
From: Jann Horn @ 2024-08-06 20:46 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Tahera Fahimi, outreachy, gnoack, paul, jmorris, serge,
linux-security-module, linux-kernel, bjorn3_gh, netdev
On Tue, Aug 6, 2024 at 9:36 PM Mickaël Salaün <mic@digikod.net> wrote:
> On Sat, Aug 03, 2024 at 01:29:09PM +0200, Mickaël Salaün wrote:
> > On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> > > This 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. It implements two hooks, unix_stream_connect
> > > and unix_may_send to enforce this restriction.
[...]
> Here is a refactoring that is easier to read and avoid potential pointer
> misuse:
>
> static bool domain_is_scoped(const struct landlock_ruleset *const client,
> const struct landlock_ruleset *const server,
> access_mask_t scope)
> {
> int client_layer, server_layer;
> struct landlock_hierarchy *client_walker, *server_walker;
>
> if (WARN_ON_ONCE(!client))
> return false;
>
> client_layer = client->num_layers - 1;
> client_walker = client->hierarchy;
>
> /*
> * client_layer must be a signed integer with greater capacity than
> * client->num_layers to ensure the following loop stops.
> */
> BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
>
> if (!server) {
> /*
> * Walks client's parent domains and checks that none of these
> * domains are scoped.
> */
> for (; client_layer >= 0; client_layer--) {
> if (landlock_get_scope_mask(client, client_layer) &
> scope)
> return true;
> }
> return false;
> }
>
> server_layer = server->num_layers - 1;
> server_walker = server->hierarchy;
>
> /*
> * Walks client's parent domains down to the same hierarchy level as
> * the server's domain, and checks that none of these client's parent
> * domains are scoped.
> */
> for (; client_layer > server_layer; client_layer--) {
> if (landlock_get_scope_mask(client, client_layer) & scope)
> return true;
>
> client_walker = client_walker->parent;
> }
>
> /*
> * Walks server's parent domains down to the same hierarchy level as
> * the client's domain.
> */
> for (; server_layer > client_layer; server_layer--)
> server_walker = server_walker->parent;
>
> for (; client_layer >= 0; client_layer--) {
> if (landlock_get_scope_mask(client, client_layer) & scope) {
> /*
> * Client and server are at the same level in the
> * hierarchy. If the client is scoped, the request is
> * only allowed if this domain is also a server's
> * ancestor.
> */
> if (server_walker == client_walker)
> return false;
>
> return true;
> }
> client_walker = client_walker->parent;
> server_walker = server_walker->parent;
> }
> return false;
> }
I think adding something like this change on top of your code would
make it more concise (though this is entirely untested):
--- /tmp/a 2024-08-06 22:37:33.800158308 +0200
+++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
@@ -15,25 +15,12 @@
* client_layer must be a signed integer with greater capacity than
* client->num_layers to ensure the following loop stops.
*/
BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
- if (!server) {
- /*
- * Walks client's parent domains and checks that none of these
- * domains are scoped.
- */
- for (; client_layer >= 0; client_layer--) {
- if (landlock_get_scope_mask(client, client_layer) &
- scope)
- return true;
- }
- return false;
- }
-
- server_layer = server->num_layers - 1;
- server_walker = server->hierarchy;
+ server_layer = server ? (server->num_layers - 1) : -1;
+ server_walker = server ? server->hierarchy : NULL;
/*
* Walks client's parent domains down to the same hierarchy level as
* the server's domain, and checks that none of these client's parent
* domains are scoped.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-06 20:46 ` Jann Horn
@ 2024-08-07 7:21 ` Mickaël Salaün
2024-08-07 13:45 ` Jann Horn
0 siblings, 1 reply; 23+ messages in thread
From: Mickaël Salaün @ 2024-08-07 7:21 UTC (permalink / raw)
To: Jann Horn
Cc: Tahera Fahimi, outreachy, gnoack, paul, jmorris, serge,
linux-security-module, linux-kernel, bjorn3_gh, netdev
On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> On Tue, Aug 6, 2024 at 9:36 PM Mickaël Salaün <mic@digikod.net> wrote:
> > On Sat, Aug 03, 2024 at 01:29:09PM +0200, Mickaël Salaün wrote:
> > > On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> > > > This 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. It implements two hooks, unix_stream_connect
> > > > and unix_may_send to enforce this restriction.
> [...]
> > Here is a refactoring that is easier to read and avoid potential pointer
> > misuse:
> >
> > static bool domain_is_scoped(const struct landlock_ruleset *const client,
> > const struct landlock_ruleset *const server,
> > access_mask_t scope)
> > {
> > int client_layer, server_layer;
> > struct landlock_hierarchy *client_walker, *server_walker;
> >
> > if (WARN_ON_ONCE(!client))
> > return false;
> >
> > client_layer = client->num_layers - 1;
> > client_walker = client->hierarchy;
> >
> > /*
> > * client_layer must be a signed integer with greater capacity than
> > * client->num_layers to ensure the following loop stops.
> > */
> > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> >
> > if (!server) {
> > /*
> > * Walks client's parent domains and checks that none of these
> > * domains are scoped.
> > */
> > for (; client_layer >= 0; client_layer--) {
> > if (landlock_get_scope_mask(client, client_layer) &
> > scope)
> > return true;
> > }
> > return false;
> > }
> >
> > server_layer = server->num_layers - 1;
> > server_walker = server->hierarchy;
> >
> > /*
> > * Walks client's parent domains down to the same hierarchy level as
> > * the server's domain, and checks that none of these client's parent
> > * domains are scoped.
> > */
> > for (; client_layer > server_layer; client_layer--) {
> > if (landlock_get_scope_mask(client, client_layer) & scope)
> > return true;
> >
> > client_walker = client_walker->parent;
> > }
> >
> > /*
> > * Walks server's parent domains down to the same hierarchy level as
> > * the client's domain.
> > */
> > for (; server_layer > client_layer; server_layer--)
> > server_walker = server_walker->parent;
> >
> > for (; client_layer >= 0; client_layer--) {
> > if (landlock_get_scope_mask(client, client_layer) & scope) {
> > /*
> > * Client and server are at the same level in the
> > * hierarchy. If the client is scoped, the request is
> > * only allowed if this domain is also a server's
> > * ancestor.
> > */
We can move this comment before the if and...
> > if (server_walker == client_walker)
> > return false;
> >
> > return true;
...add this without the curly braces:
return server_walker != client_walker;
> > }
> > client_walker = client_walker->parent;
> > server_walker = server_walker->parent;
> > }
> > return false;
> > }
>
> I think adding something like this change on top of your code would
> make it more concise (though this is entirely untested):
>
> --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> @@ -15,25 +15,12 @@
> * client_layer must be a signed integer with greater capacity than
> * client->num_layers to ensure the following loop stops.
> */
> BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
>
> - if (!server) {
> - /*
> - * Walks client's parent domains and checks that none of these
> - * domains are scoped.
> - */
> - for (; client_layer >= 0; client_layer--) {
> - if (landlock_get_scope_mask(client, client_layer) &
> - scope)
> - return true;
> - }
> - return false;
> - }
This loop is redundant with the following one, but it makes sure there
is no issue nor inconsistencies with the server or server_walker
pointers. That's the only approach I found to make sure we don't go
through a path that could use an incorrect pointer, and makes the code
easy to review.
> -
> - server_layer = server->num_layers - 1;
> - server_walker = server->hierarchy;
> + server_layer = server ? (server->num_layers - 1) : -1;
> + server_walker = server ? server->hierarchy : NULL;
We would need to change the last loop to avoid a null pointer deref.
>
> /*
> * Walks client's parent domains down to the same hierarchy level as
> * the server's domain, and checks that none of these client's parent
> * domains are scoped.
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-07 7:21 ` Mickaël Salaün
@ 2024-08-07 13:45 ` Jann Horn
2024-08-07 14:44 ` Mickaël Salaün
0 siblings, 1 reply; 23+ messages in thread
From: Jann Horn @ 2024-08-07 13:45 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Tahera Fahimi, outreachy, gnoack, paul, jmorris, serge,
linux-security-module, linux-kernel, bjorn3_gh, netdev
On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > I think adding something like this change on top of your code would
> > make it more concise (though this is entirely untested):
> >
> > --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> > +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> > @@ -15,25 +15,12 @@
> > * client_layer must be a signed integer with greater capacity than
> > * client->num_layers to ensure the following loop stops.
> > */
> > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> >
> > - if (!server) {
> > - /*
> > - * Walks client's parent domains and checks that none of these
> > - * domains are scoped.
> > - */
> > - for (; client_layer >= 0; client_layer--) {
> > - if (landlock_get_scope_mask(client, client_layer) &
> > - scope)
> > - return true;
> > - }
> > - return false;
> > - }
>
> This loop is redundant with the following one, but it makes sure there
> is no issue nor inconsistencies with the server or server_walker
> pointers. That's the only approach I found to make sure we don't go
> through a path that could use an incorrect pointer, and makes the code
> easy to review.
My view is that this is a duplication of logic for one particular
special case - after all, you can also end up walking up to the same
state (client_layer==-1, server_layer==-1, client_walker==NULL,
server_walker==NULL) with the loop at the bottom.
But I guess my preference for more concise code is kinda subjective -
if you prefer the more verbose version, I'm fine with that too.
> > -
> > - server_layer = server->num_layers - 1;
> > - server_walker = server->hierarchy;
> > + server_layer = server ? (server->num_layers - 1) : -1;
> > + server_walker = server ? server->hierarchy : NULL;
>
> We would need to change the last loop to avoid a null pointer deref.
Why? The first loop would either exit or walk the client_walker up
until client_layer is -1 and client_walker is NULL; the second loop
wouldn't do anything because the walkers are at the same layer; the
third loop's body wouldn't be executed because client_layer is -1.
The case where the server is not in any Landlock domain is just one
subcase of the more general case "client and server do not have a
common ancestor domain".
> >
> > /*
> > * Walks client's parent domains down to the same hierarchy level as
> > * the server's domain, and checks that none of these client's parent
> > * domains are scoped.
> >
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-07 13:45 ` Jann Horn
@ 2024-08-07 14:44 ` Mickaël Salaün
2024-08-08 23:17 ` Tahera Fahimi
0 siblings, 1 reply; 23+ messages in thread
From: Mickaël Salaün @ 2024-08-07 14:44 UTC (permalink / raw)
To: Jann Horn
Cc: Tahera Fahimi, outreachy, gnoack, paul, jmorris, serge,
linux-security-module, linux-kernel, bjorn3_gh, netdev
On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > I think adding something like this change on top of your code would
> > > make it more concise (though this is entirely untested):
> > >
> > > --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> > > +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> > > @@ -15,25 +15,12 @@
> > > * client_layer must be a signed integer with greater capacity than
> > > * client->num_layers to ensure the following loop stops.
> > > */
> > > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > >
> > > - if (!server) {
> > > - /*
> > > - * Walks client's parent domains and checks that none of these
> > > - * domains are scoped.
> > > - */
> > > - for (; client_layer >= 0; client_layer--) {
> > > - if (landlock_get_scope_mask(client, client_layer) &
> > > - scope)
> > > - return true;
> > > - }
> > > - return false;
> > > - }
> >
> > This loop is redundant with the following one, but it makes sure there
> > is no issue nor inconsistencies with the server or server_walker
> > pointers. That's the only approach I found to make sure we don't go
> > through a path that could use an incorrect pointer, and makes the code
> > easy to review.
>
> My view is that this is a duplication of logic for one particular
> special case - after all, you can also end up walking up to the same
> state (client_layer==-1, server_layer==-1, client_walker==NULL,
> server_walker==NULL) with the loop at the bottom.
Indeed
>
> But I guess my preference for more concise code is kinda subjective -
> if you prefer the more verbose version, I'm fine with that too.
>
> > > -
> > > - server_layer = server->num_layers - 1;
> > > - server_walker = server->hierarchy;
> > > + server_layer = server ? (server->num_layers - 1) : -1;
> > > + server_walker = server ? server->hierarchy : NULL;
> >
> > We would need to change the last loop to avoid a null pointer deref.
>
> Why? The first loop would either exit or walk the client_walker up
> until client_layer is -1 and client_walker is NULL; the second loop
> wouldn't do anything because the walkers are at the same layer; the
> third loop's body wouldn't be executed because client_layer is -1.
Correct, I missed that client_layer would always be greater than
server_layer (-1).
Tahera, could you please take Jann's proposal?
>
> The case where the server is not in any Landlock domain is just one
> subcase of the more general case "client and server do not have a
> common ancestor domain".
>
> > >
> > > /*
> > > * Walks client's parent domains down to the same hierarchy level as
> > > * the server's domain, and checks that none of these client's parent
> > > * domains are scoped.
> > >
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 2/4] selftests/landlock: Abstract unix socket restriction tests
2024-08-02 4:02 ` [PATCH v8 2/4] selftests/landlock: Abstract unix socket restriction tests Tahera Fahimi
@ 2024-08-07 15:08 ` Mickaël Salaün
0 siblings, 0 replies; 23+ messages in thread
From: Mickaël Salaün @ 2024-08-07 15:08 UTC (permalink / raw)
To: Tahera Fahimi
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev
On Thu, Aug 01, 2024 at 10:02:34PM -0600, Tahera Fahimi wrote:
> The patch introduces Landlock ABI version 6 and has four types of tests:
> 1) unix_socket: base tests of the abstract socket scoping mechanism for a
> landlocked process, same as the ptrace test.
> 2) optional_scoping: generates three processes with different domains and
> tests if a process with a non-scoped domain can connect to other
> processes.
> 3) unix_sock_special_cases: since the socket's creator credentials are used
> for scoping sockets, this test examines the cases where the socket's
> credentials are different from the process using it.
> 4) pathname_address_sockets: ensures that Unix sockets bound to a
> null-terminated filesystem can still connect to a socket outside of
> their scoped domain. This means that even if the domain is scoped with
> LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, the socket can connect to a socket
> outside the scoped domain.
>
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> ---
> Changes in versions:
> V8:
> - Move tests to scoped_abstract_unix_test.c file.
> - To avoid potential conflicts among Unix socket names in different tests,
> set_unix_address is added to common.h to set different sun_path for Unix sockets.
> - protocol_variant and service_fixture structures are also moved to common.h
> - Adding pathname_address_sockets to cover all types of address formats
> for unix sockets, and moving remove_path() to common.h to reuse in this test.
> V7:
> - Introducing landlock ABI version 6.
> - Adding some edge test cases to optional_scoping test.
> - Using `enum` for different domains in optional_scoping tests.
> - Extend unix_sock_special_cases test cases for connected(SOCK_STREAM) sockets.
> - Modifying inline comments.
> V6:
> - Introducing optional_scoping test which ensures a sandboxed process with a
> non-scoped domain can still connect to another abstract unix socket(either
> sandboxed or non-sandboxed).
> - Introducing unix_sock_special_cases test which tests examines scenarios where
> the connecting sockets have different domain than the process using them.
> V4:
> - Introducing unix_socket to evaluate the basic scoping mechanism for abstract
> unix sockets.
> ---
> tools/testing/selftests/landlock/base_test.c | 2 +-
> tools/testing/selftests/landlock/common.h | 72 ++
> tools/testing/selftests/landlock/fs_test.c | 34 -
> tools/testing/selftests/landlock/net_test.c | 31 +-
> .../landlock/scoped_abstract_unix_test.c | 1136 +++++++++++++++++
> 5 files changed, 1210 insertions(+), 65 deletions(-)
> create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index 3c1e9f35b531..52b00472a487 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -75,7 +75,7 @@ TEST(abi_version)
> const struct landlock_ruleset_attr ruleset_attr = {
> .handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
> };
> - ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
> + ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
> LANDLOCK_CREATE_RULESET_VERSION));
>
> ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
> index 7e2b431b9f90..a0c8126d94b4 100644
> --- a/tools/testing/selftests/landlock/common.h
> +++ b/tools/testing/selftests/landlock/common.h
> @@ -16,8 +16,11 @@
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <unistd.h>
> +#include <sys/un.h>
> +#include <arpa/inet.h>
>
> #include "../kselftest_harness.h"
> +#define TMP_DIR "tmp"
>
> #ifndef __maybe_unused
> #define __maybe_unused __attribute__((__unused__))
> @@ -226,3 +229,72 @@ enforce_ruleset(struct __test_metadata *const _metadata, const int ruleset_fd)
> TH_LOG("Failed to enforce ruleset: %s", strerror(errno));
> }
> }
> +
> +struct protocol_variant {
> + int domain;
> + int type;
> +};
> +
> +struct service_fixture {
> + struct protocol_variant protocol;
> + /* port is also stored in ipv4_addr.sin_port or ipv6_addr.sin6_port */
> + unsigned short port;
> + union {
> + struct sockaddr_in ipv4_addr;
> + struct sockaddr_in6 ipv6_addr;
> + struct {
> + struct sockaddr_un unix_addr;
> + socklen_t unix_addr_len;
> + };
> + };
> +};
> +
> +static pid_t __maybe_unused sys_gettid(void)
> +{
> + return syscall(__NR_gettid);
> +}
> +
> +static void __maybe_unused set_unix_address(struct service_fixture *const srv,
> + const unsigned short index)
> +{
> + srv->unix_addr.sun_family = AF_UNIX;
> + sprintf(srv->unix_addr.sun_path,
> + "_selftests-landlock-abstract-unix-tid%d-index%d", sys_gettid(),
> + index);
> + srv->unix_addr_len = SUN_LEN(&srv->unix_addr);
> + srv->unix_addr.sun_path[0] = '\0';
> +}
> +
> +static int __maybe_unused remove_path(const char *const path)
We don't need to import remove_path() for these simple filesystem
actions.
> +{
> + char *walker;
> + int i, ret, err = 0;
> +
> + walker = strdup(path);
> + if (!walker) {
> + err = ENOMEM;
> + goto out;
> + }
> + if (unlink(path) && rmdir(path)) {
> + if (errno != ENOENT && errno != ENOTDIR)
> + err = errno;
> + goto out;
> + }
> + for (i = strlen(walker); i > 0; i--) {
> + if (walker[i] != '/')
> + continue;
> + walker[i] = '\0';
> + ret = rmdir(walker);
> + if (ret) {
> + if (errno != ENOTEMPTY && errno != EBUSY)
> + err = errno;
> + goto out;
> + }
> + if (strcmp(walker, TMP_DIR) == 0)
> + goto out;
> + }
> +
> +out:
> + free(walker);
> + return err;
> +}
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 7d063c652be1..fc74cab6dfb8 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -221,40 +221,6 @@ static void create_file(struct __test_metadata *const _metadata,
> }
> }
>
> -static int remove_path(const char *const path)
> -{
> - char *walker;
> - int i, ret, err = 0;
> -
> - walker = strdup(path);
> - if (!walker) {
> - err = ENOMEM;
> - goto out;
> - }
> - if (unlink(path) && rmdir(path)) {
> - if (errno != ENOENT && errno != ENOTDIR)
> - err = errno;
> - goto out;
> - }
> - for (i = strlen(walker); i > 0; i--) {
> - if (walker[i] != '/')
> - continue;
> - walker[i] = '\0';
> - ret = rmdir(walker);
> - if (ret) {
> - if (errno != ENOTEMPTY && errno != EBUSY)
> - err = errno;
> - goto out;
> - }
> - if (strcmp(walker, TMP_DIR) == 0)
> - goto out;
> - }
> -
> -out:
> - free(walker);
> - return err;
> -}
> -
> struct mnt_opt {
> const char *const source;
> const char *const type;
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index f21cfbbc3638..4e0aeb53b225 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -36,30 +36,6 @@ enum sandbox_type {
> TCP_SANDBOX,
> };
>
> -struct protocol_variant {
> - int domain;
> - int type;
> -};
> -
> -struct service_fixture {
> - struct protocol_variant protocol;
> - /* port is also stored in ipv4_addr.sin_port or ipv6_addr.sin6_port */
> - unsigned short port;
> - union {
> - struct sockaddr_in ipv4_addr;
> - struct sockaddr_in6 ipv6_addr;
> - struct {
> - struct sockaddr_un unix_addr;
> - socklen_t unix_addr_len;
> - };
> - };
> -};
> -
> -static pid_t sys_gettid(void)
> -{
> - return syscall(__NR_gettid);
> -}
> -
> static int set_service(struct service_fixture *const srv,
> const struct protocol_variant prot,
> const unsigned short index)
> @@ -92,12 +68,7 @@ static int set_service(struct service_fixture *const srv,
> return 0;
>
> case AF_UNIX:
> - srv->unix_addr.sun_family = prot.domain;
> - sprintf(srv->unix_addr.sun_path,
> - "_selftests-landlock-net-tid%d-index%d", sys_gettid(),
> - index);
> - srv->unix_addr_len = SUN_LEN(&srv->unix_addr);
> - srv->unix_addr.sun_path[0] = '\0';
> + set_unix_address(srv, index);
Please create a standalone patch for existing code refactoring, and then
another patch for the new tests.
> return 0;
> }
> return 1;
> diff --git a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
> new file mode 100644
> index 000000000000..ffa1f01dbbce
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
> +TEST_F(unix_socket, abstract_unix_socket)
> +{
> + int status;
> + pid_t child;
> + bool can_connect_to_parent, can_connect_to_child;
> + int err, err_dgram;
> + int pipe_child[2], pipe_parent[2];
> + char buf_parent;
> +
> + /*
> + * can_connect_to_child is true if a parent process can connect to its
> + * child process. The parent process is not isolated from the child
> + * with a dedicated Landlock domain.
> + */
> + can_connect_to_child = !variant->domain_parent;
> + /*
> + * can_connect_to_parent is true if a child process can connect to its
> + * parent process. This depends on the child process is not isolated from
> + * the parent with a dedicated Landlock domain.
> + */
> + can_connect_to_parent = !variant->domain_child;
> +
> + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
> + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
> + if (variant->domain_both) {
> + create_unix_domain(_metadata);
> + if (!__test_passed(_metadata))
> + return;
> + }
> +
> + child = fork();
> + ASSERT_LE(0, child);
> + if (child == 0) {
> + char buf_child;
> +
> + ASSERT_EQ(0, close(pipe_parent[1]));
> + ASSERT_EQ(0, close(pipe_child[0]));
> + if (variant->domain_child)
> + create_unix_domain(_metadata);
> +
> + /* Waits for the parent to be in a domain, if any. */
> + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> +
> + if (variant->connect_to_parent) {
> + self->client = socket(AF_UNIX, SOCK_STREAM, 0);
> + self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
> +
> + ASSERT_NE(-1, self->client);
> + ASSERT_NE(-1, self->dgram_client);
> + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> +
> + err = connect(self->client,
> + &self->stream_address.unix_addr,
> + (self->stream_address).unix_addr_len);
> + err_dgram =
> + connect(self->dgram_client,
> + &self->dgram_address.unix_addr,
> + (self->dgram_address).unix_addr_len);
> +
> + if (can_connect_to_parent) {
> + EXPECT_EQ(0, err);
> + EXPECT_EQ(0, err_dgram);
> + } else {
> + EXPECT_EQ(-1, err);
> + EXPECT_EQ(-1, err_dgram);
> + EXPECT_EQ(EPERM, errno);
> + }
> + } else {
> + self->server = socket(AF_UNIX, SOCK_STREAM, 0);
> + self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
> + ASSERT_NE(-1, self->server);
> + ASSERT_NE(-1, self->dgram_server);
> +
> + ASSERT_EQ(0,
> + bind(self->server,
> + &self->stream_address.unix_addr,
> + (self->stream_address).unix_addr_len));
> + ASSERT_EQ(0, bind(self->dgram_server,
> + &self->dgram_address.unix_addr,
> + (self->dgram_address).unix_addr_len));
> + ASSERT_EQ(0, listen(self->server, 32));
Please, avoid the use of hardcoded values (e.g. in all listen calls).
You can create a global backlog variable like in net_test.c (just copy
it, no need to include it in common.h).
> +
> + /* signal to parent that child is listening */
> + ASSERT_EQ(1, write(pipe_child[1], ".", 1));
> + /* wait to connect */
> + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> + }
> + _exit(_metadata->exit_code);
> + return;
> + }
> +
> + ASSERT_EQ(0, close(pipe_child[1]));
> + ASSERT_EQ(0, close(pipe_parent[0]));
> +
> + if (variant->domain_parent)
> + create_unix_domain(_metadata);
> +
> + /* Signals that the parent is in a domain, if any. */
> + ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> +
> + if (!variant->connect_to_parent) {
> + self->client = socket(AF_UNIX, SOCK_STREAM, 0);
> + self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
> +
> + ASSERT_NE(-1, self->client);
> + ASSERT_NE(-1, self->dgram_client);
> +
> + /* Waits for the child to listen */
> + ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1));
> + err = connect(self->client, &self->stream_address.unix_addr,
> + (self->stream_address).unix_addr_len);
> + err_dgram = connect(self->dgram_client,
> + &self->dgram_address.unix_addr,
> + (self->dgram_address).unix_addr_len);
> +
> + if (can_connect_to_child) {
> + EXPECT_EQ(0, err);
> + EXPECT_EQ(0, err_dgram);
> + } else {
> + EXPECT_EQ(-1, err);
> + EXPECT_EQ(-1, err_dgram);
> + EXPECT_EQ(EPERM, errno);
> + }
> + ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> + } else {
> + self->server = socket(AF_UNIX, SOCK_STREAM, 0);
> + self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
> + ASSERT_NE(-1, self->server);
> + ASSERT_NE(-1, self->dgram_server);
> + ASSERT_EQ(0, bind(self->server, &self->stream_address.unix_addr,
> + (self->stream_address).unix_addr_len));
> + ASSERT_EQ(0, bind(self->dgram_server,
> + &self->dgram_address.unix_addr,
> + (self->dgram_address).unix_addr_len));
> + ASSERT_EQ(0, listen(self->server, 32));
> +
> + /* signal to child that parent is listening */
> + ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> + }
> +
> + ASSERT_EQ(child, waitpid(child, &status, 0));
> +
> + if (WIFSIGNALED(status) || !WIFEXITED(status) ||
> + WEXITSTATUS(status) != EXIT_SUCCESS)
> + _metadata->exit_code = KSFT_FAIL;
> +}
> +
> +enum sandbox_type {
> + NO_SANDBOX,
> + SCOPE_SANDBOX,
> + /* Any other type of sandboxing domain */
> + OTHER_SANDBOX,
> +};
> +
> +/* clang-format off */
> +FIXTURE(optional_scoping)
> +{
> + struct service_fixture parent_address, child_address;
> + int parent_server, child_server, client;
The variables should be local ones.
> +};
> +
> +/* clang-format on */
> +FIXTURE_VARIANT(optional_scoping)
> +{
> + const int domain_all;
> + const int domain_parent;
> + const int domain_children;
> + const int domain_child;
> + const int domain_grand_child;
domain_grand_child is always NO_SANDBOX in the variants.
> + const int type;
> +};
> +
> +FIXTURE_SETUP(optional_scoping)
s/optional_scoping/scoping/ ?
> +{
> + memset(&self->parent_address, 0, sizeof(self->parent_address));
> + memset(&self->child_address, 0, sizeof(self->child_address));
> +
> + set_unix_address(&self->parent_address, 0);
> + set_unix_address(&self->child_address, 1);
> +}
> +TEST_F(optional_scoping, unix_scoping)
> +{
> + pid_t child;
> + int status;
> + bool can_connect_to_parent, can_connect_to_child;
> + int pipe_parent[2];
> +
> + if (variant->domain_grand_child == SCOPE_SANDBOX)
> + can_connect_to_child = false;
We never go through this case.
> + else
> + can_connect_to_child = true;
> +
> + if (!can_connect_to_child || variant->domain_children == SCOPE_SANDBOX)
> + can_connect_to_parent = false;
> + else
> + can_connect_to_parent = true;
We never go through this case.
> +
> + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
> +
> + if (variant->domain_all == OTHER_SANDBOX)
> + create_fs_domain(_metadata);
> + else if (variant->domain_all == SCOPE_SANDBOX)
> + create_unix_domain(_metadata);
> +
> + child = fork();
> + ASSERT_LE(0, child);
> + if (child == 0) {
> + int pipe_child[2];
> +
> + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
> + pid_t grand_child;
> +
> + if (variant->domain_children == OTHER_SANDBOX)
> + create_fs_domain(_metadata);
> + else if (variant->domain_children == SCOPE_SANDBOX)
> + create_unix_domain(_metadata);
> +
> + grand_child = fork();
> + ASSERT_LE(0, grand_child);
> + if (grand_child == 0) {
> + ASSERT_EQ(0, close(pipe_parent[1]));
> + ASSERT_EQ(0, close(pipe_child[1]));
> +
> + char buf1, buf2;
> + int err;
> +
> + if (variant->domain_grand_child == OTHER_SANDBOX)
> + create_fs_domain(_metadata);
> + else if (variant->domain_grand_child == SCOPE_SANDBOX)
> + create_unix_domain(_metadata);
> +
> + self->client = socket(AF_UNIX, variant->type, 0);
> + ASSERT_NE(-1, self->client);
> +
> + ASSERT_EQ(1, read(pipe_child[0], &buf2, 1));
> + err = connect(self->client,
> + &self->child_address.unix_addr,
> + (self->child_address).unix_addr_len);
No need for these extra parenthesis.
> + if (can_connect_to_child) {
> + EXPECT_EQ(0, err);
> + } else {
> + EXPECT_EQ(-1, err);
> + EXPECT_EQ(EPERM, errno);
> + }
> +
> + if (variant->type == SOCK_STREAM) {
> + EXPECT_EQ(0, close(self->client));
> + self->client =
> + socket(AF_UNIX, variant->type, 0);
> + ASSERT_NE(-1, self->client);
> + }
> +
> + ASSERT_EQ(1, read(pipe_parent[0], &buf1, 1));
> + err = connect(self->client,
> + &self->parent_address.unix_addr,
> + (self->parent_address).unix_addr_len);
> + if (can_connect_to_parent) {
> + EXPECT_EQ(0, err);
> + } else {
> + EXPECT_EQ(-1, err);
> + EXPECT_EQ(EPERM, errno);
> + }
> + EXPECT_EQ(0, close(self->client));
> +
> + _exit(_metadata->exit_code);
> + return;
> + }
> +
> + ASSERT_EQ(0, close(pipe_child[0]));
> + if (variant->domain_child == OTHER_SANDBOX)
> + create_fs_domain(_metadata);
> + else if (variant->domain_child == SCOPE_SANDBOX)
> + create_unix_domain(_metadata);
> +
> + self->child_server = socket(AF_UNIX, variant->type, 0);
> + ASSERT_NE(-1, self->child_server);
> + ASSERT_EQ(0, bind(self->child_server,
> + &self->child_address.unix_addr,
> + (self->child_address).unix_addr_len));
> + if (variant->type == SOCK_STREAM)
> + ASSERT_EQ(0, listen(self->child_server, 32));
> +
> + ASSERT_EQ(1, write(pipe_child[1], ".", 1));
> + ASSERT_EQ(grand_child, waitpid(grand_child, &status, 0));
> + return;
> + }
> + ASSERT_EQ(0, close(pipe_parent[0]));
> +
> + if (variant->domain_parent == OTHER_SANDBOX)
> + create_fs_domain(_metadata);
> + else if (variant->domain_parent == SCOPE_SANDBOX)
> + create_unix_domain(_metadata);
> +
> + self->parent_server = socket(AF_UNIX, variant->type, 0);
> + ASSERT_NE(-1, self->parent_server);
> + ASSERT_EQ(0, bind(self->parent_server, &self->parent_address.unix_addr,
> + (self->parent_address).unix_addr_len));
> +
> + if (variant->type == SOCK_STREAM)
> + ASSERT_EQ(0, listen(self->parent_server, 32));
> +
> + ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> + ASSERT_EQ(child, waitpid(child, &status, 0));
> + if (WIFSIGNALED(status) || !WIFEXITED(status) ||
> + WEXITSTATUS(status) != EXIT_SUCCESS)
> + _metadata->exit_code = KSFT_FAIL;
> +}
> +FIXTURE_SETUP(pathname_address_sockets)
> +{
> + /* setup abstract addresses */
> + memset(&self->stream_address, 0, sizeof(self->stream_address));
> + set_unix_address(&self->stream_address, 0);
> +
> + memset(&self->dgram_address, 0, sizeof(self->dgram_address));
> + set_unix_address(&self->dgram_address, 0);
> +
> + const char *s_path = variant->stream_path;
> + const char *dg_path = variant->dgram_path;
> +
> + disable_caps(_metadata);
> + umask(0077);
> + ASSERT_EQ(0, mkdir(TMP_DIR, 0700));
> +
> + ASSERT_EQ(0, mknod(s_path, S_IFREG | 0700, 0))
> + {
> + TH_LOG("Failed to create file \"%s\": %s", s_path,
> + strerror(errno));
> + remove_path(TMP_DIR);
Just use rmdir() instead of remove_path() everywhere.
> + }
> + ASSERT_EQ(0, mknod(dg_path, S_IFREG | 0700, 0))
> + {
> + TH_LOG("Failed to create file \"%s\": %s", dg_path,
> + strerror(errno));
> + remove_path(TMP_DIR);
> + }
> +}
> +
> +FIXTURE_TEARDOWN(pathname_address_sockets)
> +{
> + const char *s_path = variant->stream_path;
> + const char *dg_path = variant->dgram_path;
> +
> + ASSERT_EQ(0, remove_path(dg_path));
Just use unlink() and rmdir().
> + ASSERT_EQ(0, remove_path(s_path));
> + ASSERT_EQ(0, remove_path(TMP_DIR));
> +}
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 4/4] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning
2024-08-02 4:02 ` [PATCH v8 4/4] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning Tahera Fahimi
@ 2024-08-07 15:14 ` Mickaël Salaün
0 siblings, 0 replies; 23+ messages in thread
From: Mickaël Salaün @ 2024-08-07 15:14 UTC (permalink / raw)
To: Tahera Fahimi
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev
On Thu, Aug 01, 2024 at 10:02:36PM -0600, Tahera Fahimi wrote:
> Introducing LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET as an IPC scoping
> mechanism in Landlock ABI version 6, and updating ruleset_attr,
> Landlock ABI version, and access rights code blocks based on that.
>
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> ---
> v8:
> - Improving documentation by specifying differences between scoped and
> non-scoped domains.
> - Adding review notes of version 7.
> - Update date
> v7:
> - Add "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" explanation to IPC scoping
> section and updating ABI to version 6.
> - Adding "scoped" attribute to the Access rights section.
> - In current limitation, unnamed sockets are specified as sockets that
> are not restricted.
> - Update date
> ---
> Documentation/userspace-api/landlock.rst | 33 ++++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 07b63aec56fa..d602567b5139 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -8,7 +8,7 @@ Landlock: unprivileged access control
> =====================================
>
> :Author: Mickaël Salaün
> -:Date: April 2024
> +:Date: August 2024
>
> The goal of Landlock is to enable to restrict ambient rights (e.g. global
> filesystem or network access) for a set of processes. Because Landlock
> @@ -81,6 +81,8 @@ to be explicit about the denied-by-default access rights.
> .handled_access_net =
> LANDLOCK_ACCESS_NET_BIND_TCP |
> LANDLOCK_ACCESS_NET_CONNECT_TCP,
> + .scoped =
> + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
> };
>
> Because we may not know on which kernel version an application will be
> @@ -119,6 +121,9 @@ version, and only use the available subset of access rights:
> case 4:
> /* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
> ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + case 5:
> + /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
> + ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> }
>
> This enables to create an inclusive ruleset that will contain our rules.
> @@ -306,6 +311,23 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
> process, a sandboxed process should have a subset of the target process rules,
> which means the tracee must be in a sub-domain of the tracer.
>
> +IPC Scoping
> +-----------
> +
> +Similar to the implicit `Ptrace restrictions`_, we may want to further restrict
> +interactions between sandboxes. Each Landlock domain can be explicitly scoped
> +for a set of actions by specifying it on a ruleset. For example, if a sandboxed
> +process should not be able to :manpage:`connect(2)` to a non-sandboxed process
> +through abstract :manpage:`unix(7)` sockets, we can specify such restriction
> +with ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``.
> +
> +A sandboxed process can connect to a non-sandboxed process when its domain is
> +not scoped. If a process's domain is scoped, it can only connect to processes in
...it can only connect to sockets created by proccesses in the same
scoped domain.
> +the same scoped domain.
> +
> +IPC scoping does not support Landlock rules, so if a domain is scoped, no rules
> +can be added to allow accessing to a resource outside of the scoped domain.
> +
> Truncating files
> ----------------
>
> @@ -404,7 +426,7 @@ Access rights
> -------------
>
> .. kernel-doc:: include/uapi/linux/landlock.h
> - :identifiers: fs_access net_access
> + :identifiers: fs_access net_access scope
>
> Creating a new ruleset
> ----------------------
> @@ -541,6 +563,13 @@ earlier ABI.
> Starting with the Landlock ABI version 5, it is possible to restrict the use of
> :manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right.
>
> +Abstract Unix sockets Restriction (ABI < 6)
Let's follow the capitalization used by man pages: "UNIX" instead of
"Unix".
> +--------------------------------------------
> +
> +With ABI version 6, it is possible to restrict connection to an abstract Unix socket
> +through ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``, thanks to the ``scoped`` ruleset
> +attribute.
> +
> .. _kernel_support:
>
> Kernel support
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-03 11:29 ` Mickaël Salaün
2024-08-06 19:35 ` Mickaël Salaün
@ 2024-08-07 15:37 ` Tahera Fahimi
2024-08-09 14:13 ` Mickaël Salaün
1 sibling, 1 reply; 23+ messages in thread
From: Tahera Fahimi @ 2024-08-07 15:37 UTC (permalink / raw)
To: Mickaël Salaün
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev
On Sat, Aug 03, 2024 at 01:29:04PM +0200, Mickaël Salaün wrote:
> On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> > This 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. It implements two hooks, unix_stream_connect
> > and unix_may_send to enforce this restriction.
> >
> > Closes: https://github.com/landlock-lsm/linux/issues/7
> > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> >
> > ---
> > v8:
> > - Code refactoring (improve code readability, renaming variable, etc.) based
> > on reviews by Mickaël Salaün on version 7.
> > - Adding warn_on_once to check (impossible) inconsistencies.
> > - Adding inline comments.
> > - Adding check_unix_address_format to check if the scoping socket is an abstract
> > unix sockets.
> > v7:
> > - 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/
> > 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 socket's file 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/all/20240610.Aifee5ingugh@digikod.net/
> > ----
> > ---
> > include/uapi/linux/landlock.h | 30 +++++++
> > security/landlock/limits.h | 3 +
> > security/landlock/ruleset.c | 7 +-
> > security/landlock/ruleset.h | 23 ++++-
> > security/landlock/syscalls.c | 14 ++-
> > security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
> > 6 files changed, 225 insertions(+), 7 deletions(-)
>
> > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > index 849f5123610b..7e8579ebae83 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,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> > return task_ptrace(parent, current);
> > }
> >
> > +static bool walk_and_check(const struct landlock_ruleset *const child,
> > + struct landlock_hierarchy **walker,
> > + size_t base_layer, size_t deep_layer,
> > + access_mask_t check_scoping)
>
> s/check_scoping/scope/
>
> > +{
> > + if (!child || base_layer < 0 || !(*walker))
>
> I guess it should be:
> WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
>
> > + return false;
> > +
> > + for (deep_layer; base_layer < deep_layer; deep_layer--) {
>
> No need to pass deep_layer as argument:
> deep_layer = child->num_layers - 1
>
> > + if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> > + return false;
> > + *walker = (*walker)->parent;
> > + if (WARN_ON_ONCE(!*walker))
> > + /* there is an inconsistency between num_layers
>
> Please use full sentences starting with a capital letter and ending with
> a dot, and in this case start with "/*"
>
> > + * and landlock_hierarchy in the ruleset
> > + */
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > +/**
> > + * domain_IPC_scope - Checks if the client domain is scoped in the same
> > + * domain as the server.
>
> Actually, you can remove IPC from the function name.
>
> > + *
> > + * @client: IPC sender domain.
> > + * @server: IPC receiver domain.
> > + *
> > + * Check if the @client domain is scoped to access the @server; the @server
> > + * must be scoped in the same domain.
>
> Returns true if...
>
> > + */
> > +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> > + const struct landlock_ruleset *const server,
> > + access_mask_t ipc_type)
> > +{
> > + size_t client_layer, server_layer = 0;
> > + int base_layer;
> > + struct landlock_hierarchy *client_walker, *server_walker;
> > + bool is_scoped;
> > +
> > + /* Quick return if client has no domain */
> > + if (!client)
> > + return true;
> > +
> > + client_layer = client->num_layers - 1;
> > + client_walker = client->hierarchy;
> > + if (server) {
> > + server_layer = server->num_layers - 1;
> > + server_walker = server->hierarchy;
> > + }
>
> } else {
> server_layer = 0;
> server_walker = NULL;
> }
>
> > + base_layer = (client_layer > server_layer) ? server_layer :
> > + client_layer;
> > +
> > + /* For client domain, walk_and_check ensures the client domain is
> > + * not scoped until gets to base_layer.
>
> until gets?
>
> > + * For server_domain, it only ensures that the server domain exist.
> > + */
> > + if (client_layer != server_layer) {
>
> bool is_scoped;
It is defined above.
> > + if (client_layer > server_layer)
> > + is_scoped = walk_and_check(client, &client_walker,
> > + server_layer, client_layer,
> > + ipc_type);
> > + else
>
> server_walker may be uninitialized and still read here, and maybe later
> in the for loop. The whole code should maks sure this cannot happen,
> and a test case should check this.
I think this case never happens, since the server_walker can be read
here if there are more than one layers in server domain which means that
the server_walker is not unintialized.
> > + is_scoped = walk_and_check(server, &server_walker,
> > + client_layer, server_layer,
> > + ipc_type & 0);
>
> "ipc_type & 0" is the same as "0"
>
> > + if (!is_scoped)
>
> The name doesn't reflect the semantic. walk_and_check() should return
> the inverse.
>
> > + return false;
> > + }
>
> This code would be simpler:
>
> if (client_layer > server_layer) {
> base_layer = server_layer;
> // TODO: inverse boolean logic
> if (!walk_and_check(client, &client_walker,
> base_layer, ipc_type))
> return false;
> } else (client_layer < server_layer) {
> base_layer = client_layer;
> // TODO: inverse boolean logic
> if (!walk_and_check(server, &server_walker,
> base_layer, 0))
> return false;
> } else {
> base_layer = client_layer;
> }
>
>
> I think we can improve more to make sure there is no path/risk of
> inconsistent pointers.
>
>
> > + /* client and server are at the same level in hierarchy. If client is
> > + * scoped, the server must be scoped in the same domain
> > + */
> > + for (base_layer; base_layer >= 0; base_layer--) {
> > + if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
>
> With each multi-line comment, the first line should be empty:
> /*
> * This check must be here since access would be denied only if
>
> > + /* This check must be here since access would be denied only if
> > + * the client is scoped and the server has no domain, so
> > + * if the client has a domain but is not scoped and the server
> > + * has no domain, access is guaranteed.
> > + */
> > + if (!server)
> > + return false;
> > +
> > + if (server_walker == client_walker)
> > + return true;
> > +
> > + return false;
> > + }
> > + client_walker = client_walker->parent;
> > + server_walker = server_walker->parent;
> > + /* Warn if there is an incosistenncy between num_layers and
>
> Makes sure there is no inconsistency between num_layers and
>
>
> > + * landlock_hierarchy in each of rulesets
> > + */
> > + if (WARN_ON_ONCE(base_layer > 0 &&
> > + (!server_walker || !client_walker)))
> > + return false;
> > + }
> > + return true;
> > +}
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-07 14:44 ` Mickaël Salaün
@ 2024-08-08 23:17 ` Tahera Fahimi
2024-08-09 8:49 ` Mickaël Salaün
0 siblings, 1 reply; 23+ messages in thread
From: Tahera Fahimi @ 2024-08-08 23:17 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Jann Horn, outreachy, gnoack, paul, jmorris, serge,
linux-security-module, linux-kernel, bjorn3_gh, netdev
On Wed, Aug 07, 2024 at 04:44:36PM +0200, Mickaël Salaün wrote:
> On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> > On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > > I think adding something like this change on top of your code would
> > > > make it more concise (though this is entirely untested):
> > > >
> > > > --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> > > > +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> > > > @@ -15,25 +15,12 @@
> > > > * client_layer must be a signed integer with greater capacity than
> > > > * client->num_layers to ensure the following loop stops.
> > > > */
> > > > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > > >
> > > > - if (!server) {
> > > > - /*
> > > > - * Walks client's parent domains and checks that none of these
> > > > - * domains are scoped.
> > > > - */
> > > > - for (; client_layer >= 0; client_layer--) {
> > > > - if (landlock_get_scope_mask(client, client_layer) &
> > > > - scope)
> > > > - return true;
> > > > - }
> > > > - return false;
> > > > - }
> > >
> > > This loop is redundant with the following one, but it makes sure there
> > > is no issue nor inconsistencies with the server or server_walker
> > > pointers. That's the only approach I found to make sure we don't go
> > > through a path that could use an incorrect pointer, and makes the code
> > > easy to review.
> >
> > My view is that this is a duplication of logic for one particular
> > special case - after all, you can also end up walking up to the same
> > state (client_layer==-1, server_layer==-1, client_walker==NULL,
> > server_walker==NULL) with the loop at the bottom.
>
> Indeed
>
> >
> > But I guess my preference for more concise code is kinda subjective -
> > if you prefer the more verbose version, I'm fine with that too.
> >
> > > > -
> > > > - server_layer = server->num_layers - 1;
> > > > - server_walker = server->hierarchy;
> > > > + server_layer = server ? (server->num_layers - 1) : -1;
> > > > + server_walker = server ? server->hierarchy : NULL;
> > >
> > > We would need to change the last loop to avoid a null pointer deref.
> >
> > Why? The first loop would either exit or walk the client_walker up
> > until client_layer is -1 and client_walker is NULL; the second loop
> > wouldn't do anything because the walkers are at the same layer; the
> > third loop's body wouldn't be executed because client_layer is -1.
>
> Correct, I missed that client_layer would always be greater than
> server_layer (-1).
>
> Tahera, could you please take Jann's proposal?
Done.
We will have duplicate logic, but it would be easier to read and review.
>
> >
> > The case where the server is not in any Landlock domain is just one
> > subcase of the more general case "client and server do not have a
> > common ancestor domain".
> >
> > > >
> > > > /*
> > > > * Walks client's parent domains down to the same hierarchy level as
> > > > * the server's domain, and checks that none of these client's parent
> > > > * domains are scoped.
> > > >
> >
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-08 23:17 ` Tahera Fahimi
@ 2024-08-09 8:49 ` Mickaël Salaün
2024-08-09 17:54 ` Tahera Fahimi
0 siblings, 1 reply; 23+ messages in thread
From: Mickaël Salaün @ 2024-08-09 8:49 UTC (permalink / raw)
To: Tahera Fahimi
Cc: Jann Horn, outreachy, gnoack, paul, jmorris, serge,
linux-security-module, linux-kernel, bjorn3_gh, netdev
On Thu, Aug 08, 2024 at 05:17:10PM -0600, Tahera Fahimi wrote:
> On Wed, Aug 07, 2024 at 04:44:36PM +0200, Mickaël Salaün wrote:
> > On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> > > On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > > > I think adding something like this change on top of your code would
> > > > > make it more concise (though this is entirely untested):
> > > > >
> > > > > --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> > > > > +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> > > > > @@ -15,25 +15,12 @@
> > > > > * client_layer must be a signed integer with greater capacity than
> > > > > * client->num_layers to ensure the following loop stops.
> > > > > */
> > > > > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > > > >
> > > > > - if (!server) {
> > > > > - /*
> > > > > - * Walks client's parent domains and checks that none of these
> > > > > - * domains are scoped.
> > > > > - */
> > > > > - for (; client_layer >= 0; client_layer--) {
> > > > > - if (landlock_get_scope_mask(client, client_layer) &
> > > > > - scope)
> > > > > - return true;
> > > > > - }
> > > > > - return false;
> > > > > - }
> > > >
> > > > This loop is redundant with the following one, but it makes sure there
> > > > is no issue nor inconsistencies with the server or server_walker
> > > > pointers. That's the only approach I found to make sure we don't go
> > > > through a path that could use an incorrect pointer, and makes the code
> > > > easy to review.
> > >
> > > My view is that this is a duplication of logic for one particular
> > > special case - after all, you can also end up walking up to the same
> > > state (client_layer==-1, server_layer==-1, client_walker==NULL,
> > > server_walker==NULL) with the loop at the bottom.
> >
> > Indeed
> >
> > >
> > > But I guess my preference for more concise code is kinda subjective -
> > > if you prefer the more verbose version, I'm fine with that too.
> > >
> > > > > -
> > > > > - server_layer = server->num_layers - 1;
> > > > > - server_walker = server->hierarchy;
> > > > > + server_layer = server ? (server->num_layers - 1) : -1;
> > > > > + server_walker = server ? server->hierarchy : NULL;
> > > >
> > > > We would need to change the last loop to avoid a null pointer deref.
> > >
> > > Why? The first loop would either exit or walk the client_walker up
> > > until client_layer is -1 and client_walker is NULL; the second loop
> > > wouldn't do anything because the walkers are at the same layer; the
> > > third loop's body wouldn't be executed because client_layer is -1.
> >
> > Correct, I missed that client_layer would always be greater than
> > server_layer (-1).
> >
> > Tahera, could you please take Jann's proposal?
> Done.
> We will have duplicate logic, but it would be easier to read and review.
With Jann's proposal we don't have duplicate logic.
> >
> > >
> > > The case where the server is not in any Landlock domain is just one
> > > subcase of the more general case "client and server do not have a
> > > common ancestor domain".
> > >
> > > > >
> > > > > /*
> > > > > * Walks client's parent domains down to the same hierarchy level as
> > > > > * the server's domain, and checks that none of these client's parent
> > > > > * domains are scoped.
> > > > >
> > >
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction
2024-08-02 4:02 ` [PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction Tahera Fahimi
@ 2024-08-09 14:11 ` Mickaël Salaün
2024-08-09 18:16 ` Tahera Fahimi
0 siblings, 1 reply; 23+ messages in thread
From: Mickaël Salaün @ 2024-08-09 14:11 UTC (permalink / raw)
To: Tahera Fahimi
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev
On Thu, Aug 01, 2024 at 10:02:35PM -0600, Tahera Fahimi wrote:
> A sandboxer can receive the character "a" as input from the environment
> variable LL_SCOPE to restrict the abstract unix sockets from connecting
> to a process outside its scoped domain.
>
> Example
> =======
> Create an abstract unix socket to listen with socat(1):
> socat abstract-listen:mysocket -
> Create a sandboxed shell and pass the character "a" to LL_SCOPED:
> LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash
> If the sandboxed process tries to connect to the listening socket
> with command "socat - abstract-connect:mysocket", the connection
> will fail.
>
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
>
> ---
> v8:
> - Adding check_ruleset_scope function to parse the scope environment
> variable and update the landlock attribute based on the restriction
> provided by the user.
> - Adding Mickaël Salaün reviews on version 7.
>
> v7:
> - Adding IPC scoping to the sandbox demo by defining a new "LL_SCOPED"
> environment variable. "LL_SCOPED" gets value "a" to restrict abstract
> unix sockets.
> - Change LANDLOCK_ABI_LAST to 6.
> ---
> samples/landlock/sandboxer.c | 56 +++++++++++++++++++++++++++++++++---
> 1 file changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index e8223c3e781a..98132fd823ad 100644
> --- a/samples/landlock/sandboxer.c
> +++ b/samples/landlock/sandboxer.c
> @@ -14,6 +14,7 @@
> #include <fcntl.h>
> #include <linux/landlock.h>
> #include <linux/prctl.h>
> +#include <linux/socket.h>
> #include <stddef.h>
> #include <stdio.h>
> #include <stdlib.h>
> @@ -22,6 +23,7 @@
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <unistd.h>
> +#include <stdbool.h>
>
> #ifndef landlock_create_ruleset
> static inline int
> @@ -55,6 +57,7 @@ static inline int landlock_restrict_self(const int ruleset_fd,
> #define ENV_FS_RW_NAME "LL_FS_RW"
> #define ENV_TCP_BIND_NAME "LL_TCP_BIND"
> #define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT"
> +#define ENV_SCOPED_NAME "LL_SCOPED"
> #define ENV_DELIMITER ":"
>
> static int parse_path(char *env_path, const char ***const path_list)
> @@ -184,6 +187,38 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
> return ret;
> }
>
> +static bool check_ruleset_scope(const char *const env_var,
> + struct landlock_ruleset_attr *ruleset_attr)
> +{
> + bool ret = true;
> + char *env_type_scope, *env_type_scope_next, *ipc_scoping_name;
> +
> + ruleset_attr->scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
Why always removing the suported scope?
What happen if ABI < 6 ?
> + env_type_scope = getenv(env_var);
> + /* scoping is not supported by the user */
> + if (!env_type_scope)
> + return true;
> + env_type_scope = strdup(env_type_scope);
> + unsetenv(env_var);
> +
> + env_type_scope_next = env_type_scope;
> + while ((ipc_scoping_name =
> + strsep(&env_type_scope_next, ENV_DELIMITER))) {
> + if (strcmp("a", ipc_scoping_name) == 0) {
> + ruleset_attr->scoped |=
> + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
There are two issues here:
1. this would not work for ABI < 6
2. "a" can be repeated several times, which should probably not be
allowed because we don't want to support this
unspecified/undocumented behavior.
> + } else {
> + fprintf(stderr, "Unsupported scoping \"%s\"\n",
> + ipc_scoping_name);
> + ret = false;
> + goto out_free_name;
> + }
> + }
> +out_free_name:
> + free(env_type_scope);
> + return ret;
> +}
> +
> /* clang-format off */
>
> #define ACCESS_FS_ROUGHLY_READ ( \
> @@ -208,7 +243,7 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
>
> /* clang-format on */
>
> -#define LANDLOCK_ABI_LAST 5
> +#define LANDLOCK_ABI_LAST 6
>
> int main(const int argc, char *const argv[], char *const *const envp)
> {
> @@ -223,14 +258,15 @@ int main(const int argc, char *const argv[], char *const *const envp)
> .handled_access_fs = access_fs_rw,
> .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
> LANDLOCK_ACCESS_NET_CONNECT_TCP,
> + .scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
> };
>
> if (argc < 2) {
> fprintf(stderr,
> - "usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
> + "usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
> "<cmd> [args]...\n\n",
> ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> - ENV_TCP_CONNECT_NAME, argv[0]);
> + ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> fprintf(stderr,
> "Execute a command in a restricted environment.\n\n");
> fprintf(stderr,
> @@ -251,15 +287,18 @@ int main(const int argc, char *const argv[], char *const *const envp)
> fprintf(stderr,
> "* %s: list of ports allowed to connect (client).\n",
> ENV_TCP_CONNECT_NAME);
> + fprintf(stderr, "* %s: list of restrictions on IPCs.\n",
> + ENV_SCOPED_NAME);
> fprintf(stderr,
> "\nexample:\n"
> "%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
> "%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
> "%s=\"9418\" "
> "%s=\"80:443\" "
> + "%s=\"a\" "
> "%s bash -i\n\n",
> ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> - ENV_TCP_CONNECT_NAME, argv[0]);
> + ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> fprintf(stderr,
> "This sandboxer can use Landlock features "
> "up to ABI version %d.\n",
> @@ -327,6 +366,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
> /* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
> ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
>
> + __attribute__((fallthrough));
> + case 5:
> + /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
> + ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> fprintf(stderr,
> "Hint: You should update the running kernel "
> "to leverage Landlock features "
> @@ -358,6 +401,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
> ~LANDLOCK_ACCESS_NET_CONNECT_TCP;
> }
>
> + if (!check_ruleset_scope(ENV_SCOPED_NAME, &ruleset_attr)) {
You should use the same pattern as for TCP access rigths: if the
environment variable is not set then remove the ruleset's scopes.
> + perror("Unsupported IPC scoping requested");
> + return 1;
> + }
> +
> ruleset_fd =
> landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> if (ruleset_fd < 0) {
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-07 15:37 ` Tahera Fahimi
@ 2024-08-09 14:13 ` Mickaël Salaün
0 siblings, 0 replies; 23+ messages in thread
From: Mickaël Salaün @ 2024-08-09 14:13 UTC (permalink / raw)
To: Tahera Fahimi
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev
On Wed, Aug 07, 2024 at 09:37:15AM -0600, Tahera Fahimi wrote:
> On Sat, Aug 03, 2024 at 01:29:04PM +0200, Mickaël Salaün wrote:
> > On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> > > This 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. It implements two hooks, unix_stream_connect
> > > and unix_may_send to enforce this restriction.
> > >
> > > Closes: https://github.com/landlock-lsm/linux/issues/7
> > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > >
> > > ---
> > > v8:
> > > - Code refactoring (improve code readability, renaming variable, etc.) based
> > > on reviews by Mickaël Salaün on version 7.
> > > - Adding warn_on_once to check (impossible) inconsistencies.
> > > - Adding inline comments.
> > > - Adding check_unix_address_format to check if the scoping socket is an abstract
> > > unix sockets.
> > > v7:
> > > - 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/
> > > 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 socket's file 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/all/20240610.Aifee5ingugh@digikod.net/
> > > ----
> > > ---
> > > include/uapi/linux/landlock.h | 30 +++++++
> > > security/landlock/limits.h | 3 +
> > > security/landlock/ruleset.c | 7 +-
> > > security/landlock/ruleset.h | 23 ++++-
> > > security/landlock/syscalls.c | 14 ++-
> > > security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
> > > 6 files changed, 225 insertions(+), 7 deletions(-)
> >
> > > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > > index 849f5123610b..7e8579ebae83 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,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> > > return task_ptrace(parent, current);
> > > }
> > >
> > > +static bool walk_and_check(const struct landlock_ruleset *const child,
> > > + struct landlock_hierarchy **walker,
> > > + size_t base_layer, size_t deep_layer,
> > > + access_mask_t check_scoping)
> >
> > s/check_scoping/scope/
> >
> > > +{
> > > + if (!child || base_layer < 0 || !(*walker))
> >
> > I guess it should be:
> > WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
> >
> > > + return false;
> > > +
> > > + for (deep_layer; base_layer < deep_layer; deep_layer--) {
> >
> > No need to pass deep_layer as argument:
> > deep_layer = child->num_layers - 1
> >
> > > + if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> > > + return false;
> > > + *walker = (*walker)->parent;
> > > + if (WARN_ON_ONCE(!*walker))
> > > + /* there is an inconsistency between num_layers
> >
> > Please use full sentences starting with a capital letter and ending with
> > a dot, and in this case start with "/*"
> >
> > > + * and landlock_hierarchy in the ruleset
> > > + */
> > > + return false;
> > > + }
> > > + return true;
> > > +}
> > > +
> > > +/**
> > > + * domain_IPC_scope - Checks if the client domain is scoped in the same
> > > + * domain as the server.
> >
> > Actually, you can remove IPC from the function name.
> >
> > > + *
> > > + * @client: IPC sender domain.
> > > + * @server: IPC receiver domain.
> > > + *
> > > + * Check if the @client domain is scoped to access the @server; the @server
> > > + * must be scoped in the same domain.
> >
> > Returns true if...
> >
> > > + */
> > > +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> > > + const struct landlock_ruleset *const server,
> > > + access_mask_t ipc_type)
> > > +{
> > > + size_t client_layer, server_layer = 0;
> > > + int base_layer;
> > > + struct landlock_hierarchy *client_walker, *server_walker;
> > > + bool is_scoped;
> > > +
> > > + /* Quick return if client has no domain */
> > > + if (!client)
> > > + return true;
> > > +
> > > + client_layer = client->num_layers - 1;
> > > + client_walker = client->hierarchy;
> > > + if (server) {
> > > + server_layer = server->num_layers - 1;
> > > + server_walker = server->hierarchy;
> > > + }
> >
> > } else {
> > server_layer = 0;
> > server_walker = NULL;
> > }
> >
> > > + base_layer = (client_layer > server_layer) ? server_layer :
> > > + client_layer;
> > > +
> > > + /* For client domain, walk_and_check ensures the client domain is
> > > + * not scoped until gets to base_layer.
> >
> > until gets?
> >
> > > + * For server_domain, it only ensures that the server domain exist.
> > > + */
> > > + if (client_layer != server_layer) {
> >
> > bool is_scoped;
> It is defined above.
Yes, but it should be defined here because it is not used outside of
this block.
> > > + if (client_layer > server_layer)
> > > + is_scoped = walk_and_check(client, &client_walker,
> > > + server_layer, client_layer,
> > > + ipc_type);
> > > + else
> >
> > server_walker may be uninitialized and still read here, and maybe later
> > in the for loop. The whole code should maks sure this cannot happen,
> > and a test case should check this.
> I think this case never happens, since the server_walker can be read
> here if there are more than one layers in server domain which means that
> the server_walker is not unintialized.
Yes, but this code makes it more difficult to convince yourself. The
proposed refactoring should help.
>
> > > + is_scoped = walk_and_check(server, &server_walker,
> > > + client_layer, server_layer,
> > > + ipc_type & 0);
> >
> > "ipc_type & 0" is the same as "0"
> >
> > > + if (!is_scoped)
> >
> > The name doesn't reflect the semantic. walk_and_check() should return
> > the inverse.
> >
> > > + return false;
> > > + }
> >
> > This code would be simpler:
> >
> > if (client_layer > server_layer) {
> > base_layer = server_layer;
> > // TODO: inverse boolean logic
> > if (!walk_and_check(client, &client_walker,
> > base_layer, ipc_type))
> > return false;
> > } else (client_layer < server_layer) {
> > base_layer = client_layer;
> > // TODO: inverse boolean logic
> > if (!walk_and_check(server, &server_walker,
> > base_layer, 0))
> > return false;
> > } else {
> > base_layer = client_layer;
> > }
> >
> >
> > I think we can improve more to make sure there is no path/risk of
> > inconsistent pointers.
> >
> >
> > > + /* client and server are at the same level in hierarchy. If client is
> > > + * scoped, the server must be scoped in the same domain
> > > + */
> > > + for (base_layer; base_layer >= 0; base_layer--) {
> > > + if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
> >
> > With each multi-line comment, the first line should be empty:
> > /*
> > * This check must be here since access would be denied only if
> >
> > > + /* This check must be here since access would be denied only if
> > > + * the client is scoped and the server has no domain, so
> > > + * if the client has a domain but is not scoped and the server
> > > + * has no domain, access is guaranteed.
> > > + */
> > > + if (!server)
> > > + return false;
> > > +
> > > + if (server_walker == client_walker)
> > > + return true;
> > > +
> > > + return false;
> > > + }
> > > + client_walker = client_walker->parent;
> > > + server_walker = server_walker->parent;
> > > + /* Warn if there is an incosistenncy between num_layers and
> >
> > Makes sure there is no inconsistency between num_layers and
> >
> >
> > > + * landlock_hierarchy in each of rulesets
> > > + */
> > > + if (WARN_ON_ONCE(base_layer > 0 &&
> > > + (!server_walker || !client_walker)))
> > > + return false;
> > > + }
> > > + return true;
> > > +}
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
2024-08-09 8:49 ` Mickaël Salaün
@ 2024-08-09 17:54 ` Tahera Fahimi
0 siblings, 0 replies; 23+ messages in thread
From: Tahera Fahimi @ 2024-08-09 17:54 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Jann Horn, outreachy, gnoack, paul, jmorris, serge,
linux-security-module, linux-kernel, bjorn3_gh, netdev
On Fri, Aug 09, 2024 at 10:49:17AM +0200, Mickaël Salaün wrote:
> On Thu, Aug 08, 2024 at 05:17:10PM -0600, Tahera Fahimi wrote:
> > On Wed, Aug 07, 2024 at 04:44:36PM +0200, Mickaël Salaün wrote:
> > > On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> > > > On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > > > > I think adding something like this change on top of your code would
> > > > > > make it more concise (though this is entirely untested):
> > > > > >
> > > > > > --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> > > > > > +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> > > > > > @@ -15,25 +15,12 @@
> > > > > > * client_layer must be a signed integer with greater capacity than
> > > > > > * client->num_layers to ensure the following loop stops.
> > > > > > */
> > > > > > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > > > > >
> > > > > > - if (!server) {
> > > > > > - /*
> > > > > > - * Walks client's parent domains and checks that none of these
> > > > > > - * domains are scoped.
> > > > > > - */
> > > > > > - for (; client_layer >= 0; client_layer--) {
> > > > > > - if (landlock_get_scope_mask(client, client_layer) &
> > > > > > - scope)
> > > > > > - return true;
> > > > > > - }
> > > > > > - return false;
> > > > > > - }
> > > > >
> > > > > This loop is redundant with the following one, but it makes sure there
> > > > > is no issue nor inconsistencies with the server or server_walker
> > > > > pointers. That's the only approach I found to make sure we don't go
> > > > > through a path that could use an incorrect pointer, and makes the code
> > > > > easy to review.
> > > >
> > > > My view is that this is a duplication of logic for one particular
> > > > special case - after all, you can also end up walking up to the same
> > > > state (client_layer==-1, server_layer==-1, client_walker==NULL,
> > > > server_walker==NULL) with the loop at the bottom.
> > >
> > > Indeed
> > >
> > > >
> > > > But I guess my preference for more concise code is kinda subjective -
> > > > if you prefer the more verbose version, I'm fine with that too.
> > > >
> > > > > > -
> > > > > > - server_layer = server->num_layers - 1;
> > > > > > - server_walker = server->hierarchy;
> > > > > > + server_layer = server ? (server->num_layers - 1) : -1;
> > > > > > + server_walker = server ? server->hierarchy : NULL;
> > > > >
> > > > > We would need to change the last loop to avoid a null pointer deref.
> > > >
> > > > Why? The first loop would either exit or walk the client_walker up
> > > > until client_layer is -1 and client_walker is NULL; the second loop
> > > > wouldn't do anything because the walkers are at the same layer; the
> > > > third loop's body wouldn't be executed because client_layer is -1.
> > >
> > > Correct, I missed that client_layer would always be greater than
> > > server_layer (-1).
> > >
> > > Tahera, could you please take Jann's proposal?
> > Done.
> > We will have duplicate logic, but it would be easier to read and review.
>
> With Jann's proposal we don't have duplicate logic.
Still the first two for loops apply the same logic for client and server
domains, but I totally understand that it is much easier to review and
understand.
> > >
> > > >
> > > > The case where the server is not in any Landlock domain is just one
> > > > subcase of the more general case "client and server do not have a
> > > > common ancestor domain".
> > > >
> > > > > >
> > > > > > /*
> > > > > > * Walks client's parent domains down to the same hierarchy level as
> > > > > > * the server's domain, and checks that none of these client's parent
> > > > > > * domains are scoped.
> > > > > >
> > > >
> >
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction
2024-08-09 14:11 ` Mickaël Salaün
@ 2024-08-09 18:16 ` Tahera Fahimi
2024-08-12 17:06 ` Mickaël Salaün
0 siblings, 1 reply; 23+ messages in thread
From: Tahera Fahimi @ 2024-08-09 18:16 UTC (permalink / raw)
To: Mickaël Salaün
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev
On Fri, Aug 09, 2024 at 04:11:47PM +0200, Mickaël Salaün wrote:
> On Thu, Aug 01, 2024 at 10:02:35PM -0600, Tahera Fahimi wrote:
> > A sandboxer can receive the character "a" as input from the environment
> > variable LL_SCOPE to restrict the abstract unix sockets from connecting
> > to a process outside its scoped domain.
> >
> > Example
> > =======
> > Create an abstract unix socket to listen with socat(1):
> > socat abstract-listen:mysocket -
> > Create a sandboxed shell and pass the character "a" to LL_SCOPED:
> > LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash
> > If the sandboxed process tries to connect to the listening socket
> > with command "socat - abstract-connect:mysocket", the connection
> > will fail.
> >
> > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> >
> > ---
> > v8:
> > - Adding check_ruleset_scope function to parse the scope environment
> > variable and update the landlock attribute based on the restriction
> > provided by the user.
> > - Adding Mickaël Salaün reviews on version 7.
> >
> > v7:
> > - Adding IPC scoping to the sandbox demo by defining a new "LL_SCOPED"
> > environment variable. "LL_SCOPED" gets value "a" to restrict abstract
> > unix sockets.
> > - Change LANDLOCK_ABI_LAST to 6.
> > ---
> > samples/landlock/sandboxer.c | 56 +++++++++++++++++++++++++++++++++---
> > 1 file changed, 52 insertions(+), 4 deletions(-)
> >
> > diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> > index e8223c3e781a..98132fd823ad 100644
> > --- a/samples/landlock/sandboxer.c
> > +++ b/samples/landlock/sandboxer.c
> > @@ -14,6 +14,7 @@
> > #include <fcntl.h>
> > #include <linux/landlock.h>
> > #include <linux/prctl.h>
> > +#include <linux/socket.h>
> > #include <stddef.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > @@ -22,6 +23,7 @@
> > #include <sys/stat.h>
> > #include <sys/syscall.h>
> > #include <unistd.h>
> > +#include <stdbool.h>
> >
> > #ifndef landlock_create_ruleset
> > static inline int
> > @@ -55,6 +57,7 @@ static inline int landlock_restrict_self(const int ruleset_fd,
> > #define ENV_FS_RW_NAME "LL_FS_RW"
> > #define ENV_TCP_BIND_NAME "LL_TCP_BIND"
> > #define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT"
> > +#define ENV_SCOPED_NAME "LL_SCOPED"
> > #define ENV_DELIMITER ":"
> >
> > static int parse_path(char *env_path, const char ***const path_list)
> > @@ -184,6 +187,38 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
> > return ret;
> > }
> >
> > +static bool check_ruleset_scope(const char *const env_var,
> > + struct landlock_ruleset_attr *ruleset_attr)
> > +{
> > + bool ret = true;
> > + char *env_type_scope, *env_type_scope_next, *ipc_scoping_name;
> > +
> > + ruleset_attr->scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
>
> Why always removing the suported scope?
> What happen if ABI < 6 ?
Right, I will add this check before calling chek_ruleset_scope function.
> > + env_type_scope = getenv(env_var);
> > + /* scoping is not supported by the user */
> > + if (!env_type_scope)
> > + return true;
> > + env_type_scope = strdup(env_type_scope);
> > + unsetenv(env_var);
> > +
> > + env_type_scope_next = env_type_scope;
> > + while ((ipc_scoping_name =
> > + strsep(&env_type_scope_next, ENV_DELIMITER))) {
> > + if (strcmp("a", ipc_scoping_name) == 0) {
> > + ruleset_attr->scoped |=
> > + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
>
> There are two issues here:
> 1. this would not work for ABI < 6
> 2. "a" can be repeated several times, which should probably not be
> allowed because we don't want to support this
> unspecified/undocumented behavior.
For the second note, I think even if the user provides multiple "a"
(something like "a:a"), It would not have a different effect (for now).
Do you suggest that I change this way of handeling this environment
variable or add documents that mention this note?
>
> > + } else {
> > + fprintf(stderr, "Unsupported scoping \"%s\"\n",
> > + ipc_scoping_name);
> > + ret = false;
> > + goto out_free_name;
> > + }
> > + }
> > +out_free_name:
> > + free(env_type_scope);
> > + return ret;
> > +}
> > +
> > /* clang-format off */
> >
> > #define ACCESS_FS_ROUGHLY_READ ( \
> > @@ -208,7 +243,7 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
> >
> > /* clang-format on */
> >
> > -#define LANDLOCK_ABI_LAST 5
> > +#define LANDLOCK_ABI_LAST 6
> >
> > int main(const int argc, char *const argv[], char *const *const envp)
> > {
> > @@ -223,14 +258,15 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > .handled_access_fs = access_fs_rw,
> > .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
> > LANDLOCK_ACCESS_NET_CONNECT_TCP,
> > + .scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
> > };
> >
> > if (argc < 2) {
> > fprintf(stderr,
> > - "usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
> > + "usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
> > "<cmd> [args]...\n\n",
> > ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> > - ENV_TCP_CONNECT_NAME, argv[0]);
> > + ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> > fprintf(stderr,
> > "Execute a command in a restricted environment.\n\n");
> > fprintf(stderr,
> > @@ -251,15 +287,18 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > fprintf(stderr,
> > "* %s: list of ports allowed to connect (client).\n",
> > ENV_TCP_CONNECT_NAME);
> > + fprintf(stderr, "* %s: list of restrictions on IPCs.\n",
> > + ENV_SCOPED_NAME);
> > fprintf(stderr,
> > "\nexample:\n"
> > "%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
> > "%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
> > "%s=\"9418\" "
> > "%s=\"80:443\" "
> > + "%s=\"a\" "
> > "%s bash -i\n\n",
> > ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> > - ENV_TCP_CONNECT_NAME, argv[0]);
> > + ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> > fprintf(stderr,
> > "This sandboxer can use Landlock features "
> > "up to ABI version %d.\n",
> > @@ -327,6 +366,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > /* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
> > ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
> >
> > + __attribute__((fallthrough));
> > + case 5:
> > + /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
> > + ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> > fprintf(stderr,
> > "Hint: You should update the running kernel "
> > "to leverage Landlock features "
> > @@ -358,6 +401,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > ~LANDLOCK_ACCESS_NET_CONNECT_TCP;
> > }
> >
> > + if (!check_ruleset_scope(ENV_SCOPED_NAME, &ruleset_attr)) {
>
> You should use the same pattern as for TCP access rigths: if the
> environment variable is not set then remove the ruleset's scopes.
I think this happens in check_ruleset_scope function. However, I will
add a condition (abi >=6) to this "if" statement.
> > + perror("Unsupported IPC scoping requested");
> > + return 1;
> > + }
> > +
> > ruleset_fd =
> > landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> > if (ruleset_fd < 0) {
> > --
> > 2.34.1
> >
> >
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction
2024-08-09 18:16 ` Tahera Fahimi
@ 2024-08-12 17:06 ` Mickaël Salaün
0 siblings, 0 replies; 23+ messages in thread
From: Mickaël Salaün @ 2024-08-12 17:06 UTC (permalink / raw)
To: Tahera Fahimi
Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev
On Fri, Aug 09, 2024 at 12:16:37PM -0600, Tahera Fahimi wrote:
> On Fri, Aug 09, 2024 at 04:11:47PM +0200, Mickaël Salaün wrote:
> > On Thu, Aug 01, 2024 at 10:02:35PM -0600, Tahera Fahimi wrote:
> > > A sandboxer can receive the character "a" as input from the environment
> > > variable LL_SCOPE to restrict the abstract unix sockets from connecting
> > > to a process outside its scoped domain.
> > >
> > > Example
> > > =======
> > > Create an abstract unix socket to listen with socat(1):
> > > socat abstract-listen:mysocket -
> > > Create a sandboxed shell and pass the character "a" to LL_SCOPED:
> > > LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash
> > > If the sandboxed process tries to connect to the listening socket
> > > with command "socat - abstract-connect:mysocket", the connection
> > > will fail.
> > >
> > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > >
> > > ---
> > > v8:
> > > - Adding check_ruleset_scope function to parse the scope environment
> > > variable and update the landlock attribute based on the restriction
> > > provided by the user.
> > > - Adding Mickaël Salaün reviews on version 7.
> > >
> > > v7:
> > > - Adding IPC scoping to the sandbox demo by defining a new "LL_SCOPED"
> > > environment variable. "LL_SCOPED" gets value "a" to restrict abstract
> > > unix sockets.
> > > - Change LANDLOCK_ABI_LAST to 6.
> > > ---
> > > samples/landlock/sandboxer.c | 56 +++++++++++++++++++++++++++++++++---
> > > 1 file changed, 52 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> > > index e8223c3e781a..98132fd823ad 100644
> > > --- a/samples/landlock/sandboxer.c
> > > +++ b/samples/landlock/sandboxer.c
> > > @@ -14,6 +14,7 @@
> > > #include <fcntl.h>
> > > #include <linux/landlock.h>
> > > #include <linux/prctl.h>
> > > +#include <linux/socket.h>
> > > #include <stddef.h>
> > > #include <stdio.h>
> > > #include <stdlib.h>
> > > @@ -22,6 +23,7 @@
> > > #include <sys/stat.h>
> > > #include <sys/syscall.h>
> > > #include <unistd.h>
> > > +#include <stdbool.h>
> > >
> > > #ifndef landlock_create_ruleset
> > > static inline int
> > > @@ -55,6 +57,7 @@ static inline int landlock_restrict_self(const int ruleset_fd,
> > > #define ENV_FS_RW_NAME "LL_FS_RW"
> > > #define ENV_TCP_BIND_NAME "LL_TCP_BIND"
> > > #define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT"
> > > +#define ENV_SCOPED_NAME "LL_SCOPED"
> > > #define ENV_DELIMITER ":"
> > >
> > > static int parse_path(char *env_path, const char ***const path_list)
> > > @@ -184,6 +187,38 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
> > > return ret;
> > > }
> > >
> > > +static bool check_ruleset_scope(const char *const env_var,
> > > + struct landlock_ruleset_attr *ruleset_attr)
> > > +{
> > > + bool ret = true;
> > > + char *env_type_scope, *env_type_scope_next, *ipc_scoping_name;
> > > +
> > > + ruleset_attr->scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> >
> > Why always removing the suported scope?
> > What happen if ABI < 6 ?
> Right, I will add this check before calling chek_ruleset_scope function.
>
> > > + env_type_scope = getenv(env_var);
> > > + /* scoping is not supported by the user */
> > > + if (!env_type_scope)
> > > + return true;
> > > + env_type_scope = strdup(env_type_scope);
> > > + unsetenv(env_var);
> > > +
> > > + env_type_scope_next = env_type_scope;
> > > + while ((ipc_scoping_name =
> > > + strsep(&env_type_scope_next, ENV_DELIMITER))) {
> > > + if (strcmp("a", ipc_scoping_name) == 0) {
> > > + ruleset_attr->scoped |=
> > > + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> >
> > There are two issues here:
> > 1. this would not work for ABI < 6
> > 2. "a" can be repeated several times, which should probably not be
> > allowed because we don't want to support this
> > unspecified/undocumented behavior.
> For the second note, I think even if the user provides multiple "a"
> (something like "a:a"), It would not have a different effect (for now).
> Do you suggest that I change this way of handeling this environment
> variable or add documents that mention this note?
We should have a stricter approach to only allow zero or one "a" letter.
> >
> > > + } else {
> > > + fprintf(stderr, "Unsupported scoping \"%s\"\n",
> > > + ipc_scoping_name);
> > > + ret = false;
> > > + goto out_free_name;
> > > + }
> > > + }
> > > +out_free_name:
> > > + free(env_type_scope);
> > > + return ret;
> > > +}
> > > +
> > > /* clang-format off */
> > >
> > > #define ACCESS_FS_ROUGHLY_READ ( \
> > > @@ -208,7 +243,7 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
> > >
> > > /* clang-format on */
> > >
> > > -#define LANDLOCK_ABI_LAST 5
> > > +#define LANDLOCK_ABI_LAST 6
> > >
> > > int main(const int argc, char *const argv[], char *const *const envp)
> > > {
> > > @@ -223,14 +258,15 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > > .handled_access_fs = access_fs_rw,
> > > .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
> > > LANDLOCK_ACCESS_NET_CONNECT_TCP,
> > > + .scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
> > > };
> > >
> > > if (argc < 2) {
> > > fprintf(stderr,
> > > - "usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
> > > + "usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
> > > "<cmd> [args]...\n\n",
> > > ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> > > - ENV_TCP_CONNECT_NAME, argv[0]);
> > > + ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> > > fprintf(stderr,
> > > "Execute a command in a restricted environment.\n\n");
> > > fprintf(stderr,
> > > @@ -251,15 +287,18 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > > fprintf(stderr,
> > > "* %s: list of ports allowed to connect (client).\n",
> > > ENV_TCP_CONNECT_NAME);
> > > + fprintf(stderr, "* %s: list of restrictions on IPCs.\n",
> > > + ENV_SCOPED_NAME);
> > > fprintf(stderr,
> > > "\nexample:\n"
> > > "%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
> > > "%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
> > > "%s=\"9418\" "
> > > "%s=\"80:443\" "
> > > + "%s=\"a\" "
> > > "%s bash -i\n\n",
> > > ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> > > - ENV_TCP_CONNECT_NAME, argv[0]);
> > > + ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> > > fprintf(stderr,
> > > "This sandboxer can use Landlock features "
> > > "up to ABI version %d.\n",
> > > @@ -327,6 +366,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > > /* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
> > > ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
> > >
> > > + __attribute__((fallthrough));
> > > + case 5:
> > > + /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
> > > + ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> > > fprintf(stderr,
> > > "Hint: You should update the running kernel "
> > > "to leverage Landlock features "
> > > @@ -358,6 +401,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > > ~LANDLOCK_ACCESS_NET_CONNECT_TCP;
> > > }
> > >
> > > + if (!check_ruleset_scope(ENV_SCOPED_NAME, &ruleset_attr)) {
> >
> > You should use the same pattern as for TCP access rigths: if the
> > environment variable is not set then remove the ruleset's scopes.
> I think this happens in check_ruleset_scope function. However, I will
> add a condition (abi >=6) to this "if" statement.
>
> > > + perror("Unsupported IPC scoping requested");
> > > + return 1;
> > > + }
> > > +
> > > ruleset_fd =
> > > landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> > > if (ruleset_fd < 0) {
> > > --
> > > 2.34.1
> > >
> > >
>
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2024-08-12 17:06 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-02 4:02 [PATCH v8 0/4] Landlock: Add abstract unix socket connect Tahera Fahimi
2024-08-02 4:02 ` [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction Tahera Fahimi
2024-08-02 16:47 ` Mickaël Salaün
2024-08-03 11:29 ` Mickaël Salaün
2024-08-06 19:35 ` Mickaël Salaün
2024-08-06 20:46 ` Jann Horn
2024-08-07 7:21 ` Mickaël Salaün
2024-08-07 13:45 ` Jann Horn
2024-08-07 14:44 ` Mickaël Salaün
2024-08-08 23:17 ` Tahera Fahimi
2024-08-09 8:49 ` Mickaël Salaün
2024-08-09 17:54 ` Tahera Fahimi
2024-08-07 15:37 ` Tahera Fahimi
2024-08-09 14:13 ` Mickaël Salaün
2024-08-06 19:36 ` Jann Horn
2024-08-02 4:02 ` [PATCH v8 2/4] selftests/landlock: Abstract unix socket restriction tests Tahera Fahimi
2024-08-07 15:08 ` Mickaël Salaün
2024-08-02 4:02 ` [PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction Tahera Fahimi
2024-08-09 14:11 ` Mickaël Salaün
2024-08-09 18:16 ` Tahera Fahimi
2024-08-12 17:06 ` Mickaël Salaün
2024-08-02 4:02 ` [PATCH v8 4/4] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning Tahera Fahimi
2024-08-07 15:14 ` Mickaël Salaün
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).