[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240816.Bi8EitheeV2o@digikod.net>
Date: Fri, 16 Aug 2024 23:19:58 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Tahera Fahimi <fahimitahera@...il.com>
Cc: outreachy@...ts.linux.dev, gnoack@...gle.com, paul@...l-moore.com,
jmorris@...ei.org, serge@...lyn.com, linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org, bjorn3_gh@...tonmail.com, jannh@...gle.com,
netdev@...r.kernel.org
Subject: Re: [PATCH v9 1/5] Landlock: Add abstract unix socket connect
restriction
On Wed, Aug 14, 2024 at 12:22:19AM -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@...il.com>
>
> ---
> v9:
> - Editting inline comments.
> - Major refactoring in domain_is_scoped() and is_abstract_socket
> 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/
> ----
Useless "----"
> ---
> include/uapi/linux/landlock.h | 27 +++++++
> security/landlock/limits.h | 3 +
> security/landlock/ruleset.c | 7 +-
> security/landlock/ruleset.h | 23 +++++-
> security/landlock/syscalls.c | 17 +++--
> security/landlock/task.c | 129 ++++++++++++++++++++++++++++++++++
> 6 files changed, 198 insertions(+), 8 deletions(-)
>
> +static bool sock_is_scoped(struct sock *const other,
> + const struct landlock_ruleset *const dom)
Please rename "dom" to "domain". Function arguments with full names
make the API more consistent and easier to understand.
> +{
> + const struct landlock_ruleset *dom_other;
> +
> + /* 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_is_scoped(dom, dom_other,
> + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET);
> +}
> +
> +static bool is_abstract_socket(struct sock *const sock)
> +{
> + struct unix_address *addr = unix_sk(sock)->addr;
> +
> + if (!addr)
> + return false;
> +
> + if (addr->len >= offsetof(struct sockaddr_un, sun_path) + 1 &&
> + addr->name[0].sun_path[0] == '\0')
> + return true;
> +
> + return false;
Much better!
> +}
> +
> +static int hook_unix_stream_connect(struct sock *const sock,
> + struct sock *const other,
> + struct sock *const newsk)
> +{
> + const struct landlock_ruleset *const dom =
> + landlock_get_current_domain();
> +
> + /* quick return for non-sandboxed processes */
> + if (!dom)
> + return 0;
> +
> + if (is_abstract_socket(other))
> + if (sock_is_scoped(other, dom))
if (is_abstract_socket(other) && sock_is_scoped(other, dom))
(We might want to extend this hook in the future but we'll revise this
notation when needed)
> + return -EPERM;
> +
> + return 0;
> +}
> +
> +static int hook_unix_may_send(struct socket *const sock,
> + struct socket *const other)
> +{
> + const struct landlock_ruleset *const dom =
> + landlock_get_current_domain();
> +
> + if (!dom)
> + return 0;
> +
> + if (is_abstract_socket(other->sk))
> + if (sock_is_scoped(other->sk, dom))
ditto
> + return -EPERM;
> +
> + return 0;
> +}
> +
> 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
>
>
Powered by blists - more mailing lists