[RFC PATCH v1 0/3] Use socket's Landlock domain

[RFC PATCH v1 0/3] Use socket's Landlock domain

[Mickaël Salaün]

Hi,

While the current approach works, I think we should change the way
Landlock restricts network actions.  Because this feature is relatively
new, we can still fix this inconsistency.  In a nutshell, let's follow a
more capability-based model.  Please let me know what you think.

Regards,

Mickaël Salaün (3):
  landlock: Use socket's domain instead of current's domain
  selftests/landlock: Add test for socket's domain
  landlock: Document network restrictions tied to sockets

 Documentation/userspace-api/landlock.rst    |  4 ++-
 security/landlock/net.c                     | 22 ++++++++--------
 tools/testing/selftests/landlock/net_test.c | 29 +++++++++++++++++++++
 3 files changed, 43 insertions(+), 12 deletions(-)


base-commit: f4b89d8ce5a835afa51404977ee7e3889c2b9722
-- 
2.45.2

[RFC PATCH v1 1/3] landlock: Use socket's domain instead of current's domain

[Mickaël Salaün]

Before this change, network restrictions were enforced according to the
calling thread's Landlock domain, leading to potential inconsistent
results when the same socket was used by different threads or processes
(with different domains).  This change fixes such access control
inconsistency by enforcing the socket's Landlock domain instead of the
caller's Landlock domain.

Socket's Landlock domain is inherited from the thread that created this
socket.  This means that a socket created without sandboxing will be
free to connect and bind without limitation.  This also means that a
socket created by a sandboxed thread will inherit the thread's policy,
which will be enforced on this socket even when used by another thread
or passed to another process.

The initial rationale [1] was that a socket does not directly grants
access to data, but it is an object used to define an access (e.g.
connection to a peer).  Contrary to my initial assumption, we can
identify to which protocol/port a newly created socket can give access
to with the socket's file->f_cred inherited from its creator.  Moreover,
from a kernel point of view, especially for shared objects, we need a
more consistent access model.  This means that the same action on the
same socket performed by different threads will have the same effect.
This follows the same approach as for file descriptors tied to the file
system (e.g. LANDLOCK_ACCESS_FS_TRUNCATE).

One potential risk of this change is for unsandboxed processes to send
socket file descriptors to sandboxed processes, which could give
unrestricted network access to the sandboxed process (by reconfigure the
socket).  While it makes sense for processes to transfer (AF_UNIX)
socketpairs, which is OK because they can only exchange data between
themselves, it should be rare for processes to legitimately pass other
kind of sockets (e.g. AF_INET).

Another potential risk of this approach is socket file descriptor leaks.
This is the same risk as with regular file descriptor leaks giving
access to the content of a file, which is well known and documented.
This could be mitigated with a future complementary restriction on
received or inherited file descriptors.

One interesting side effect of this new approach is that a process can
create a socket that will only allow to connect to a set of ports.  This
can be done by creating a thread, sandboxing it, creating a socket, and
using the related file descriptor (in the same process).  Passing this
restricted socket to a more sandboxed process makes it possible to have
a more dynamic security policy.

This new approach aligns with SELinux and Smack instead of AppArmor and
Tomoyo.  It is also in line with capability-based security mechanisms
such as Capsicum.

This slight semantic change is important for current and future
Landlock's consistency, and it must be backported.

Current tests are still OK because this behavior wasn't covered.  A
following commit adds new tests.

Cc: Günther Noack <gnoack@google.com>
Cc: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com>
Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Tahera Fahimi <fahimitahera@gmail.com>
Cc: <stable@vger.kernel.org> # 6.7.x: 088e2efaf3d2: landlock: Simplify current_check_access_socket()
Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
Link: https://lore.kernel.org/r/263c1eb3-602f-57fe-8450-3f138581bee7@digikod.net [1]
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20240719150618.197991-2-mic@digikod.net
---
 security/landlock/net.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/security/landlock/net.c b/security/landlock/net.c
index c8bcd29bde09..78e027a74819 100644
--- a/security/landlock/net.c
+++ b/security/landlock/net.c
@@ -50,10 +50,11 @@ get_raw_handled_net_accesses(const struct landlock_ruleset *const domain)
 	return access_dom;
 }
 
-static const struct landlock_ruleset *get_current_net_domain(void)
+static const struct landlock_ruleset *
+get_socket_net_domain(const struct socket *const sock)
 {
 	const struct landlock_ruleset *const dom =
-		landlock_get_current_domain();
+		landlock_cred(sock->file->f_cred)->domain;
 
 	if (!dom || !get_raw_handled_net_accesses(dom))
 		return NULL;
@@ -61,10 +62,9 @@ static const struct landlock_ruleset *get_current_net_domain(void)
 	return dom;
 }
 
