[PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction
Mickaël Salaün
mic at digikod.net
Sat Aug 3 11:29:04 UTC 2024
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 at 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;
> +}
More information about the Linux-security-module-archive
mailing list