contrib: provide support for MQTT-SN by miri64 · Pull Request #2131 · secdev/scapy

miri64 · 2019-07-05T09:10:09Z

MQTT-SN is a sister protocol to MQTT that allows for MQTT-like traffic over UDP. It was designed to be as close as possible to MQTT but has some differences in its header format, so the MQTT layer of scapy can't be used to handle MQTT-SN packets.

miri64 · 2019-07-05T09:10:35Z

I did some testing locally, but did not integrate with your test framework yet. I will provide tests for that ASAP.

miri64 · 2019-07-05T09:13:07Z

scapy/contrib/mqttsn.py

This field also should manipulate the adjust of the len field of the MQTTSN wrapping packet, as the len for this packet type only signifies the length of the MQTTSNEncaps header without the encapsulated message (see section 5.5 of the spec). Is there a way to express this?

Can't you use pkt.underlayer in the adjust ?

In the adjust of this field you mean?

If I see this correctly, StrLenField does not have an adjust parameter in its constructor.

If I understand correctly, I meant

the adjust of the len field of the MQTTSN wrapping packet

It might be better if you clarify 😄 from what understand len in MQTTSN needs info from w_node_id. It can get this info during the adjust of the len

It might be better if you clarify 😄

Ok, I'll try my best 😅:

For all packet types except this one the len field of MQTTSN (the underlayer of these packet types) denotes the total length of the packet. Only for this it denotes the length until the end of w_node_id (called "wireless node id" in the spec), and the length of the remainder is denoted by len field of a following MQTTSN packet (the encapsulated MQTT-SN datagram). So only for this I need to manipulate the len field of MQTTSN somehow to only go to the end of w_node_id string.

from what understand len in MQTTSN needs info from w_node_id. It can get this info during the adjust of the len

Do you mean something like this?

class MQTTSN(Packet): name = "MQTT-SN header" fields_desc = [ # … VariableFieldLenField("len", None, length_of="len", adjust=lambda pkt, x: x + len(pkt.payload)) if type(pkt.payload) is not MQTTSNEncaps else x + len(pkt.payload.w_node_id) + 1, # … ]

Ok, I somewhat did it like that :-)

codecov · 2019-07-05T15:51:09Z

Codecov Report

Merging #2131 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2131      +/-   ##
==========================================
+ Coverage   87.25%   87.33%   +0.07%     
==========================================
  Files         198      200       +2     
  Lines       44681    45384     +703     
==========================================
+ Hits        38986    39635     +649     
- Misses       5695     5749      +54

Impacted Files	Coverage Δ
scapy/contrib/mqttsn.py	`100% <100%> (ø)`
scapy/pton_ntop.py	`89.04% <0%> (-8.22%)`	⬇️
scapy/sessions.py	`95.9% <0%> (-1.93%)`	⬇️
scapy/utils.py	`81.39% <0%> (-0.63%)`	⬇️
scapy/arch/bpf/supersocket.py	`77.77% <0%> (-0.37%)`	⬇️
scapy/sendrecv.py	`84.09% <0%> (-0.18%)`	⬇️
scapy/arch/windows/__init__.py	`71.81% <0%> (-0.16%)`	⬇️
scapy/layers/inet6.py	`88.1% <0%> (-0.12%)`	⬇️
scapy/compat.py	`100% <0%> (ø)`	⬆️
scapy/error.py	`100% <0%> (ø)`	⬆️
... and 13 more

miri64 · 2019-07-05T15:53:42Z

Fixed some errors I found during writing of the tests and provided tests.

miri64 · 2019-07-05T15:58:19Z

Let me know if I should squash, the checklist said, I should but the changes I fixed-up were done after I opened the PR, and I know from experience that the preferences of each reviewer can be different ;-).

gpotter2

It's looking good.
Could you add a test that dissects a full packet, so that it tests the bindings with UDP ?

miri64 · 2019-07-05T18:13:05Z

Could you add a test that dissects a full packet, so that it tests the bindings with UDP ?

Will do

miri64 · 2019-07-05T18:24:33Z

