search

VirusTotal / yara

The pattern matching swiss knife
https://virustotal.github.io/yara/
BSD 3-Clause "New" or "Revised" License
8.32k stars 1.45k forks source link

modules/pe: make resources sanity check stricter #2031

Closed PeterMatula closed 8 months ago

PeterMatula commented 10 months ago

Make number-based PE resources sanity checks a bit stricter to avoid more potentially corrupt files.

We came across this sample, whose NumberOfNamedEntries and NumberOfIdEntries resource entries would pass the resource-numbers-based PE parsing sanity checks, but its resources are corrupt. Resource parsing for the sample in question is still skipped thanks to yr_le32toh(resource_dir->Characteristics) != 0 part of the condition, but if it were not there, it would pass and run into trouble.

The "solution" is not ideal, it only makes the existing conditions a bit stricter, but it should not really have any downside.

cc @metthal @ladislav-zezula

plusvic commented 9 months ago

My concern with this change is that it doesn't tackle any real issue with files that are corrupt but have a low enough NumberOfNamedEntries and NumberOfIdEntries that passes the check. If the problem is that YARA can crash while scanning certain corrupted files, we should aim for avoiding the crash. By lowering the threshold we are only reducing the odds for the crash to occur, but we are not fixing the real issue.

PeterMatula commented 9 months ago

I agree with you, this is by no means a comprehensive fix. It only makes the current checks slightly stricter, and maybe, the current state slightly better. Currently, it is not really in our powers to look into it deeper and implement a better solution. It is fine if its not merged.