Skip to content

Added support of usbmon. #3666

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added support of usbmon. #3666

wants to merge 1 commit into from

Conversation

KOLANICH
Copy link
Contributor

Checklist:

  • If you are new to Scapy: I have checked CONTRIBUTING.md (esp. section submitting-pull-requests)
  • I squashed commits belonging together
  • I added unit tests or explained why they are not relevant - I have added a permissively-licensed pcap file from an another project, I hope it is enough.
  • I executed the regression tests for Python2 and Python3 (using tox or, cd test && ./run_tests_py2, cd test && ./run_tests_py3) - haven't. And these instructions are just incorrect.

Adds USBMon support. It may look somehow alien since I have compiled it from my Kaitai Struct spec into Construct target, then rewritten it into primitives used in Scapy and then there was a long way of fighting with it because Scapy spec DSL is somehow inferior to the one provided by KS. I think that Scapy needs a major version upgrade reducing the gap, and maybe even allowing to use KS-compiled specs by just adding them into a certain dir.

@codecov
Copy link

codecov bot commented Jun 27, 2022 •
edited
Loading

Codecov Report

Merging #3666 (4beb8aa) into master (9473f77) will decrease coverage by 0.00%.
The diff coverage is 81.81%.

❗ Current head 4beb8aa differs from pull request most recent head ef2750e. Consider uploading reports for the commit ef2750e to get more accurate results

@@            Coverage Diff             @@
##           master    #3666      +/-   ##
==========================================
- Coverage   84.57%   84.56%   -0.01%     
==========================================
  Files         296      296              
  Lines       62214    62342     +128     
==========================================
+ Hits        52616    52721     +105     
- Misses       9598     9621      +23
Impacted Files Coverage Δ
scapy/layers/usb.py 75.12% <81.57%> (+3.58%) ⬆️
scapy/data.py 87.61% <100.00%> (+0.03%) ⬆️
scapy/__init__.py 74.54% <0.00%> (-1.82%) ⬇️
scapy/arch/windows/__init__.py 67.16% <0.00%> (-0.56%) ⬇️
scapy/contrib/isotp/isotp_soft_socket.py 82.53% <0.00%> (-0.39%) ⬇️
scapy/supersocket.py 56.58% <0.00%> (-0.30%) ⬇️
scapy/automaton.py 73.62% <0.00%> (-0.22%) ⬇️
scapy/packet.py 82.66% <0.00%> (+0.07%) ⬆️
scapy/volatile.py 86.05% <0.00%> (+0.78%) ⬆️

gpotter2
gpotter2 requested changes Jun 27, 2022
View reviewed changes
Copy link
Member

@gpotter2 gpotter2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowing to use KS-compiled specs

Katai structs are far from being flexible enough to really be used within Scapy.

Scapy spec DSL is somehow inferior to the one provided by KS

You're probably using it wrong.

To be honnest, there are quite a bunch of issues in this PR, i noted a few but please note this will need a major refactor.
Thanks a lot for your interest in Scapy and your time

scapy/layers/usb.py
class SetupSetup(Packet):
name = "Setup"

class PcapUsbSetup(Packet):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically don't do Packets in Packets

scapy/layers/usb.py
]

fields_desc = [
PacketLenField("s", None, PcapUsbSetup, length_from=lambda pkt: 8),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use PacketField here. Since your sub-packet has a static size, it's not useful to use PacketLenField. This applies everywhere else.

Also what's with the name "s" ?

scapy/layers/usb.py

@staticmethod
def _getSize(*_args):
return 12
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You likely don't need that + it's really not scapy-alike.

setup.py
@@ -59,6 +59,9 @@ def process_ignore_tags(buffer):
'sphinx>=3.0.0',
'sphinx_rtd_theme>=0.4.3',
'tox>=3.0.0'
],
'usbmon': [
'enum34 ; python_version < "3.5"'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't include this and don't use Python 34 enums, but keep Scapy's dict for now. (Sure it would be nice to use Python 34+ enums everywhere... cf #3665)

scapy/layers/usb.py
# we have to do it since when using `PacketLenField`:
# `.parent` is not populated
# alternatives are always evaluated no matter what
# no way to provide parameters even with severe perversions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're likely doing something wrong here

@KOLANICH
Copy link
Contributor Author

not required thanks to #3665 (lucky me?)

Rebased upon that branch.

Please don't include this and don't use Python 34 enums, but keep Scapy's dict for now. (Sure it would be nice to use Python 34+ enums everywhere... cf #3665)

I guess we can delay landing this untill depending on enum34 for python2 or just dropping python 2 be considered acceptable. Because otherwise it would mean useless work on first removing the Enums and then reintroducing that when they are considered acceptable. Why not to consider them acceptable right now, given there is a backport of that enum for Python 2? It 8s surely a dependency, but only python 2 users are affected, it is their problem that they have to have a yet another package installed in their system.

Use PacketField here. Since your sub-packet has a static size, it's not useful to use PacketLenField.

It seems that just PacketField has a bug. If I replace, for example, timestamp, it would break parsing and result in different results. That's why we have the methods returning sizes. In the KS spec this field is defined without specifying number of bytes it occupies.

We typically don't do Packets in Packets

I know, though I'm not sure is that keeping everything flat is the right approach, since using nesting allows to decompose tasks.

This perversion with copying fields into different types was caused by:

  1. I want to keep the things declarative, with staying away of overriding methods as much as possible
  2. I want the Packets to have the same structure as structs in my KS spec, because it allows using either scapy or the code generated from my KS spec in software easier, makes the abstraction layers thinner. And my spec is nested.
  3. In KS we can refer parent's fields. In Scapy it feels not very possible.
  4. In KS we can also specify parameters. In Scapy it also feels like not very possible. Of corse de-facto it is possible, but with much perversion.

Also what's with the name "s" ?

We need to invent a more sensigle name, I don't remember why I have chosen that one, but I haven't utilized this field (and the whole setup structure) in my software yet.

@KOLANICH KOLANICH force-pushed the usbmon branch 2 times, most recently from 39057be to 58e1038 Compare June 29, 2022 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpotter2 gpotter2 gpotter2 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
enhancement needs rework/cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KOLANICH @gpotter2 @guedou