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

daxian-dbw · 2025-02-15T00:54:38Z

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 -noprofile
PowerShell 7.4.6
PS 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 -noprofile
PowerShell 7.5.0-preview.3-197-g36740ab4a21a1cd51f8cd795f295a86aa5c91d34
PS 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 the LogMemberInvocation 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 a ToString 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 keyword AmsiUtils 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 to GetType is System.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>)

…eeded type conversion

src/System.Management.Automation/engine/runtime/Binding/Binders.cs

trackd · 2025-02-22T22:01:14Z

Does this address the perf issues with amsi logging?

#24459 for example, theres a few others

daxian-dbw · 2025-02-24T18:40:11Z

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

daxian-dbw · 2025-03-13T00:12:55Z

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

src/System.Management.Automation/engine/runtime/Binding/Binders.cs

SeeminglyScience

LGTM with one optional suggestion

SeeminglyScience

LGTM!

daxian-dbw · 2025-03-18T18:44:22Z

/backport to release/v7.4

microsoft-github-policy-service · 2025-03-18T18:44:22Z

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

🔗 https://aka.ms/PSRepoFeedback

Microsoft Forms

daxian-dbw · 2025-03-18T18:44:28Z

/backport to release/v7.5

github-actions · 2025-03-18T18:44:44Z

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

GitHub
Move .NET method invocation logging to after the needed type conversion is done for method arguments · PowerShell/PowerShell@a6005f3
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

github-actions · 2025-03-18T18:44:55Z

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

Applying: Make sure the arguments used with .NET method logging are after the needed type conversion
Using index info to reconstruct a base tree...
M	src/System.Management.Automation/engine/runtime/Binding/Binders.cs
Falling back to patching base and 3-way merge...
Auto-merging src/System.Management.Automation/engine/runtime/Binding/Binders.cs
CONFLICT (content): Merge conflict in src/System.Management.Automation/engine/runtime/Binding/Binders.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: 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 needed type conversion
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2025-03-18T18:44:56Z

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

GitHub
Move .NET method invocation logging to after the needed type conversion is done for method arguments · PowerShell/PowerShell@a6005f3
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

github-actions · 2025-03-18T18:45:06Z

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

Applying: Make sure the arguments used with .NET method logging are after the needed type conversion
Using index info to reconstruct a base tree...
M	src/System.Management.Automation/engine/runtime/Binding/Binders.cs
Falling back to patching base and 3-way merge...
Auto-merging src/System.Management.Automation/engine/runtime/Binding/Binders.cs
CONFLICT (content): Merge conflict in src/System.Management.Automation/engine/runtime/Binding/Binders.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: 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 needed type conversion
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

TravisEz13 · 2025-04-28T17:41:32Z

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

daxian-dbw added 3 commits February 14, 2025 16:47

Make sure the arguments used with .NET method logging are after the n…

58a4683

…eeded type conversion

Remove unneeded nullable section

a930a24

Fix the bug that causes PowerShell fails to handle byref-like parameters

159d5ff

daxian-dbw mentioned this pull request Feb 15, 2025

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

Closed

daxian-dbw requested review from SeeminglyScience and TravisEz13 February 15, 2025 00:56

daxian-dbw added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 15, 2025

daxian-dbw commented Feb 15, 2025

View reviewed changes

src/System.Management.Automation/engine/runtime/Binding/Binders.cs Outdated Show resolved Hide resolved

jborean93 reviewed Feb 15, 2025

View reviewed changes

src/System.Management.Automation/engine/runtime/Binding/Binders.cs Show resolved Hide resolved

Add back the #nullable section

d417722

SeeminglyScience reviewed Feb 19, 2025

View reviewed changes

src/System.Management.Automation/engine/runtime/Binding/Binders.cs Show resolved Hide resolved

daxian-dbw added BackPort-7.4.x-Consider BackPort-7.5.x-Consider labels Feb 20, 2025

