| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240807.mieloh8bi8Ae@digikod.net>
Date: Wed, 7 Aug 2024 09:21:03 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Jann Horn <jannh@...gle.com>
Cc: Tahera Fahimi <fahimitahera@...il.com>, 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,
netdev@...r.kernel.org
Subject: Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect
restriction
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@...ikod.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.
>
Powered by blists - more mailing lists