Messages in this thread | | | Date | Tue, 2 Apr 2024 11:53:09 +0200 | From | Mickaël Salaün <> | Subject | Re: [PATCH v2] landlock: Add abstract unix socket connect restrictions |
| |
Thanks for this patch. Please CC the netdev mailing list too, they may be interested by this feature. I also added a few folks that previously showed their interest for this feature.
On Thu, Mar 28, 2024 at 05:12:13PM -0600, TaheraFahimi wrote: > Abstract unix sockets are used for local interprocess communication without > relying on filesystem. Since landlock has no restriction for connecting to > a UNIX socket in the abstract namespace, a sandboxed process can connect to > a socket outside the sandboxed environment. Access to such sockets should > be scoped the same way ptrace access is limited.
This is good but it would be better to explain that Landlock doesn't currently control abstract unix sockets and that it would make sense for a sandbox.
> > For a landlocked process to be allowed to connect to a target process, it > must have a subset of the target process’s rules (the connecting socket > must be in a sub-domain of the listening socket). This patch adds a new > LSM hook for connect function in unix socket with the related access rights.
Because of compatibility reasons, and because Landlock should be flexible, we need to extend the user space interface. As explained in the GitHub issue, we need to add a new "scoped" field to the landlock_ruleset_attr struct. This field will optionally contain a LANDLOCK_RULESET_SCOPED_ABSTRACT_UNIX_SOCKET flag to specify that this ruleset will deny any connection from within the sandbox to its parents (i.e. any parent sandbox or not-sandboxed processes).
> > Link to first draft: > https://lore.kernel.org/outreachy/20240328.ShoR4Iecei8o@digikod.net/
You can move this sentence in the below changelog.
>
You can add this:
Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
Your Git (or email app) configuration doesn't use the same name as here.
Please run ./scripts/checkpatch.pl on this patch and fix the warnings.
> > ---- > Changes in v2: > - Remove wrapper functions, noted by Casey Schaufler <casey@schaufler-ca.com> > --- > security/landlock/task.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/security/landlock/task.c b/security/landlock/task.c > index 849f5123610b..67528f87b7de 100644 > --- a/security/landlock/task.c > +++ b/security/landlock/task.c > @@ -13,6 +13,7 @@ > #include <linux/lsm_hooks.h> > #include <linux/rcupdate.h> > #include <linux/sched.h> > +#include <net/sock.h> > > #include "common.h" > #include "cred.h" > @@ -108,9 +109,48 @@ static int hook_ptrace_traceme(struct task_struct *const parent) > return task_ptrace(parent, current); > } > > +static bool unix_sock_is_scoped(struct sock *const sock,
For consistency with task_is_scoped(), you can rename this to sock_is_scoped().
> + struct sock *const other) > +{ > + bool is_scoped = true; > + > + /* get the ruleset of connecting sock*/
These comments don't help more than the following line, you can remove them.
> + const struct landlock_ruleset *const dom_sock =
According to the name it looks like the domain of the socket but it is just the domain of the current task. Just "dom" would be clearer and more consistent with security/landlock/fs.c
> + landlock_get_current_domain(); > + > + if (!dom_sock) > + return true; > + > + /* get credential of listening sock*/ > + const struct cred *cred_other = get_cred(other->sk_peer_cred);
We have a get but not a put call, so the credentials will never be freed. The put call must be called before any return, so you probably need to follow the goto/error pattern.
In the context of these LSM hooks, only unix_listen() sets the "other" socket credential, and unix_listen() is guarded by unix_state_lock() which locks unix_sk(s)->lock . When security_unix_stream_connect() or security_unix_may_send() are called, unix_sk(s)->lock is locked as well, which protects the credentials against race-conditions (TOCTOU: time-of-check to time-of-use). We should then make that explicit with this assertion (which also documents it):
lockdep_assert_held(&unix_sk(other)->lock);
In theory it is then not required to call get_cred(). However, because the performance impact should be negligible and to avoid a potential use-after-free (not possible in theory with the current code), it would be safer to still call get/put. It would be worse to have a use-after-free rather than an access control issue.
Another thing to keep in mind is that for this hook to be race-condition-free, the credential must not change anyway. A comment should highlight that.
> + > + if (!cred_other) > + return true; > + > + /* retrieve the landlock_rulesets */ > + const struct landlock_ruleset *dom_parent;
All declarations should be at the top of functions.
> + > + rcu_read_lock();
No need for this RCU lock because the lock is managed by unix_state_lock() in this case.
> + dom_parent = landlock_cred(cred_other)->domain; > + is_scoped = domain_scope_le(dom_parent, dom_sock); > + rcu_read_unlock(); > + > + return is_scoped; > +} > + > +static int hook_unix_stream_connect(struct sock *const sock, > + struct sock *const other, > + struct sock *const newsk) > +{ > + if (unix_sock_is_scoped(sock, other)) > + 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),
Please add a hook for security_unix_may_send() too, it should be quite similar, and simplify the patch's subject accordingly.
You now need to add tests (in a dedicated patch) extending tools/testing/selftests/landlock/ptrace_test.c (I'll rename the file later).
These tests should also check with unnamed and named unix sockets. I guess the current code doesn't differentiate them and control all kind of unix sockets. Because they must explicitly be passed, sockets created with socketpair(2) (i.e. unnamed socket) should never be denied.
> }; > > __init void landlock_add_task_hooks(void) > -- > 2.34.1 > >
| |