yara icon indicating copy to clipboard operation
yara copied to clipboard

modules/pe: make resources sanity check stricter

Open PeterMatula opened this issue 1 year ago • 2 comments

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

PeterMatula avatar Jan 27 '24 12:01 PeterMatula

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.

plusvic avatar Jan 30 '24 09:01 plusvic

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.

PeterMatula avatar Jan 31 '24 11:01 PeterMatula