| Message ID | 20240327120036.233641-1-mic@digikod.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v1,1/2] lsm: Check and handle error priority for socket_bind and socket_connect | expand |
On 3/27/2024 5:00 AM, Mickaël Salaün wrote: > Because the security_socket_bind and the security_socket_bind hooks are > called before the network stack, it is easy to introduce error code > inconsistencies. Instead of adding new checks to current and future > LSMs, let's fix the related hook instead. The new checks are already > (partially) implemented by SELinux and Landlock, and it should not > change user space behavior but improve error code consistency instead. > > The first check is about the minimal sockaddr length according to the > address family. This improves the security of the AF_INET and AF_INET6 > sockaddr parsing for current and future LSMs. > > The second check is about AF_UNSPEC. This fixes error priority for bind > on PF_INET6 socket when SELinux (and potentially others) is enabled. > Indeed, the IPv6 network stack first checks the sockaddr length (-EINVAL > error) before checking the family (-EAFNOSUPPORT error). See commit > bbf5a1d0e5d0 ("selinux: Fix error priority for bind with AF_UNSPEC on > PF_INET6 socket"). > > The third check is about consistency between socket family and address > family. Only AF_INET and AF_INET6 are tested (by Landlock tests), so no > other protocols are checked for now. > > These new checks should enable to simplify current LSM implementations, > but we may want to first land this patch on all stable branches. > > A following patch adds new tests improving AF_UNSPEC test coverage for > Landlock. > > Cc: Alexey Kodanev <alexey.kodanev@oracle.com> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Günther Noack <gnoack@google.com> > Cc: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com> > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > Cc: Muhammad Usama Anjum <usama.anjum@collabora.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: Serge E. Hallyn <serge@hallyn.com> > Fixes: 20510f2f4e2d ("security: Convert LSM into a static interface") > Signed-off-by: Mickaël Salaün <mic@digikod.net> > --- > security/security.c | 96 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 96 insertions(+) > > diff --git a/security/security.c b/security/security.c > index 7e118858b545..64fe07a73b14 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -28,7 +28,9 @@ > #include <linux/xattr.h> > #include <linux/msg.h> > #include <linux/overflow.h> > +#include <linux/in.h> > #include <net/flow.h> > +#include <net/ipv6.h> > > /* How many LSMs were built into the kernel? */ > #define LSM_COUNT (__end_lsm_info - __start_lsm_info) > @@ -4415,6 +4417,82 @@ int security_socket_socketpair(struct socket *socka, struct socket *sockb) > } > EXPORT_SYMBOL(security_socket_socketpair); > > +static int validate_inet_addr(struct socket *sock, struct sockaddr *address, > + int addrlen, bool bind) > +{ > + const int sock_family = sock->sk->sk_family; > + > + /* Checks for minimal header length to safely read sa_family. */ > + if (addrlen < offsetofend(typeof(*address), sa_family)) > + return -EINVAL; > + > + /* Only handle inet sockets for now. */ > + switch (sock_family) { > + case PF_INET: > + case PF_INET6: > + break; > + default: > + return 0; > + } Seems like a clunky way to say: if (sock_family != PF_INET && sock_family != PF_INET6) return 0; > + > + /* Checks minimal address length for inet sockets. */ > + switch (address->sa_family) { > + case AF_UNSPEC: { > + const struct sockaddr_in *sa_in; > + > + /* Cf. inet_dgram_connect(), __inet_stream_connect() */ > + if (!bind) > + return 0; > + > + if (sock_family == PF_INET6) { > + /* Length check from inet6_bind_sk() */ > + if (addrlen < SIN6_LEN_RFC2133) > + return -EINVAL; > + > + /* Family check from __inet6_bind() */ > + goto err_af; > + } > + > + /* Length check from inet_bind_sk() */ > + if (addrlen < sizeof(struct sockaddr_in)) > + return -EINVAL; > + > + sa_in = (struct sockaddr_in *)address; > + if (sa_in->sin_addr.s_addr != htonl(INADDR_ANY)) > + goto err_af; > + > + return 0; > + } > + case AF_INET: > + /* Length check from inet_bind_sk() */ > + if (addrlen < sizeof(struct sockaddr_in)) > + return -EINVAL; > + break; > + case AF_INET6: > + /* Length check from inet6_bind_sk() */ > + if (addrlen < SIN6_LEN_RFC2133) > + return -EINVAL; > + break; > + } > + > + /* > + * Checks sa_family consistency to not wrongfully return -EACCES > + * instead of -EINVAL. Valid sa_family changes are only (from AF_INET > + * or AF_INET6) to AF_UNSPEC. > + */ > + if (address->sa_family != sock_family) > + return -EINVAL; > + > + return 0; > + > +err_af: > + /* SCTP services expect -EINVAL, others -EAFNOSUPPORT. */ > + if (sock->sk->sk_protocol == IPPROTO_SCTP) > + return -EINVAL; > + > + return -EAFNOSUPPORT; > +} > + > /** > * security_socket_bind() - Check if a socket bind operation is allowed > * @sock: socket > @@ -4425,11 +4503,23 @@ EXPORT_SYMBOL(security_socket_socketpair); > * and the socket @sock is bound to the address specified in the @address > * parameter. > * > + * For security reasons and to get consistent error code whatever LSM are > + * enabled, we first do the same sanity checks against sockaddr as the ones > + * done by the network stack (executed after hook). Currently only AF_UNSPEC, > + * AF_INET, and AF_INET6 are handled. Please add support for other family > + * specificities when handled by an LSM. > + * > * Return: Returns 0 if permission is granted. > */ > int security_socket_bind(struct socket *sock, > struct sockaddr *address, int addrlen) > { > + int err; > + > + err = validate_inet_addr(sock, address, addrlen, true); > + if (err) > + return err; > + > return call_int_hook(socket_bind, sock, address, addrlen); > } > > @@ -4447,6 +4537,12 @@ int security_socket_bind(struct socket *sock, > int security_socket_connect(struct socket *sock, > struct sockaddr *address, int addrlen) > { > + int err; > + > + err = validate_inet_addr(sock, address, addrlen, false); > + if (err) > + return err; The smack_socket_connect() code is among my ugliest. I don't think this would do any harm, but if you haven't run the Smack test suite you probably should. > + > return call_int_hook(socket_connect, sock, address, addrlen); > } >
On 3/27/2024 3:00 PM, Mickaël Salaün wrote: > Because the security_socket_bind and the security_socket_bind hooks are > called before the network stack, it is easy to introduce error code > inconsistencies. Instead of adding new checks to current and future > LSMs, let's fix the related hook instead. The new checks are already > (partially) implemented by SELinux and Landlock, and it should not > change user space behavior but improve error code consistency instead. It would probably be better to allow the network stack to perform such checks before calling LSM hooks. This may lead to following improvements: 1. Fixing extra checks. In the current design, (address)checks are performed both in validate_inet_addr() function and in network stack methods. 2. The network stack can choose which error cases should not be hidden during the LSM access check, and which ones can be. 3. LSM will not be responsible for performing all necessary checks for every (necessary) protocol. This may result in adding new method to socket->ops. > > The first check is about the minimal sockaddr length according to the > address family. This improves the security of the AF_INET and AF_INET6 > sockaddr parsing for current and future LSMs. > > The second check is about AF_UNSPEC. This fixes error priority for bind > on PF_INET6 socket when SELinux (and potentially others) is enabled. > Indeed, the IPv6 network stack first checks the sockaddr length (-EINVAL > error) before checking the family (-EAFNOSUPPORT error). See commit > bbf5a1d0e5d0 ("selinux: Fix error priority for bind with AF_UNSPEC on > PF_INET6 socket"). > > The third check is about consistency between socket family and address > family. Only AF_INET and AF_INET6 are tested (by Landlock tests), so no > other protocols are checked for now. > > These new checks should enable to simplify current LSM implementations, > but we may want to first land this patch on all stable branches. > > A following patch adds new tests improving AF_UNSPEC test coverage for > Landlock. > > Cc: Alexey Kodanev <alexey.kodanev@oracle.com> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Günther Noack <gnoack@google.com> > Cc: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com> > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > Cc: Muhammad Usama Anjum <usama.anjum@collabora.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: Serge E. Hallyn <serge@hallyn.com> > Fixes: 20510f2f4e2d ("security: Convert LSM into a static interface") > Signed-off-by: Mickaël Salaün <mic@digikod.net> > --- > security/security.c | 96 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 96 insertions(+) > > diff --git a/security/security.c b/security/security.c > index 7e118858b545..64fe07a73b14 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -28,7 +28,9 @@ > #include <linux/xattr.h> > #include <linux/msg.h> > #include <linux/overflow.h> > +#include <linux/in.h> > #include <net/flow.h> > +#include <net/ipv6.h> > > /* How many LSMs were built into the kernel? */ > #define LSM_COUNT (__end_lsm_info - __start_lsm_info) > @@ -4415,6 +4417,82 @@ int security_socket_socketpair(struct socket *socka, struct socket *sockb) > } > EXPORT_SYMBOL(security_socket_socketpair); > > +static int validate_inet_addr(struct socket *sock, struct sockaddr *address, > + int addrlen, bool bind) > +{ > + const int sock_family = sock->sk->sk_family; > + > + /* Checks for minimal header length to safely read sa_family. */ > + if (addrlen < offsetofend(typeof(*address), sa_family)) > + return -EINVAL; > + > + /* Only handle inet sockets for now. */ > + switch (sock_family) { > + case PF_INET: > + case PF_INET6: > + break; > + default: > + return 0; > + } > + > + /* Checks minimal address length for inet sockets. */ > + switch (address->sa_family) { > + case AF_UNSPEC: { > + const struct sockaddr_in *sa_in; > + > + /* Cf. inet_dgram_connect(), __inet_stream_connect() */ > + if (!bind) > + return 0; > + > + if (sock_family == PF_INET6) { > + /* Length check from inet6_bind_sk() */ > + if (addrlen < SIN6_LEN_RFC2133) > + return -EINVAL; > + > + /* Family check from __inet6_bind() */ > + goto err_af; > + } > + > + /* Length check from inet_bind_sk() */ > + if (addrlen < sizeof(struct sockaddr_in)) > + return -EINVAL; > + > + sa_in = (struct sockaddr_in *)address; > + if (sa_in->sin_addr.s_addr != htonl(INADDR_ANY)) > + goto err_af; > + > + return 0; > + } > + case AF_INET: > + /* Length check from inet_bind_sk() */ > + if (addrlen < sizeof(struct sockaddr_in)) > + return -EINVAL; > + break; > + case AF_INET6: > + /* Length check from inet6_bind_sk() */ > + if (addrlen < SIN6_LEN_RFC2133) > + return -EINVAL; > + break; > + } > + > + /* > + * Checks sa_family consistency to not wrongfully return -EACCES > + * instead of -EINVAL. Valid sa_family changes are only (from AF_INET > + * or AF_INET6) to AF_UNSPEC. > + */ > + if (address->sa_family != sock_family) > + return -EINVAL; > + > + return 0; > + > +err_af: > + /* SCTP services expect -EINVAL, others -EAFNOSUPPORT. */ > + if (sock->sk->sk_protocol == IPPROTO_SCTP) > + return -EINVAL; > + > + return -EAFNOSUPPORT; > +} > + > /** > * security_socket_bind() - Check if a socket bind operation is allowed > * @sock: socket > @@ -4425,11 +4503,23 @@ EXPORT_SYMBOL(security_socket_socketpair); > * and the socket @sock is bound to the address specified in the @address > * parameter. > * > + * For security reasons and to get consistent error code whatever LSM are > + * enabled, we first do the same sanity checks against sockaddr as the ones > + * done by the network stack (executed after hook). Currently only AF_UNSPEC, > + * AF_INET, and AF_INET6 are handled. Please add support for other family > + * specificities when handled by an LSM. > + * > * Return: Returns 0 if permission is granted. > */ > int security_socket_bind(struct socket *sock, > struct sockaddr *address, int addrlen) > { > + int err; > + > + err = validate_inet_addr(sock, address, addrlen, true); > + if (err) > + return err; > + > return call_int_hook(socket_bind, sock, address, addrlen); > } > > @@ -4447,6 +4537,12 @@ int security_socket_bind(struct socket *sock, > int security_socket_connect(struct socket *sock, > struct sockaddr *address, int addrlen) > { > + int err; > + > + err = validate_inet_addr(sock, address, addrlen, false); > + if (err) > + return err; > + > return call_int_hook(socket_connect, sock, address, addrlen); > } >
On Thu, Mar 28, 2024 at 11:11 AM Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com> wrote: > On 3/27/2024 3:00 PM, Mickaël Salaün wrote: > > Because the security_socket_bind and the security_socket_bind hooks are > > called before the network stack, it is easy to introduce error code > > inconsistencies. Instead of adding new checks to current and future > > LSMs, let's fix the related hook instead. The new checks are already > > (partially) implemented by SELinux and Landlock, and it should not > > change user space behavior but improve error code consistency instead. > > It would probably be better to allow the network stack to perform such > checks before calling LSM hooks. This may lead to following improvements: ... > This may result in adding new method to socket->ops. I don't think there is a "may result" here, it will *require* a new socket::proto_ops function (addr_validate?). If you want to pursue this with the netdev folks to see if they would be willing to adopt such an approach I think that would be a great idea. Just be warned, there have been difficulties in the past when trying to get any sort of LSM accommodations from the netdev folks.
On Fri, Mar 29, 2024 at 4:07 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Mar 28, 2024 at 11:11 AM Ivanov Mikhail > <ivanov.mikhail1@huawei-partners.com> wrote: > > On 3/27/2024 3:00 PM, Mickaël Salaün wrote: > > > Because the security_socket_bind and the security_socket_bind hooks are > > > called before the network stack, it is easy to introduce error code > > > inconsistencies. Instead of adding new checks to current and future > > > LSMs, let's fix the related hook instead. The new checks are already > > > (partially) implemented by SELinux and Landlock, and it should not > > > change user space behavior but improve error code consistency instead. > > > > It would probably be better to allow the network stack to perform such > > checks before calling LSM hooks. This may lead to following improvements: > > ... > > > This may result in adding new method to socket->ops. > > I don't think there is a "may result" here, it will *require* a new > socket::proto_ops function (addr_validate?). If you want to pursue > this with the netdev folks to see if they would be willing to adopt > such an approach I think that would be a great idea. Just be warned, > there have been difficulties in the past when trying to get any sort > of LSM accommodations from the netdev folks. [Dropping alexey.kodanev@oracle.com due to email errors (unknown recipient)] I'm looking at the possibility of doing the above and this is slowly coming back to me as to why we haven't seriously tried this in the past. It's not as simple as calling one level down into a proto_ops function table, the proto_ops function table would then need to jump down into an associated proto function table. Granted we aren't talking per-packet overhead, but I can see this having a measurable impact on connections/second benchmarks which likely isn't going to be a welcome change.
On Wed, Mar 27, 2024 at 8:00 AM Mickaël Salaün <mic@digikod.net> wrote: > > Because the security_socket_bind and the security_socket_bind hooks are > called before the network stack, it is easy to introduce error code > inconsistencies. Instead of adding new checks to current and future > LSMs, let's fix the related hook instead. The new checks are already > (partially) implemented by SELinux and Landlock, and it should not > change user space behavior but improve error code consistency instead. > > The first check is about the minimal sockaddr length according to the > address family. This improves the security of the AF_INET and AF_INET6 > sockaddr parsing for current and future LSMs. > > The second check is about AF_UNSPEC. This fixes error priority for bind > on PF_INET6 socket when SELinux (and potentially others) is enabled. > Indeed, the IPv6 network stack first checks the sockaddr length (-EINVAL > error) before checking the family (-EAFNOSUPPORT error). See commit > bbf5a1d0e5d0 ("selinux: Fix error priority for bind with AF_UNSPEC on > PF_INET6 socket"). > > The third check is about consistency between socket family and address > family. Only AF_INET and AF_INET6 are tested (by Landlock tests), so no > other protocols are checked for now. > > These new checks should enable to simplify current LSM implementations, > but we may want to first land this patch on all stable branches. [Dropping Alexey Kodanev due to email problems] This isn't something I would want to see backported to the various stable trees, this is a consolidation and cleanup for future work, not really a bugfix. If an individual LSM is currently missing an address sanity check that should be resolved with a targeted patch that can be safely backported without affecting other LSMs. Now, all that doesn't mean I don't think this is a good idea. Assuming we can't get the network stack to validate addresses before calling into these LSM hooks, I think this is an improvement over the current approach. I would like to see the patchset include individual patches which do the desired adjustments to the Smack, TOMOYO, AppArmor, Landlock, and SELinux code now that the sanity checks have migrated to the LSM layer. I expect that to be fairly straightforward, but given all the corner cases I want to make sure all the individual LSMs are okay with the changes. > A following patch adds new tests improving AF_UNSPEC test coverage for > Landlock. > > Cc: Alexey Kodanev <alexey.kodanev@oracle.com> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Günther Noack <gnoack@google.com> > Cc: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com> > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > Cc: Muhammad Usama Anjum <usama.anjum@collabora.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: Serge E. Hallyn <serge@hallyn.com> > Fixes: 20510f2f4e2d ("security: Convert LSM into a static interface") > Signed-off-by: Mickaël Salaün <mic@digikod.net> > --- > security/security.c | 96 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 96 insertions(+)
diff --git a/security/security.c b/security/security.c index 7e118858b545..64fe07a73b14 100644 --- a/security/security.c +++ b/security/security.c @@ -28,7 +28,9 @@ #include <linux/xattr.h> #include <linux/msg.h> #include <linux/overflow.h> +#include <linux/in.h> #include <net/flow.h> +#include <net/ipv6.h> /* How many LSMs were built into the kernel? */ #define LSM_COUNT (__end_lsm_info - __start_lsm_info) @@ -4415,6 +4417,82 @@ int security_socket_socketpair(struct socket *socka, struct socket *sockb) } EXPORT_SYMBOL(security_socket_socketpair); +static int validate_inet_addr(struct socket *sock, struct sockaddr *address, + int addrlen, bool bind) +{ + const int sock_family = sock->sk->sk_family; + + /* Checks for minimal header length to safely read sa_family. */ + if (addrlen < offsetofend(typeof(*address), sa_family)) + return -EINVAL; + + /* Only handle inet sockets for now. */ + switch (sock_family) { + case PF_INET: + case PF_INET6: + break; + default: + return 0; + } + + /* Checks minimal address length for inet sockets. */ + switch (address->sa_family) { + case AF_UNSPEC: { + const struct sockaddr_in *sa_in; + + /* Cf. inet_dgram_connect(), __inet_stream_connect() */ + if (!bind) + return 0; + + if (sock_family == PF_INET6) { + /* Length check from inet6_bind_sk() */ + if (addrlen < SIN6_LEN_RFC2133) + return -EINVAL; + + /* Family check from __inet6_bind() */ + goto err_af; + } + + /* Length check from inet_bind_sk() */ + if (addrlen < sizeof(struct sockaddr_in)) + return -EINVAL; + + sa_in = (struct sockaddr_in *)address; + if (sa_in->sin_addr.s_addr != htonl(INADDR_ANY)) + goto err_af; + + return 0; + } + case AF_INET: + /* Length check from inet_bind_sk() */ + if (addrlen < sizeof(struct sockaddr_in)) + return -EINVAL; + break; + case AF_INET6: + /* Length check from inet6_bind_sk() */ + if (addrlen < SIN6_LEN_RFC2133) + return -EINVAL; + break; + } + + /* + * Checks sa_family consistency to not wrongfully return -EACCES + * instead of -EINVAL. Valid sa_family changes are only (from AF_INET + * or AF_INET6) to AF_UNSPEC. + */ + if (address->sa_family != sock_family) + return -EINVAL; + + return 0; + +err_af: + /* SCTP services expect -EINVAL, others -EAFNOSUPPORT. */ + if (sock->sk->sk_protocol == IPPROTO_SCTP) + return -EINVAL; + + return -EAFNOSUPPORT; +} + /** * security_socket_bind() - Check if a socket bind operation is allowed * @sock: socket @@ -4425,11 +4503,23 @@ EXPORT_SYMBOL(security_socket_socketpair); * and the socket @sock is bound to the address specified in the @address * parameter. * + * For security reasons and to get consistent error code whatever LSM are + * enabled, we first do the same sanity checks against sockaddr as the ones + * done by the network stack (executed after hook). Currently only AF_UNSPEC, + * AF_INET, and AF_INET6 are handled. Please add support for other family + * specificities when handled by an LSM. + * * Return: Returns 0 if permission is granted. */ int security_socket_bind(struct socket *sock, struct sockaddr *address, int addrlen) { + int err; + + err = validate_inet_addr(sock, address, addrlen, true); + if (err) + return err; + return call_int_hook(socket_bind, sock, address, addrlen); } @@ -4447,6 +4537,12 @@ int security_socket_bind(struct socket *sock, int security_socket_connect(struct socket *sock, struct sockaddr *address, int addrlen) { + int err; + + err = validate_inet_addr(sock, address, addrlen, false); + if (err) + return err; + return call_int_hook(socket_connect, sock, address, addrlen); }
Because the security_socket_bind and the security_socket_bind hooks are called before the network stack, it is easy to introduce error code inconsistencies. Instead of adding new checks to current and future LSMs, let's fix the related hook instead. The new checks are already (partially) implemented by SELinux and Landlock, and it should not change user space behavior but improve error code consistency instead. The first check is about the minimal sockaddr length according to the address family. This improves the security of the AF_INET and AF_INET6 sockaddr parsing for current and future LSMs. The second check is about AF_UNSPEC. This fixes error priority for bind on PF_INET6 socket when SELinux (and potentially others) is enabled. Indeed, the IPv6 network stack first checks the sockaddr length (-EINVAL error) before checking the family (-EAFNOSUPPORT error). See commit bbf5a1d0e5d0 ("selinux: Fix error priority for bind with AF_UNSPEC on PF_INET6 socket"). The third check is about consistency between socket family and address family. Only AF_INET and AF_INET6 are tested (by Landlock tests), so no other protocols are checked for now. These new checks should enable to simplify current LSM implementations, but we may want to first land this patch on all stable branches. A following patch adds new tests improving AF_UNSPEC test coverage for Landlock. Cc: Alexey Kodanev <alexey.kodanev@oracle.com> Cc: Eric Dumazet <edumazet@google.com> Cc: Günther Noack <gnoack@google.com> Cc: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com> Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> Cc: Muhammad Usama Anjum <usama.anjum@collabora.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Serge E. Hallyn <serge@hallyn.com> Fixes: 20510f2f4e2d ("security: Convert LSM into a static interface") Signed-off-by: Mickaël Salaün <mic@digikod.net> --- security/security.c | 96 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+)