Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Move .NET method invocation logging to after the needed type conversion is done for method arguments#25022

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
daxian-dbw merged 6 commits intoPowerShell:masterfromdaxian-dbw:amsilog
Mar 18, 2025

Conversation

daxian-dbw
Copy link
Member

PR Summary

This PR is to fix a MSRC report about a bug in the .NET method logging implementation. The report is not considered a vulnerability, so I'm submitting the fix in GitHub.

The .NET method invocation logging is currently called before PowerShell does any of the needed type conversion for the method arguments, and it results in inaccurate information being logged to AMSI.

The fix is to move .NET method invocation logging to after the needed type conversion is done for method arguments.
The following is the comparison between before and after the fix when turning on the console logging for the AMSI notification feature:

Before the fix -- it logs.GetType(<Foo>)

C:\>pwsh -noprofilePowerShell 7.4.6PS C:\>class Foo {>>     [string] ToString() {>>         return "System.Management.Automation." + ([char[]]@(0x41, 0x6D, 0x73, 0x69, 0x55, 0x74, 0x69, 0x6C, 0x73) -join "")>>     }>> }PS C:\>$utilTypeName = [Foo]::new()=== Amsi notification report content ===<Foo>.new()=== Amsi notification report success: True ====== Amsi notification report content ===<System.Object>.new()=== Amsi notification report success: True ===PS C:\>$utilType = [PSObject].Assembly.GetType($utilTypeName)=== Amsi notification report content ===<System.Reflection.RuntimeAssembly>.GetType(<Foo>)=== Amsi notification report success: True ===PS C:\>exit

After the fix -- it logs.GetType(<System.Management.Automation.AmsiUtils>)

C:\>E:\arena\source\PowerShell\src\powershell-win-core\bin\Debug\net9.0\win7-x64\publish\pwsh.exe -noprofilePowerShell 7.5.0-preview.3-197-g36740ab4a21a1cd51f8cd795f295a86aa5c91d34PS C:\>class Foo {>>     [string] ToString() {>>         return "System.Management.Automation." + ([char[]]@(0x41, 0x6D, 0x73, 0x69, 0x55, 0x74, 0x69, 0x6C, 0x73) -join "")>>     }>> }PS C:\>$utilTypeName = [Foo]::new()=== Amsi notification report content ===<Foo>.new()=== Amsi notification report success: True ====== Amsi notification report content ===<System.Object>.new()=== Amsi notification report success: True ===PS C:\>$utilType = [PSObject].Assembly.GetType($utilTypeName)=== Amsi notification report content ===<System.Reflection.RuntimeAssembly>.GetType(<System.Management.Automation.AmsiUtils>)=== Amsi notification report success: True ===PS C:\>exit

PR Context

PowerShell 7.3 added a new feature that will log any .NET method calls and provide it to the IAntimalwareProvider2.Notify call. As an example running the following will result in the string below being provided to the AMSI provider to check before running

$foo = 'bar'[Console]::WriteLine($foo)
<System.Console>.WriteLine(<bar>)

This allows antimalware providers registered with AMSI to be able to detect the runtime values provided to .NET methods even if they are obfuscated in the code and missed by any static analysis of the scriptblock text being run.

A bug in the implementation means that the provided args in theLogMemberInvocation method that builds the string to provide to AMSI are the ones before they are casted when actually invoking them.

For any .NET method that accepts a string we can now provide a custom class with aToString method that returns the value we want to provide to the .NET call but the AMSI notify string value will just be the class name. With this we can now bypass the checks that may be in place to stop things like the below which would typically be picked up by AMSI due to the presence of the keywordAmsiUtils in the string.

[PSObject].Assembly.GetType("System.Management.Automation.AmsiUtils")

Repro Steps

Run the following PowerShell script

class Foo {    [string] ToString() {        return "System.Management.Automation." + ([char[]]@(0x41, 0x6D, 0x73, 0x69, 0x55, 0x74, 0x69, 0x6C, 0x73) -join "")    }}$utilTypeName = [Foo]::new()$utilType = [PSObject].Assembly.GetType($utilTypeName)

The type name string provided at runtime toGetType isSystem.Management.Automation.AmsiUtils but AMSI will see the following in the member invocation log, masking the true intention of the call.

<System.Reflection.RuntimeAssembly>.GetType(<Foo>)

@daxian-dbwdaxian-dbw added the CL-GeneralIndicates that a PR should be marked as a general cmdlet change in the Change Log labelFeb 15, 2025
@iSazonov

This comment was marked as off-topic.

@daxian-dbw

This comment was marked as off-topic.

@iSazonov

This comment was marked as off-topic.

@trackd
Copy link

Does this address the perf issues with amsi logging?

#24459 for example, theres a few others

iSazonov reacted with thumbs up emoji

@daxian-dbw
Copy link
MemberAuthor

@iSazonov Let's not sidetrack this PR and keep the perf discussion in those specific issues.
@trackd No, this PR has nothing to do with the perf issue.

