Error with created Connection for chained decoders

[amm3]

Pull #99 created a case where, if a decoder (such as xor) internally creates a Connection (e.g. for use with downstream chained decoders), the new object's nextoffset member references NoneType values instead of integers. And within the context of normal chained decoder operation, there is no condition wherein IPHandler would be called with a SYN flag to establish the natural starting offsets.

Prior to #99, the default values were 0, so the internal object would function, albeit with artificial sequence numbers.

My proposed solution is to manually (in the decoder) set the starting values of nextoffset to match the values of the "parent" Connection.

Note: This condition doesn't impact other chainable decoders (such as country filters or grep) because those decoders don't create a new Connection object. They simply select which connections to pass downstream vs not.

For reference, the following error was observed running xor+followstream, leading to the identification of this bug:

ARNING:xor:unsupported operand type(s) for +: 'NoneType' and 'int'
WARNING:xor:unsupported operand type(s) for +: 'NoneType' and 'int'
WARNING:xor:unsupported operand type(s) for +: 'NoneType' and 'int'

[amm3]

Hey guys, just curious if there's an issue with this PR. Let me know if you need clarification or see a better direction to go. Thanks!

[dev195]

Sorry, I don't know how this fell off of our plate!

The change you made didn't seem to work for me. I think the problem is that the Connection does not have the offsets set until the first packet is seen. As a result, copying the offsets from conn to xorconn just copies the Nones.

I looked into it, and I think I have a fix that solves the problem. I added some code to the xor decoder for testing purposes, and it seems to work as it did before. However, we don't have the same data as you, so I'm not sure if it fixes the offset issue.

If you're willing to be a guinea pig, try adding this code to the xor.py decoder class and letting us know if it fixes the issue:

    def TCP(self, addr, tcp, ts, **kwargs):
        super(DshellDecoder, self).TCP(addr, tcp, ts, **kwargs)
        conn = self.find(addr)
        if conn and self.xorconn[conn.addr].direction == "init":
            self.xorconn[conn.addr].proto = conn.proto
            self.xorconn[conn.addr].info(proto=conn.proto)
            self.xorconn[conn.addr].nextoffset = conn.nextoffset.copy()

If it works, we can add it (and some documentation) to the other decoders and commit.

[amm3]

No worries. It's easy for github alerts to slide by. Good catch on also setting the proto field!

Your solution definitely works, but to me it seems more elegant to initialize these values in connectionInitHandler immediate after instantiating the new Connection (we can handle proto here as well). Is there a reason you wanted to override TCP() to handle this?

Amending. I didn't read carefully at first.... I see what you're saying about the offset initialization, but in my tests that part worked. However, I didn't previously test anything that would have checked the proto value.

Let me know if this works:

    def connectionInitHandler(self, conn):
        # need to set up a custom connection tracker to handle
        self.xorconn[conn.addr] = dshell.Connection(self, conn.addr, conn.ts)
        self.xorconn[conn.addr].nextoffset = conn.nextoffset
        self.xorconn[conn.addr].proto = conn.proto
        self.xorconn[conn.addr].info(proto=conn.proto)

[dev195]

Now that you mention it, your way will probably work.

I was concerned that setting xorconn's nextoffset to a pointer of conn.nextoffset (your code) instead of a copy (my code) might cause issues elsewhere in the decoder, but it looks like the xor decoder doesn't manipulate offsets anywhere.

I agree that handling it in connectionInitHandler is more elegant. If you want to commit and push, I will accept the pull request.

[amm3]

Sounds good. Just pushed the combined changes. Thanks!