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

PeNet.FileParser.StreamFile - ReadUnicodeString function issue - infinate loop #251

Closed
Bobbymac001 opened this issue Sep 16, 2022 · 7 comments

Comments

@Bobbymac001
Copy link

Bobbymac001 commented Sep 16, 2022

Discovered an issue with PeNet.FileParser.StreamFile in ReadUnicodeString(long) function.

Basically, the functions' intent is to keeps reading bytes from the stream until it reaches two bytes with 0x00 0x00 (or unicode /u0000 when combined), then break and return the Unicode encoded string. Unfortunately, I found several malware samples where it seems a Unicode null character did not terminate the strings in their PE file. As such, it went into an infinite loop and, for some reason, kept reading chars way farther than the length of the stream - /u0510 characters ad infinitum (which I separately don't understand.)

At any rate, the quick solution was just to put a check in place to ensure the chars' length was never equal to the steam.Length - offset or steam.Length - offset -1. If it is, it breaks the loop, returns what was read, and leaves it for the source function that called it to deal with. (And it looks like everything is dealt with there cause it's working with no issues for me now.)

// this is kind of sloppy but had a situation where test cases were getting negative numbers
// sometimes, so just dealt with it this way. Added to the very end of the while loop...
long sLen = (_stream.Length - offset < 0)? offset - _stream.Length: _stream.Length - offset;

if (sLen == chars.Count || sLen == chars.Count-1) {
break;
}

@secana
Copy link
Owner

secana commented Sep 16, 2022

Hi @Bobbymac001, thanks for reporting the bug. Do you have any file I can test the behavior with? Probably a SHA256 or MD5 is enough, as it may already be in my malware collection.

@Bobbymac001
Copy link
Author

Hello @secana, not a problem. Thank you for building this library and maintaining it. 🥇

I was running through a VirusShare batch from 00437 when started getting several that triggered it. It took me a while to figure out what was going on - it was shifting into an infinite loop statement when I checked properties for null values in the resources object of the PeFile.

`if (pef.Resources != null && pef.Resources.VsVersionInfo != null && pef.Resources.VsVersionInfo.StringFileInfo != null && pef.Resources.VsVersionInfo.StringFileInfo.StringTable != null) {

It was the VsVersionInfo check that first fell into the infinite loop.

There were a bunch in the VirusShare batch 00437 that triggered the same behaviour, but one that I identified and reproduced it myself to figure out what was going on was Trojan.Win32.Agent.icgh-184d64373ca83cfcf50c535fa70d68bfdf32392dbe4e42171fe8b818309a0ee0.

I can remove the escape clause and find more if that doesn't work.

@secana secana closed this as completed in 356b4ac Sep 19, 2022
@secana
Copy link
Owner

secana commented Sep 19, 2022

@Bobbymac001 can you check if it is fixed in 2.9.9-pre1?

@Bobbymac001
Copy link
Author

@secana, I'll apply both sets of changes to the codebase over the weekend, and test/validate soon as I have a window to dig out the samples...

@Bobbymac001
Copy link
Author

@secana, I applied the code changes and tested and didn't have any issues with the specific samples I knew about. I'm about to rerun the data extraction on the whole repository (over a few days) and will add another 40K samples through the week - I'll let you know if anything else pops on either, but looks good/solved.

@secana
Copy link
Owner

secana commented Sep 25, 2022

@Bobbymac001 thanks a lot. If everything works as expected, I'll release the non-preview version.

@Bobbymac001
Copy link
Author

Hey There, sorry for the late reply, had some other work that took priority. I ran a small subset of the batch and didn't have any issues. I need to do an 80K run later this week, but as far as I can tell it's working as expected now.

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