slayers: unmap IPv4-mapped IPv6 addresses

[oncilla]

The Go standard library can produce IPv4-mapped IPv6 addresses when resolving IP addresses. These IP addresses need to be unmapped before putting them on the wire.

Before this patch, we could observe the following with tshark:

Len=1304 SCION 1-ff00:0:110,[::ffff:172.20.2.2] -> 1-ff00:0:111,[::ffff:172.20.3.2] UDP 32769 -> 32768 1208

The regression was introduced in #4346, which removed the unmapping behavior in slayers.PackAddr. This patch restores the behavior to ensure only unmapped IPv4 addresses make it on the wire.

---

This change is 

[marcfrei]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf and @oncilla)

---

pkg/slayers/scion.go line 409 at r1 (raw file):

			t = T16Ip
		}
		return t, ip.AsSlice(), nil

With the change above the following would now maybe look a little nicer:

if ip.Is6() {
	return T16Ip, ip.AsSlice(), nil
}
return T4Ip, ip.AsSlice(), nil

Code quote:

		t := T4Ip
		if ip.Is6() {
			t = T16Ip
		}
		return t, ip.AsSlice(), nil

[oncilla]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @marcfrei and @matzf)

---

pkg/slayers/scion.go line 409 at r1 (raw file):

Previously, marcfrei (Marc Frei) wrote…

With the change above the following would now maybe look a little nicer:

if ip.Is6() {
	return T16Ip, ip.AsSlice(), nil
}
return T4Ip, ip.AsSlice(), nil

I simplified the logic a bit more.
Unwrap can be called on everything, even invalid IP addresses.

[marcfrei]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf and @oncilla)

---

pkg/slayers/scion.go line 409 at r1 (raw file):

Previously, oncilla (Dominik Roos) wrote…

I simplified the logic a bit more.
Unwrap can be called on everything, even invalid IP addresses.

Indeed. This reads even nicer.

[marcfrei]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matzf)

[matzf]

Ouch. This is obviously not good, and my mistake in #4346.
IMO the fix is in the wrong place now though. If we want to put in a 4in6 address as a destination address, this should be possible at this layer.
The bug is that unmapped addresses are created and passed as destination address here, likely the result of a direct conversation from a net.IP using netip.AddrFromSlice. In particular, the call in snet/writer.go should unmap the IP address that comes out of UDPAddr.Host. Perhaps we need to revisit all netip.AddrFromSlice calls.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

[oncilla]

Hm, I guess you have a point given that slayers should just be about encoding/decoding.

I just assumed that there is no valid use case where you would put a mapped IPv4 address on the wire, given they should never be used as source or destination of IPv6 packets.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

[matzf]

Ah interesting, I was not aware that these addresses are never supposed to be used on the wire (note: specified in RFC 4213, nodes SHOULD silently discard packets with IPv4-mapped IPv6 source addresses and a few other addresses).

Ideally we'd fix the callers to do the unmapping and raise an error for 4in6 addresses in PackAddr -- we could even raise the error and allow SetSrc/DstAddr to continue, so that slayers could still be used to create bogus packets if you really, really want.

The difference is (almost) only semantics, so if you think this is too much effort, I'm also ok with keeping this as it is now.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

[oncilla]

I'm not 100% sure how we need to adjust the code to handle dual-stack correctly in a dispatcher-less world.
I think the reason we even get the mapped IPv4 addresses is because we are technically in a dual-stacked world.

I guess unmapping the address in snet before invoking slayers should be safe. But I'm not sure if we can do this broadly for all the netip.AddrFromSlice calls. I think some of them need to keep this information (e.g. next hop needs to keep being mapped in a dual stack env AFAIU, but here I lack knowledge)

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

[matzf]

I guess unmapping the address in snet before invoking slayers should be safe. But I'm not sure if we can do this broadly for all the netip.AddrFromSlice calls

Ah, sure, I didn't mean to unmap this in for netip.AddrFromSlice calls. I meant to check all of the calls. Those that will be turned into a SCION address should be unmapped. The ones that are underlay or other local IP connectivity, we'd leave "mapped" (guessing that this is the right thing to do there).

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

[oncilla]

Ack. I vetted some locations where the netip.AddrFromSlice is called. But they are so distant from the place where it is passed to snet and in turn to slayers that it seems very easy to forget to do the unmap.

We could do it in snet, but implementations not using snet can easily run into this issue.
I have a feeling that we save us and the users of the slayers package a lot of trouble by doing the unmapping in slayers.

I wonder if we should have a follow-up PR that validates in the router that we do not forward mapped addresses too?

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

[matzf]

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

[matzf]

I wonder if we should have a follow-up PR that validates in the router that we do not forward mapped addresses too?

Created #4415