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

Overrun in TLV parser #59

Open
dmercer-google opened this issue Jul 26, 2023 · 1 comment
Open

Overrun in TLV parser #59

dmercer-google opened this issue Jul 26, 2023 · 1 comment
Labels

Comments

@dmercer-google
Copy link
Collaborator

dmercer-google commented Jul 26, 2023

The TLV parser will read nested TLVs beyond the length of their parent container. Consider the nested TLV below:

010108 020100 0303000000

this parses correctly.

Now consider the erroneous TLV below:

010106 020100 0303000000

This also parses with no error despite the length of the top level container being set, erroneously, to 6 vs 8.

We encountered this while reviewing our pre-personalization of the CHUID container. In our case we had changed from 1 byte to 3 byte container IDs but we had neglected to add 2 to the overall container length. Despite the error, pre-provisioning worked fine when it should have failed.

@makinako makinako added the bug label Aug 1, 2023
@makinako
Copy link
Owner

makinako commented Aug 1, 2023

This is correct and I've tagged it as a bug. My intention is to review the TLVReader class, but most importantly it needs a validate() method that checks the tag and length values for comformance and sanity before allowing it to be used.

Unfortunately as in your case, it 'failed successfully', which may result in other perso scripts that have worked in the past failing because they had incorrect length values on the outer container.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants