-
Notifications
You must be signed in to change notification settings - Fork 494
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
support negative offset features (and add IDA tests) #208
Conversation
@@ -144,12 +144,13 @@ def extract_insn_offset_features(f, bb, insn): | |||
continue | |||
p_info = capa.features.extractors.ida.helpers.get_op_phrase_info(op) | |||
op_off = p_info.get("offset", 0) | |||
if 0 == op_off: | |||
continue |
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.
for consistency with the viv extractor, we support offset=0x0.
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.
Love this commit! I've added some comments/suggestions but otherwise awesome!
if idaapi.is_mapped(op_off): | ||
# Ignore: | ||
# mov esi, dword_1005B148[esi] | ||
continue | ||
|
||
op_off = capa.features.extractors.helpers.twos_complement(op_off, 32) |
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.
What about 8 and 16?
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.
or 64?
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.
i got the impression that IDA returns the offset as twos complement in a u32, never as u8/u16/u64. if this is wrong, i'd very much like to fix it.
according to some sources, like here, there is no 64-bit offset/displacement.
# some versions of IDA return a truncated version of the MD5. | ||
# https://github.com/idapython/bin/issues/11 | ||
found = idautils.GetInputFileMD5().rstrip(b"\x00").decode("ascii").lower() |
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.
Good to know!
codifies our decision to support offset features with negative values #197 by relaxing the lint and updating the IDA extractor.
in order to demonstrate that the IDA changes work, i implemented tests for the IDA extractor. these will not run during CI, but we can invoke them manually during development. they should be run with the
mimikatz.exe_
database open.closes #197
closes #202