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

Helpers::ipMatch not working correctly #61

Closed
Valicek1 opened this issue Jun 10, 2015 · 3 comments

Comments

Projects
None yet
4 participants
@Valicek1
Copy link

commented Jun 10, 2015

Hi, after some testing, i figured out, that ipMatch function is not working correctly.

Example:
Network 192.168.1.0/24 (IPs: 192.168.1.0-192.168.1.255)
ipMatch(192.168.1.1, 192.168.1.0/24) -> true, great
ipMatch(192.168.2.1, 192.168.1.0/24) -> true, what isn't nice.

Even if tests are passing:

Assert::true( Helpers::ipMatch('192.168.68.233', '192.168.68.233') ); 

woks fine

Assert::false( Helpers::ipMatch('192.168.68.234', '192.168.68.233') );

works fine

Assert::true( Helpers::ipMatch('192.168.64.0', '192.168.68.233/12') );

Network 192.168.68.233/12 (IPs: 192.160.0.1-192.175.255.255)
fine, it's in subnet

Assert::false( Helpers::ipMatch('192.168.63.255', '192.168.68.233/12') );

not fine, even in subnet, but returns false

Assert::true( Helpers::ipMatch('192.168.79.254', '192.168.68.233/12') );

fine, in subnet

Assert::false( Helpers::ipMatch('192.168.80.0', '192.168.68.233/12') );

in subnet, but returns false

Assert::true( Helpers::ipMatch('127.0.0.1', '192.168.68.233/32') );

not sure, if this should return true

Maybe I missunderstood something, but this is how Mikrotik and Cisco devices are interpreting CIDR network.

Maybe, the logic of function is working backwards...

real expected addresses
/0 /32 1
/1 /31 2
/2 /30 4
/3 /29 8
... ... ...
/12 /20 4096

If I use 192.168.68.233/20 (192.168.64.0-192.168.79.255), it matches expected behaviour as seen in tests..

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2015

cc @milo seems like the guy who may know something about CIDR and subnets


PR with proposed fix

JanTvrdik added a commit to JanTvrdik/nette-http that referenced this issue Jun 10, 2015

@milo

This comment has been minimized.

Copy link
Member

commented Jun 10, 2015

If someone interested in theory... CIDR is a short notation of IP and Netmask. It defines subnet (first and last IP in some range).

IP: 192.168.2.9   (so called network (actually, to be exact, it is some IP from network))
NM: 255.255.128.0 (called netmask)

in binary:
IP: 11000000 10101000 00000010 00001001 (192.168.2.9)
NM: 11111111 11111111 10000000 00000000 (255.255.128.0)
    ^ do you see this 17 ones? That's /17

so CIDR notation is:
192.168.2.9/17

How to find, if some IP is in defined subnet? Let's say, we are testing IP 192.168.210.9:

isInSubnet = (IP & NM) === (192.168.210.9 & NM)

In binary:

IP: 11000000 10101000 00000010 00001001 (192.168.2.9)
NM: 11111111 11111111 10000000 00000000 (255.255.128.0)
---------------------------- make AND -
    11000000 10101000 00000000 00000000 (192.168.0.0) <- this is called network
                       ^ effect is, it trims ones from there

    11000000 10101000 11010010 00001001 (192.168.210.9)
NM: 11111111 11111111 10000000 00000000 (255.255.128.0)
---------------------------- make AND -
    11000000 10101000 10000000 00000000 (192.168.128.0) <- this is called network

Is the 192.168.0.0 === 192.168.128.0? No, so it's not in subnet.

IPv4 has 32 bits, so IPv4 subnet can be from /0 to /32.
IPv6 has 128 bits, so IPv6 subnet can be from /0 to /128.

Support for IP addresses handling in PHP is weak and support for 128 bit integers too. That's why using 'binary strings' comparison.

I can check the fix later this evening.

@dg

This comment has been minimized.

Copy link
Member

commented Jun 10, 2015

I have always wondered why the mask in IPv4 is counted backwards than in IPv6. Now I wonder how it came to me...

@dg dg closed this in ea94d28 Jun 10, 2015

dg added a commit that referenced this issue Jun 10, 2015

Merge pull request #62 from JanTvrdik/ip_match_fix
Helpers: fixed ipMatch() for IPv4 [closes #61]

dg added a commit to nette/nette that referenced this issue Jun 18, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.