Create directory to group native samples #2623
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @hcnpeiris, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses issue capa#1787 by creating a directory structure to group native samples within the test data. The primary change involves modifying file paths in tests/fixtures.py to reflect the new directory structure, which includes segregating samples into benign, malware, and pma_labs subdirectories. Additionally, file paths in tests/test_binexport_accessors.py, tests/test_elffile_features.py, and tests/test_scripts.py have been updated to align with the new directory structure. The author notes that one expected failure test unexpectedly passed after the file path updates and is seeking feedback on this issue.
Highlights
- Directory Restructuring: Creates a new directory structure under
tests/datato group native samples intobenign,malware, andpma_labssubdirectories. - File Path Updates: Updates file paths in
tests/fixtures.py,tests/test_binexport_accessors.py,tests/test_elffile_features.py, andtests/test_scripts.pyto reflect the new directory structure. - Unexpected Test Result: An expected failure test unexpectedly passed after the file path updates, requiring further investigation.
Changelog
Click here to see the changelog
- tests/fixtures.py
- Modified search paths for binexport2 extractor to include
data/malware. - Updated file paths for various test samples to reflect the new directory structure (e.g., moving mimikatz to
data/benign, kernel32 todata/malware, and PMA labs todata/pma_labs). - Updated file paths for various malware samples.
- Modified search paths for binexport2 extractor to include
- tests/test_binexport_accessors.py
- Updated the file path for the mimikatz binexport extractor to point to the new location in
data/benign.
- Updated the file path for the mimikatz binexport extractor to point to the new location in
- tests/test_elffile_features.py
- Updated sample paths for ELF files to reflect the new location in
data/malware.
- Updated sample paths for ELF files to reflect the new location in
- tests/test_scripts.py
- Updated the binary file path and binexport2 file path to reflect the new locations in
data/malwareanddata/benignrespectively. - Updated the CAPA_SAMPLES_DIR environment variable to point to the
data/benigndirectory for binexport scripts. - Updated the source file path for bulk processing tests to reflect the new location in
data/malware.
- Updated the binary file path and binexport2 file path to reflect the new locations in
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
The tests did run, the paths aligned,
A structure new, carefully designed.
But a test that failed, now sees the light,
A puzzle to solve, both day and night.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request aims to improve the organization of test files by grouping native samples into a dedicated directory. The changes primarily involve updating file paths in the test suite. The author has identified an unexpected test pass and is seeking feedback on resolving it. Overall, the changes seem reasonable, but the identified test issue needs further investigation.
Summary of Findings
- Inconsistent file categorization: The categorization of files as 'benign' or 'malware' seems inconsistent. For example, mimikatz is categorized as benign, which might not be accurate in all contexts. Review the categorization to ensure it aligns with the intended use case.
- Unexpected test pass: The pull request description mentions that one expected failure test unexpectedly passed. While the author notes that this also occurs on the latest master branch, it's important to investigate and address this issue to ensure tests are behaving as expected.
Merge Readiness
The pull request should not be merged until the unexpected test pass is investigated and resolved. While the file path updates seem correct, the test results indicate a potential underlying issue that needs to be addressed. I am unable to approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the high severity issue should be addressed before merging.
tests/fixtures.py
Outdated
| if name == "mimikatz": | ||
| return CD / "data" / "mimikatz.exe_" | ||
| return CD / "data" / "benign" / "mimikatz.exe_" |
There was a problem hiding this comment.
Is mimikatz truly benign? While it can be used for legitimate purposes, it's often associated with malicious activity. Consider whether it should be categorized as malware or have a separate 'potentially unwanted program' category.
tests/test_binexport_accessors.py
Outdated
| @@ -593,7 +593,7 @@ def test_pattern_matching_not_stack(): | |||
| assert match_address_with_be2(BE2_EXTRACTOR_687, queries, 0x107918) is None | |||
|
|
|||
|
|
|||
| BE2_EXTRACTOR_MIMI = fixtures.get_binexport_extractor(CD / "data" / "binexport2" / "mimikatz.exe_.ghidra.BinExport") | |||
| BE2_EXTRACTOR_MIMI = fixtures.get_binexport_extractor(CD / "data" / "benign" / "binexport2" / "mimikatz.exe_.ghidra.BinExport") | |||
There was a problem hiding this comment.
Similar to the previous comment, is mimikatz truly benign in this context? Ensure the categorization aligns with the test's purpose.
tests/test_scripts.py
Outdated
| @@ -96,7 +96,7 @@ def test_scripts(script, args): | |||
| ) | |||
| def test_binexport_scripts(script, args): | |||
| # define sample bytes location | |||
| os.environ["CAPA_SAMPLES_DIR"] = str(Path(CD / "data")) | |||
| os.environ["CAPA_SAMPLES_DIR"] = str(Path(CD / "data" / "benign" )) | |||
There was a problem hiding this comment.
Why is CAPA_SAMPLES_DIR set to the benign directory here? Does this mean that the binexport scripts are only intended to be run on benign samples?
|
@mr-tz Here is the |
|
I'll take another closer look and think about the naming. Maybe we add OS or arch but I'd like to keep it simple. |
|
Hi @mr-tz, just checking in — any updates on the PR or anything you’d like me to adjust? |
|
Hey, sorry it may take a bit on my end. Thanks for the ping. |
Create directory to group native samples
Fixes capa#1787
Depends on: mandiant/capa-testfiles#279
Since after changing the native samples path, pytest file paths have been updated accordingly
However, one expected failure test (xfailed) unexpectedly passed.
I ran
pyteston the latest master branch, and it produced the same result.Pytest Results After Updating File Paths
Pytest Results for latest master branch
Request for Feedback
Can I get feedback on resolving this issue?
Checklist