On Fri, Jun 22, 2018 at 06:24:51PM +0200, Florian Westphal wrote:
> Máté Eckl <ecklm94@xxxxxxxxx> wrote:
> > 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.
>
> Yes, looks ok.
>
> sk_buff references packet data in two ways:
>
> 1. linear part, this referenced via skb->head and skb->data (head points
> to start of buffer, skb->data is moved around as headers are
> added/removed).
>
> 2. nonlinar part. This is referenced via skb_shinfo area, and points to
> pages of memory, i.e. an skb can be made up of dozens of pages.
>
> It will typically be both for packets aggregated via GRO (receive
> offloading) or built on transmit.
>
> In this particular case (LISTEN lookup, we'll normally always see 1)
> only, which explains why this wasn't noticed before, in case of 1)
> current code is fine.
>
> In second case, accessing skb->data[bignum] may cause out-of-bounds read,
> as the data might reside in the "non-linear area", i.e. skb->head memory
> is much smaller.
>
> One has to use the 'big hammer', skb_linearize(), to force reallocation
> of skb->head + copy of all pages' contents into it (very expensive,
> might not work if memory is fragmented and allocation request is large),
> or pskb_may_pull() with the amount of bytes you expect to be accessible
> via skb->head[] (doesn't do anything if its already accessible, or
> causes reallocation of skb->head if its too small, but will only 'pull'
> all pages, just whatever was requested).
>
> To get access to a (small) header, you can also use
> skb_header_pointer(), which never reallocates skb->head.
>
> If the requested access is already ok (because requested start offset +
> size is accessible via skb->head), it just returns a pointer to the
> offset in the linear area.
>
> If not, it copies that requested size into the buffer that gets passed
> to the function, and returns a pointer to that buffer rather than to
> skb->head region.
>
> In this case, we asked for sizeof(udphdr).
> If sizeof(tcphdr) (yes, tcphdr) would be ok/in linear area, everything
> is fine, the returned udp-header pointer can be re-used/treated like
> tcp.
>
> But in the second case, the pointer returned could be the address of the
> _udphdr stack-buffer.
> It would be fine if only accessing those tcp fields that are <=
> sizeof(udphdr), but tcphdr->doff isn't within that region.
>
> If you're interested in the gory details, look at skb_header_pointer and
> pskb_may_pull() implementations.
Thanks very much for the extensive description.
--
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
