Re: [RFC PATCH v1 01/10] landlock: Support socket access-control

["Günther Noack" <gnoack@google.com>]

Hello!

On Thu, Apr 11, 2024 at 06:16:31PM +0300, Ivanov Mikhail wrote:
> 4/8/2024 10:49 PM, Günther Noack wrote:
> > On Mon, Apr 08, 2024 at 05:39:18PM +0800, Ivanov Mikhail wrote:
> > > diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> > > index c7f152678..f4213db09 100644
> > > --- a/security/landlock/ruleset.h
> > > +++ b/security/landlock/ruleset.h
> > > @@ -92,6 +92,12 @@ enum landlock_key_type {
> > >   	 * node keys.
> > >   	 */
> > >   	LANDLOCK_KEY_NET_PORT,
> > > +
> > > +	/**
> > > +	 * @LANDLOCK_KEY_SOCKET: Type of &landlock_ruleset.root_socket's
> > > +	 * node keys.
> > > +	 */
> > > +	LANDLOCK_KEY_SOCKET,
> > >   };
> > >   /**
> > > @@ -177,6 +183,15 @@ struct landlock_ruleset {
> > >   	struct rb_root root_net_port;
> > >   #endif /* IS_ENABLED(CONFIG_INET) */
> > > +	/**
> > > +	 * @root_socket: Root of a red-black tree containing &struct
> > > +	 * landlock_rule nodes with socket type, described by (domain, type)
> > > +	 * pair (see socket(2)). Once a ruleset is tied to a
> > > +	 * process (i.e. as a domain), this tree is immutable until @usage
> > > +	 * reaches zero.
> > > +	 */
> > > +	struct rb_root root_socket;
> > 
> > The domain is a value between 0 and 45,
> > and the socket type is one of 1, 2, 3, 4, 5, 6, 10.
> > 
> > The bounds of these are defined with AF_MAX (include/linux/socket.h) and
> > SOCK_MAX (include/linux/net.h).
> > 
> > Why don't we just combine these two numbers into an index and create a big bit
> > vector here, like this:
> > 
> >      socket_type_mask_t socket_domains[AF_MAX];
> > 
> > socket_type_mask_t would need to be typedef'd to u16 and ideally have a static
> > check to test that it has more bits than SOCK_MAX.
> > 
> > Then you can look up whether a socket creation is permitted by checking:
> > 
> >      /* assuming appropriate bounds checks */
> >      if (dom->socket_domains[domain] & (1 << type)) { /* permitted */ }
> > 
> > and merging the socket_domains of two domains would be a bitwise-AND.
> > 
> > (We can also cram socket_type_mask_t in a u8 but it would require mapping the
> > existing socket types onto a different number space.)
> > 
> 
> I chose rbtree based on the current storage implementation in fs,net and
> decided to leave the implementation of better variants in a separate
> patch, which should redesign the entire storage system in Landlock
> (e.g. implementation of a hashtable for storing rules by FDs,
> port values) [4].
> 
> Do you think that it is bad idea and more appropriate storage for socket
> rules(e.g. what you suggested) should be implemented by current patch?
> 
> [4] https://github.com/landlock-lsm/linux/issues/1

I realized that my suggestion might be at odds with Mickaël's Landlock audit
patch set [1].  IIRC, the goal there is to log the reasons for a denial,
together with the Landlock ruleset on which this decision was based.

[1] https://lore.kernel.org/all/20230921061641.273654-1-mic@digikod.net/

I'd recommend to wait for Mickaël to chime in on this one before spending the
time to reimplement that.


—Günther