Fix issue #359: Bin to str conversion of IPv6 addresses #431

0xb35c · 2016-12-31T14:15:34Z

Starting over with my PR (old one was #395)
I hope this is better now (regarding rebase instead of merge)
Should fix issue #359 and also includes regression tests. When compressing the notation of an IPv6 address it is according to RFC 5952

A note on the implementation: I separated my fix into a own function for better unit-testing.

codecov-io · 2016-12-31T14:41:38Z

Current coverage is 67.63% (diff: 85.71%)

Merging #431 into master will increase coverage by 0.15%

@@             master       #431   diff @@
==========================================
  Files           103        103          
  Lines         25372      25365     -7   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          17121      17156    +35   
+ Misses         8251       8209    -42   
  Partials          0          0

Diff Coverage	File Path
•••••••• 85%	scapy/pton_ntop.py

Powered by Codecov. Last update 0d401eb...c459de9

p-l- · 2017-01-01T18:26:28Z

scapy/pton_ntop.py

+    if len(addr) != 16:
+        raise Exception("Illegal syntax for IP address")
+    parts = []
+    for left in [0, 2, 4, 6, 8, 10, 12, 14]:


Why do you create a list instead of for left in xrange(0, 16, 2):?

p-l- · 2017-01-01T18:33:11Z

scapy/pton_ntop.py

+def _ipv6_bin_to_str(addr):
+    # IPv6 addresses have 128bits (16 bytes)
+    if len(addr) != 16:
+        raise Exception("Illegal syntax for IP address")


Since the address is in binary form, maybe "Illegal IPv6 address" would be better?
Or even better, we should mimic the behavior of socket.inet_ntop:

raise ValueError("invalid length of packed IP address string")

p-l- · 2017-01-01T18:34:06Z

scapy/pton_ntop.py

+        try:
+            value = struct.unpack("!H", addr[left:left + 2])[0]
+            hexstr = hex(value)[2:]
+        except TypeError:


I'm not sure to understand: where would a TypeError come from here?

p-l- · 2017-01-01T18:35:24Z

scapy/pton_ntop.py

-        return result
+        parts.append(hexstr.lower())
+
+    address = ":".join(parts)


This whole block (starting with parts = []) could be replaced by an easier to read and more Pythonic:

address = ":".join(addr[idx:idx + 2].encode('hex').lstrip('0') or '0' for idx in xrange(0, 16, 2))

p-l- · 2017-01-01T18:36:09Z

scapy/pton_ntop.py

+    matches = re.findall('(?::|^)(0(?::0)+)(?::|$)', address)
+    if matches:
+        # If multiple consecutive blocks have the same length, take the leftmost
+        match = max(matches)


Don't you men match = max(matches, key=len)?

p-l- · 2017-01-01T18:54:29Z

scapy/pton_ntop.py

+    address = ":".join(parts)
+
+    # Find all consecutive zero blocks
+    matches = re.findall('(?::|^)(0(?::0)+)(?::|$)', address)


Using re.findall() produces a (useless) list, plus you lose the regexp match object (which has interesting attributes for what you want to do).
I think the code starting from here to the end of the function can be rewritten as:

try: match = max(re.finditer('(?::|^)(0(?::0)+)(?::|$)', address), key=lambda x: x.end() - x.start()) except ValueError: return address return '%s::%s' % (address[:match.start()], address[match.end():])

I have not tested that code, however, so it might require some adjustments.

Also, the regular expression should be defined and compiled as a global constant:

_IP6_ZEROS = re.compile('(?::|^)(0(?::0)+)(?::|$)')

Then

try: match = max(_IP6_ZEROS.finditer(address), key=lambda x: x.end() - x.start()) except ValueError: return address return '%s::%s' % (address[:match.start()], address[match.end():])

p-l- · 2017-01-01T18:58:48Z

scapy/pton_ntop.py

@@ -77,25 +77,53 @@ def inet_ntop(af, addr):
        except AttributeError:
            pass

-        # IPv6 addresses have 128bits (16 bytes)
-        if len(addr) != 16:
+        return _ipv6_bin_to_str(addr)


Why don't you move this call to the except block?

p-l- · 2017-01-01T19:01:16Z

test/regression.uts

@@ -6879,3 +6879,66 @@ x = f.i2repr(mp, {'*', '+', 'bit 2'})
 assert(re.match(r'^.*Star \(\*\).*$', x) is not None)
 assert(re.match(r'^.*Plus \(\+\).*$', x) is not None)
 assert(re.match(r'^.*bit 2.*$', x) is not None)
+
+###########################################################################################################


The tests sound good to me, but I think you should call inet_ntop and let Scapy call either socket.inet_ntop or _ipv6_bin_to_str(), so that we really test how Scapy will behave. What do you think?

The reason for that is that I'm able to test my code and not the code of socket.inet_ntop() (also that is the reason that I extracted a separate function)
Do you see a better way to achieve this?

Sorry I missed that. You're right. But maybe you could double the tests? Like that:

compressed1, compressed2 = _ipv6_bin_to_str(address), inet_ntop(socket.AF_INET6, address) assert compressed1 == compressed2 == '::'

What do you think?

0xb35c · 2017-01-03T19:27:34Z

Thanks for your suggestions, @p-l-
The code is also reduced by a few lines. Still not sure how to better peform the tests. Should I call both _ipv6_bin_to_str() and inet_ntop? Depending on the availibilty of socket.inet_ntop _ipv6_bin_to_str() would never be called.

p-l-

Thanks a lot! I still have some remarks (sorry about that).

p-l- · 2017-01-05T17:46:33Z

scapy/pton_ntop.py

@@ -10,7 +10,9 @@
 without IPv6 support, on Windows for instance.
 """

-import socket,struct
+import socket,struct,re


Can you rewrite this on separate lines? Also, PEP-8 requires a space after ,.

import re import socket import struct

p-l- · 2017-01-05T17:49:23Z

test/regression.uts

@@ -6879,3 +6879,66 @@ x = f.i2repr(mp, {'*', '+', 'bit 2'})
 assert(re.match(r'^.*Star \(\*\).*$', x) is not None)
 assert(re.match(r'^.*Plus \(\+\).*$', x) is not None)
 assert(re.match(r'^.*bit 2.*$', x) is not None)
+
+###########################################################################################################


Sorry I missed that. You're right. But maybe you could double the tests? Like that:

compressed1, compressed2 = _ipv6_bin_to_str(address), inet_ntop(socket.AF_INET6, address) assert compressed1 == compressed2 == '::'

What do you think?

p-l- · 2017-01-05T17:50:08Z

test/regression.uts

+from scapy.pton_ntop import _ipv6_bin_to_str
+address=b'\x10\x00\x02\x00\x00\x30\x00\x04\x00\x05\x00\x60\x07\x00\x80\x00' # Leading zero suppression
+compressed = _ipv6_bin_to_str(address)
+assert(compressed == '1000:200:30:4:5:60:700:8000')


Please add a newline at the end of your file (yeah I know I'm picky ;))

p-l- · 2017-01-05T17:53:22Z

scapy/pton_ntop.py

+    try:
+        # Get the longest set of zero blocks
+        # Actually we need to take a look at group 1 regarding the length as 0:0:1:0:0:2:3:4 would have two matches:
+        # 0:0: and :0:0: where the latter is longer, though the first one should be taken. Group 1 is in both cases 0:0.


Nice catch and good comment! 👍

0xb35c · 2017-01-09T12:00:10Z

@p-l- I found why the travis build failes. On Mac OS it seems that
socket.inet_ntop() compresses 1111:2222:3333:4444:5555:6666:0000:8888 to 1111:2222:3333:4444:5555:6666::8888 which is valid, but not RFC 5952 compliant. (See here) To be RFC 5952 compliant the result should be 1111:2222:3333:4444:5555:6666:0:8888 (note the explicit zero).

As I wouldn't consider this an issue of scapy I'd modify the regression tests to accept the alternative representation. What do you think?

p-l- · 2017-01-09T13:36:43Z

@0xb35c it's OK to accept the "wrong" MacOS solution, but add a comment to explain that while it's not RFC-compliant, it's the result given under MacOS.

Handle ipv6 addresses according to RFC 5952 Add regression tests

Better message for exception

PEP-8 compliance

…on Mac OS

0xb35c · 2017-01-13T11:13:34Z

@p-l- I don't know of you get notified of new commits here. So this is just a friendly notification from me ;)

0xb35c mentioned this pull request Dec 31, 2016

Fix issue #359: properly handle ipv6 adresses with multiple zero blocks #395

Closed

p-l- requested changes Jan 1, 2017

View reviewed changes

0xb35c force-pushed the issue_#359 branch from 3c2b9d0 to e640bba Compare January 3, 2017 18:05

p-l- requested changes Jan 5, 2017

View reviewed changes

0xb35c force-pushed the issue_#359 branch from e640bba to 1db52c5 Compare January 6, 2017 10:22

Benjamin added 4 commits January 10, 2017 19:59

Fix issue secdev#359

f25f641

Handle ipv6 addresses according to RFC 5952 Add regression tests

Code optimization

d2b43f6

Better message for exception

Improve regression tests

3f710fb

PEP-8 compliance

Modify regression test as socket.inet_pton is not RFC 5952 compliant …

c459de9

…on Mac OS

0xb35c force-pushed the issue_#359 branch from 1db52c5 to c459de9 Compare January 10, 2017 19:01

p-l- merged commit fcb62e7 into secdev:master Jan 18, 2017

0xb35c mentioned this pull request Jan 20, 2017

Zero block in IPv6 route prefix misinterpreted #359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #359: Bin to str conversion of IPv6 addresses #431

Fix issue #359: Bin to str conversion of IPv6 addresses #431

0xb35c commented Dec 31, 2016 •

edited

Loading

codecov-io commented Dec 31, 2016 •

edited

Loading

p-l- Jan 1, 2017

p-l- Jan 1, 2017

p-l- Jan 1, 2017

p-l- Jan 1, 2017

p-l- Jan 1, 2017

p-l- Jan 1, 2017 •

edited

Loading

p-l- Jan 2, 2017

p-l- Jan 1, 2017

p-l- Jan 1, 2017

0xb35c Jan 2, 2017

p-l- Jan 5, 2017

0xb35c commented Jan 3, 2017

p-l- left a comment

p-l- Jan 5, 2017

p-l- Jan 5, 2017

p-l- Jan 5, 2017

p-l- Jan 5, 2017

0xb35c commented Jan 9, 2017

p-l- commented Jan 9, 2017

0xb35c commented Jan 13, 2017

Fix issue #359: Bin to str conversion of IPv6 addresses #431

Fix issue #359: Bin to str conversion of IPv6 addresses #431

Conversation

0xb35c commented Dec 31, 2016 • edited Loading

codecov-io commented Dec 31, 2016 • edited Loading

Current coverage is 67.63% (diff: 85.71%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-l- Jan 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xb35c commented Jan 3, 2017

p-l- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xb35c commented Jan 9, 2017

p-l- commented Jan 9, 2017

0xb35c commented Jan 13, 2017

0xb35c commented Dec 31, 2016 •

edited

Loading

codecov-io commented Dec 31, 2016 •

edited

Loading

p-l- Jan 1, 2017 •

edited

Loading