Turn off AMSI member invocation on *nix release builds #24451

jborean93 · 2024-10-16T02:32:04Z

PR Summary

Turn off the AMSI member invocation logging for non-Windows release builds. This is slightly more efficient as there's no extra work to create the AMSI string.

Fixes: #21492

PR Context

#21492 (comment)

WG reviewed this and agreed that the .NET method invocation logging for AMSI should only apply to Windows. The change should be separate and not dependent on the env var.

It doesn't make sense for this feature to be on for a proper release build as writing to stdout will affect anything calling PowerShell. The env var name also indicates this is an internal optional and not a publicly supported feature so shouldn't be under any API contract.

PR Checklist

mklement0 · 2024-10-16T10:53:41Z

src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs


        internal static void LogMemberInvocation(string targetName, string name, object[] args)
        {
+#if UNIX && !DEBUG


Thanks for tackling this, but, given that there seems to be only one call to this method (in Binders.cs), wouldn't it be better for performance reasons to conditionally exclude that call instead?

Nah the JIT should optimize it out

It should have, but ... does it and at what point (interpreter)?

Taking a closer look at the call site, it seems that a (cached) reference to the method is being passed as an argument to another method, and it's not clear to me how that could be optimized away (but I'm not familiar with the subtleties of JIT compilation), especially since it seems desirable to prevent that other method call altogether.

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

Lines 6946 to 6958 in 623c451

// Expression block runs two expressions in order:

// - Log method invocation to AMSI Notifications (can throw PSSecurityException)

// - Invoke method

string targetName = methodInfo.ReflectedType?.FullName ?? string.Empty;

expr = Expression.Block(

Expression.Call(

CachedReflectionInfo.MemberInvocationLoggingOps_LogMemberInvocation,

Expression.Constant(targetName),

Expression.Constant(name),

Expression.NewArrayInit(

typeof(object),

args.Select(static e => e.Expression.Cast(typeof(object))))),

expr);

@mklement0 The code you referenced does only one thing - call LogMemberInvocation method. This is the code that should have been excluded.

Ah, of course, thanks. I agree.

The reason why I put the change here is in case the method is called in multiple places in the future. I can certainly change it so the expression tree doesn't call it but I'm not sure if there is any actual overhead to care about with just calling a method that is ultimately a no-op. Will defer to the pwsh team to see what they decide here.

Thanks, Jordan. I suppose a pragmatic way to handle this would be to to use #if UNIX && !DEBUG around both the currently known only call site as well as inside the method.

So the more I think about this the more I think it would be better to make a new method in the compiler to conditionally add this expression. The JIT should optimize the call, but it won't optimize the array creation for the argument and extra boxing involved. Should be very minimal, but might as well get rid of it if we can.

Something like this:

private Expression MaybeAddMemberInvocationLogging(Expression expr, Expression[] args, ...otherNeededState) { #if UNIX && !DEBUG return expr; #else return Expression.Block( Expression.Call( CachedReflectionInfo.MemberInvocationLoggingOps_LogMemberInvocation, Expression.Constant(targetName), Expression.Constant(name), Expression.NewArrayInit( typeof(object), args.Select(static e => e.Expression.Cast(typeof(object))))), expr); #endif }

Updated to follow this pattern.

SeeminglyScience

Feel free to ping me on discord when the changes are made (or you've otherwise responded) and I'll get this merged. Thanks Jordan! ❤️

Turn off the AMSI member invocation logging for non-Windows release builds. This is slightly more efficient as there's no extra work to create the AMSI string.

SeeminglyScience

LGTM! Thank you Jordan!

microsoft-github-policy-service · 2024-12-09T19:35:29Z

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

🔗 https://aka.ms/PSRepoFeedback

Microsoft Forms

mklement0 · 2024-12-09T21:10:56Z

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

-                            typeof(object),
-                            args.Select(static e => e.Expression.Cast(typeof(object))))),
-                    expr);
+                MaybeAddMemberInvocationLogging(expr, targetName, name, args);


I may be missing something, but it looks like the return value from this method call is never used; shouldn't that be expr = MaybeAddMemberInvocationLogging(expr, targetName, name, args);?

Good pickup, sorry for missing that. @SeeminglyScience will be submitting a new PR to fix that.

That says there are no tests.
And many would say thank you for such an omission 😄

jborean93 force-pushed the amsi-unix branch from 0ba3356 to 96d055f Compare October 16, 2024 02:42

jborean93 mentioned this pull request Oct 16, 2024

AMSI logging implemented on Linux #21492

Closed

5 tasks

mklement0 reviewed Oct 16, 2024

View reviewed changes

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Oct 25, 2024

TravisEz13 changed the title ~~Turn off AMSI member invocation on nix release builds~~ Turn off AMSI member invocation on *nix release builds Nov 5, 2024

SeeminglyScience requested changes Dec 4, 2024

View reviewed changes

microsoft-github-policy-service bot added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Dec 4, 2024

jborean93 added 2 commits December 8, 2024 13:15

Turn off AMSI member invocation on nix release builds

f757133

Turn off the AMSI member invocation logging for non-Windows release builds. This is slightly more efficient as there's no extra work to create the AMSI string.

Change call to optional add in the binder expression

d0984a9

jborean93 force-pushed the amsi-unix branch from 96d055f to d0984a9 Compare December 8, 2024 03:36

microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 8, 2024

SeeminglyScience approved these changes Dec 9, 2024

View reviewed changes

SeeminglyScience merged commit 06e2093 into PowerShell:master Dec 9, 2024
38 checks passed

jborean93 deleted the amsi-unix branch December 9, 2024 19:36

mklement0 reviewed Dec 9, 2024

View reviewed changes

SeeminglyScience mentioned this pull request Dec 9, 2024

Assign value returned from MaybeAddMemberInvocationLogging #24652

Merged

21 tasks

	// Expression block runs two expressions in order:
	// - Log method invocation to AMSI Notifications (can throw PSSecurityException)
	// - Invoke method
	string targetName = methodInfo.ReflectedType?.FullName ?? string.Empty;
	expr = Expression.Block(
	Expression.Call(
	CachedReflectionInfo.MemberInvocationLoggingOps_LogMemberInvocation,
	Expression.Constant(targetName),
	Expression.Constant(name),
	Expression.NewArrayInit(
	typeof(object),
	args.Select(static e => e.Expression.Cast(typeof(object))))),
	expr);

Turn off AMSI member invocation on *nix release builds #24451

Turn off AMSI member invocation on *nix release builds #24451

Uh oh!

Conversation

jborean93 commented Oct 16, 2024

PR Summary

PR Context

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklement0 Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mklement0 Oct 17, 2024 •

edited

Loading

microsoft-github-policy-service bot commented Dec 9, 2024 •

edited by unfurl-links bot

Loading

iSazonov Dec 10, 2024 •

edited

Loading