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

VMRay and dynamic improvements #2537

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

VMRay and dynamic improvements #2537

wants to merge 5 commits into from

Conversation

mr-tz
Copy link
Collaborator

@mr-tz mr-tz commented Dec 12, 2024

A few improvements based on analysis of around 200 random VMRay archives.

Checklist

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

Comment on lines +196 to +200
assert (sv2p.pid, sv2p.monitor_id, sv2p.origin_monitor_id) == (
vmray_monitor_process.pid,
vmray_monitor_process.monitor_id,
vmray_monitor_process.origin_monitor_id,
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thoughts on being even more lenient here and not asserting this but just reporting it?
I'm encountering more inconsistencies between the two files, e.g., the monitor_id not being set

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so we could do the check and just log it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm hesitant to be lenient here because we rely on sane process and thread monitor IDs for indexing. I'd consider differences between SummaryV2.json and flog.xml to be VMRay bugs and, if true, can we trust capa's results at that point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, let's leave as is until we find more samples that fail.

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.

Great improvements, thank you! I've left a few comments for your review.

capa/features/extractors/vmray/__init__.py Outdated Show resolved Hide resolved
capa/features/extractors/vmray/models.py Outdated Show resolved Hide resolved
capa/main.py Show resolved Hide resolved
Comment on lines +196 to +200
assert (sv2p.pid, sv2p.monitor_id, sv2p.origin_monitor_id) == (
vmray_monitor_process.pid,
vmray_monitor_process.monitor_id,
vmray_monitor_process.origin_monitor_id,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm hesitant to be lenient here because we rely on sane process and thread monitor IDs for indexing. I'd consider differences between SummaryV2.json and flog.xml to be VMRay bugs and, if true, can we trust capa's results at that point?

Copy link
Collaborator Author

@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.

addressed feedback

Comment on lines +196 to +200
assert (sv2p.pid, sv2p.monitor_id, sv2p.origin_monitor_id) == (
vmray_monitor_process.pid,
vmray_monitor_process.monitor_id,
vmray_monitor_process.origin_monitor_id,
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, let's leave as is until we find more samples that fail.

@@ -276,7 +276,7 @@ class ElfFileHeader(BaseModel):

class ElfFile(BaseModel):
# file_header: ElfFileHeader
sections: Optional[list[ElfFileSection]] = None
sections: list[ElfFileSection] = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

incidentally, is this the correct way to set the default value, particularly as a list? i see this pattern used throughout the file.

my worry is that the default value = [] uses the same instance of a mutable list, rather than copies of it. sorta like when you have a kwarg parameter def foo(bar=[]).

in the past, i've used pydantic.Field for these. but maybe pydantic is extra smart and doesn't require this. @mr-tz @mike-hunhoff

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, great question, this is how mypy accepted the change and I saw the pattern throughout. Other files use Optional[list[<foo>]] = None or Field, we should cleanup the inconsistencies (separately).

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