iSazonov and trackd reacted with thumbs up emoji

@microsoft-github-policy-servicemicrosoft-github-policy-servicebot added the Review - NeededThe PR is being reviewed labelMar 4, 2025
@daxian-dbw
Copy link
MemberAuthor

daxian-dbw commentedMar 13, 2025•
edited
Loading

@SeeminglyScience and@TravisEz13 The comment#25022 (review) was addressed. The PR is ready for another pass of review from both of you. Thanks!

Copy link
Collaborator

@SeeminglyScienceSeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM with one optional suggestion

Copy link
Collaborator

@SeeminglyScienceSeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM!

@daxian-dbwdaxian-dbw merged commita6005f3 intoPowerShell:masterMar 18, 2025
34 checks passed
@daxian-dbw
Copy link
MemberAuthor

/backport to release/v7.4

@microsoft-github-policy-serviceMicrosoft GitHub Policy Service
Copy link
Contributor

microsoft-github-policy-servicebot commentedMar 18, 2025•
edited by unfurl-linksbot
Loading

📣 Hey@daxian-dbw, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗https://aka.ms/PSRepoFeedback

@daxian-dbw
Copy link
MemberAuthor

/backport to release/v7.5

@github-actionsGitHub Actions
Copy link
Contributor

github-actionsbot commentedMar 18, 2025•
edited by unfurl-linksbot
Loading

Started backporting torelease/v7.4:https://github.com/PowerShell/PowerShell/actions/runs/13931074040

GitHub
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

@github-actionsGitHub Actions
Copy link
Contributor

@daxian-dbw backporting to "release/v7.4" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patchApplying: Make sure the arguments used with .NET method logging are after the neededtype conversionUsing index info to reconstruct a base tree...Msrc/System.Management.Automation/engine/runtime/Binding/Binders.csFalling back to patching base and 3-way merge...Auto-merging src/System.Management.Automation/engine/runtime/Binding/Binders.csCONFLICT (content): Merge conflictin src/System.Management.Automation/engine/runtime/Binding/Binders.cserror: Failed to mergein the changes.hint: Use'git am --show-current-patch=diff' to see the failed patchhint: When you have resolved this problem, run"git am --continue".hint: If you prefer to skip this patch, run"git am --skip" instead.hint: To restore the original branch and stop patching, run"git am --abort".hint: Disable this message with"git config set advice.mergeConflict false"Patch failed at 0001 Make sure the arguments used with .NET method logging are after the neededtype conversionError: The process'/usr/bin/git' failed withexit code 128

Please backport manually!

@github-actionsGitHub Actions
Copy link
Contributor

github-actionsbot commentedMar 18, 2025•
edited by unfurl-linksbot
Loading

Started backporting torelease/v7.5:https://github.com/PowerShell/PowerShell/actions/runs/13931076169

GitHub
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

@github-actionsGitHub Actions
Copy link
Contributor

@daxian-dbw backporting to "release/v7.5" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patchApplying: Make sure the arguments used with .NET method logging are after the neededtype conversionUsing index info to reconstruct a base tree...Msrc/System.Management.Automation/engine/runtime/Binding/Binders.csFalling back to patching base and 3-way merge...Auto-merging src/System.Management.Automation/engine/runtime/Binding/Binders.csCONFLICT (content): Merge conflictin src/System.Management.Automation/engine/runtime/Binding/Binders.cserror: Failed to mergein the changes.hint: Use'git am --show-current-patch=diff' to see the failed patchhint: When you have resolved this problem, run"git am --continue".hint: If you prefer to skip this patch, run"git am --skip" instead.hint: To restore the original branch and stop patching, run"git am --abort".hint: Disable this message with"git config set advice.mergeConflict false"Patch failed at 0001 Make sure the arguments used with .NET method logging are after the neededtype conversionError: The process'/usr/bin/git' failed withexit code 128

Please backport manually!

@daxian-dbwdaxian-dbw added CL-EngineIndicates that a PR should be marked as an engine change in the Change Log and removed CL-GeneralIndicates that a PR should be marked as a general cmdlet change in the Change Log labelsMar 24, 2025
pwshBot pushed a commit to pwshBot/PowerShell that referenced this pull requestApr 11, 2025
pwshBot pushed a commit to pwshBot/PowerShell that referenced this pull requestApr 14, 2025
@TravisEz13
Copy link
Member

@PowerShell/powershell-maintainers triage decision - More time in the 7.5 release before considering for 7.4

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

@jborean93jborean93jborean93 left review comments

@SeeminglyScienceSeeminglyScienceSeeminglyScience approved these changes

@TravisEz13TravisEz13Awaiting requested review from TravisEz13

Labels
Backport-7.4.x-MigratedBackport-7.5.x-MigratedCL-EngineIndicates that a PR should be marked as an engine change in the Change Log
Projects
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@daxian-dbw@iSazonov@trackd@TravisEz13@jborean93@SeeminglyScience
[8]ページ先頭
©2009-2025 Movatter.jp