On Thu, Oct 13, 2016 at 21:43 +0200, Markus Friedl wrote: > > > Am 13.10.2016 um 13:06 schrieb Christian Weisgerber <[email protected]>: > > > >> After the second m_makespace(): > >> > >> +------+-----+ +------+ +--------+-----+ > >> | IPv6 | ESP | ---- | IPv6 | ---- | ICMPv6 | ESP | > >> +------+-----+ +------+ +--------+-----+ > >> > >> With m_inject(), it would instead be something like this: > >> > >> +------+ +-----+ +------+ +-------- > >> | IPv6 |----| ESP | ---- | IPv6 | ---- | ICMPv6 ... > >> +------+ +-----+ +------+ +-------- > > > > Found it. It's this snippet of nd6_ns_output() that handles those > > mbuf chains differently: > > > > 454 if (ln && ln->ln_hold) { > > 455 hip6 = mtod(ln->ln_hold, struct ip6_hdr *); > > 456 /* XXX pullup? */ > > 457 if (sizeof(*hip6) < ln->ln_hold->m_len) > > 458 saddr6 = &hip6->ip6_src; > > 459 else > > 460 saddr6 = NULL; > > 461 } else > > 462 saddr6 = NULL; > > > > Did this only ever work by accident? > > ok, to get it right, the following is the difference: > > with m_inject() the first mbuf always contains the 40 byte ipv6 header > while with m_makespace() it also contains the ESP header. > > so with m_inject() the ln_hold->m_len is 40 and since this is > exactly the size of hip6, the code falls back to saddr6 = NULL. > > IMHO the code should use <= and not <: > if (sizeof(*hip6) <= ln->ln_hold->m_len) > but then your example will also fail with the old m_inject() code. >
I agree with the above. > If this intended address selection is indeed correct then we > need to figure out if a bypass flow for NDP is necessary, or > if NDP should always bypass IPsec (but what about bringing NDP > over IPsec?) > There are apparently some discussions in infomational RFCs regarding this issue. For instance https://tools.ietf.org/html/rfc3756 states: More specifically, the currently used key agreement protocol, IKE, suffers from a chicken-and-egg problem [8]: one needs an IP address to run IKE, IKE is needed to establish IPsec SAs, and IPsec SAs are required to configure an IP address. Which goes one step further: how to protect all ND in general, but is still applicable in our situation. There were attempts to protect ND in alternative way, e.g. SEND (https://tools.ietf.org/html/rfc3971). FreeBSD has picked up on it and has had a SoC project which seems to be integrated right now: https://wiki.freebsd.org/SOC2009AnaKukec https://www.freebsd.org/cgi/man.cgi?query=send&sektion=4 Would it be possible for us to disable the check and always set saddr6 to NULL for now? > With IPv4 this problem does not exist, because ARP packet are not IP > packets, so they are not matched by the IPsec flow. > > -m
