Infinite loop when parsing packet

vitalis89 commented 2 years ago

I found a scenario when https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/utils.py#L34 leads to an infinite loop. The example I provide here is an edge-case scenario when some parts of the packet are filled with zero bytes, probably because of packet loss.

The bug is in the constructor of V9ExportPacket, specifically, the infinite loop occurs because the function stuck in the while loop https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L489 Below is an example for such a packet, an explanation of why the loop never finishes, and a proposed solution:

The packet (UDP data):

b'\x00\t\x00\x05{r\xe80b\x0bq}x\xcf4\xe3\x01\x02\x00\x00\x01\x04\x00H\x00\x00\x00\x00\x00\x00\x00n\x00\x00\x00\x01\x01\x00\x00\x00\x00\n \x07j\x06\x06\\\x08\x00\x00\rk\x15\xc8\x00\x00\x00\x0b{r\xe4H{r\xe4H\x08\x00\x00\x00\x00\x00\x00\x048\xbfd\x01\xc7e\xad\x1e\rk\x15\xc8\x00\x00\x00\x00\x00\x01\x04\x00H\x00\x00\x00\x00\x00\x00\x00g\x00\x00\x00\x01\x11\x00\x00\xc9Q\xac\x18\x0b\x03\x06\x06\\\x08\x005\x01\x00\x00\x01\x00\x00\x00\x0b{r\xe4H{r\xe4H\x00\x00\x00\x00\x00\x00\x00\x04C\x17{\x01\xc7e\xad\xa5\x01\x00\x00\x01\xc3\x9c\x005\x00\x01\x04\x00H\x00\x00\x00\x00\x00\x00\x00J\x00\x00\x00\x01\x01\x00\x00\x00\x00\xac\x19\xbc2\x06\x06\\\x08\x00\x00(pH\xcd\x00\x00\x00\x0b{r\xe80{r\xe80\x08\x00\x00\x00\x00\x00\x00\x04\x8f\x07\x1a\x01\xc7e\xaeB(pH\xcd\x00\x00\x00\x00\x00\x01\x04\x00H\x00\x00\x00\x00\x00\x00\x00F\x00\x00\x00\x01\x06\x00\x02\xcb\xef\n0h\x1f\x06\x06\\\x08\x01\xbb\x14*I\x18\x00\x00\x00\x0b{r\xe80{r\xe80\x00\x00\x00\x00\x00\x00\x00\x04\x80\x1cx\x01\xc7e\xae\x1d\x14*I\x18Z+\x01\xbb\x00\x01\x04\x00H\x00\x00\x00\x00\x00\x00\x00F\x00\x00\x00\x01\x06\x00\x02\xfe\x80\n/`\x12\x06\x06\\\x08\x01\xbb\x14*I\x18\x00\x00\x00\x0b{r\xe80{r\xe80\x00\x00\x00\x00\x00\x00\x00\x04\x08\x06\xb0\x01\xc7e\xae(\x14*I\x18\xd4\xb5\x01\xbb\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'

A Demonstration of the Infinite Loop

Most of this packet consists of zero bytes. It's a V9ExportPacket https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L472 (starts with \x00\t) At the start of the constructor, the V9Header https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L454 is calculated. The length https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L457 is constant in the code so the offset https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L486 starts with 20. If we follow the code by using the above payload we can see that pack https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L490 is always equal to (260, 72). So flowset_id https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L491 equals 260 and the following piece of code is executed:

# Data / option flowsets
# First, check if template is known
if flowset_id not in self._templates:
    # Could not be parsed, continue to check for templates
    skipped_flowsets_offsets.append(offset)
    offset += flowset_length
    continue

Specifically, 260 doesn't appear in self._templates as this key is missing from V9_FIELD_TYPES https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L24 which means that https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L520 is executed, so offset is increased by 72. This is performed 5 times, so eventually offset equals 20 + 5 * 72 = 380. Starting from this offset all remaining bytes of data are zeros (\x00). So https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L490 is (0, 0) and the following piece of code is executed:

# Data template flowsets
if flowset_id == 0:  # TemplateFlowSet always have id 0
    tfs = V9TemplateFlowSet(data[offset:])
    # Update the templates with the provided templates, even if they are the same
    for id_, template in tfs.templates.items():
        if id_ not in self._templates:
            self._new_templates = True
        self._templates[id_] = template
    offset += tfs.length
    continue

The constructor of V9TemplateFlowSet also calculates pack and sets length to pack[1] which is 0. The offset here starts from 4. Since the condition https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L425 is false the loop doesn't even start, so we get V9TemplateFlowSet with empty templates. Hence the loop over templates https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L498 is skipped, https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L502 is executed and offset remains 380. The loop continues and since offset never grows, it will repeat infinitely.

Proposed Solution

The solution I propose is to add a check right after https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L502 to ensure that the offset grew. If it remains the same, break the loop. We can also add a safety that checks data[offset:] right after the main loop condition https://github.com/bitkeks/python-netflow-v9-softflowd/blob/87c1bfb89245d89ef80aefb8efb583ffcb37c5af/netflow/v9.py#L489 condition, and breaks if set(data[offset:]) == {0} (rest of data is all zeros).

Would like to get your opinion about the bug and the solution I propose.

bitkeks commented 2 years ago

Hey @vitalis89 ! Wow, thank you so much for your extensive research and solution! I'll have a look into it the next days and proceed with your MR.

bitkeks commented 2 years ago

You are absolutely right about the missing checks for zeros. This might happen at different other struct.unpack as well, I fear. Nevertheless, your finding happens inside a loop, so we should remove this possible bug.

Did you have a setup where zero'ed packets were created? If there's a connection loss, wouldn't the UDP packet simply be shorter?

bitkeks commented 2 years ago

Fixed in b8e911a40af83a93b02d8dbdd87854510d00a623 and 8b5675913decf25ef6278a9111a1b1a7311d8863

bitkeks / python-netflow-v9-softflowd