| Message ID | 20241003143932.2431249-2-ivanov.mikhail1@huawei-partners.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Fix non-TCP sockets restriction | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote: > 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: https://github.com/landlock-lsm/linux/issues/40 > Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect") > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> > --- > security/landlock/net.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/landlock/net.c b/security/landlock/net.c > index bc3d943a7118..6f59dd98bb13 100644 > --- a/security/landlock/net.c > +++ b/security/landlock/net.c > @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock, > return -EACCES; > > /* Checks if it's a (potential) TCP socket. */ > - if (sock->type != SOCK_STREAM) > + if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP) > return 0; > > /* Checks for minimal header length to safely read sa_family. */ > -- > 2.34.1 > Thank you! Good catch! Reviewed-by: Günther Noack <gnoack@google.com>
Please also add Matthieu in Cc for the network patch series. On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote: > 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: https://github.com/landlock-lsm/linux/issues/40 > Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect") > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> > --- > security/landlock/net.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/landlock/net.c b/security/landlock/net.c > index bc3d943a7118..6f59dd98bb13 100644 > --- a/security/landlock/net.c > +++ b/security/landlock/net.c > @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock, > return -EACCES; > > /* Checks if it's a (potential) TCP socket. */ We can extend this comment to explain that we don't use sk_is_tcp() because we need to handle the AF_UNSPEC case. > - if (sock->type != SOCK_STREAM) > + if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP) I think we should check sock->sk->sk_type instead of sock->type (even if it should be the same). To make it simpler, we should only use sk in current_check_access_socket(): struct sock *sk = sock->sk; Could you please also do s/__sk_common\.skc_/sk_/g ? > return 0; > > /* Checks for minimal header length to safely read sa_family. */ > -- > 2.34.1 > >
On 10/3/2024 8:45 PM, Mickaël Salaün wrote: > Please also add Matthieu in Cc for the network patch series. > > On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote: >> 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: https://github.com/landlock-lsm/linux/issues/40 >> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect") >> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> >> --- >> security/landlock/net.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/security/landlock/net.c b/security/landlock/net.c >> index bc3d943a7118..6f59dd98bb13 100644 >> --- a/security/landlock/net.c >> +++ b/security/landlock/net.c >> @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock, >> return -EACCES; >> >> /* Checks if it's a (potential) TCP socket. */ > > We can extend this comment to explain that we don't use sk_is_tcp() > because we need to handle the AF_UNSPEC case. Indeed, I'll do this. > >> - if (sock->type != SOCK_STREAM) >> + if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP) > > I think we should check sock->sk->sk_type instead of sock->type (even if > it should be the same). To make it simpler, we should only use sk in > current_check_access_socket(): > struct sock *sk = sock->sk; Agreed. > > Could you please also do s/__sk_common\.skc_/sk_/g ? Ofc Btw, there is probably incorrect read of skc_family in this function [1]. I'll add READ_ONCE for sk->sk_family. [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/ > >> return 0; >> >> /* Checks for minimal header length to safely read sa_family. */ >> -- >> 2.34.1 >> >>
On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
> Please also add Matthieu in Cc for the network patch series.
Ok
On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote: > On 10/3/2024 8:45 PM, Mickaël Salaün wrote: > > Please also add Matthieu in Cc for the network patch series. > > > > On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote: > > > 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: https://github.com/landlock-lsm/linux/issues/40 > > > Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect") > > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> > > > --- > > > security/landlock/net.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/security/landlock/net.c b/security/landlock/net.c > > > index bc3d943a7118..6f59dd98bb13 100644 > > > --- a/security/landlock/net.c > > > +++ b/security/landlock/net.c > > > @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock, > > > return -EACCES; > > > /* Checks if it's a (potential) TCP socket. */ > > > > We can extend this comment to explain that we don't use sk_is_tcp() > > because we need to handle the AF_UNSPEC case. > > Indeed, I'll do this. > > > > > > - if (sock->type != SOCK_STREAM) > > > + if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP) > > > > I think we should check sock->sk->sk_type instead of sock->type (even if > > it should be the same). To make it simpler, we should only use sk in > > current_check_access_socket(): > > struct sock *sk = sock->sk; > > Agreed. > > > > > Could you please also do s/__sk_common\.skc_/sk_/g ? > > Ofc > > Btw, there is probably incorrect read of skc_family in this function > [1]. I'll add READ_ONCE for sk->sk_family. > > [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/ I think it should not be a bug with the current code (IPv6 -> IPV4, and socket vs. sock) but we should indeed use READ_ONCE() (and add this link to the commit message). > > > > > > return 0; > > > /* Checks for minimal header length to safely read sa_family. */ > > > -- > > > 2.34.1 > > > > > > >
On 10/4/2024 1:13 PM, Mickaël Salaün wrote: > On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote: >> On 10/3/2024 8:45 PM, Mickaël Salaün wrote: >>> Please also add Matthieu in Cc for the network patch series. >>> >>> On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote: >>>> 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: https://github.com/landlock-lsm/linux/issues/40 >>>> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect") >>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> >>>> --- >>>> security/landlock/net.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/security/landlock/net.c b/security/landlock/net.c >>>> index bc3d943a7118..6f59dd98bb13 100644 >>>> --- a/security/landlock/net.c >>>> +++ b/security/landlock/net.c >>>> @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock, >>>> return -EACCES; >>>> /* Checks if it's a (potential) TCP socket. */ >>> >>> We can extend this comment to explain that we don't use sk_is_tcp() >>> because we need to handle the AF_UNSPEC case. >> >> Indeed, I'll do this. I've noticed that we still should check sk->sk_family = AF_INET{,6} here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to addresses and we should not provide any checks (for address) if socket is unrestrictable (i.e. it's not TCP). It's not useful and might lead to error incosistency for non-TCP sockets. Btw, I suppose we can improve error consistency by bringing more checks from INET/TCP stack. For example it may be useful to return EISCONN instead of EACCES while connect(2) is called on a connected socket. This should be done really carefully and only for some useful cases. Anyway it's not related to the current patch (since it's not a bug). >> >>> >>>> - if (sock->type != SOCK_STREAM) >>>> + if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP) >>> >>> I think we should check sock->sk->sk_type instead of sock->type (even if >>> it should be the same). To make it simpler, we should only use sk in >>> current_check_access_socket(): >>> struct sock *sk = sock->sk; >> >> Agreed. >> >>> >>> Could you please also do s/__sk_common\.skc_/sk_/g ? >> >> Ofc >> >> Btw, there is probably incorrect read of skc_family in this function >> [1]. I'll add READ_ONCE for sk->sk_family. >> >> [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/ > > I think it should not be a bug with the current code (IPv6 -> IPV4, and > socket vs. sock) but we should indeed use READ_ONCE() (and add this link > to the commit message). ok > >> >>> >>>> return 0; >>>> /* Checks for minimal header length to safely read sa_family. */ >>>> -- >>>> 2.34.1 >>>> >>>> >>
On Fri, Oct 04, 2024 at 09:16:56PM +0300, Mikhail Ivanov wrote: > On 10/4/2024 1:13 PM, Mickaël Salaün wrote: > > On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote: > > > On 10/3/2024 8:45 PM, Mickaël Salaün wrote: > > > > Please also add Matthieu in Cc for the network patch series. > > > > > > > > On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote: > > > > > 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: https://github.com/landlock-lsm/linux/issues/40 > > > > > Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect") > > > > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> > > > > > --- > > > > > security/landlock/net.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/security/landlock/net.c b/security/landlock/net.c > > > > > index bc3d943a7118..6f59dd98bb13 100644 > > > > > --- a/security/landlock/net.c > > > > > +++ b/security/landlock/net.c > > > > > @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock, > > > > > return -EACCES; > > > > > /* Checks if it's a (potential) TCP socket. */ > > > > > > > > We can extend this comment to explain that we don't use sk_is_tcp() > > > > because we need to handle the AF_UNSPEC case. > > > > > > Indeed, I'll do this. > > I've noticed that we still should check sk->sk_family = AF_INET{,6} > here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to > addresses and we should not provide any checks (for address) if socket > is unrestrictable (i.e. it's not TCP). It's not useful and might lead to > error incosistency for non-TCP sockets. Good catch, let's use sk_is_tcp(). > > Btw, I suppose we can improve error consistency by bringing more checks > from INET/TCP stack. For example it may be useful to return EISCONN > instead of EACCES while connect(2) is called on a connected socket. Yes, that would be nice (with the related tests). > > This should be done really carefully and only for some useful cases. > Anyway it's not related to the current patch (since it's not a bug). Sure. The following patch series could probably be extended for all LSM to benefit from these fixes: https://lore.kernel.org/all/20240327120036.233641-1-mic@digikod.net/ Mikhail, according to your SCTP tests with SELinux, it looks like this patch series should be updated, but that should be simple. Paul, what is the status of this LSM patch series? Could Mikhail integrate this LSM patch (with the SCTP fix) as part of the current Landlock patch series? This would help fixing the Landlock tests (which check SCTP error consistency) when run with SELinux. > > > > > > > > > > > > > - if (sock->type != SOCK_STREAM) > > > > > + if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP) > > > > > > > > I think we should check sock->sk->sk_type instead of sock->type (even if > > > > it should be the same). To make it simpler, we should only use sk in > > > > current_check_access_socket(): > > > > struct sock *sk = sock->sk; > > > > > > Agreed. > > > > > > > > > > > Could you please also do s/__sk_common\.skc_/sk_/g ? > > > > > > Ofc > > > > > > Btw, there is probably incorrect read of skc_family in this function > > > [1]. I'll add READ_ONCE for sk->sk_family. > > > > > > [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/ > > > > I think it should not be a bug with the current code (IPv6 -> IPV4, and > > socket vs. sock) but we should indeed use READ_ONCE() (and add this link > > to the commit message). > > ok > > > > > > > > > > > > > > > return 0; > > > > > /* Checks for minimal header length to safely read sa_family. */ > > > > > -- > > > > > 2.34.1 > > > > > > > > > > > > > >
On Sat, Oct 05, 2024 at 05:49:59PM +0200, Mickaël Salaün wrote: > On Fri, Oct 04, 2024 at 09:16:56PM +0300, Mikhail Ivanov wrote: > > On 10/4/2024 1:13 PM, Mickaël Salaün wrote: > > > On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote: > > > > On 10/3/2024 8:45 PM, Mickaël Salaün wrote: > > > > > Please also add Matthieu in Cc for the network patch series. > > > > > > > > > > On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote: > > > > > > 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: https://github.com/landlock-lsm/linux/issues/40 > > > > > > Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect") > > > > > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> > > > > > > --- > > > > > > security/landlock/net.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/security/landlock/net.c b/security/landlock/net.c > > > > > > index bc3d943a7118..6f59dd98bb13 100644 > > > > > > --- a/security/landlock/net.c > > > > > > +++ b/security/landlock/net.c > > > > > > @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock, > > > > > > return -EACCES; > > > > > > /* Checks if it's a (potential) TCP socket. */ > > > > > > > > > > We can extend this comment to explain that we don't use sk_is_tcp() > > > > > because we need to handle the AF_UNSPEC case. > > > > > > > > Indeed, I'll do this. > > > > I've noticed that we still should check sk->sk_family = AF_INET{,6} > > here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to > > addresses and we should not provide any checks (for address) if socket > > is unrestrictable (i.e. it's not TCP). It's not useful and might lead to > > error incosistency for non-TCP sockets. > > Good catch, let's use sk_is_tcp(). > > > > > Btw, I suppose we can improve error consistency by bringing more checks > > from INET/TCP stack. For example it may be useful to return EISCONN > > instead of EACCES while connect(2) is called on a connected socket. > > Yes, that would be nice (with the related tests). > > > > > This should be done really carefully and only for some useful cases. > > Anyway it's not related to the current patch (since it's not a bug). > > Sure. > > The following patch series could probably be extended for all LSM to > benefit from these fixes: > https://lore.kernel.org/all/20240327120036.233641-1-mic@digikod.net/ > > Mikhail, according to your SCTP tests with SELinux, it looks like this > patch series should be updated, but that should be simple. > > Paul, what is the status of this LSM patch series? Could Mikhail > integrate this LSM patch (with the SCTP fix) as part of the current > Landlock patch series? This would help fixing the Landlock tests (which > check SCTP error consistency) when run with SELinux. Well, this whole LSM network check should probably be part of another patch series. For now, let's just fix SELinux (and check that other LSMs don't return wrong error codes).
On 10/5/2024 6:49 PM, Mickaël Salaün wrote: > On Fri, Oct 04, 2024 at 09:16:56PM +0300, Mikhail Ivanov wrote: >> On 10/4/2024 1:13 PM, Mickaël Salaün wrote: >>> On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote: >>>> On 10/3/2024 8:45 PM, Mickaël Salaün wrote: >>>>> Please also add Matthieu in Cc for the network patch series. >>>>> >>>>> On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote: >>>>>> 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: https://github.com/landlock-lsm/linux/issues/40 >>>>>> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect") >>>>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> >>>>>> --- >>>>>> security/landlock/net.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c >>>>>> index bc3d943a7118..6f59dd98bb13 100644 >>>>>> --- a/security/landlock/net.c >>>>>> +++ b/security/landlock/net.c >>>>>> @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock, >>>>>> return -EACCES; >>>>>> /* Checks if it's a (potential) TCP socket. */ >>>>> >>>>> We can extend this comment to explain that we don't use sk_is_tcp() >>>>> because we need to handle the AF_UNSPEC case. >>>> >>>> Indeed, I'll do this. >> >> I've noticed that we still should check sk->sk_family = AF_INET{,6} >> here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to >> addresses and we should not provide any checks (for address) if socket >> is unrestrictable (i.e. it's not TCP). It's not useful and might lead to >> error incosistency for non-TCP sockets. > > Good catch, let's use sk_is_tcp(). > >> >> Btw, I suppose we can improve error consistency by bringing more checks >> from INET/TCP stack. For example it may be useful to return EISCONN >> instead of EACCES while connect(2) is called on a connected socket. > > Yes, that would be nice (with the related tests). > >> >> This should be done really carefully and only for some useful cases. >> Anyway it's not related to the current patch (since it's not a bug). > > Sure. I have a little question to clarify before sending a next version. Are we condisering order of network checks for error consistency? For example, in the current_check_access_socket() we have following order of checks for ipv4 connect(2) action: (1) addrlen < sizeof(struct sockaddr_in) -> return -EINVAL (2) sa_family != sk_family -> return -EINVAL The ipv4 stack has a check for sock->state before (1) and (2), which can return -EISCONN if the socket is already connected. This results in the possiblity of two following scenarios: Landlock enabled: 1. socket(ipv4) -> OK 2. connect(ipv4 address) -> OK 3. connect(ipv6 address) -> -EINVAL (sa_family != sk_family) Landlock disabled: 1. socket(ipv4) -> OK 2. connect(ipv4 address) -> OK 3. connect(ipv6 address) -> -EISCONN (socket is already connected) I have always considered the order of network checks as part of error consistency, and I'd like to make sure that we're on the same page before extending current patch with error inconsistency fixes. > > The following patch series could probably be extended for all LSM to > benefit from these fixes: > https://lore.kernel.org/all/20240327120036.233641-1-mic@digikod.net/ > > Mikhail, according to your SCTP tests with SELinux, it looks like this > patch series should be updated, but that should be simple. > > Paul, what is the status of this LSM patch series? Could Mikhail > integrate this LSM patch (with the SCTP fix) as part of the current > Landlock patch series? This would help fixing the Landlock tests (which > check SCTP error consistency) when run with SELinux. > >> >>>> >>>>> >>>>>> - if (sock->type != SOCK_STREAM) >>>>>> + if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP) >>>>> >>>>> I think we should check sock->sk->sk_type instead of sock->type (even if >>>>> it should be the same). To make it simpler, we should only use sk in >>>>> current_check_access_socket(): >>>>> struct sock *sk = sock->sk; >>>> >>>> Agreed. >>>> >>>>> >>>>> Could you please also do s/__sk_common\.skc_/sk_/g ? >>>> >>>> Ofc >>>> >>>> Btw, there is probably incorrect read of skc_family in this function >>>> [1]. I'll add READ_ONCE for sk->sk_family. >>>> >>>> [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/ >>> >>> I think it should not be a bug with the current code (IPv6 -> IPV4, and >>> socket vs. sock) but we should indeed use READ_ONCE() (and add this link >>> to the commit message). >> >> ok >> >>> >>>> >>>>> >>>>>> return 0; >>>>>> /* Checks for minimal header length to safely read sa_family. */ >>>>>> -- >>>>>> 2.34.1 >>>>>> >>>>>> >>>> >>
On 10/7/2024 2:06 PM, Mikhail Ivanov wrote: > On 10/5/2024 6:49 PM, Mickaël Salaün wrote: >> On Fri, Oct 04, 2024 at 09:16:56PM +0300, Mikhail Ivanov wrote: >>> On 10/4/2024 1:13 PM, Mickaël Salaün wrote: >>>> On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote: >>>>> On 10/3/2024 8:45 PM, Mickaël Salaün wrote: >>>>>> Please also add Matthieu in Cc for the network patch series. >>>>>> >>>>>> On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote: >>>>>>> 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: https://github.com/landlock-lsm/linux/issues/40 >>>>>>> Fixes: fff69fb03dde ("landlock: Support network rules with TCP >>>>>>> bind and connect") >>>>>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> >>>>>>> --- >>>>>>> security/landlock/net.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c >>>>>>> index bc3d943a7118..6f59dd98bb13 100644 >>>>>>> --- a/security/landlock/net.c >>>>>>> +++ b/security/landlock/net.c >>>>>>> @@ -68,7 +68,7 @@ static int current_check_access_socket(struct >>>>>>> socket *const sock, >>>>>>> return -EACCES; >>>>>>> /* Checks if it's a (potential) TCP socket. */ >>>>>> >>>>>> We can extend this comment to explain that we don't use sk_is_tcp() >>>>>> because we need to handle the AF_UNSPEC case. >>>>> >>>>> Indeed, I'll do this. >>> >>> I've noticed that we still should check sk->sk_family = AF_INET{,6} >>> here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to >>> addresses and we should not provide any checks (for address) if socket >>> is unrestrictable (i.e. it's not TCP). It's not useful and might lead to >>> error incosistency for non-TCP sockets. >> >> Good catch, let's use sk_is_tcp(). >> >>> >>> Btw, I suppose we can improve error consistency by bringing more checks >>> from INET/TCP stack. For example it may be useful to return EISCONN >>> instead of EACCES while connect(2) is called on a connected socket. >> >> Yes, that would be nice (with the related tests). >> >>> >>> This should be done really carefully and only for some useful cases. >>> Anyway it's not related to the current patch (since it's not a bug). >> >> Sure. > > I have a little question to clarify before sending a next version. Are > we condisering order of network checks for error consistency? > > For example, in the current_check_access_socket() we have following > order of checks for ipv4 connect(2) action: > (1) addrlen < sizeof(struct sockaddr_in) -> return -EINVAL > (2) sa_family != sk_family -> return -EINVAL > > The ipv4 stack has a check for sock->state before (1) and (2), which can > return -EISCONN if the socket is already connected. > > This results in the possiblity of two following scenarios: > > Landlock enabled: > 1. socket(ipv4) -> OK > 2. connect(ipv4 address) -> OK > 3. connect(ipv6 address) -> -EINVAL (sa_family != sk_family) > > Landlock disabled: > 1. socket(ipv4) -> OK > 2. connect(ipv4 address) -> OK > 3. connect(ipv6 address) -> -EISCONN (socket is already connected) > > I have always considered the order of network checks as part of error > consistency, and I'd like to make sure that we're on the same page > before extending current patch with error inconsistency fixes. BTW, a similar inconsistency in the error order was also found in selinux hooks. Accounting [1], I wonder if validating socket state in security hooks for bind/connect actions has been considered before. [1] https://lore.kernel.org/all/20231228113917.62089-1-mic@digikod.net/
On Mon, Oct 07, 2024 at 02:58:43PM +0300, Mikhail Ivanov wrote: > On 10/7/2024 2:06 PM, Mikhail Ivanov wrote: > > On 10/5/2024 6:49 PM, Mickaël Salaün wrote: > > > On Fri, Oct 04, 2024 at 09:16:56PM +0300, Mikhail Ivanov wrote: > > > > On 10/4/2024 1:13 PM, Mickaël Salaün wrote: > > > > > On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote: > > > > > > On 10/3/2024 8:45 PM, Mickaël Salaün wrote: > > > > > > > Please also add Matthieu in Cc for the network patch series. > > > > > > > > > > > > > > On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote: > > > > > > > > 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: https://github.com/landlock-lsm/linux/issues/40 > > > > > > > > Fixes: fff69fb03dde ("landlock: Support network > > > > > > > > rules with TCP bind and connect") > > > > > > > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> > > > > > > > > --- > > > > > > > > security/landlock/net.c | 2 +- > > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/security/landlock/net.c b/security/landlock/net.c > > > > > > > > index bc3d943a7118..6f59dd98bb13 100644 > > > > > > > > --- a/security/landlock/net.c > > > > > > > > +++ b/security/landlock/net.c > > > > > > > > @@ -68,7 +68,7 @@ static int > > > > > > > > current_check_access_socket(struct socket *const > > > > > > > > sock, > > > > > > > > return -EACCES; > > > > > > > > /* Checks if it's a (potential) TCP socket. */ > > > > > > > > > > > > > > We can extend this comment to explain that we don't use sk_is_tcp() > > > > > > > because we need to handle the AF_UNSPEC case. > > > > > > > > > > > > Indeed, I'll do this. > > > > > > > > I've noticed that we still should check sk->sk_family = AF_INET{,6} > > > > here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to > > > > addresses and we should not provide any checks (for address) if socket > > > > is unrestrictable (i.e. it's not TCP). It's not useful and might lead to > > > > error incosistency for non-TCP sockets. > > > > > > Good catch, let's use sk_is_tcp(). > > > > > > > > > > > Btw, I suppose we can improve error consistency by bringing more checks > > > > from INET/TCP stack. For example it may be useful to return EISCONN > > > > instead of EACCES while connect(2) is called on a connected socket. > > > > > > Yes, that would be nice (with the related tests). > > > > > > > > > > > This should be done really carefully and only for some useful cases. > > > > Anyway it's not related to the current patch (since it's not a bug). > > > > > > Sure. > > > > I have a little question to clarify before sending a next version. Are > > we condisering order of network checks for error consistency? > > > > For example, in the current_check_access_socket() we have following > > order of checks for ipv4 connect(2) action: > > (1) addrlen < sizeof(struct sockaddr_in) -> return -EINVAL > > (2) sa_family != sk_family -> return -EINVAL > > > > The ipv4 stack has a check for sock->state before (1) and (2), which can > > return -EISCONN if the socket is already connected. > > > > This results in the possiblity of two following scenarios: > > > > Landlock enabled: > > 1. socket(ipv4) -> OK > > 2. connect(ipv4 address) -> OK > > 3. connect(ipv6 address) -> -EINVAL (sa_family != sk_family) > > > > Landlock disabled: > > 1. socket(ipv4) -> OK > > 2. connect(ipv4 address) -> OK > > 3. connect(ipv6 address) -> -EISCONN (socket is already connected) > > > > I have always considered the order of network checks as part of error > > consistency, and I'd like to make sure that we're on the same page > > before extending current patch with error inconsistency fixes. Yes, we should try to stick to the same error ordering, and this should be covered by tests. > > BTW, a similar inconsistency in the error order was also found in > selinux hooks. Accounting [1], I wonder if validating socket state > in security hooks for bind/connect actions has been considered before. > > [1] https://lore.kernel.org/all/20231228113917.62089-1-mic@digikod.net/ I think Landlock has a better test coverage than any other (access-control) LSM, which is why we find these inconsistencies. The LSM hooks should be better integrated into the network stack to benefit from all the inconsistency checks. On the other end, one benefit of being call earlier is that an LSM can stop invalid requests (I don't think it's worth it though). However, before trying to change the hook call sites, we should first make sure the side effects are OK for every LSMs: https://lore.kernel.org/all/20240327120036.233641-1-mic@digikod.net/ ...which also include testing (which is what we do for Landlock). Any though from the network folks?
diff --git a/security/landlock/net.c b/security/landlock/net.c index bc3d943a7118..6f59dd98bb13 100644 --- a/security/landlock/net.c +++ b/security/landlock/net.c @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock, return -EACCES; /* Checks if it's a (potential) TCP socket. */ - if (sock->type != SOCK_STREAM) + if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP) return 0; /* Checks for minimal header length to safely read sa_family. */
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: https://github.com/landlock-lsm/linux/issues/40 Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect") Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> --- security/landlock/net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)