netdev - Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAG48ez2ZYzB+GyDLAx7y2TobE=MLXWucQx0qjitfhPSDaaqjiA@mail.gmail.com>
Date: Tue, 6 Aug 2024 22:46:43 +0200
From: Jann Horn <jannh@...gle.com>
To: Mickaël Salaün <mic@...ikod.net>
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 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.
>                          */
>                         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.