And done. I used some dumps from some experiments of mine. The IPv6 prefix is an example prefix (2001:db8::/32) and the IIDs of the address are only locally valid (generated from the MCU's serial number).

miri64 · 2019-07-06T14:14:22Z

Fixed errors pointed out by travis.

MQTT-SN is a sister protocol to MQTT that allows for MQTT-like traffic over UDP. It was designed to be as close as possible to MQTT but has some differences in its header format, so the MQTT layer of `scapy` can't be used to handle MQTT-SN packets.

miri64 · 2019-07-06T14:15:13Z

And squashed

Amend test to show that payload is parsed wrongly

Add 2 extra byte for >0xff length fields on adjust

Fix test for fixed length field calculation

miri64 · 2019-07-09T12:53:56Z

While working with my implementation I noticed a bug in it. I fixed it and amended the tests to detect it.

I provided a this also upstream [1] but it might take a while until there is a version of that one can install with `pip` etc. So for now I'd like to include this into our repo to provide some tests for our two MQTT-SN libraries, `emcute` and `asymcute`. [1]: secdev/scapy#2131

gpotter2 · 2019-07-10T11:59:19Z

scapy/contrib/mqttsn.py

+
+# Layer bindings
+bind_layers(UDP, MQTTSN, sport=1883)
+bind_layers(UDP, MQTTSN, dport=1883)


A better way of doing this is to use 2xbind_bottom_up and one bind_layers with both port set.
This makes sure that packets have the two ports set when building, therefore UDP doesn't by default fallback on DNS or some other thing

Done and also provided a test for that case.

see secdev/scapy#2131 (comment)

gpotter2 · 2019-07-15T19:51:30Z

scapy/contrib/mqttsn.py

+        elif val > 0xff:
+            return s + b"".join([chb(0x01), chb(val >> 8), chb(val & 0xff)])
+        else:
+            return s + b"".join([chb(val)])


That looks highly inefficient

gpotter2 · 2019-07-15T19:52:15Z

scapy/contrib/mqttsn.py

+            raise Scapy_Exception("%s: invalid length field value" %
+                                  self.__class__.__name__)
+        elif val > 0xff:
+            return s + b"".join([chb(0x01), chb(val >> 8), chb(val & 0xff)])


something like b"\x01" + struct.pack("!H", val) would probably be more appropriate

Too used to C programming ^^

gpotter2 · 2019-07-16T10:26:43Z

Thanks for the PR !

miri64 · 2019-07-16T11:10:54Z

I did not squash yet :-/

gpotter2 · 2019-07-16T11:16:09Z

It's alright, you can squash while merging a PR:
be824b6

(Though the messages will remain in the commit, but that's fine)

miri64 commented Jul 5, 2019

View reviewed changes

miri64 force-pushed the mqttsn_layer branch from 30b80ef to aee3a9d Compare July 5, 2019 15:57

gpotter2 reviewed Jul 5, 2019

View reviewed changes

miri64 added 2 commits July 6, 2019 16:14

contrib: provide support for MQTT-SN

ec8be09

MQTT-SN is a sister protocol to MQTT that allows for MQTT-like traffic over UDP. It was designed to be as close as possible to MQTT but has some differences in its header format, so the MQTT layer of `scapy` can't be used to handle MQTT-SN packets.

test/contrib: provide tests for MQTT-SN layer

d8c5b4f

miri64 force-pushed the mqttsn_layer branch from e64baee to d8c5b4f Compare July 6, 2019 14:15

miri64 added 3 commits July 9, 2019 14:50

fixup! contrib: provide support for MQTT-SN

9265c9d

Amend test to show that payload is parsed wrongly

fixup! contrib: provide support for MQTT-SN

e42bba2

Add 2 extra byte for >0xff length fields on adjust

fixup! test/contrib: provide tests for MQTT-SN layer

8ab7db5

Fix test for fixed length field calculation

miri64 mentioned this pull request Jul 9, 2019

scapy_mqttsn: import MQTT-SN layer for scapy RIOT-OS/RIOT#11822

Closed

gpotter2 added the new layer label Jul 10, 2019

gpotter2 reviewed Jul 10, 2019

View reviewed changes

miri64 added 2 commits July 12, 2019 13:51

fixup! contrib: provide support for MQTT-SN

7079f15

fixup! test/contrib: provide tests for MQTT-SN layer

c518525

miri64 added a commit to miri64/RIOT that referenced this pull request Jul 12, 2019

fixup! scapy_mqttsn: import MQTT-SN layer for scapy

a21df34

see secdev/scapy#2131 (comment)

gpotter2 reviewed Jul 15, 2019

View reviewed changes

fixup! contrib: provide support for MQTT-SN

48c6c59

gpotter2 approved these changes Jul 16, 2019

View reviewed changes

gpotter2 merged commit be824b6 into secdev:master Jul 16, 2019

miri64 deleted the mqttsn_layer branch July 16, 2019 11:10

miri64 mentioned this pull request Aug 9, 2019

Add scapy as requirement RIOT-OS/riotdocker#77

Merged

Uh oh!

Conversation

miri64 commented Jul 5, 2019

Uh oh!

miri64 commented Jul 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

miri64 commented Jul 5, 2019

Uh oh!

miri64 commented Jul 5, 2019

Uh oh!

gpotter2 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jul 5, 2019

Uh oh!

miri64 commented Jul 5, 2019

Uh oh!

miri64 commented Jul 6, 2019

Uh oh!

miri64 commented Jul 6, 2019

Uh oh!

miri64 commented Jul 9, 2019

Uh oh!

gpotter2 Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpotter2 commented Jul 16, 2019

Uh oh!

miri64 commented Jul 16, 2019

Uh oh!

gpotter2 commented Jul 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

miri64 Jul 5, 2019 •

edited

Loading

codecov bot commented Jul 5, 2019 •

edited

Loading

gpotter2 Jul 10, 2019 •

edited

Loading