asymcute: Compare request message type when matching acknowledgement#18434
asymcute: Compare request message type when matching acknowledgement#18434benpicco merged 1 commit intoRIOT-OS:masterfrom
Conversation
Currently, asymcute only matches an MQTT-SN request to its acknowledgement using the MsgId header. However, I strongly believe this to be insufficient as asymcute would thus also match a SUBACK to a prior PUBLISH message (for example) as long as the message ID matches. To address this issue, this commit modifies _req_preprocess to also compare the request message type in addition to the message id.
nmeum
requested review from
PeterKietzmann,
cgundogan,
haukepetersen and
miri64
as code owners
benpicco
added
the
Type: bug
maribu
left a comment
maribu
left a comment
There was a problem hiding this comment.
ACK. Code change looks good, reasoning makes sense, and I trust your testing.
maribu
added
the
CI: ready for build
FYI: I found this with SymEx-VP so I haven't done any tests with a particular MQTT-SN broker implementation (yet). |
|
OK, I guess I should test it prior hitting the merge button just to be sure, then. :) |
|
❗ This may cause a semantic merge conflict with #18433, once that is merged: |
miri64
added
the
Process: needs backport
|
@nmeum can you please provide a backport to the 2022.07 branch, e.g. by using the |
4 participants
Contribution description
Currently, asymcute matches an MQTT-SN request to its acknowledgement using only the MsgId header. However, I strongly believe this to be insufficient as asymcute would thus also match a SUBACK to a prior PUBLISH request (for example) as long as the message ID matches. If this happens (e.g. because of a malicious MQTT-SN broker) then this can trigger various bugs in asymcute, e.g. due to incorrect casts of the
req->argvoid*pointer. To address this issue, this commit modifies_req_preprocessto also compare the request message type in addition to the message id.Testing procedure
I haven't tested my fix extensively yet but existing tests should continue to work. Unfortunately, it also a bit difficult to provide a simple test setup for triggering this edge case.
Issues/PRs references
None.