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

bdshemu: Fix edge case with unsupported instrs #107

Merged

Conversation

ScimitarEnjoyer
Copy link
Contributor

@ScimitarEnjoyer ScimitarEnjoyer commented Sep 23, 2024

Fixes #106

Adjusted this PR to be more consistent with the code style.

Note: InstructionsCount is incremented before stop on exploit is checked. If shellcode is detected after n instructions, InstructionsCount will be n+1, even though instruction n+1 was never emulated. Context->InstructionsCount++ could be done after the stop on exploit check in order to be more accurate, but this is nitpicking.

@ScimitarEnjoyer ScimitarEnjoyer force-pushed the bugfix/unsupported-inst-edge-case branch 2 times, most recently from 0bfbfd3 to 66b983c Compare September 23, 2024 16:01
@vlutas
Copy link
Collaborator

vlutas commented Sep 23, 2024

This was intended from the beginning (i.e., cycle the emulator for these instructions), but this may also be problematic, because when we reach the maximum number of instruction, we would exit with SHEMU_SUCCESS, confusing the caller.
Your change means that in those cases, we would get fewer emulated instructions, which will also cause the tests to fail (they will have to be adjusted accordingly).
I'll give this some thinking time, to see if the change can be merged or if there are better ways to handle this.
Thanks for the PR!

@ScimitarEnjoyer
Copy link
Contributor Author

Yeah, I was calling ShemuEmulate in a loop with MaxInstructionsCount = 1 in order to be able to "hook" into the emulator and inspect the context after every instruction when I realized that it was succeeding when it shouldn't have (like on syscalls).

@vlutas
Copy link
Collaborator

vlutas commented Sep 23, 2024

So in conclusion, the change looks fine, and we can simply return STATUS_ABORT_CANT_EMULATE before cycling the emulator, as in the PR.
However, please make sure to also run the tests, and adjust those that fail (since now the number of emulated instructions will be different).
Once you also fix the tests, I will merge this.
Thanks again for the PR!
LE: To run the tests, check out the tests/main.py script. To regenerate the result files, check out the --regenerate option, which will re-generate all the output files to match the latest emulator output.

@ScimitarEnjoyer
Copy link
Contributor Author

I have checked and regenerated the tests, should be good to merge now :)
Thanks!

@vlutas
Copy link
Collaborator

vlutas commented Sep 24, 2024

The change will be part of a future v2.2.1 release, as the v2.2.0 was already drafted. Thanks again for the PR!

@vlutas vlutas merged commit 7d005fa into bitdefender:master Sep 24, 2024
10 checks passed
@ScimitarEnjoyer ScimitarEnjoyer deleted the bugfix/unsupported-inst-edge-case branch September 24, 2024 08:58
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

Successfully merging this pull request may close these issues.

Edge case / off by 1 error in bdshemu
2 participants