-static int current_check_access_socket(struct socket *const sock,
-				       struct sockaddr *const address,
-				       const int addrlen,
-				       access_mask_t access_request)
+static int check_access_socket(struct socket *const sock,
+			       struct sockaddr *const address,
+			       const int addrlen, access_mask_t access_request)
 {
 	__be16 port;
 	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
@@ -72,7 +72,7 @@ static int current_check_access_socket(struct socket *const sock,
 	struct landlock_id id = {
 		.type = LANDLOCK_KEY_NET_PORT,
 	};
-	const struct landlock_ruleset *const dom = get_current_net_domain();
+	const struct landlock_ruleset *const dom = get_socket_net_domain(sock);
 
 	if (!dom)
 		return 0;
@@ -175,16 +175,16 @@ static int current_check_access_socket(struct socket *const sock,
 static int hook_socket_bind(struct socket *const sock,
 			    struct sockaddr *const address, const int addrlen)
 {
-	return current_check_access_socket(sock, address, addrlen,
-					   LANDLOCK_ACCESS_NET_BIND_TCP);
+	return check_access_socket(sock, address, addrlen,
+				   LANDLOCK_ACCESS_NET_BIND_TCP);
 }
 
 static int hook_socket_connect(struct socket *const sock,
 			       struct sockaddr *const address,
 			       const int addrlen)
 {
-	return current_check_access_socket(sock, address, addrlen,
-					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
+	return check_access_socket(sock, address, addrlen,
+				   LANDLOCK_ACCESS_NET_CONNECT_TCP);
 }
 
 static struct security_hook_list landlock_hooks[] __ro_after_init = {
-- 
2.45.2

[RFC PATCH v1 2/3] selftests/landlock: Add test for socket's domain

[Mickaël Salaün]

This new ipv4_tcp.socket_domain test checks that the restrictions are
tied to the socket at creation time, but not tied to the thread
requesting a bind action.

Properly close file descriptor in ipv4.with_fs test.

Cc: Günther Noack <gnoack@google.com>
Cc: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com>
Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Tahera Fahimi <fahimitahera@gmail.com>
Cc: stable@vger.kernel.org
Fixes: a549d055a22e ("selftests/landlock: Add network tests")
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20240719150618.197991-3-mic@digikod.net
---
 tools/testing/selftests/landlock/net_test.c | 29 +++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index f21cfbbc3638..79251e27d26d 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -1579,6 +1579,35 @@ TEST_F(ipv4_tcp, with_fs)
 	bind_fd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0);
 	ASSERT_LE(0, bind_fd);
 	EXPECT_EQ(-EACCES, bind_variant(bind_fd, &self->srv1));
+	EXPECT_EQ(0, close(bind_fd));
+}
+
+TEST_F(ipv4_tcp, socket_domain)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
+	};
+	int ruleset_fd, bind_fd;
+
+	/* Creates socket before sandboxing. */
+	bind_fd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0);
+	ASSERT_LE(0, bind_fd);
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	/* Tests port binding with unsandboxed socket. */
+	EXPECT_EQ(0, bind_variant(bind_fd, &self->srv1));
+	EXPECT_EQ(0, close(bind_fd));
+
+	/* Tests port binding with new sandboxed socket. */
+	bind_fd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0);
+	ASSERT_LE(0, bind_fd);
+	EXPECT_EQ(-EACCES, bind_variant(bind_fd, &self->srv1));
+	EXPECT_EQ(0, close(bind_fd));
 }
 
 FIXTURE(port_specific)
-- 
2.45.2

[RFC PATCH v1 3/3] landlock: Document network restrictions tied to sockets

[Mickaël Salaün]

The Landlock domain used to restrict operations on a socket is the
domain from the thread that created this socket.

Cc: Günther Noack <gnoack@google.com>
Cc: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com>
Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Tahera Fahimi <fahimitahera@gmail.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20240719150618.197991-4-mic@digikod.net
---
 Documentation/userspace-api/landlock.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 37dafce8038b..4a9bfff575d5 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -529,7 +529,9 @@ Network support (ABI < 4)
 Starting with the Landlock ABI version 4, it is now possible to restrict TCP
 bind and connect actions to only a set of allowed ports thanks to the new
 ``LANDLOCK_ACCESS_NET_BIND_TCP`` and ``LANDLOCK_ACCESS_NET_CONNECT_TCP``
