Thanks Ivanov, this looks really good!  Let me some time to review the
rest.

You can add this tag to the commit message (as reference and
documentation):
Closes: https://github.com/landlock-lsm/linux/issues/6

On Thu, Apr 11, 2024 at 06:16:31PM +0300, Ivanov Mikhail wrote:

Hello! Big thanks for your review and ideas :)

P.S.: Sorry, previous mail was rejected by linux mailboxes
due to HTML formatting.

4/8/2024 10:49 PM, Günther Noack wrote:

quoted

Hello!

Just zooming in on what I think are the most high level questions here,
so that we get the more dramatic changes out of the way early, if needed.

On Mon, Apr 08, 2024 at 05:39:18PM +0800, Ivanov Mikhail wrote:

quoted

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 25c8d7677..8551ade38 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h

@@ -37,6 +37,13 @@ struct landlock_ruleset_attr {
  	 * rule explicitly allow them.
  	 */
  	__u64 handled_access_net;
+
+	/**
+	 * @handled_access_net: Bitmask of actions (cf. `Socket flags`_)

                            ^^^
			   Typo

Thanks, will be fixed.

quoted

quoted

+	 * that is handled by this ruleset and should then be forbidden if no
+	 * rule explicitly allow them.
+	 */
+	__u64 handled_access_socket;

What is your rationale for introducing and naming this additional field?

I am not convinced that "socket" is the right name to use in this field,
but it is well possible that I'm missing some context.

* If we introduce this additional field in the landlock_ruleset_attr, which
   other socket-related operations will go in the remaining 63 bits?  (I'm having
   a hard time coming up with so many of them.)

If i understood correctly Mickaël suggested saving some space for
actions related not only to creating sockets, but also to sending
and receiving socket FDs from another processes, marking pre-sandboxed
sockets as allowed or denied after sandboxing [2]. This may be necessary
in order to achieve complete isolation of the sandbox, which will be
able to create, receive and send sockets of specific protocols.

In future this field may become more generic by including rules for
other entities with similar actions (e.g. files, pipes).

I think it would make sense to have one field per file kind (not
necessarily type) because not all actions would make sense.

I think it is good approach, but we should discuss this design before
generalizing the name. For now `handled_access_socket` can be a good
name for actions related to accessing specific sockets (protocols).
What do you think?

I'm OK with this name for now unless someone has a better proposition.

[2]
https://lore.kernel.org/all/b8a2045a-e7e8-d141-7c01-bf47874c7930@digikod.net/ (local)

quoted

* Should this have a more general name than "socket", so that other planned
   features from the bug tracker [1] fit in?

I have not found any similar features for our case. Do you have any in
mind?

quoted

The other alternative is of course to piggy back on the existing
handled_access_net field, whose name already is pretty generic.

handled_access_net is indeed quite generic, but the question is: would
this new access right make sense for the net_port rule?  In the case of
socket creation, this is not the case because we don't know at this time
which port will be used.

quoted

For that, I believe we would need to clarify in struct landlock_net_port_attr
which exact values are permitted there.

Potentially anything that would be possible to check against a port.

quoted

I imagine you have considered this approach?  Are there more reasons why this
was ruled out, which I am overlooking?

[1] https://github.com/orgs/landlock-lsm/projects/1/views/1

Currently `handled_access_net` stands for restricting actions for
specific network protocols by port values: LANDLOCK_ACCESS_NET_BIND_TCP,
LANDLOCK_ACCESS_NET_SEND_UDP (possibly will be added with UDP feature
[3]).

I dont think that complicating semantics with adding fields for
socket_create()-like actions would fit well here. Purpose of current
patch is to restrict usage of unwanted protocols, not to add logic
to restrict their actions. In addition, it is worth considering that we
want to restrict not only network protocols (e.g. Bluetooth).

Correct.  It's worth it mentionning this rationale in the patch
description.

[3] https://github.com/landlock-lsm/linux/issues/1

quoted

quoted

@@ -244,4 +277,20 @@ struct landlock_net_port_attr {
  #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
  #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
  /* clang-format on */
+
+/**
+ * DOC: socket_acess
+ *
+ * Socket flags
+ * ~~~~~~~~~~~~~~~~

Mega-Nit: This ~~~ underline should only be as long as the text above it ;-)
You might want to fix it for the "Network Flags" headline as well.

Ofc, thanks!

quoted

quoted

+ *
+ * These flags enable to restrict a sandboxed process to a set of
+ * socket-related actions for specific protocols. This is supported
+ * since the Landlock ABI version 5.
+ *
+ * - %LANDLOCK_ACCESS_SOCKET_CREATE: Create a socket
+ */

quoted

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?

Günther's suggestion would be a good optimization, but I agree that it
should be part of another series.  We also need to keep in mind that the
layer level should be known for audit and debugging reasons.

[4] https://github.com/landlock-lsm/linux/issues/1

quoted

As I said before, I am very excited to see this patch.

I think this will unlock a tremendous amount of use cases for many programs,
especially for programs that do not use networking at all, which can now lock
themselves down to guarantee that with a sandbox.

Thank you very much for looking into it!

Same :)