Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SCTP bind/connect mishandling #40

Open
sm1ling-knight opened this issue Sep 4, 2024 · 2 comments
Open

Fix SCTP bind/connect mishandling #40

sm1ling-knight opened this issue Sep 4, 2024 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@sm1ling-knight
Copy link

sm1ling-knight commented Sep 4, 2024

SCTP is connection-oriented protocol that can be used to establish one-to-many and one-to-one communication between endpoints.

One-to-one style can be used by specifying AF_INET family, SOCK_STREAM type and IPPROTO_SCTP protocol value in the socket(2):

int sctp_client_fd;

sctp_client_fd = socket(AF_INET, SOCK_STREAM, IPPROTO_SCTP);

Current implementation of LANDLOCK_ACCESS_NET_BIND_TCP, LANDLOCK_ACCESS_NET_CONNECT_TCP allows to restrict bind/connect actions for both classic TCP sockets and SCTP sockets.

SCTP allows to bind and connect sockets not only with bind(2), connect(2), but also with setsockopt(3p). Options SCTP_SOCKOPT_CONNECT*, SCTP_SOCKOPT_BIND*, ... (Cf. SCTP) are provided for this purpose.

For example:

setsockopt(sctp_client_fd, IPPROTO_SCTP, SCTP_SOCKOPT_CONNECTX, &addr, sizeof(addr));

It is not possible to restrict such calls using LANDLOCK_ACCESS_NET_BIND_TCP, LANDLOCK_ACCESS_NET_CONNECT_TCP which leads to inconsistency of Landlock behavior.

There are a few ways to fix this issue:

  1. Change behavior of TCP access rights so that they check only classic TCP sockets (with protocol=0).
  2. Implement restriction of SCTP bind/connect via setsockopt(3p). This can be done by adding a hook on security_sctp_bind_connect (Cf. net/sctp/socket.c).
@l0kod
Copy link
Member

l0kod commented Sep 5, 2024

Well spotted! That restriction on SCTP sockets should be considered a bug because the LANDLOCK_ACCESS_NET_BIND_TCP (and the related documentation) is explicitly about TCP, not SCTP. We need a fix and related tests for that, and they should be backported. Do you want to work on that? It might be easier to first merge this fix (because of the backport) and then merge your work on socket creation (I'll review it too).

Complementary to this fix, it would be nice to be able to control SCTP sockets with a new LANDLOCK_ACCESS_NET_BIND_SCTP that would also handle binding via setsockopt(), but should come with another patch series.

@l0kod l0kod added the bug Something isn't working label Sep 5, 2024
@sm1ling-knight
Copy link
Author

Hello Mickaël!

I'd like to implement dedicated patch-fix. Please assign this issue to me.

@l0kod l0kod changed the title Restrict SCTP bind/connect via setsockopt Fix SCTP bind/connect mishandling Sep 9, 2024
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Oct 3, 2024
Do not check TCP access right if socket protocol is not IPPROTO_TCP.
LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
should not restrict bind(2) and connect(2) for non-TCP protocols
(SCTP, MPTCP, SMC).

Closes: landlock-lsm#40
Fixes: fff69fb ("landlock: Support network rules with TCP bind and connect")
Signed-off-by: Mikhail Ivanov <[email protected]>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Oct 17, 2024
Do not check TCP access right if socket protocol is not IPPROTO_TCP.
LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
should not restrict bind(2) and connect(2) for non-TCP protocols
(SCTP, MPTCP, SMC).

sk_is_tcp() is used for this to check address family of the socket
before doing INET-specific address length validation. This is required
for error consistency.

Closes: landlock-lsm#40
Fixes: fff69fb ("landlock: Support network rules with TCP bind and connect")
Signed-off-by: Mikhail Ivanov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Ready
Development

No branches or pull requests

2 participants