dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.47k stars 4.76k forks source link

IPNetwork.Contains for IPv4 networks should match IPv4-Mapped IPv6 Addresses #98427

Closed hacst closed 5 months ago

hacst commented 9 months ago

Background and motivation

Dual stack applications in .NET see connecting IPv4 addresses as IPv4-Mapped IPv6 Addresses. This is problematic when combined with the IPNetworks.Contains function specifying IPv4 networks. E.g. if I want to let the user provide a CIDR string for disallowing access I might write something like:

var disallowed = IPNetwork.Parse(disallowedCidrString);
if (disallowed.Contains(remoteIp)) {
  // Deny
} else {
  // Allow
}

As the user wants to deny an IPv4 range they write something like "10.0.0.8/8". However this creates problems because it will produce an IPv4 Network that will not match the dual stack Ipv4 mapped ipv6 address.

My interpretation is that the IPv4-Mapped IPv6 Address is actually an IPv4 address and hence should be matchable using a IPv4 IPNetwork. Under this assumption a change to the IPNetwork.Contains implementation as proposed here could prevent such confusions.

ps: I was unsure whether reporting this as an API Suggestion is correct as this does not change the visible API. However as it would be an API Behavior change it seemed to be the most fitting category. Let me know if this was incorrect.

API Proposal

No visible changes except maybe to documentation. My suggestion would be to make the Contains function use IPAddress.IsIPv4MappedToIPv6 to identify that it is actually passed an IPv4 address to make contains behave as expected.

API Usage


var address = IPAddress.Parse("::ffff:10.0.0.12");
var network = IPNetwork.Parse("10.0.0.0/8");

Debug.Assert(network.Contains(address));

Alternative Designs

Handling this as a developer without the proposed change certainly is possible by inspecting the address and network at the point of comparison to work around the existing behavior. However it has to be done carefully to get it right. E.g. simply trying to always match using a IPv6 network would fail if the server configuration was changed to bind only to IPv4.

Risks

Existing usage might in some way expect IPv6 addresses to never get be reported as being contained in IPv4 networks. I cannot think of actual use-case for this though.

ghost commented 9 months ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation Dual stack applications in .NET see connecting IPv4 addresses as [IPv4-Mapped IPv6 Addresses](https://datatracker.ietf.org/doc/html/rfc4291#section-2.5.5.2). This is problematic when combined with the IPNetworks.Contains function specifying IPv4 networks. E.g. if I want to let the user provide a CIDR string for disallowing access I might write something like: ```cs var disallowed = IPNetwork.Parse(disallowedCidrString); if (disallowed.Contains(remoteIp)) { // Deny } else { // Allow } ``` As the user wants to deny an IPv4 range they write something like "10.0.0.8/8". However this creates problems because it will produce an IPv4 Network that will not match the dual stack Ipv4 mapped ipv6 address. My interpretation is that the IPv4-Mapped IPv6 Address is actually an IPv4 address and hence should be matchable using a IPv4 IPNetwork. Under this assumption a change to the IPNetwork.Contains implementation as proposed here could prevent such confusions. ps: I was unsure whether reporting this as an API Suggestion is correct as this does not change the visible API. However as it would be an API Behavior change it seemed to be the most fitting category. Let me know if this was incorrect. ### API Proposal No visible changes except maybe to documentation. My suggestion would be to make the Contains function use [IPAddress.IsIPv4MappedToIPv6](https://learn.microsoft.com/de-de/dotnet/api/system.net.ipaddress.isipv4mappedtoipv6?view=net-8.0) to identify that it is actually passed an IPv4 address to make contains behave as expected. ### API Usage ```csharp var address = IPAddress.Parse("::ffff:10.0.0.12"); var network = IPNetwork.Parse("10.0.0.0/8"); Debug.Assert(network.Contains(address)); ``` ### Alternative Designs Handling this as a developer without the proposed change certainly is possible by inspecting the address and network at the point of comparison to work around the existing behavior. However it has to be done carefully to get it right. E.g. simply trying to always match using a IPv6 network would fail if the server configuration was changed to bind only to IPv4. ### Risks Existing usage might in some way expect IPv6 addresses to never get be reported as being contained in IPv4 networks. I cannot think of actual use-case for this though.
Author: hacst
Assignees: -
Labels: `api-suggestion`, `area-System.Net`
Milestone: -
antonfirsov commented 9 months ago

Apart from being a breaking change, the proposed behavior would contradict the semantics of IPAddress.Equals and the difference between the two methods could be counter-intuitive to some users. Another option to address this is to introduce a separate overload, eg. Contains(IPAddress, bool) or Contains(IPAddress, SomeComparisonMethodSelectorEnum).

@wfurt @MihaZupan thoughts/preferences?

hacst commented 9 months ago

That makes a lot of sense. Especially if it allows having something similar for IPAddress equality comparisons.

wfurt commented 9 months ago

IPAddress has IPAddress.MapToIPv6 but IPNetwork does not. If somebody wants to treat dual mode same as IPv4 it seems easy for the IPAddress.Equals. Maybe we could use some helpers for handling the duality.

MihaZupan commented 9 months ago

IMO users aren't generally creating IPv4-mapped IPv6 addresses themselves, this is more of an implementation detail of dual mode sockets. E.g. someone connects to your Kestrel server over IPv4, but IPNetwork.Contains(HttpContext.Connection.RemoteIpAddress) fails because it's mapped to IPv6.

From a usability perspective, I would prefer if we treated such addresses as IPv4 for the purposes of IPNetwork.Contains. I think we should consider the breaking change to the existing overload.

antonfirsov commented 9 months ago

IPAddress has IPAddress.MapToIPv6 but IPNetwork does not. If somebody wants to treat dual mode same as IPv4 it seems easy for the IPAddress.Equals. Maybe we could use some helpers for handling the duality.

I don't know what helpers did you mean, but I think it's easier to map the IPAddress to be tested with Contains(address), than the IPNetwork. But now I tend to agree with OP and https://github.com/dotnet/runtime/issues/98427#issuecomment-1947390225, and I think we should consider changing the default behavior in 9.0. If we decide not to do it, we should document it providing examples for the mapping.

wfurt commented 9 months ago

I'm ok if we choose to change the behavior @antonfirsov. But I wanted to explore other options as well if we are concerned about compatibiliyty.