On Thu, Jun 21, 2018 at 04:31:48PM +0200, Florian Westphal wrote:
> Máté Eckl <ecklm94@xxxxxxxxx> wrote:
> > > This looks like its subtly broken, inherited from xt_TPROXY.
> > > Above skb_header_pointer uses sizeof(udphdr) only, but nf_tproxy_get_sock_v4
> > > assumes it gets tcphdr (it checks th->doff, and that might be garbage).
> >
> > I thought about why iptables uses udphdr consequently and I think it does
> > because we do not nead other than source and destination address and port which
> > is part of the udp header too at the same position.
>
> It does for LISTEN case, see __tcp_hdrlen() usage in
> nf_tproxy_get_sock_v4.
>
> > I think they paid attention to this. nf_tproxy_get_sock_v4 treats that pointer
> > as a tcphdr indeed, but it only uses tcp-related attributes and functions if ip
> > protocol is IPPROTO_TCP, so what you described does not happen with an udp
> > packet.
>
> It doesn't happen with an udp packet. But in case tcp header wasn't in
> linar area (skb->data), but in pagefrags (or split), it will be copied
> by skb_header_pointer to __udphdr (on stack), so in that case we then
> get out-of-bounds read access.
>
> Hence my suggestion to remove 'hp' arg and repeat skb_header_pointer()
> call with struct tcphdr.
Ok, I made the patch. Do you agree with this commit message? This linearisation
thing is not clear to me yet.
netfilter: Refactor nf_tproxy_get_sock_v{4,6}
This patch fixes a silent out-of-bound read possibility that was present
because of the misuse of this function.
Mostly it was called with a struct udphdr *hp which had only the udphdr
part linearized by the skb_header_pointer, however the
nf_tproxy_get_sock_v{4,6} uses it as a tcphdr pointer, so some reads for
tcp specific flags may be invalid.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