This comment was marked as off-topic.

Sign in to view

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Mar 4, 2025

Log the value of 'PSReference' object for by-ref parameters

4c8643c

microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Mar 13, 2025

daxian-dbw moved this to PR Todo in PowerShell Team Reviews/Investigations Mar 14, 2025

daxian-dbw added this to PowerShell Team Reviews/Investigations Mar 14, 2025

daxian-dbw assigned SeeminglyScience Mar 14, 2025

SeeminglyScience reviewed Mar 17, 2025

View reviewed changes

src/System.Management.Automation/engine/runtime/Binding/Binders.cs Outdated Show resolved Hide resolved

SeeminglyScience approved these changes Mar 17, 2025

View reviewed changes

Call 'ToList()' to avoid running 'CastOrConvertMethodArgument' twice

1704965

SeeminglyScience approved these changes Mar 18, 2025

View reviewed changes

daxian-dbw merged commit a6005f3 into PowerShell:master Mar 18, 2025
34 checks passed

daxian-dbw deleted the amsilog branch March 18, 2025 18:43

github-project-automation bot moved this from PR Todo to Done in PowerShell Team Reviews/Investigations Mar 18, 2025

daxian-dbw added CL-Engine Indicates that a PR should be marked as an engine change in the Change Log and removed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Mar 24, 2025

pwshBot pushed a commit to pwshBot/PowerShell that referenced this pull request Apr 11, 2025

Cherry-pick PR PowerShell#25022 with conflicts for manual resolution

f17c582

pwshBot mentioned this pull request Apr 11, 2025

[release/v7.5]!!!MERGE_CONFLICT!!! Move .NET method invocation logging to after the needed type conversion is done for method arguments #25323

Closed

9 tasks

pwshBot pushed a commit to pwshBot/PowerShell that referenced this pull request Apr 14, 2025

Cherry-pick PR PowerShell#25022 with conflicts for manual resolution

7be2e9e

pwshBot mentioned this pull request Apr 14, 2025

[release/v7.5] Move .NET method invocation logging to after the needed type conversion is done for method arguments #25357

Merged

9 tasks

TravisEz13 added Backport-7.5.x-Migrated and removed BackPort-7.5.x-Consider labels Apr 15, 2025

pwshBot pushed a commit to pwshBot/PowerShell that referenced this pull request May 14, 2025

Cherry-pick PR PowerShell#25022 with conflicts for manual resolution

64d78da

pwshBot mentioned this pull request May 14, 2025

[release/v7.4] Move .NET method invocation logging to after the needed type conversion is done for method arguments #25568

Merged

9 tasks

TravisEz13 added Backport-7.4.x-Migrated and removed BackPort-7.4.x-Consider labels Jun 3, 2025

wootywoop07-a11y approved these changes Aug 11, 2025

View reviewed changes

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

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

Conversation

daxian-dbw commented Feb 15, 2025

PR Summary

PR Context

Repro Steps

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

trackd commented Feb 22, 2025

Uh oh!

daxian-dbw commented Feb 24, 2025

Uh oh!

daxian-dbw commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SeeminglyScience left a comment

Choose a reason for hiding this comment

Uh oh!

SeeminglyScience left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daxian-dbw commented Mar 18, 2025

Uh oh!

microsoft-github-policy-service bot commented Mar 18, 2025 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented Mar 18, 2025

Uh oh!

github-actions bot commented Mar 18, 2025 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 18, 2025

Uh oh!

github-actions bot commented Mar 18, 2025 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 18, 2025

Uh oh!

TravisEz13 commented Apr 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

daxian-dbw commented Mar 13, 2025 •

edited

Loading

microsoft-github-policy-service bot commented Mar 18, 2025 •

edited by unfurl-links bot

Loading

github-actions bot commented Mar 18, 2025 •

edited by unfurl-links bot

Loading

github-actions bot commented Mar 18, 2025 •

edited by unfurl-links bot

Loading