-access rights.
+access rights.  These restrictions are tied to a socket and are inherited from
+the sandboxed thread that created this socket.  Hence, sockets created before
+sandboxing are not restricted.
 
 IOCTL (ABI < 5)
 ---------------
-- 
2.45.2

Re: [RFC PATCH v1 0/3] Use socket's Landlock domain

[Günther Noack]

Hello Mickaël!

On Fri, Jul 19, 2024 at 05:06:15PM +0200, Mickaël Salaün wrote:
> While the current approach works, I think we should change the way
> Landlock restricts network actions.  Because this feature is relatively
> new, we can still fix this inconsistency.  In a nutshell, let's follow a
> more capability-based model.  Please let me know what you think.

Thanks for sending the patch.  The implementation with ->f_cred is much simpler
than I had thought it would be.  Some higher level questions:

 * I assume that the plan is to backport this as a fix to older kernels that
   already have the feature?  (Otherwise, we would potentially have backwards
   compatibility issues.)

 * I believe it clashes a little bit with the TCP server example [1],
   which I found a useful use case for the TCP connect/bind and socket
   restriction features.

 * accept(2) on a passive (listen-mode) socket will give you a new socket FD
   -- does that new socket inherit its f_cred from the server socket,
   or does it inherit its f_cred from the thread?

Regarding the TCP server example, the current implementation is *very* simple,
and does the following steps:

 1. create socket with socket(2)
 2. bind(2) the socket to the desired port
 3. enforce a Landlock ruleset that disables all networking features
    (TCP bind, TCP connect and socket creation with the new patch set)
 4. listen(2) on the socket
 5. go into the accept(2) loop

With the old behaviour, step 3 is going to affect the existing passive socket:
It will not be possible any more to bind(2) that passive socket to another port.

With the new behaviour (after your patch), step 3 does *not* affect the existing
socket, and the server socket can be reused to bind(2) to other ports.

Or, in other words: If the relevant domain is tied to the socket at creation
time, that means that a future client connection which takes over the process
might be able to use that socket's Landlock domain, which potentially grants
more permissions than the thread's domain

I think it would be nice if a use case like in the TCP server example would
still be possible with Landlock; there are multiple ways to reach that:

 - We could enforce two layers of Landlock rules, one before socket creation
   that restricts bind(2) to a given port, and one after socket creation that
   restricts other bind(2), create(2) and socket(2) operations.

   Drawbacks:

   - One Landlock layer more, and needs to add a Landlock rule:
     This is a bit more complicated to implement.
   - The bind(2) restriction on the socket is still only per port *number*,
     but it is still possible to bind with that port number on different IP
     addresses.

 - Alternatively, I wish we could just lock the passive server socket in, so
   that it can't be made to reconnect anywhere else.  After all, the socket
   disassociation with connect(2) with AF_UNSPEC seems to be a somewhat obscure
   and seldomly used feature - if we could just disallow that operation, we
   could ensure that this socket gets reused for such a nefarious purpose.

   It would still require two nested Landlock rulesets to make the TCP server
   example work, but they would be a bit simpler than in the alternative above.

 - There are probably more alternatives...?

What do you think?

—Günther

[1] https://wiki.gnoack.org/LandlockTcpServerExample

Re: [RFC PATCH v1 0/3] Use socket's Landlock domain

[Mickaël Salaün]

On Tue, Jul 23, 2024 at 03:07:53PM +0200, Günther Noack wrote:
> Hello Mickaël!
> 
> On Fri, Jul 19, 2024 at 05:06:15PM +0200, Mickaël Salaün wrote:
> > While the current approach works, I think we should change the way
> > Landlock restricts network actions.  Because this feature is relatively
> > new, we can still fix this inconsistency.  In a nutshell, let's follow a
> > more capability-based model.  Please let me know what you think.
> 
> Thanks for sending the patch.  The implementation with ->f_cred is much simpler
> than I had thought it would be.  Some higher level questions:
> 
>  * I assume that the plan is to backport this as a fix to older kernels that
>    already have the feature?  (Otherwise, we would potentially have backwards
>    compatibility issues.)

Correct, if this patch is merged it must be backported too, but there
might be better alternatives, or we might just stick to the initial
approach.

> 
>  * I believe it clashes a little bit with the TCP server example [1],
>    which I found a useful use case for the TCP connect/bind and socket
>    restriction features.

Indeed, because the socket is created before the sandboxing, the socket
could be reused to bind (or connect) to other ports.  This is a good
example of why using current's instead of socket's credentials may be
less surprising.  From my point of view, the main issue is that a socket
can be reconfigured.

