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: > > 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: > > > 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. > > > > + * 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@xxxxxxxxxxx/ > > > > > * 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? > > > > > 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. > > > > 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. > > > > 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 > > > > @@ -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! > > > > + * > > > + * 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 > > > + */ > > > > > > > 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 > > > > > 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 :)
