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

Fixed infinite loop when parsing dotnet TypeRef table #2045

Closed
wants to merge 1 commit into from

Conversation

x9090
Copy link

@x9090 x9090 commented Apr 2, 2024

There was a TypeRef table infinite loop issue when dotnet parser parsing a crafted dotnet sample with ref index refer to each other:

problematic-dotnet
 
Let me know if you need the sample for testing, I could upload it here.

Checklist

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

Copy link

google-cla bot commented Apr 2, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please add bug fixes, new features, breaking changes and anything else you think is worthwhile mentioning to the master (unreleased) section of CHANGELOG.md. If no CHANGELOG update is needed add the following to the PR description: [x] No CHANGELOG update needed

@github-actions github-actions bot dismissed their stale review April 2, 2024 08:10

CHANGELOG updated or no update needed, thanks! 😄

@mr-tz mr-tz requested a review from mike-hunhoff April 2, 2024 10:16
@mike-hunhoff
Copy link
Collaborator

There was a TypeRef table infinite loop issue when dotnet parser parsing a crafted dotnet sample with ref index refer to each other:

problematic-dotnet   Let me know if you need the sample for testing, I could upload it here.

Checklist

  • No CHANGELOG update needed

  • No new tests needed

  • No documentation update needed

Hi @x9090 , thank you for the find and suggested fix - apologies for not getting back to you sooner! Please update the sample for testing and review the CLA requirements so we can move this PR forward.

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.

Thanks @x9090 , I've left comments for your review 🚀

@@ -370,6 +370,7 @@ def resolve_nested_typeref_name(
# If the ResolutionScope decodes to a typeRef type then it is nested
if isinstance(typeref.ResolutionScope.table, dnfile.mdtable.TypeRef):
typeref_name = []
typeref_tb = []
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 not sure what _tb means, please add a comment or consider updating the name to something more obvious like _visited.

typeref_name.append(name)
name = table_row.TypeName
table_row = get_dotnet_table_row(pe, dnfile.mdtable.TypeRef.number, table_row.ResolutionScope.row_index)
if table_row is None:
if table_row is None or table_row in typeref_tb:
Copy link
Collaborator

@mike-hunhoff mike-hunhoff Apr 15, 2024

Choose a reason for hiding this comment

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

We use similar logic in resolve_nested_typedef_name. What are your thoughts on also adding this check there?

@williballenthin williballenthin added this to the v7.1 milestone Jun 7, 2024
@williballenthin
Copy link
Collaborator

@x9090 would you please sign the CLA so that we can merge this PR into capa? We'd love to get it in as part of the v7.1 release soon.

@mr-tz
Copy link
Collaborator

mr-tz commented Jun 11, 2024

friendly bump, @x9090

@williballenthin
Copy link
Collaborator

Without the CLA signed, we cannot merge this PR.

I haven't been able to find the file shown in the screenshot on VT, so I can't reproduce this nor reimplement it.

Perhaps we should close this PR until @x9090 returns?

@mr-tz
Copy link
Collaborator

mr-tz commented Jun 13, 2024

yes, let's wait for that or other people raising this issue

@mr-tz mr-tz closed this Jun 13, 2024
@r0ny123
Copy link

r0ny123 commented Jun 13, 2024

I haven't been able to find the file shown in the screenshot on VT, so I can't reproduce this nor reimplement it.

Can we hunt for it on VT using a YARA rule? :)

@williballenthin
Copy link
Collaborator

I did some VTGrep searches for the random looking strings in the screenshot and didn't come up with anything. Have you had any luck?

@r0ny123
Copy link

r0ny123 commented Jun 13, 2024

I mean crafting a YARA for that specific behaviour mentioned. Possible?

@williballenthin
Copy link
Collaborator

maybe by using the Yara .NET extension.

It might be easier to manually craft a file by hand: just tweak two bytes (the table references).

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.

5 participants