get_router_version() robustness

[gelim]

Hi,

while fiddling with SAPRouter protocol, I stumbled on some strange scenario where get_router_version() where returning None (and then everything else failing with building packets with version=2 as it's the default in the dissector).

I noticed that was the case when there is network delay issues, and SAP Router will answer with a timeout control packet, thus we are failing the test:

    if router_is_control(response) and response.opcode == 2:
        return response.version

because reponse.opcode is not 2 in this special scenario.

That made me notice, that actually we don't need to check this condition, as without regard to opcode, the SAP Router will always send it's version via the version field.

I would just discard this check, and return response.version. Of course there will be other problems if network is clumsy...

NB: For what reason do you initialize version to 0x2 and not something like version_ni_version?
NB2: We usually speak about routers version in decimal (38, 39, 40) You could init version_ni_version in decimal, for the sake of clarity

[martingalloar]

Hi Mathieu, great feedback!

I would just discard this check, and return response.version. Of course there will be other problems if network is clumsy...

Yes man, you're right. That way we would cover both cases (version response and timeout error).

NB: For what reason do you initialize version to 0x2 and not something like version_ni_version?

There're actually two different version fields, and some packets uses one while others uses two. For example, a route request would include the route_version set to 2 and the ni_version set to 38/39/40, while an error request would only use the ni_version set to 38/39/40. Thing is, Scapy doesn't play whell with two conditional fields with the same name even if the condition is mutually exclusive, so I find this way to sort it out.

NB2: We usually speak about routers version in decimal (38, 39, 40) You could init version_ni_version in decimal, for the sake of clarity

Yes, I don't know why I made it that way, doesn't make sense actually!

So all in all, we need:

• Remove the check on the response type/opcode and return the version in the response.
• Initialize ni_version field with decimal values.

I'm happy if you can make PRs for that, otherwise I'll make my try later.