Skip to content

Split #3054: Refactoring of UDS Scanner#3352

Merged
gpotter2 merged 1 commit intosecdev:masterfrom
polybassa:split_3054_uds_scanner
Nov 11, 2021
Merged

Split #3054: Refactoring of UDS Scanner#3352
gpotter2 merged 1 commit intosecdev:masterfrom
polybassa:split_3054_uds_scanner

Conversation

@polybassa
Copy link
Copy Markdown
Contributor

No description provided.

@polybassa polybassa force-pushed the split_3054_uds_scanner branch from a5c01bd to a066a18 Compare August 30, 2021 06:30
@polybassa polybassa closed this Aug 30, 2021
@polybassa polybassa reopened this Aug 30, 2021
@polybassa polybassa closed this Aug 30, 2021
@polybassa polybassa reopened this Aug 30, 2021
@polybassa polybassa closed this Aug 30, 2021
@polybassa polybassa reopened this Aug 30, 2021
@polybassa polybassa force-pushed the split_3054_uds_scanner branch from c9c9b95 to 46b1a07 Compare September 1, 2021 07:27
@polybassa polybassa closed this Sep 1, 2021
@polybassa polybassa reopened this Sep 1, 2021
@polybassa
Copy link
Copy Markdown
Contributor Author

@gpotter2 This is ready for review

@polybassa polybassa force-pushed the split_3054_uds_scanner branch from bd73038 to 0421836 Compare September 6, 2021 06:38
Copy link
Copy Markdown
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.

What's the candump_uds_scanner.log.gz for ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for asking... this requires a little bit of background information. Automotive ECUs which support HSFZ or DoIP usually have multiple different "States" and usually at least two interfaces, CAN and Ethernet. On state is the normal firmware, another state is for example the bootloader. Since the bootloader has a much smaller code base, than the firmware, in most cases, Ethernet (DoIP or HSFZ) is not supported. This means, as soon as an ECU enters the bootloader state, the Ethernet interface goes down.

This has some drawbacks for an automated scan, I want to do with UDS_Scanner. During a scan, the scanner will find a bootloader state and by default will try to run all enumerators against this bootloader to check which services are supported. The scanner will send an ECU to bootloader state, an then try to execute the enumerator. The enumerator will execute this line: https://github.com/secdev/scapy/blob/0421836e53447400668119edce8ae8843d6c678f/scapy/contrib/automotive/scanner/enumerator.py#L174-L176
Since, in the meantime the ECU put's its interface down, an Exception during send (_sndrcv_snd) will come up. Here is a very general catch:

scapy/scapy/sendrecv.py

Lines 251 to 252 in 23ff728

except Exception:
log_runtime.exception("--- Error sending packets")

This catch now hides the exception from my enumerator. So the enumerator has now way to figure out, if the interface of the ECU went down. The enumerator will receive a None, and will try the next packet. This means the enumerator will fire a huge number of requests against a dead interface which floods the output with exception log messages.

I didn't want to change the code in _sndrcv_snd, therefore I did the exception handling inside this custom sockets. In this way, the socket goes down and the enumerator will know, that the socket died.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that you should add some comments to the code =)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's this for

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I want to ensure that no pending messages in the queues of these two TestSocket objects are left. This basically drains these two Sockets

@polybassa
Copy link
Copy Markdown
Contributor Author

What's the candump_uds_scanner.log.gz for ?

This file is a logfile of a scan of a real ECU. In the unit tests, I feed this logfile to an Ecu objects, which creates me an Answeringmachine based on all responses, found in this log file.

This is on the one hand, a good unit test to ensure that this "on-the-fly" cloning of a real world ECU's communication behavior works. Secondly, I use this virtual ECU (the answeringmachine) to run the UDS_scanner against it.

@polybassa polybassa force-pushed the split_3054_uds_scanner branch from 0421836 to 63964c5 Compare September 13, 2021 13:15
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 13, 2021

Codecov Report

Merging #3352 (213ebc3) into master (e28aa57) will decrease coverage by 0.25%.
The diff coverage is 86.71%.

❗ Current head 213ebc3 differs from pull request most recent head ab82736. Consider uploading reports for the commit ab82736 to get more accurate results

@@            Coverage Diff             @@
##           master    #3352      +/-   ##
==========================================
- Coverage   85.28%   85.03%   -0.26%     
==========================================
  Files         277      278       +1     
  Lines       57192    57623     +431     
