| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 12 Sep 2023 09:33:05 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jordan Rife <jrife@...gle.com>,
davem@...emloft.net,
edumazet@...gle.com,
kuba@...nel.org,
pabeni@...hat.com,
netdev@...r.kernel.org
Cc: dborkman@...nel.org,
Jordan Rife <jrife@...gle.com>
Subject: Re: [PATCH net] net: prevent address overwrite in connect() and
sendmsg()
Jordan Rife wrote:
> commit 0bdf399342c5 ("net: Avoid address overwrite in kernel_connect")
> ensured that kernel_connect() will not overwrite the address parameter
> in cases where BPF connect hooks perform an address rewrite. However,
> there remain other cases where BPF hooks can overwrite an address held
> by a kernel client.
>
> ==Scenarios Tested==
>
> * Code in the SMB and Ceph modules calls sock->ops->connect() directly,
> allowing the address overwrite to occur. In the case of SMB, this can
> lead to broken mounts.
These should probably call kernel_connect instead.
> * NFS v3 mounts with proto=udp call sock_sendmsg() for each RPC call,
> passing a pointer to the mount address in msg->msg_name which is
> later overwritten by a BPF sendmsg hook. This can lead to broken NFS
> mounts.
Similarly, this could call kernel_sendmsg, and the extra copy handled
in that wrapper. The arguments are not exacty the same, so not 100%
this is feasible.
But it's preferable if in-kernel callers use the kernel_.. API rather
than bypass it. Exactly for issues like the one you report.
> In order to more comprehensively fix this class of problems, this patch
> pushes the address copy deeper into the stack and introduces an address
> copy to both udp_sendmsg() and udpv6_sendmsg() to insulate all callers
> from address rewrites.
>
> Signed-off-by: Jordan Rife <jrife@...gle.com>
> ---
> net/ipv4/af_inet.c | 18 ++++++++++++++++++
> net/ipv4/udp.c | 21 ++++++++++++++++-----
> net/ipv6/udp.c | 23 +++++++++++++++++------
> net/socket.c | 7 +------
> 4 files changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 3d2e30e204735..c37d484fbee34 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -568,6 +568,7 @@ int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr,
> {
> struct sock *sk = sock->sk;
> const struct proto *prot;
> + struct sockaddr_storage addr;
> int err;
>
> if (addr_len < sizeof(uaddr->sa_family))
> @@ -580,6 +581,14 @@ int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr,
> return prot->disconnect(sk, flags);
>
> if (BPF_CGROUP_PRE_CONNECT_ENABLED(sk)) {
> + if (uaddr && addr_len <= sizeof(addr)) {
> + /* pre_connect can rewrite uaddr, so make a copy to
> + * insulate the caller.
> + */
> + memcpy(&addr, uaddr, addr_len);
> + uaddr = (struct sockaddr *)&addr;
> + }
> +
> err = prot->pre_connect(sk, uaddr, addr_len);
> if (err)
> return err;
> @@ -625,6 +634,7 @@ int __inet_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> int addr_len, int flags, int is_sendmsg)
> {
> struct sock *sk = sock->sk;
> + struct sockaddr_storage addr;
> int err;
> long timeo;
>
> @@ -668,6 +678,14 @@ int __inet_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> goto out;
>
> if (BPF_CGROUP_PRE_CONNECT_ENABLED(sk)) {
> + if (uaddr && addr_len <= sizeof(addr)) {
> + /* pre_connect can rewrite uaddr, so make a copy to
> + * insulate the caller.
> + */
> + memcpy(&addr, uaddr, addr_len);
> + uaddr = (struct sockaddr *)&addr;
> + }
> +
> err = sk->sk_prot->pre_connect(sk, uaddr, addr_len);
> if (err)
> goto out;
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index f39b9c8445808..5f5ee2752eeb7 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1142,18 +1142,29 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> }
>
> if (cgroup_bpf_enabled(CGROUP_UDP4_SENDMSG) && !connected) {
> + struct sockaddr_in tmp_addr;
> + struct sockaddr_in *addr = usin;
> +
> + /* BPF_CGROUP_RUN_PROG_UDP4_SENDMSG_LOCK can rewrite usin, so make a
> + * copy to insulate the caller.
> + */
> + if (usin && msg->msg_namelen <= sizeof(tmp_addr)) {
> + memcpy(&tmp_addr, usin, msg->msg_namelen);
> + addr = &tmp_addr;
> + }
> +
> err = BPF_CGROUP_RUN_PROG_UDP4_SENDMSG_LOCK(sk,
> - (struct sockaddr *)usin, &ipc.addr);
> + (struct sockaddr *)addr, &ipc.addr);
> if (err)
> goto out_free;
> - if (usin) {
> - if (usin->sin_port == 0) {
> + if (addr) {
> + if (addr->sin_port == 0) {
> /* BPF program set invalid port. Reject it. */
> err = -EINVAL;
> goto out_free;
> }
> - daddr = usin->sin_addr.s_addr;
> - dport = usin->sin_port;
> + daddr = addr->sin_addr.s_addr;
> + dport = addr->sin_port;
> }
> }
>
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 86b5d509a4688..cbc1917fad629 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1506,26 +1506,37 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> fl6->fl6_sport = inet->inet_sport;
>
> if (cgroup_bpf_enabled(CGROUP_UDP6_SENDMSG) && !connected) {
> + struct sockaddr_in6 tmp_addr;
> + struct sockaddr_in6 *addr = sin6;
> +
> + /* BPF_CGROUP_RUN_PROG_UDP6_SENDMSG_LOCK can rewrite sin6, so make a
> + * copy to insulate the caller.
> + */
> + if (sin6 && addr_len <= sizeof(tmp_addr)) {
> + memcpy(&tmp_addr, sin6, addr_len);
> + addr = &tmp_addr;
> + }
> +
> err = BPF_CGROUP_RUN_PROG_UDP6_SENDMSG_LOCK(sk,
> - (struct sockaddr *)sin6,
> + (struct sockaddr *)addr,
> &fl6->saddr);
> if (err)
> goto out_no_dst;
> - if (sin6) {
> - if (ipv6_addr_v4mapped(&sin6->sin6_addr)) {
> + if (addr) {
> + if (ipv6_addr_v4mapped(&addr->sin6_addr)) {
> /* BPF program rewrote IPv6-only by IPv4-mapped
> * IPv6. It's currently unsupported.
> */
> err = -ENOTSUPP;
> goto out_no_dst;
> }
> - if (sin6->sin6_port == 0) {
> + if (addr->sin6_port == 0) {
> /* BPF program set invalid port. Reject it. */
> err = -EINVAL;
> goto out_no_dst;
> }
> - fl6->fl6_dport = sin6->sin6_port;
> - fl6->daddr = sin6->sin6_addr;
> + fl6->fl6_dport = addr->sin6_port;
> + fl6->daddr = addr->sin6_addr;
> }
> }
>
> diff --git a/net/socket.c b/net/socket.c
> index c8b08b32f097e..39794d026fa11 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -3570,12 +3570,7 @@ EXPORT_SYMBOL(kernel_accept);
> int kernel_connect(struct socket *sock, struct sockaddr *addr, int addrlen,
> int flags)
> {
> - struct sockaddr_storage address;
> -
> - memcpy(&address, addr, addrlen);
> -
> - return READ_ONCE(sock->ops)->connect(sock, (struct sockaddr *)&address,
> - addrlen, flags);
> + return READ_ONCE(sock->ops)->connect(sock, addr, addrlen, flags);
> }
> EXPORT_SYMBOL(kernel_connect);
>
> --
> 2.42.0.283.g2d96d420d3-goog
>
Powered by blists - more mailing lists