Page MenuHomeFreeBSD

ICMPv6: fix redirects containing global addresses
ClosedPublic

Authored by vangyzen on Feb 7 2018, 8:44 PM.

Details

Summary

Lots of sanity checks are done on ICMP6 redirect messages. One of
these is to ensure that the source address in the ip6 header matches
the next-hop gateway address of the destination in the redirect
payload. The problem is that the source address in the ip6 header
will have its scope id embedded in it but the next-hop address will
not if the target is a global address. For globals, the scope id
is explicitly stripped.

The fix is to grab the interface index from the interface pointer
passed in through the header mbuf, and stuff that back into the
next-hop gateway.

Test Plan

This fixes 35 UNH IPv6 conformance test cases.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vangyzen created this revision.Feb 7 2018, 8:44 PM

This fixes the test cases, but is it the best fix, or should this be fixed in some other way?

Note to self: SCOS-47387

ae added a comment.Feb 7 2018, 9:18 PM

The description looks confusing to me. As I understand, the problem is that fib6_lookup_nh_basic() returns next hop address without embedded scope zone id, so when it is IPv6 link-local address, the IN6_ARE_ADDR_EQUAL() comparison will fail due to src6 has this zone id embedded in.

sys/netinet6/icmp6.c
2313 ↗(On Diff #39022)

I think this check is redundant, because in6_setscope() will do it again.
And the comment looks wrong. I suggest something like:

/*
  * Embed scope zone id into next hop address, since fib6_lookup_nh_basic() returns
  * address without embedded scope zone id.
  */
In D14254#298820, @ae wrote:

The description looks confusing to me. As I understand, the problem is that fib6_lookup_nh_basic() returns next hop address without embedded scope zone id, so when it is IPv6 link-local address, the IN6_ARE_ADDR_EQUAL() comparison will fail due to src6 has this zone id embedded in.

Your understanding is correct.

vangyzen added inline comments.Feb 7 2018, 10:11 PM
sys/netinet6/icmp6.c
2313 ↗(On Diff #39022)

This makes sense.

vangyzen updated this revision to Diff 39028.Feb 7 2018, 10:14 PM
  • review feedback
ae accepted this revision.Feb 7 2018, 10:40 PM
This revision is now accepted and ready to land.Feb 7 2018, 10:40 PM
melifaro accepted this revision.Feb 7 2018, 11:03 PM

Thanks for the review, guys. I'll commit after I re-test the final change.

If you have time, I would appreciate your comments on D14257, too.

dab accepted this revision.Feb 7 2018, 11:38 PM
This revision was automatically updated to reflect the committed changes.