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 <[email protected]> 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.
>