| Message ID | 28947A39-55C4-4C68-8421-DEC950CF7963@apple.com |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [libnetfilter_queue] src: fix IPv6 header handling | expand |
Hi Etan, On Wed, Jan 13, 2021 at 10:58:52AM +0100, Etan Kissling wrote: > diff --git a/src/extra/ipv6.c b/src/extra/ipv6.c > index 42c5e25..1eb822f 100644 > --- a/src/extra/ipv6.c > +++ b/src/extra/ipv6.c > @@ -72,7 +72,8 @@ int nfq_ip6_set_transport_header(struct pkt_buff *pktb, struct ip6_hdr *ip6h, Note: nfq_ip6_set_transport_header() is very much similar to ipv6_skip_exthdr() in the Linux kernel, see net/ipv6/exthdrs_core.c > uint32_t hdrlen; > > /* No more extensions, we're done. */ > - if (nexthdr == IPPROTO_NONE) { > + if (nexthdr == IPPROTO_TCP || nexthdr == IPPROTO_UDP || nexthdr == IPPROTO_ESP || > + nexthdr == IPPROTO_ICMPV6 || nexthdr == IPPROTO_NONE) { > cur = NULL; > break; > } > @@ -107,7 +108,7 @@ int nfq_ip6_set_transport_header(struct pkt_buff *pktb, struct ip6_hdr *ip6h, > } else if (nexthdr == IPPROTO_AH) > hdrlen = (ip6_ext->ip6e_len + 2) << 2; > else > - hdrlen = ip6_ext->ip6e_len; > + hdrlen = (ip6_ext->ip6e_len + 1) << 3; This looks correct, IPv6 optlen is miscalculated. The chunk above to stop the iteration, so I think the chunk that fixes optlen is sufficient to fix the bug. Thanks.
On 09.02.21, 17:33, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote: Thanks for looking into this. > Note: nfq_ip6_set_transport_header() is very much similar to > ipv6_skip_exthdr() in the Linux kernel, see net/ipv6/exthdrs_core.c I submitted a revised version that uses similar logic as in the kernel to exit the loop once extension headers are fully processed. > > uint32_t hdrlen; > > > > /* No more extensions, we're done. */ > > - if (nexthdr == IPPROTO_NONE) { > > + if (nexthdr == IPPROTO_TCP || nexthdr == IPPROTO_UDP || nexthdr == IPPROTO_ESP || > > + nexthdr == IPPROTO_ICMPV6 || nexthdr == IPPROTO_NONE) { > > cur = NULL; > > break; > > } > > @@ -107,7 +108,7 @@ int nfq_ip6_set_transport_header(struct pkt_buff *pktb, struct ip6_hdr *ip6h, > > } else if (nexthdr == IPPROTO_AH) > > hdrlen = (ip6_ext->ip6e_len + 2) << 2; > > else > > - hdrlen = ip6_ext->ip6e_len; > > + hdrlen = (ip6_ext->ip6e_len + 1) << 3; > > This looks correct, IPv6 optlen is miscalculated. > > The chunk above to stop the iteration, so I think the chunk that fixes > optlen is sufficient to fix the bug. The optlen fix is not sufficient when a non-existent 'target' is given. For example, if a UDP packet is passed with 'target' IPPROTO_TCP, the loop will go on and attempt to interpret the UDP packet body as another IP extension header. Instead, by exiting the loop also when all extension headers are processed, the function will now return 0 in that case, as documented in the header file. Thanks Etan
diff --git a/src/extra/ipv6.c b/src/extra/ipv6.c index 42c5e25..1eb822f 100644 --- a/src/extra/ipv6.c +++ b/src/extra/ipv6.c @@ -72,7 +72,8 @@ int nfq_ip6_set_transport_header(struct pkt_buff *pktb, struct ip6_hdr *ip6h, uint32_t hdrlen; /* No more extensions, we're done. */ - if (nexthdr == IPPROTO_NONE) { + if (nexthdr == IPPROTO_TCP || nexthdr == IPPROTO_UDP || nexthdr == IPPROTO_ESP || + nexthdr == IPPROTO_ICMPV6 || nexthdr == IPPROTO_NONE) { cur = NULL; break; } @@ -107,7 +108,7 @@ int nfq_ip6_set_transport_header(struct pkt_buff *pktb, struct ip6_hdr *ip6h, } else if (nexthdr == IPPROTO_AH) hdrlen = (ip6_ext->ip6e_len + 2) << 2; else - hdrlen = ip6_ext->ip6e_len; + hdrlen = (ip6_ext->ip6e_len + 1) << 3; nexthdr = ip6_ext->ip6e_nxt; cur += hdrlen;
This corrects issues in IPv6 header handling that sometimes resulted in an endless loop. Signed-off-by: Etan Kissling <etan_kissling@apple.com> --- src/extra/ipv6.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)