==========================================
+ Hits        48774    48997     +223     
- Misses       8418     8626     +208     
Impacted Files Coverage Δ
scapy/contrib/automotive/bmw/hsfz.py 51.66% <14.28%> (-3.89%) ⬇️
scapy/contrib/automotive/doip.py 54.83% <14.28%> (-2.64%) ⬇️
scapy/contrib/automotive/uds.py 95.52% <37.50%> (+1.91%) ⬆️
scapy/contrib/automotive/uds_scan.py 89.27% <89.27%> (ø)
scapy/contrib/automotive/gm/gmlan_scanner.py 86.22% <100.00%> (+0.07%) ⬆️
scapy/contrib/automotive/scanner/enumerator.py 90.14% <100.00%> (-0.09%) ⬇️
scapy/contrib/automotive/uds_ecu_states.py 93.10% <100.00%> (+6.89%) ⬆️
scapy/compat.py 48.46% <0.00%> (-47.27%) ⬇️
scapy/pton_ntop.py 89.18% <0.00%> (-8.11%) ⬇️
scapy/error.py 88.31% <0.00%> (-6.50%) ⬇️
... and 29 more

@polybassa polybassa force-pushed the split_3054_uds_scanner branch 2 times, most recently from abfdbbd to 37071f7 Compare September 20, 2021 07:31
@polybassa polybassa closed this Sep 20, 2021
@polybassa polybassa reopened this Sep 20, 2021
@polybassa polybassa force-pushed the split_3054_uds_scanner branch from e583dc1 to 7d6723e Compare September 27, 2021 12:41
@polybassa polybassa closed this Sep 28, 2021
@polybassa polybassa reopened this Sep 28, 2021
@polybassa polybassa force-pushed the split_3054_uds_scanner branch from a8ae38e to ab28263 Compare September 28, 2021 12:45
@polybassa polybassa closed this Sep 28, 2021
@polybassa polybassa reopened this Sep 28, 2021
@polybassa polybassa closed this Sep 28, 2021
@polybassa polybassa reopened this Sep 28, 2021
@polybassa polybassa force-pushed the split_3054_uds_scanner branch from ab28263 to d590186 Compare October 11, 2021 12:22
@polybassa polybassa closed this Oct 12, 2021
@guedou
Copy link
Copy Markdown
Member

guedou commented Nov 3, 2021

What is the use case of propagating exceptions? We should move this discussion to a proper issue =)

@polybassa
Copy link
Copy Markdown
Contributor Author

@guedou I've added all your remarks. Can this get merged?

@guedou
Copy link
Copy Markdown
Member

guedou commented Nov 6, 2021

There is a failed test on macOS. I restarted the Github Actions jobs.

It looks good to me, but I wonder if it is possible to slim down the pcap files.

@polybassa
Copy link
Copy Markdown
Contributor Author

polybassa commented Nov 6, 2021 via email

@guedou
Copy link
Copy Markdown
Member

guedou commented Nov 6, 2021

Thanks for your answer!

@polybassa
Copy link
Copy Markdown
Contributor Author

polybassa commented Nov 6, 2021 via email

@guedou
Copy link
Copy Markdown
Member

guedou commented Nov 6, 2021

Did you rebase against master? BTW, could you squash your commits?

@polybassa
Copy link
Copy Markdown
Contributor Author

polybassa commented Nov 6, 2021 via email

@polybassa polybassa force-pushed the split_3054_uds_scanner branch 2 times, most recently from ab82736 to 76d7644 Compare November 7, 2021 18:39
@polybassa
Copy link
Copy Markdown
Contributor Author

All Done

@polybassa polybassa closed this Nov 8, 2021
@polybassa polybassa reopened this Nov 8, 2021
@polybassa polybassa force-pushed the split_3054_uds_scanner branch from 22ff139 to 4b5e185 Compare November 10, 2021 10:46
@polybassa polybassa closed this Nov 11, 2021
@polybassa polybassa reopened this Nov 11, 2021
@polybassa
Copy link
Copy Markdown
Contributor Author

@guedou This PR is ready from my side. I think the failed tests are not related to my changes, since some machines fail randomly always in the beginning of a test execution.
Could this PR please get merged?

Copy link
Copy Markdown
Member

@guedou guedou left a comment

Choose a reason for hiding this comment

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

LGTM. Thanlks @polybassa for your changes!

@gpotter2 I will let you perform an ultimate check as you reviewed this PR too.

@gpotter2 gpotter2 merged commit d2b9bb8 into secdev:master Nov 11, 2021
@gpotter2 gpotter2 added this to the 2.5.0 milestone Mar 29, 2022
bzalkilani pushed a commit to bzalkilani/scapy that referenced this pull request Jun 12, 2022
@polybassa polybassa deleted the split_3054_uds_scanner branch July 27, 2022 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants