Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent IPNetwork.Contains behavior for prefixes which are not at start of subnet range. #6674

Closed
pavel12345 opened this issue Jan 14, 2019 · 2 comments · Fixed by #31573
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware bug This issue describes a behavior which is not expected - a bug. good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue
Milestone

Comments

@pavel12345
Copy link

Describe the bug

In AspNetCore/src/Middleware/HttpOverrides/src/IPNetwork.cs

The code below assumes that the prefix will have all bits in position > mask length set to 0. I do not believe that this a requirement for CIDR notation. If it is, it is not applied consistently since the routine will break if the mask is == 0 at a given byte.

        for (int i = 0; i < PrefixBytes.Length && Mask[i] != 0; i++)
        {
            if (PrefixBytes[i] != (addressBytes[i] & Mask[i]))
            {
                return false;
            }
        }

To Reproduce

Create instance with Prefix/Prefix Length 192.168.0.1/31.
Contains returns false for 192.168.0.0.

Expected behavior

Expectation is that 192.168.0.1/31 would match 192.168.0.0

@pavel12345 pavel12345 changed the title IPNetwork.Contains does not match non-zero subnets. Inconsistent IPNetwork.Contains behavior for prefixes not ending in 0. Jan 14, 2019
@pavel12345 pavel12345 changed the title Inconsistent IPNetwork.Contains behavior for prefixes not ending in 0. Inconsistent IPNetwork.Contains behavior for prefixes which are not at start of subnet range. Jan 14, 2019
@Tratcher
Copy link
Member

@analogrelay analogrelay added bug This issue describes a behavior which is not expected - a bug. and removed triage-review labels Jul 24, 2019
@analogrelay analogrelay added this to the Backlog milestone Jul 24, 2019
@davidfowl davidfowl added help wanted Up for grabs. We would accept a PR to help resolve this issue good first issue Good for newcomers. labels Mar 29, 2021
@fvoronin
Copy link
Contributor

fvoronin commented Apr 3, 2021

The problem that, because here

public bool Contains(IPAddress address)
{
if (Prefix.AddressFamily != address.AddressFamily)
{
return false;
}
var addressBytes = address.GetAddressBytes();
for (int i = 0; i < PrefixBytes.Length && Mask[i] != 0; i++)
{
if (PrefixBytes[i] != (addressBytes[i] & Mask[i]))
{
return false;
}
}
return true;
}
does not correctly determine if the host address is in the same subnet:

if (PrefixBytes[i] != (addressBytes[i] & Mask[i])) 

We need to get a subnet address for both addresses (for initial address - PrefixBytes, and for the address to be checked - addressBytes) and compare the octets of those subnet addresses.

Also I think it would be nice to add a check for the prefixLength parameter in the constructor for going beyond the range 0-32 for IPv4 and 0-128 for IPv6.

@Tratcher Tratcher modified the milestones: Backlog, 6.0-preview4 Apr 6, 2021
@ghost ghost closed this as completed in #31573 Apr 6, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 6, 2021
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware and removed area-runtime labels Jun 2, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware bug This issue describes a behavior which is not expected - a bug. good first issue Good for newcomers. help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
8 participants