netdev - Re: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening socket

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 27 May 2015 10:11:16 +0200
From:	Daniel Borkmann <daniel@...earbox.net>
To:	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	Neil Horman <nhorman@...driver.com>
CC:	linux-sctp@...r.kernel.org, Vlad Yasevich <vyasevich@...il.com>,
	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH] sctp: Fix mangled IPv4 addresses on a IPv6 listening
 socket

On 05/27/2015 01:30 AM, Jason Gunthorpe wrote:
> sctp_v4_map_v6 was subtly writing and reading from members
> of a union in a way the clobbered data it needed to read before

s/the/that/

> it read it.
>
> Zeroing the v6 flowinfo overwrites the v4 sin_addr with 0, meaning
> that every place that calls sctp_v4_map_v6 gets ::ffff:0.0.0.0 as the
> result.
>
> Reorder things to guarantee correct behaviour no matter what the
> union layout is.
>
> This impacts user space clients that open an IPv6 SCTP socket and
> receive IPv4 connections. Prior to 299ee user space would see a
> sockaddr with AF_INET and a correct address, after 299ee the sockaddr
> is AF_INET6, but the address is wrong.
>
> Fixes: 299ee123e198 (sctp: Fixup v4mapped behaviour to comply with Sock API)
> Signed-off-by: Jason Gunthorpe <jgunthorpe@...idianresearch.com>
> ---
>   include/net/sctp/sctp.h | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> This bugfix should be a candidate for -stable
>
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 856f01cb51dd..230775f5952a 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -571,11 +571,14 @@ static inline void sctp_v6_map_v4(union sctp_addr *addr)
>   /* Map v4 address to v4-mapped v6 address */
>   static inline void sctp_v4_map_v6(union sctp_addr *addr)
>   {
> +	__be16 port;
> +
> +	port = addr->v4.sin_port;
> +	addr->v6.sin6_addr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
> +	addr->v6.sin6_port = port;
>   	addr->v6.sin6_family = AF_INET6;
>   	addr->v6.sin6_flowinfo = 0;
>   	addr->v6.sin6_scope_id = 0;
> -	addr->v6.sin6_port = addr->v4.sin_port;
> -	addr->v6.sin6_addr.s6_addr32[3] = addr->v4.sin_addr.s_addr;

Change looks good to me. You actually would only need to address
the issue where the v4.sin_addr is copied before you overwrite/zero
the flowinfo as only these two overlap in the union. Given that
sockaddr_in and sockaddr_in6 are exported to uapi, they are immutable,
but I see the point that union sctp_addr is kernel private - bad
things happen if v4/v6 don't overlap the way anymore as they'd do
now. ;)

Anyways:

Acked-by: Daniel Borkmann <daniel@...earbox.net>

>   	addr->v6.sin6_addr.s6_addr32[0] = 0;
>   	addr->v6.sin6_addr.s6_addr32[1] = 0;
>   	addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html