Received-SPF: pass (google.com: domain of linux-kernel+bounces-197417-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 31 May 2024 14:04:44 -0600
From: Tahera Fahimi <fahimitahera@gmail.com>
To: =?iso-8859-1?Q?Micka=EBl_Sala=FCn?= <mic@digikod.net>
Cc: aul Moore <paul@paul-moore.com>, James Morris <jmorris@namei.org>,
	"Serge E.Hallyn" <serge@hallyn.com>,
	linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org,
	outreachy@lists.linux.dev, netdev@vger.kernel.org,
	=?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
	Jann Horn <jannh@google.com>,
	=?iso-8859-1?Q?G=FCnther?= Noack <gnoack@google.com>
Subject: Re: [PATCH v2] landlock: Add abstract unix socket connect
 restrictions
Message-ID: <ZlotXL4sfY5Ez3I5@tahera-OptiPlex-5000>
References: <ZgX5TRTrSDPrJFfF@tahera-OptiPlex-5000>
 <20240401.ieC2uqua5sha@digikod.net>
 <ZhcRnhVKFUgCleDi@tahera-OptiPlex-5000>
 <20240411.ahgeefeiNg4i@digikod.net>
 <ZlkIAIpWG/l64Pl9@tahera-OptiPlex-5000>
 <20240531.Ahg5aap6caeG@digikod.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240531.Ahg5aap6caeG@digikod.net>

On Fri, May 31, 2024 at 11:39:12AM +0200, Mickaël Salaün wrote:
> On Thu, May 30, 2024 at 05:13:04PM -0600, Tahera Fahimi wrote:
> > On Tue, Apr 30, 2024 at 05:24:45PM +0200, Mickaël Salaün wrote:
> > > On Wed, Apr 10, 2024 at 04:24:30PM -0600, Tahera Fahimi wrote:
> > > > On Tue, Apr 02, 2024 at 11:53:09AM +0200, Mickaël Salaün wrote:
> > > > > Thanks for this patch.  Please CC the netdev mailing list too, they may
> > > > > be interested by this feature. I also added a few folks that previously
> > > > > showed their interest for this feature.
> > > > > 
> > > > > On Thu, Mar 28, 2024 at 05:12:13PM -0600, TaheraFahimi wrote:
> > > > > > Abstract unix sockets are used for local interprocess communication without
> > > > > > relying on filesystem. Since landlock has no restriction for connecting to
> > > > > > a UNIX socket in the abstract namespace, a sandboxed process can connect to
> > > > > > a socket outside the sandboxed environment. Access to such sockets should
> > > > > > be scoped the same way ptrace access is limited.
> > > > > 
> > > > > This is good but it would be better to explain that Landlock doesn't
> > > > > currently control abstract unix sockets and that it would make sense for
> > > > > a sandbox.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > For a landlocked process to be allowed to connect to a target process, it
> > > > > > must have a subset of the target process’s rules (the connecting socket
> > > > > > must be in a sub-domain of the listening socket). This patch adds a new
> > > > > > LSM hook for connect function in unix socket with the related access rights.
> > > > > 
> > > > > Because of compatibility reasons, and because Landlock should be
> > > > > flexible, we need to extend the user space interface.  As explained in
> > > > > the GitHub issue, we need to add a new "scoped" field to the
> > > > > landlock_ruleset_attr struct. This field will optionally contain a
> > > > > LANDLOCK_RULESET_SCOPED_ABSTRACT_UNIX_SOCKET flag to specify that this
> > > > > ruleset will deny any connection from within the sandbox to its parents
> > > > > (i.e. any parent sandbox or not-sandboxed processes).
> > > 
> > > > Thanks for the feedback. Here is what I understood, please correct me if
> > > > I am wrong. First, I should add another field to the
> > > > landlock_ruleset_attr (a field like handled_access_net, but for the unix
> > > > sockets) with a flag LANDLOCK_ACCESS_UNIX_CONNECT (it is a flag like
> > > > LANDLOCK_ACCESS_NET_CONNECT_TCP but fot the unix sockets connect).
> > > 
> > > That was the initial idea, but after thinking more about it and talking
> > > with some users, I now think we can get a more generic interface.
> > > 
> > > Because unix sockets, signals, and other IPCs are fully controlled by
> > > the kernel (contrary to inet sockets that get out of the system), we can
> > > add ingress and egress control according to the source and the
> > > destination.
> > > 
> > > To control the direction we could add an
> > > LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_RECEIVE and a
> > > LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_SEND rights (these names are a bit
> > > long but at least explicit).  To control the source and destination, it
> > > makes sense to use Landlock domain (i.e. sandboxes):
> > > LANDLOCK_DOMAIN_HIERARCHY_PARENT, LANDLOCK_DOMAIN_HIERARCHY_SELF, and
> > > LANDLOCK_DOMAIN_HIERARCHY_CHILD.  This could be used by extending the
> > > landlock_ruleset_attr type and adding a new
> > > landlock_domain_hierarchy_attr type:
> > > 
> > > struct landlock_ruleset_attr ruleset_attr = {
> > >   .handled_access_dom = LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_RECEIVE | \
> > >                         LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_SEND,
> > > }
> > > 
> > > // Allows sending data to and receiving data from processes in the same
> > > // domain or a child domain, through abstract unix sockets.
> > > struct landlock_domain_hierarchy_attr dom_attr = {
> > >   .allowed_access = LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_RECEIVE | \
> > >                     LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_SEND,
> > >   .relationship = LANDLOCK_DOMAIN_HIERARCHY_SELF | \
> > >                   LANDLOCK_DOMAIN_HIERARCHY_CHILD,
> > > };
> > > 
> > > It should also work with other kind of IPCs:
> > > * LANDLOCK_ACCESS_DOM_UNIX_PATHNAME_RECEIVE/SEND (signal)
> > > * LANDLOCK_ACCESS_DOM_SIGNAL_RECEIVE/SEND (signal)
> > > * LANDLOCK_ACCESS_DOM_XSI_RECEIVE/SEND (XSI message queue)
> > > * LANDLOCK_ACCESS_DOM_MQ_RECEIVE/SEND (POSIX message queue)
> > > * LANDLOCK_ACCESS_DOM_PTRACE_RECEIVE/SEND (ptrace, which would be
> > >   limited)
> > > 
> > > What do you think?
> > 
> > I was wondering if you expand your idea on the following example. 
> > 
> > Considering P1 with the rights that you mentioned in your email, forks a
> > new process (P2). Now both P1 and P2 are on the same domain and are
> > allowed to send data to and receive data from processes in the same
> > domain or a child domain. 
> > /*
> >  *         Same domain (inherited)
> >  * .-------------.
> >  * | P1----.     |      P1 -> P2 : allow
> >  * |        \    |      P2 -> P1 : allow
> >  * |         '   |
> >  * |         P2  |
> >  * '-------------'
> >  */
> > (P1 domain) = (P2 domain) = {
> > 		.allowed_access =
> > 			LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_RECEIVE | 
> > 			LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_SEND,
> > 		.relationship = 
> > 			LANDLOCK_DOMAIN_HIERARCHY_SELF | 
> > 			LANDLOCK_DOMAIN_HIERARCHY_CHILD,
> 
> In this case LANDLOCK_DOMAIN_HIERARCHY_CHILD would not be required
> because P1 and P2 are on the same domain.
> 
> > 		}
> > 
> > In another example, if P1 has the same domain as before but P2 has
> > LANDLOCK_DOMAIN_HIERARCHY_PARENT in their domain, so P1 still can 
> > connect to P2. 
> > /*
> >  *        Parent domain
> >  * .------.
> >  * |  P1  --.           P1 -> P2 : allow
> >  * '------'  \          P2 -> P1 : allow
> >  *            '
> >  *            P2
> >  */
> > 
> > (P1 domain) = {
> >                 .allowed_access =
> >                         LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_RECEIVE |
> >                         LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_SEND,
> >                 .relationship = 
> >                         LANDLOCK_DOMAIN_HIERARCHY_SELF |
> >                         LANDLOCK_DOMAIN_HIERARCHY_CHILD,
> 
> Hmm, in this case P2 doesn't have a domain, so
> LANDLOCK_DOMAIN_HIERARCHY_CHILD doesn't make sense.
> 
> >                 }
> > (P2 domain) = {
> >                 .allowed_access =
> >                         LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_RECEIVE |
> >                         LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_SEND,
> >                 .relationship = 
> >                         LANDLOCK_DOMAIN_HIERARCHY_SELF |
> >                         LANDLOCK_DOMAIN_HIERARCHY_CHILD |
> > 			LANDLOCK_DOMAIN_HIERARCHY_PARENT,
> > 		}
> 
> I think you wanted to use the "Inherited + child domain" example here,
> in which case the domain policies make sense.
> 
> I was maybe too enthusiastic with the "relationship" field.  Let's
> rename landlock_domain_hierarchy_attr to landlock_domain_attr and remove
> the "relationship" field.  We'll always consider that
> LANDLOCK_DOMAIN_HIERARCHY_SELF is set as well as
> LANDLOCK_DOMAIN_HIERARCHY_CHILD (i.e. no restriction to send/received
> to/from a child domain or our own domain).  In a nutshell, please only
> keep the LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_{RECEIVE,SEND} rights and
> follow the same logic as with ptrace restrictions.  It will be easier to
> reason about and will be useful for most cases.  We could later extend
> that with more features.
> 
> LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_RECEIVE will then translates to "allow
> to receive from the parent domain".
> LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_SEND will then translates to "allow to
> send to the parent domain".
If we consider LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_* shows the
ability to send/recieve data to/from the parent domain, different
scenarios would be as follow(again using your drawings from the
ptrace_test):

/*
 *        No domain
 *
 *   P1-.               P1 -> P2 : allow
 *       \              P2 -> P1 : allow
 *        'P2
 */

(Child domain): Since child can not send/recieve data to/from parent,the
connection of both direction is banned.
/*
 *        Child domain:
 *
 *   P1--.              P1 -> P2 : deny
 *        \             P2 -> P1 : deny
 *        .'-----.
 *        |  P2  |
 *        '------'
 */

(Parent domain): The parent's access to its parent is restricted, so the
child and parent can establish connection.
/*
 *        Parent domain
 * .------.
 * |  P1  --.           P1 -> P2 : allow
 * '------'  \          P2 -> P1 : allow
 *            '
 *            P2
 */

(Parent + child domain): Same as (child domain) scenario
/*
 *        Parent + child domain(inherited)
 * .------.
 * |  P1  ---.          P1 -> P2 : deny
 * '------'   \         P2 -> P1 : deny
 *         .---'--.
 *         |  P2  |
 *         '------'
 */

(Same domain): An example is when a process fork two child processes and
they inherit the parent's access. In this case, children proccess can
send/recieve data to/from each other since they are in the same domain.
/*
 *         Same domain (sibling)
 * .-------------.
 * | P1----.     |      P1 -> P2 : allow
 * |        \    |      P2 -> P1 : allow
 * |         '   |
 * |         P2  |
 * '-------------'
 */

/*
 *         Inherited + child domain
 * .-----------------.
 * |  P1----.        |  P1 -> P2 : deny
 * |         \       |  P2 -> P1 : deny
 * |        .-'----. |
 * |        |  P2  | |
 * |        '------' |
 * '-----------------'
 */

/*
 *         Inherited + parent domain
 * .-----------------.
 * |.------.         |  P1 -> P2 : allow
 * ||  P1  ----.     |  P2 -> P1 : allow
 * |'------'    \    |
 * |             '   |
 * |             P2  |
 * '-----------------'
 */

/*
 *         Inherited + parent and child domain
 * .-----------------.
 * | .------.        |  P1 -> P2 : deny
 * | |  P1  .        |  P2 -> P1 : deny
 * | '------'\       |
 * |          \      |
 * |        .--'---. |
 * |        |  P2  | |
 * |        '------' |
 * '-----------------'
 */
Any feedback on this logic is appreciated.

> As for other Landlock access rights, the restrictions of domains should
> only be changed if LANDLOCK_ACCESS_DOM_UNIX_ABSTRACT_* is "handled" by
> the ruleset/domain.