automotive/uds: special case to allow sending malformed 7f request

[BenGardiner]

Similar to the motivation in #3947 (comment) : I would like to scan all the possible services including a nonsense/malformed/invalid request to a 7f service.

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 executed the regression tests (using cd test && ./run_tests or tox)
• If the PR is still not finished, please create a Draft Pull Request

enables uds scanning of 0x00 - 0xff without crash

Similar to the motivation in #3947 (comment) : I would like to scan all the possible services including a nonsense/malformed/invalid request to a 7f service.

It is definitely invalid UDS to make requests to services with 0x40 bit set; however, the motivation is precisely to test targets with invalid UDS.

At present UDS_Scanner(isock, test_cases=[UDS_ServiceEnumerator], UDS_ServiceEnumerator_kwargs={...,'scan_range': range(256)}) will fail with AttributeError: requestServiceId when the scanner gets to sending 017f. This small patch makes scapy.contrib.automotive.uds.UDS.hashret() check if the payload.fields has requestedServiceId before trying to grab that field value for struct.pack().

I'm proposing the patch here even though this is invalid UDS because a crash seems like the wrong outcome. Happy to adapt the patch or testing -- or withdraw at you discretion.

[codecov]

Codecov Report

Merging #4347 (26d21f9) into master (0a2b2bc) will decrease coverage by 0.55%.
Report is 5 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4347      +/-   ##
==========================================
- Coverage   82.03%   81.48%   -0.55%     
==========================================
  Files         350      350              
  Lines       82888    83136     +248     
==========================================
- Hits        67994    67744     -250     
- Misses      14894    15392     +498     

Files	Coverage Δ
scapy/contrib/automotive/uds.py	98.41% <100.00%> (ø)

... and 40 files with indirect coverage changes

[polybassa]

Hi, thanks for the PR. I think an additional check in hashret is a good idea in general.

I suggest to change the check to

‘’’ … and len(self) >= 2… ‘’’

I’m not a fan of string comparisons inside hashret.

[polybassa]

Could you please provide a test case as well?

[BenGardiner]

I suggest to change the check to ... I’m not a fan of string comparisons inside hashret.

Will do

Could you please provide a test case as well?

I can, yes. I don't really know where to start; could you please tell me which file to add to @polybassa ?

[polybassa]

Great.

I think you can copy this test case and change the scan_range

scapy/test/contrib/automotive/scanner/uds_scanner.uts

Line 259 in 731c4c0

= UDS_ServiceEnumerator

[BenGardiner]

Actually the UTS is harder to grok than I anticipated 😅 I can keep trying but it's not clear to me what write just yet.

…

On Wed, Apr 10, 2024, 15:16 Nils Weiss ***@***.***> wrote: Great. I think you can copy this test case and change the scan_range https://github.com/secdev/scapy/blob/731c4c050ac14e5ed8fc8ad80489b368dfeeb5f5/test/contrib/automotive/scanner/uds_scanner.uts#L259 — Reply to this email directly, view it on GitHub <#4347 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB3M6KJ35NYWH6SBXVWSITY4WFXLAVCNFSM6AAAAABF6673VCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBYGI3TAMRSHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[polybassa]

I think you can just copy the testcase from line 259 to 305 and change the kwargs in line 284.

finally you need to change the asserts

[BenGardiner]

OK I did eventually understand it enough to create testcases.

using … and len(self) >= 2… in hashret() was insufficient; there appears to be another case where len(self) > 1 BUT also .requestServiceId does not exist... (where bytes(self.payload) == b'\x00' or bytes(self.payload) == b'\x00\x00') . I added another compound condition here... but I am not confident this is correct.

[polybassa]

Looks good so far