Repository: corelight/pycommunityid

Issue:
FlowTupleError port invalid for specific ports
Hi, I started to experiment with community ID and pycommunityid and I think that I found a bug in function in_nbo():
https://github.com/corelight/pycommunityid/blob/b4467350446dde632eef59004a6b4e49cc55a85f/communityid/algo.py#L194-L213

- The problem is with the creation of the new FlowTuple at the end of function,
- exception will occur if sport or dport is from range 11569 - 11577:
`communityid.error.FlowTupleError: Destination port "b'-1'" invalid`

You can test it with your sample application:
`$ community-id tcp 10.0.0.1 10.0.0.2 10 11569`

Number 11569 in hex is 0x2D31 and that is '-1' in ASCII. I think the problem is with this line in function is_port(val):
https://github.com/corelight/pycommunityid/blob/b4467350446dde632eef59004a6b4e49cc55a85f/communityid/algo.py#L249

- bytes value of number 11569 is decoded as -1 and that is wrong
- I think other big port numbers can be problematic because they can be represented as ASCII characters too
    - port with number 14392 is 0x3838 in hex and 88 in ASCII

I hope somebody will check this bug and will found a solution.


Discussion:

Patch:
diff --git a/README.md b/README.md
index 2613ecc..241bfee 100644
--- a/README.md
+++ b/README.md
@@ -5,7 +5,7 @@ This package provides a Python implementation of the open
 [Community ID](https://github.com/corelight/community-id-spec)
 flow hashing standard.
 
-It supports Python versions 2.7+ and 3+.
+It supports Python versions 2.7+ (for not much longer) and 3+.
 
 ![example foobar](https://github.com/corelight/pycommunityid/actions/workflows/python.yaml/badge.svg)
 
diff --git a/communityid/algo.py b/communityid/algo.py
index 2b38e35..0876e05 100644
--- a/communityid/algo.py
+++ b/communityid/algo.py
@@ -57,9 +57,10 @@ class FlowTuple:
         representations.
 
         The sport and dport arguments are numeric port numbers, either
-        provided as ints or in packed 16-bit network byte order. When
-        the protocol number is one of PORT_PROTOS (TCP, UDP, etc),
-        they are required. For other IP protocols they are optional.
+        provided as ints or in packed 16-bit network byte order, of
+        type "bytes". When the protocol number is one of PORT_PROTOS
+        (TCP, UDP, etc), they are required. For other IP protocols
+        they are optional.
 
         The optional Boolean is_one_way argument indicates whether the
         tuple captures a bidirectional flow (the default) or
@@ -245,17 +246,15 @@ class FlowTuple:
 
     @staticmethod
     def is_port(val):
-        try:
-            port = int(val)
-            return 0 <= port <= 65535
-        except ValueError:
-            pass
+        if isinstance(val, bytes):
+            try:
+                port = struct.unpack('!H', val)[0]
+                return 0 <= port <= 65535
+            except (struct.error, IndexError, TypeError):
+                pass
 
-        try:
-            port = struct.unpack('!H', val)[0]
-            return 0 <= port <= 65535
-        except (struct.error, IndexError, TypeError):
-            pass
+        if isinstance(val, int):
+            return 0 <= val <= 65535
 
         return False
 
diff --git a/scripts/community-id b/scripts/community-id
index 3f3cf54..43d1104 100755
--- a/scripts/community-id
+++ b/scripts/community-id
@@ -39,10 +39,14 @@ class DefaultParser(TupleParser):
         sport, dport = None, None
 
         if num_parts == 5:
-            if (not communityid.FlowTuple.is_port(parts[3]) or
-                not communityid.FlowTuple.is_port(parts[4])):
+            try:
+                sport, dport = int(parts[3]), int(parts[4])
+            except ValueError:
+                return None, 'Could not parse port numbers'
+
+            if (not communityid.FlowTuple.is_port(sport) or
+                not communityid.FlowTuple.is_port(dport)):
                 return None, 'Could not parse port numbers'
-            sport, dport = int(parts[3]), int(parts[4])
 
         try:
             return communityid.FlowTuple(
@@ -65,15 +69,20 @@ class ZeekLogsParser(TupleParser):
         if proto is None:
             return None, 'Could not parse IP protocol number'
 
+        try:
+            sport, dport = int(parts[1]), int(parts[3])
+        except ValueError:
+            return None, 'Could not parse port numbers'
+
         if not (communityid.FlowTuple.is_ipaddr(parts[0]) and
-                communityid.FlowTuple.is_port(parts[1]) and
+                communityid.FlowTuple.is_port(sport) and
                 communityid.FlowTuple.is_ipaddr(parts[2]) and
-                communityid.FlowTuple.is_port(parts[3])):
+                communityid.FlowTuple.is_port(dport)):
             return None, 'Need two IP addresses and port numbers'
 
         try:
-            return communityid.FlowTuple(proto, parts[0], parts[2],
-                                         int(parts[1]), int(parts[3])), None
+            return communityid.FlowTuple(
+                proto, parts[0], parts[2], sport, dport), None
         except communityid.FlowTupleError as err:
             return None, repr(err)
 


Test patch:
diff --git a/tests/communityid_test.py b/tests/communityid_test.py
index 3655d56..f7bddee 100755
--- a/tests/communityid_test.py
+++ b/tests/communityid_test.py
@@ -231,6 +231,12 @@ class TestCommunityID(unittest.TestCase):
                  '1:LQU9qZlK+B5F3KDmev6m5PMibrg=',
                  '1:2d053da9994af81e45dca0e67afea6e4f3226eb8',
                  '1:3V71V58M3Ksw/yuFALMcW0LAHvc='],
+
+                # Verify https://github.com/corelight/pycommunityid/issues/3
+                ['10.0.0.1', '10.0.0.2', 10, 11569,
+                 '1:SXBGMX1lBOwhhoDrZynfROxnhnM=',
+                 '1:497046317d6504ec218680eb6729df44ec678673',
+                 '1:HmBRGR+fUyXF4t8WEtal7Y0gEAo='],
             ],
             communityid.FlowTuple.make_tcp,
             communityid.PROTO_TCP,
@@ -310,6 +316,14 @@ class TestCommunityID(unittest.TestCase):
             tpl = communityid.FlowTuple(
                 communityid.PROTO_TCP, '1.2.3.4', '5.6.7.8')
 
+    @unittest.skipIf(sys.version_info[0] < 3, 'not supported in Python 2.x')
+    def test_inputs_py3(self):
+        # Python 3 allows us to distinguish strings and byte sequences,
+        # and the following test only applies to it.
+        with self.assertRaises(communityid.FlowTupleError):
+            tpl = communityid.FlowTuple(
+                communityid.PROTO_TCP, '1.2.3.4', '5.6.7.8', 23, "80")
+
     def test_get_proto(self):
         self.assertEqual(communityid.get_proto(23), 23)
         self.assertEqual(communityid.get_proto("23"), 23)