pylessard / python-udsoncan

Python implementation of UDS (ISO-14229) standard.
MIT License
564 stars 195 forks source link

SecurityAccess mishandes a level value of 0 #220

Closed speedy-h closed 4 months ago

speedy-h commented 4 months ago

I can send a request to security access that raises an exception that isn't expected.

Code to produce the issue

from udsoncan.services import SecurityAccess
Request = SecurityAccess.make_request(level=0, mode=SecurityAccess.Mode.RequestSeed)
Request.get_payload()

Output I get

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/XXXXXX/temp/speedy-h/python-udsoncan/udsoncan/Request.py", line 97, in get_payload
    payload += struct.pack("B", subfunction)
struct.error: ubyte format requires 0 <= number <= 255

The request above doesn't make sense as the 2020 spec says level of 0 is reserved, but an error highlighting what argument caused the issue would be helpful.

The issue happens due to trying to correct the level to match the mode in the normalize_level function. when 0 is passed with request seed, 1 is subtracted from it. A value of -1 can't be turned in to a byte value and causes the error seen.

A quick fix would be changing the validate_int minimum to be 1. This would stop 0 being passed. But the spec has a number of other subfunction reserved value blocks, which I don't know how you want to handle. The other option may be to change the normalize_level function to return without checking the mode if the level is set to 0.

A similar corner case issue arises when sending a key with a level of 0x7F. The level will be corrected to 0x80, which although not error producing, is probably not what is wanted.

pylessard commented 4 months ago

I will check that. I never restrain the user from using reserved values, but I can emit a warning in that case. If a value is impossible, I will prevent it, which is the case for the boundaries here.

Thank you for reporting.

speedy-h commented 4 months ago

I think I noticed that users could send restricted values in most other services.

The only exception to this that I've noticed is in the ReadDTCInformation. The check_subfunction_valid has an initial validation range of 1 to 0xFF. This doesn't matter too much as it will get caught slightly later in the function

pylessard commented 4 months ago

Fixed. Range reduced to 01-7E as specified by ISO-14229:2020