search

ArduPilot / pymavlink

python MAVLink interface and utilities
Other
499 stars 597 forks source link

Message signature verification in the python library does not properly check the timstamp. #961

Open pv42 opened 3 months ago

pv42 commented 3 months ago

The function check_signature(..) -> bool in mavutil.py generated from mavgen_python.py https://github.com/ArduPilot/pymavlink/blob/e192ad8114f203220f404f37f971d6359dd5e3d2/generator/mavgen_python.py#L963 does not increase the value of self.signing.stream_timestamps[stream_key] after initially setting it on the first message received per logical stream. The documentation says that a packet should be rejected if

Timestamp is older than the previous packet from the same logical stream - where a logical stream is defined as the sequence of MAVLink packets with the same (SystemID, ComponentID, LinkID) tuple.

but since the value is never updated this comparison is only done against the first packet of the stream.

tridge commented 2 months ago

@pv42 I think you are right and this needs to be fixed. Would you like to do a PR yourself? or shall I do it?

pv42 commented 2 months ago

Hello @tridge, thanks for the response. I just noticed the issue when comparing the c implementation, this python implementation and the documentation. While I was reasonably confident that there is something wrong with this implementation I am not sufficiently confident to write the security critical code of a codebase I am not familiar with.