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.
--
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
