Skip to content
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.

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

SecurityAccess mishandes a level value of 0 #220

Closed
speedy-h opened this issue Mar 5, 2024 · 3 comments
Closed

SecurityAccess mishandes a level value of 0 #220

speedy-h opened this issue Mar 5, 2024 · 3 comments

Comments

@speedy-h
Copy link
Contributor

speedy-h commented Mar 5, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

speedy-h commented Mar 5, 2024

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
Copy link
Owner

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

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

No branches or pull requests

2 participants