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.

Sign up for GitHub

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

Jump to bottom

ida extractor: extract APIs from renamed globals #2201

Merged
merged 1 commit into from
Aug 13, 2024
Merged

ida extractor: extract APIs from renamed globals #2201

merged 1 commit into from
Aug 13, 2024

Conversation

Ana06
Copy link
Member

@Ana06 Ana06 commented Jul 9, 2024

Add support to extract dynamically resolved APIs stored in global variables that have been renamed (for example using the renimp.idc script included with IDA Pro).
Screenshot from 2024-07-09 15-05-30

It supports (and may close if we do not want to support the extraction of other types of API dynamic resolution) #252

Checklist

  • No CHANGELOG update needed
  • No new tests needed
  • No documentation update needed

@Ana06 Ana06 added enhancement New feature or request ida-explorer Related to IDA Pro plugin labels Jul 9, 2024
@Ana06 Ana06 self-assigned this Jul 9, 2024
@Ana06 Ana06 force-pushed the ida_apis branch 3 times, most recently from 481f93b to 8761a6e Compare July 10, 2024 12:03
williballenthin
williballenthin previously requested changes Jul 16, 2024
View reviewed changes
Copy link
Collaborator

@williballenthin williballenthin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please pull some of the GH comments into code comments so future readers have more context, then I think we're good to go here.

capa/features/extractors/ida/insn.py Outdated Show resolved Hide resolved
capa/features/extractors/ida/insn.py Outdated
op_name = match.group(1)
yield API(op_name), ih.address
if capa.features.extractors.helpers.is_aw_function(op_name):
yield API(op_name[:-1]), ih.address
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a comment that calls out the fact that the DLL name cannot be recovered here, unfortunately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we are in a debugger we maybe can by following the reference. But it complicates the code and does not work without the debugger, so I tend to think it is not worth to try to extract it. I'll add the comment.

williballenthin
williballenthin reviewed Jul 16, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
williballenthin
williballenthin reviewed Jul 16, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
mike-hunhoff
mike-hunhoff reviewed Jul 17, 2024
View reviewed changes
capa/features/extractors/ida/insn.py Outdated
Comment on lines 105 to 106
if capa.features.extractors.helpers.is_aw_function(op_name):
yield API(op_name[:-1]), ih.address
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call https://github.com/Ana06/capa/blob/8761a6e27893a398f4fa6bb013a325cfd2b2cbda/capa/features/extractors/helpers.py#L44 instead?

mike-hunhoff
mike-hunhoff reviewed Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@mike-hunhoff mike-hunhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good @Ana06 , thank you! I've left some comments in addition to Wili's.

@Ana06 Ana06 force-pushed the ida_apis branch 4 times, most recently from a1713f6 to f736613 Compare July 29, 2024 09:38
@Ana06
Copy link
Member Author

Ana06 commented Jul 29, 2024 •
edited
Loading

It seems the removal of setuptools.command.test in the setuptools library is making the CI fail. It seems capa requires version 70.0.0, where should be present, but the error is coming from intervaltree

@Ana06 Ana06 force-pushed the ida_apis branch 3 times, most recently from c113f91 to e572a03 Compare August 5, 2024 13:21
@Ana06 Ana06 force-pushed the ida_apis branch 5 times, most recently from fef1c64 to 902912b Compare August 13, 2024 09:33
@Ana06
Copy link
Member Author

Ana06 commented Aug 13, 2024

CI is green! 💚

mr-tz
mr-tz reviewed Aug 13, 2024
View reviewed changes
capa/ida/plugin/README.md Outdated Show resolved Hide resolved
mr-tz
mr-tz reviewed Aug 13, 2024
View reviewed changes
Copy link
Collaborator

@mr-tz mr-tz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only a few minor comments, then good to go, thanks for resolving the mypy issue

@Ana06
ida extractor: extract APIs from renamed globals
be84992 
Add support to extract dynamically resolved APIs stored in global
variables that have been renamed (for example using the `renimp.idc`
script included with IDA Pro).
mr-tz
mr-tz approved these changes Aug 13, 2024
View reviewed changes
Copy link
Collaborator

@mr-tz mr-tz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you very much!

@mr-tz mr-tz dismissed williballenthin’s stale review August 13, 2024 15:58

feedback has been addressed

@mr-tz mr-tz merged commit 8d5fcdf into mandiant:master Aug 13, 2024
25 checks passed
@mr-tz
Copy link
Collaborator

mr-tz commented Aug 13, 2024

🥳 thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mr-tz mr-tz mr-tz approved these changes

@williballenthin williballenthin Awaiting requested review from williballenthin

@mike-hunhoff mike-hunhoff Awaiting requested review from mike-hunhoff

Assignees

@Ana06 Ana06

Labels
enhancement New feature or request ida-explorer Related to IDA Pro plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ana06 @mr-tz @williballenthin @mike-hunhoff