RTPS contrib layer by phretor · Pull Request #3403 · secdev/scapy

phretor · 2021-10-14T09:34:52Z

Check this branch for detailed history of this change: https://github.com/phretor/scapy/commits/rtps

Disclaimer: I'm not sure this is the right way to bind layers that are not restricted to a non-fixed port range, so I put a comment. Let me or @vmayoral know if you prefer to enable the "ugly hack". All dissection tests pass even now.

# layer binding
# for port in range(7400, 7500):  # very ugly hack :-)
#     bind_layers(UDP, RTPS, dport=port)
#     bind_layers(TCP, RTPS, dport=port)

bind_layers(UDP, RTPS)
bind_layers(RTPS, RTPSMessage, magic=b"RTPS")
bind_layers(RTPS, RTPSMessage, magic=b"RTPX")

Co-authored-by: Federico Maggi fede@maggi.cc
Co-authored-by: Víctor Mayoral Vilches v.mayoralv@gmail.com
Signed-off-by: Federico Maggi fede@maggi.cc

phretor · 2021-10-14T10:04:46Z

Looks like a test unrelated to this PR is not passing.

######
## ISOTPSoftSocket MITM attack tests
######
###(047)=[failed] Send a two-frame ISOTP message with padding
>>> acks = TestSocket(CAN)
>>> cans = TestSocket(CAN)
>>> acks.pair(cans)
>>> 
>>> acker = AsyncSniffer(opened_socket=acks, store=False, prn=lambda x: acks.send(CAN(identifier = 0x241, data=dhex("30 00 00"))), timeout=1, count=1)
>>> acker.start()
>>> with TestSocket(CAN) as isocan, ISOTPSoftSocket(isocan, sid=0x641, did=0x241, padding=True) as s:
...     acks.pair(isocan)
...     cans.pair(isocan)
...     s.send(ISOTP(data=dhex("01 02 03 04 05 06 07 08")))
...     can = cans.sniff(timeout=1, count=1)[0]
...     assert(can.identifier == 0x641)
...     assert(can.data == dhex("10 08 01 02 03 04 05 06"))
...     can = cans.sniff(timeout=1, count=1)[0]
...     assert(can.identifier == 0x241)
...     assert(can.data == dhex("30 00 00"))
...     can = cans.sniff(timeout=1, count=1)[0]
...     assert(can.identifier == 0x641)
...     assert(can.data == dhex("21 07 08 CC CC CC CC CC"))
... 
11
Traceback (most recent call last):
  File "<input>", line 7, in <module>
AssertionError
UTscapy ended with error code 1
SocketCAN support: False
Using PythonCANSocket virtual
SocketCAN support: False
Using PythonCANSocket virtual
SocketCAN support: False
Using PythonCANSocket virtual
SocketCAN support: False
Using PythonCANSocket virtual
SocketCAN support: False
Using PythonCANSocket virtual
SocketCAN support: False
Using PythonCANSocket virtual
SocketCAN support: False
Using PythonCANSocket virtual
SocketCAN support: False
Using PythonCANSocket virtual
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
TEST SKIPPED: can-isotp not available
SocketCAN support: False
Using PythonCANSocket virtual
ERROR: InvocationError for command 'C:\projects\scapy\.tox\py37-windows\Scripts\python' -m coverage run -m scapy.tools.UTscapy -c test/configs/windows.utsc (exited with code 1)
___________________________________ summary ___________________________________
ERROR:   py37-windows: commands failed
Command exited with code 1

polybassa · 2021-10-14T11:06:08Z

Sorry, I'm working on a fix...

phretor · 2021-10-17T07:34:26Z

Bump ☺️

polybassa · 2021-10-17T08:49:18Z

Did you rebased on the current master? -- Viele Grüße Nils Weiß

…

On October 17, 2021 9:34:37 AM GMT+02:00, Federico Maggi ***@***.***> wrote: Bump ☺️> > -- > You are receiving this because you commented.> Reply to this email directly or view it on GitHub:> #3403 (comment)

phretor · 2021-10-17T09:22:00Z

