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

add BinExport tests #2340

Merged
merged 9 commits into from
Sep 11, 2024
Merged

add BinExport tests #2340

merged 9 commits into from
Sep 11, 2024

Conversation

mr-tz
Copy link
Collaborator

@mr-tz mr-tz commented Aug 29, 2024

WIP to discuss the approach

Is this to brittle / confusing or does it make sense?

Checklist

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

@mr-tz
Copy link
Collaborator Author

mr-tz commented Aug 29, 2024

alternative idea, we use a tiny ELF ARM binary like this instead of writing a manual BinExport proto definition

Screenshot from 2024-08-29 16-59-40

@mr-tz
Copy link
Collaborator Author

mr-tz commented Aug 29, 2024

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.

I think this is a good start, I like how BE2_DICT visualizes our expectations for the breakdown expression, operand, etc.. My concern here is that these tests won't catch if an underlying tool (Ghidra, IDA, etc.) changes how it stores expressions, operands, etc. because we our using our expectations to model the data, which as we've learned, isn't consistent across the tools. Your suggestion of generating BinExport files for smaller programs to use for these tests may be a better fit.

@mr-tz
Copy link
Collaborator Author

mr-tz commented Aug 30, 2024

I'm working on something using a small ELF file for the tests now.

tests/test_binexport_accessors.py Outdated Show resolved Hide resolved
tests/test_binexport_accessors.py Outdated Show resolved Hide resolved
)


def _TODO_test_is_stack_register_expression():
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no stack expressions in the small ELF so using the hardcoded BE2 data here

capa/features/extractors/binexport2/__init__.py Outdated Show resolved Hide resolved
tests/test_binexport_accessors.py Outdated Show resolved Hide resolved
tests/test_binexport_accessors.py Outdated Show resolved Hide resolved
@mr-tz mr-tz mentioned this pull request Sep 4, 2024
24 tasks
@mr-tz mr-tz force-pushed the tests/add-binexport branch from a97afb7 to 7142bf7 Compare September 4, 2024 12:47
),
),
),
# 00210184 02 68 61 38 ldrb w2,[x0, x1, LSL ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

BinaryNinja also sees this as ldrb w2, [x0, x1]. so perhaps when Ghidra sees lsl with no sub expressions (as is the case here), we can omit it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hm, still a bug in Ghidra? anyway, we can ignore LSL #0

02 68 61 38    ldrb w2, [x0, x1]
02 78 61 38    ldrb w2, [x0, x1, lsl #0]

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah ghidra is definitely missing the #0 constant, at least the BinExport is. locally i have a fix, but of course it would be nice for this to be fixed upstream.

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.

recommend we merge this and keep making changes to the BinExport2 PR as necessary.

@mr-tz
Copy link
Collaborator Author

mr-tz commented Sep 11, 2024

sounds good to me

@mr-tz mr-tz merged commit 2c8b3ff into feat/1755 Sep 11, 2024
17 checks passed
@mr-tz mr-tz deleted the tests/add-binexport branch September 11, 2024 08:03
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.

3 participants