> 
>  * accept(2) on a passive (listen-mode) socket will give you a new socket FD
>    -- does that new socket inherit its f_cred from the server socket,
>    or does it inherit its f_cred from the thread?

According to sock_alloc_file(), the newly created socket inherits the
caller's credentials, which is similar to a call to openat2(2) with a
directory file descriptor.

> 
> Regarding the TCP server example, the current implementation is *very* simple,
> and does the following steps:
> 
>  1. create socket with socket(2)
>  2. bind(2) the socket to the desired port
>  3. enforce a Landlock ruleset that disables all networking features
>     (TCP bind, TCP connect and socket creation with the new patch set)
>  4. listen(2) on the socket
>  5. go into the accept(2) loop
> 
> With the old behaviour, step 3 is going to affect the existing passive socket:
> It will not be possible any more to bind(2) that passive socket to another port.
> 
> With the new behaviour (after your patch), step 3 does *not* affect the existing
> socket, and the server socket can be reused to bind(2) to other ports.
> 
> Or, in other words: If the relevant domain is tied to the socket at creation
> time, that means that a future client connection which takes over the process
> might be able to use that socket's Landlock domain, which potentially grants
> more permissions than the thread's domain

Yes, that's why it might be more risky, but I wanted to have this
discussion.  Whatever the outcome, it should be explained in
Documentation/security/landlock.rst

One thing to keep in mind and that contrary to other LSMs, Landlock,
like seccomp, enables processes to (only) drop privileges, and this is
done by the process itself (not at execve time).  This was also one
argument for the initial approach.

A thing that bothered me was related to the restrictions of sockets,
especially with the WIP scoping feature.  Datagram (unix) sockets could
work before sandboxing, and suddenly become broken after sandboxing
(because the security check would be done at send time instead of
connect time).  This kind of issue should be identified quite early and
easily though.

However, there is still an inconsistency between connected stream
sockets and datagram sockets.  From a security point of view, this looks
like a good thing though.

> 
> I think it would be nice if a use case like in the TCP server example would
> still be possible with Landlock; there are multiple ways to reach that:
> 
>  - We could enforce two layers of Landlock rules, one before socket creation
>    that restricts bind(2) to a given port, and one after socket creation that
>    restricts other bind(2), create(2) and socket(2) operations.
> 
>    Drawbacks:
> 
>    - One Landlock layer more, and needs to add a Landlock rule:
>      This is a bit more complicated to implement.

Right, I think it's too complex for users.

>    - The bind(2) restriction on the socket is still only per port *number*,
>      but it is still possible to bind with that port number on different IP
>      addresses.

Good point.  That's another argument for the initial approach and the
way you sandboxed the example: dropping the *_TCP access rights, it is
not possible to rebind or reconnect a socket (to another address).

Actually, I'm not sure if using the socket's credential would not
confuse users to understand why an access is denied (or allowed).

> 
>  - Alternatively, I wish we could just lock the passive server socket in, so
>    that it can't be made to reconnect anywhere else.  After all, the socket
>    disassociation with connect(2) with AF_UNSPEC seems to be a somewhat obscure
>    and seldomly used feature - if we could just disallow that operation, we
>    could ensure that this socket gets reused for such a nefarious purpose.

We could also add a new "scope" for socket reconfiguration of sockets
created by a parent or sibling domain, similar to the ptrace
restrictions.

> 
>    It would still require two nested Landlock rulesets to make the TCP server
>    example work, but they would be a bit simpler than in the alternative above.
> 
>  - There are probably more alternatives...?
> 
> What do you think?

I see other alternatives:

- We could have a new ruleset's attribute to specify if network
  restrictions should apply on the caller or the socket.  That might be
  confusing for users though.

- We could just stick to the initial approach and add new access rights
  (denied by default, similar to FS_REFER) that will only apply to newly
  created sockets.  This is close to the previous alternative but more
  explicit. Both use cases could then be used, with a default secure
  approach (i.e. the initial one).  However, we need to have a clear
  rationale for the WIP scoping restrictions: should the caller or the
  socket be checked as the client?

- We could extend the current approach and check both the caller's
  credential and the socket's credential.  This could be confusing to
  users though.

- We could have a new fcntl(2) command to (securely) transition a file
  descriptor's credential to the caller's one (e.g. approved by
  ptrace_may_access).  That could be generic to all Linux access control
  systems.

- According to a new ruleset's attribute, we could revalidate (at use
  time) file descriptors not opened by the caller's Landlock domain, but
  users would have to be explicit (e.g. stdio issue). And how to handle
  partially allowed accesses?

> 
> —Günther
> 
> [1] https://wiki.gnoack.org/LandlockTcpServerExample
>