Yes @polybassa - I had branched rtps from the most current master. I double checked right now and fetched && rebased, but they seem aligned:

 ❯ git rebase master
Current branch rtps is up to date.

polybassa · 2021-10-17T09:54:58Z

Thanks. I'll inspect those failures. -- Viele Grüße Nils Weiß

…

On October 17, 2021 11:22:10 AM GMT+02:00, Federico Maggi ***@***.***> wrote: Yes @polybassa - I had branched `scapy` from the most current `master`. I double checked right now and fetched && rebased, but they seem aligned:> > ```> ❯ git rebase master [11:21:44]> Current branch rtps is up to date.> ```> > -- > You are receiving this because you were mentioned.> Reply to this email directly or view it on GitHub:> #3403 (comment)

phretor · 2021-10-20T08:00:10Z

Thanks. I'll inspect those failures.

Anything we can help with @polybassa?

polybassa · 2021-10-20T08:31:58Z

Well.. I think in the end I have to do a bigger refactoring of ISOTPSoftSockets. Something is causing instabilities on the CI systems. Unfortunately these bugs are very sporadic or depend on the performance of the CI-system. So catching them on my local system is almost impossible.

polybassa · 2021-10-20T08:33:07Z

Feel free to add the ~disabled tag to the isotp_soft_socket.uts file as @gpotter2 did it in #3413

phretor · 2021-10-20T09:32:05Z

Feel free to add the ~disabled tag to the isotp_soft_socket.uts file as @gpotter2 did it in #3413

Done with f2cde18

scapy/contrib/rtps/pid_types.py

gpotter2

Generally solid PR, the packet formats look fine. Just a few few minor things.

scapy/contrib/rtps/rtps.py

scapy/contrib/rtps/types.py

scapy/contrib/rtps/pid_types.py

codecov · 2021-10-20T10:15:54Z

Codecov Report

Merging #3403 (6a985e1) into master (e28aa57) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 6a985e1 differs from pull request most recent head 0f81b2c. Consider uploading reports for the commit 0f81b2c to get more accurate results

@@            Coverage Diff             @@
##           master    #3403      +/-   ##
==========================================
- Coverage   85.28%   85.27%   -0.01%     
==========================================
  Files         277      277              
  Lines       57192    57192              
==========================================
- Hits        48774    48772       -2     
- Misses       8418     8420       +2

Impacted Files	Coverage Δ
scapy/arch/windows/__init__.py	`67.73% <0.00%> (-0.57%)`	⬇️
scapy/layers/inet6.py	`88.36% <0.00%> (+0.05%)`	⬆️

phretor · 2021-10-20T14:01:48Z

Done. Pinging @polybassa and @gpotter2 for checks.

phretor · 2021-10-20T15:15:58Z

@polybassa @gpotter2 not sure what the failures are about. I tried to reproduce locally and the docs job fails as well, with a very generic error and no details:

ERROR: InvocationError for command /src/scapy/.tox/docs/bin/sphinx-build -W --keep-going -b html . _build/html (exited with code 1)

gpotter2 · 2021-10-20T15:25:50Z

The docs logs aren't the easiest to read 🙂 If you go through them carefully you'll notice the following:

reading sources... [ 51%] api/scapy.contrib.rtps
/home/runner/work/scapy/scapy/scapy/base_classes.py:324: SyntaxWarning: Packet 'EndpointFlagsPacket' has a duplicated 'reserved' field ! If you are using several ConditionalFields, have a look at MultipleTypeField instead ! This will become a SyntaxError in a future version of Scapy !
  warnings.warn(war_msg, SyntaxWarning)
/home/runner/work/scapy/scapy/scapy/fields.py:444: SyntaxWarning: All fields should have the same name in a MultipleTypeField

phretor · 2021-10-20T16:02:40Z

Oh my, I had ignored those warnings - ac62f5b should do the job @gpotter2

phretor · 2021-10-21T07:17:06Z

OK, I think 71a5f3e did the job. Still, Codecov is not passing. Any idea why @polybassa @gpotter2 ?

polybassa · 2021-10-21T08:33:21Z

I think you can ignore codecov. Since you disabled the isotp tests, it decreased the coverage. -- Viele Grüße Nils Weiß

