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: <CAG48ez3_u5ZkVY31h4J6Shap9kEsgDiLxF+s10Aea52EkrDMJg@mail.gmail.com>
Date: Wed, 7 Aug 2024 15:45:18 +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 Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@...ikod.net> wrote:
> On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > 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.

My view is that this is a duplication of logic for one particular
special case - after all, you can also end up walking up to the same
state (client_layer==-1, server_layer==-1, client_walker==NULL,
server_walker==NULL) with the loop at the bottom.

But I guess my preference for more concise code is kinda subjective -
if you prefer the more verbose version, I'm fine with that too.

> > -
> > -        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.

Why? The first loop would either exit or walk the client_walker up
until client_layer is -1 and client_walker is NULL; the second loop
wouldn't do anything because the walkers are at the same layer; the
third loop's body wouldn't be executed because client_layer is -1.

The case where the server is not in any Landlock domain is just one
subcase of the more general case "client and server do not have a
common ancestor domain".

> >
> >          /*
> >           * 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.
> >