-
Notifications
You must be signed in to change notification settings - Fork 940
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
Add sanity check for length of a PE resource type name #974
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string length solution is OK. However, I would like to discuss few things - possibly propose other solutions, that could work together with the ultimate string length limit implemented as is:
- Are these strings typically, or by definition, null-terminated? Would it make sense to add check that would break string loading if either simple null char, or wide null char, was encountered?
- Are these strings typically readable? Would it be too strict if we added a check that would break parsing if a non-printable (ASCII) character was encountered?
But these are just things to discuss - rule out. In general, I like that we would not be too strict/clever and do it like LIEF.
I am afraid that the resource string has the following format (https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#resource-directory-string): 2 byte length at start, and then the variable length Unicode string, which is probably UTF 16 (I don't see it defined there to confirm)? So no null terminators and parsing this would be probably a bit complex, but maybe doable? But I am thinking if it makes sense, to have wide null in a string even tho it is not null terminated. I am not 100 % sure about the variable length UTF 16 encoding and what it means to have 0x0000 🤔 Might be a usable heuristic. |
Well, in that case 100 chars limit it is ok:
Btw, tests are OK. They failed because tests for #973 were merged to @HoundThe I'm merging this, please add a regression test(s). |
Yes, this is a possibility. There are the following options:
If we choose to go with option 2 or 3, we will have to decide what to do in case there are merge conflicts (the build would fail in such case) and also consider proper changes to the handling of the retdec-regression-tests repo (e.g. when there is a branch having the same name in both the retdec and retdec-regression-tests repos, that branch is used for testing). |
@s3rvac I think, we are ultimately interested in state 2. Option 3 could be useful in some cases, but because builds on windows may take nearly an hour, it would often be too slow to work with. I think, merge conflicts are not a big problem - they need to be solved before merging to |
Aims to fix #959
Sometimes the type name pointer points to (probably?) junk data, which results in a string with absurd length and content. A simple sanity check on the string length could solve this problem.
As in example
097a0f8b8c3f2b90f5360f27da5ef8e5a7406c6f211c8d3e56a1671933508bc1
. More details can be seen in the mentioned issue.If this solution is "OK", I'll add a test case in regression test suite.