On October 21, 2021 9:17:16 AM GMT+02:00, Federico Maggi ***@***.***> wrote: OK, I think 71a5f3e did the job. Still, Codecov is not passing. Any idea why @polybassa

***@***.*** ?

…

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #3403 (comment)

phretor · 2021-10-25T08:04:49Z

I think everything has been sorted out with this PR. Any plans on when this could be merged? We have an upcoming talk at Black Hat next month and would be great to say "and Scapy can now successfully dissect/forge RTPS frames" 🤓

vmayoral · 2021-10-26T06:35:36Z

Ping in here, any chance we could have this merged maintainers?

gpotter2 · 2021-10-26T07:27:54Z

I need to re-review this and I'll merge it in the following couple of days. Sorry about the delay

guedou · 2021-10-26T08:41:39Z

Be assured that this PR will be merged before BHEU =)

vmayoral · 2021-10-26T08:46:25Z

Awesome, thanks a lot @guedou and @gpotter2 for the quick reaction!

gpotter2

Some last minor comments, other than that LGTM.

scapy/contrib/rtps/rtps.py

scapy/contrib/rtps/types.py

test/contrib/rtps/rtps.uts

gpotter2 · 2021-11-01T15:05:24Z

Also please rebase against master

phretor · 2021-11-04T10:04:12Z

Also please rebase against master

See the most recent commits. I rebased rtps against master. If you can get this merged quickly, I know @vmayoral has some improvements ready for a PR ;-)

vmayoral · 2021-11-04T10:10:39Z

See the most recent commits. I rebased rtps against master. If you can get this merged quickly, I know @vmayoral has some improvements ready for a PR ;-)

+1, it'd be awesome to get this in ASAP folks.

Count on us for maintaining this down the road (i.e. working on some extensions at https://github.com/vmayoral/scapy/tree/rtps). We'll push those once this first contribution is finalized.

phretor · 2021-11-29T09:00:07Z

Sorry folks. One rebase too much. I'm a hurry and I shouldn't work. I'll fix it when I have time. Otherwise, feel free to wipe the entire thing, copy the code from my repo, and start a fresh PR. I don't care too much about credits at this point, as long as this painful journey comes to an end.

Check this branch for detailed history of this change: https://github.com/phretor/scapy/commits/rtps Co-authored-by: Federico Maggi <fede@maggi.cc> Co-authored-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> Signed-off-by: Federico Maggi <fede@maggi.cc>

Co-authored-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

Co-authored-by: gpotter2 <gabriel@potter.fr>

Signed-off-by: Federico Maggi <federico@maggi.cc>

Previous EndpointFlagsPacket definition led to the following syntax warning: SyntaxWarning: Packet 'EndpointFlagsPacket' has a duplicated 'reserved' field ! If you are using several ConditionalFields, have a look at MultipleTypeField instead! This will become a SyntaxError in a future version of Scapy. This patch turns the various 'reserved' fields into a single one with length 4 bits. Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

This way, scripts don't need to manually import this file Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

This leads to packages like PID_BUILTIN_ENDPOINT_QOS building upon PIDPacketBase to have a proper alignment. Additions to rtps.uts RTPS layer test campaign to test this type of bit alignment issues Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

Crafting packages directly from raw hex data led to 'contrib/rtps/common_types.py, line 151, in set_endianness assert self.endianness is not None' type errors. By providing an else and returning FORMAT_BE, we mitigate it Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

…ypes Various packages inheriting from PIDPacketBase and part of RTPSParameterIdTypes were using Big Endian incorrectly, when Little Endian is what's used by DDS implementations. Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

- `_next_cls_cb` is now a plain function - Test cases updated accordingly - Removed `types.py` Signed-off-by: Federico Maggi <fede@maggi.cc>

Signed-off-by: Federico Maggi <fede@maggi.cc>

- Changed asserts into warnings + defaults Signed-off-by: Federico Maggi <fede@maggi.cc>

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

Signed-off-by: Federico Maggi <federico.maggi@gmail.com>

gpotter2 · 2021-11-29T10:48:47Z

No worries, thanks for the work !
I'll use my secret maintainer powers to rebase the PR and merge it ASAP :-)

gpotter2 · 2021-11-29T20:37:16Z

Thanks a lot for the great PR and sorry for the time it took. I hope we didn't disgust you from contributing to scapy :/

phretor · 2021-11-29T21:40:49Z

@gpotter2 code quality is of paramount importance. I was caught in the worst moment of my day, and I'm really glad I found in you Scapy devs and @vmayoral the supported that I needed. Right when I needed. Thanks!

* RTPS contrib layer Check this branch for detailed history of this change: https://github.com/phretor/scapy/commits/rtps Co-authored-by: Federico Maggi <fede@maggi.cc> Co-authored-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> Signed-off-by: Federico Maggi <fede@maggi.cc> * Update scapy/contrib/rtps/pid_types.py Co-authored-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> * Update scapy/contrib/rtps/rtps.py Co-authored-by: gpotter2 <gabriel@potter.fr> * Fixing various flake8 and docs build issues Signed-off-by: Federico Maggi <federico@maggi.cc> * codespell: endianess ==> endianness Signed-off-by: Federico Maggi <federico@maggi.cc> * Turn multiple duplicated 'reserved' bit fields into one Previous EndpointFlagsPacket definition led to the following syntax warning: SyntaxWarning: Packet 'EndpointFlagsPacket' has a duplicated 'reserved' field ! If you are using several ConditionalFields, have a look at MultipleTypeField instead! This will become a SyntaxError in a future version of Scapy. This patch turns the various 'reserved' fields into a single one with length 4 bits. Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> * Remove duplicated scapy metadata, into __init__.py Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> * Add RTPS PID type definitions to RTPS default imports This way, scripts don't need to manually import this file Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> * Define endianness of PIDPacketBase class This leads to packages like PID_BUILTIN_ENDPOINT_QOS building upon PIDPacketBase to have a proper alignment. Additions to rtps.uts RTPS layer test campaign to test this type of bit alignment issues Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> * Handle else in e_flags to avoid None errors Crafting packages directly from raw hex data led to 'contrib/rtps/common_types.py, line 151, in set_endianness assert self.endianness is not None' type errors. By providing an else and returning FORMAT_BE, we mitigate it Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> * Fix endianness of parameterId and parameterLength in RTPSParameterIdTypes Various packages inheriting from PIDPacketBase and part of RTPSParameterIdTypes were using Big Endian incorrectly, when Little Endian is what's used by DDS implementations. Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> * Add Alias's credit in files modified Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> * - Replaced `ByteField` with `StrField` - `_next_cls_cb` is now a plain function - Test cases updated accordingly - Removed `types.py` Signed-off-by: Federico Maggi <fede@maggi.cc> * Fixing flake8 failures + contrib line Signed-off-by: Federico Maggi <fede@maggi.cc> * Polishing and formatting fixes Signed-off-by: Federico Maggi <fede@maggi.cc> * - Removed unused classes - Changed asserts into warnings + defaults Signed-off-by: Federico Maggi <fede@maggi.cc> * Re-work scapy's preamble Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> * Using longs where applicable Signed-off-by: Federico Maggi <federico.maggi@gmail.com> * Minor code health fixes Co-authored-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> Co-authored-by: gpotter2 <gabriel@potter.fr>

gpotter2 reviewed Oct 20, 2021

View reviewed changes

scapy/contrib/rtps/pid_types.py Outdated Show resolved Hide resolved

gpotter2 reviewed Oct 20, 2021

View reviewed changes

scapy/contrib/rtps/rtps.py Outdated Show resolved Hide resolved

scapy/contrib/rtps/types.py Outdated Show resolved Hide resolved

vmayoral reviewed Oct 20, 2021

View reviewed changes

scapy/contrib/rtps/pid_types.py Outdated Show resolved Hide resolved

gpotter2 added the new layer label Oct 20, 2021

gpotter2 requested changes Nov 1, 2021

View reviewed changes

phretor and others added 18 commits November 29, 2021 11:46

RTPS contrib layer

96b4feb

Check this branch for detailed history of this change: https://github.com/phretor/scapy/commits/rtps Co-authored-by: Federico Maggi <fede@maggi.cc> Co-authored-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com> Signed-off-by: Federico Maggi <fede@maggi.cc>

Update scapy/contrib/rtps/pid_types.py

7e74019

Co-authored-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

Update scapy/contrib/rtps/rtps.py

7291040

Co-authored-by: gpotter2 <gabriel@potter.fr>

Fixing various flake8 and docs build issues

dcebe1f

Signed-off-by: Federico Maggi <federico@maggi.cc>

codespell: endianess ==> endianness

1037f82

Signed-off-by: Federico Maggi <federico@maggi.cc>

Remove duplicated scapy metadata, into __init__.py

90748dc

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

Add RTPS PID type definitions to RTPS default imports

934172c

This way, scripts don't need to manually import this file Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

Add Alias's credit in files modified

2c45e86

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

- Replaced ByteField with StrField

6c5cef7

- `_next_cls_cb` is now a plain function - Test cases updated accordingly - Removed `types.py` Signed-off-by: Federico Maggi <fede@maggi.cc>

Fixing flake8 failures + contrib line

3264947

Signed-off-by: Federico Maggi <fede@maggi.cc>

Polishing and formatting fixes

bd56150

Signed-off-by: Federico Maggi <fede@maggi.cc>

- Removed unused classes

6e8b466

- Changed asserts into warnings + defaults Signed-off-by: Federico Maggi <fede@maggi.cc>

Re-work scapy's preamble

e66d25c

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

Using longs where applicable

f5a61a9

Signed-off-by: Federico Maggi <federico.maggi@gmail.com>

gpotter2 force-pushed the rtps branch from 0f81b2c to f5a61a9 Compare November 29, 2021 10:47

Minor code health fixes

42f997a

gpotter2 merged commit 6cba1a5 into secdev:master Nov 29, 2021

vmayoral mentioned this pull request Mar 12, 2022

Add introspection and monitor verbs ros2/sros2#277

Open

gpotter2 added this to the 2.5.0 milestone Mar 29, 2022

vmayoral mentioned this pull request Jul 22, 2022

[REP-2012] Service introspection ros-infrastructure/rep#360

Open

vmayoral mentioned this pull request Dec 6, 2022

Add TCPROS layer to contrib #3462

Merged

Uh oh!

Conversation

phretor commented Oct 14, 2021

Uh oh!

phretor commented Oct 14, 2021

Uh oh!

polybassa commented Oct 14, 2021

Uh oh!

phretor commented Oct 17, 2021

Uh oh!

polybassa commented Oct 17, 2021 via email

Uh oh!

phretor commented Oct 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polybassa commented Oct 17, 2021 via email

Uh oh!

phretor commented Oct 20, 2021

Uh oh!

polybassa commented Oct 20, 2021

Uh oh!

polybassa commented Oct 20, 2021

Uh oh!

phretor commented Oct 20, 2021

Uh oh!

Uh oh!

gpotter2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

phretor commented Oct 20, 2021

Uh oh!

phretor commented Oct 20, 2021

Uh oh!

gpotter2 commented Oct 20, 2021

Uh oh!

phretor commented Oct 20, 2021

Uh oh!

phretor commented Oct 21, 2021

Uh oh!

polybassa commented Oct 21, 2021 via email

Uh oh!

phretor commented Oct 25, 2021

Uh oh!

vmayoral commented Oct 26, 2021

Uh oh!

gpotter2 commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guedou commented Oct 26, 2021 via email

Uh oh!

vmayoral commented Oct 26, 2021

Uh oh!

gpotter2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gpotter2 commented Nov 1, 2021

Uh oh!

phretor commented Nov 4, 2021

Uh oh!

vmayoral commented Nov 4, 2021

Uh oh!

phretor commented Nov 29, 2021

Uh oh!

gpotter2 commented Nov 29, 2021

Uh oh!

gpotter2 commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phretor commented Oct 17, 2021 •

edited

Loading

codecov bot commented Oct 20, 2021 •

edited

Loading

gpotter2 commented Oct 26, 2021 •

edited

Loading

gpotter2 commented Nov 29, 2021 •

edited

Loading