PowerShell / PowerShell

PowerShell for every system!
https://microsoft.com/PowerShell
MIT License
43.56k stars 7.06k forks source link

What programming guidelines are there for PowerShell against the AMSI threat vector? #21536

Open rhubarb-geek-nz opened 3 weeks ago

rhubarb-geek-nz commented 3 weeks ago

As far as I can tell, through experiment, PowerShell will happily live stream all method invocations and arguments through AMSI.

What programming guidelines do the PowerShell Team have to this novel programming model where nothing is private, your machine is no longer your machine and everything you do in PowerShell can be seen by others?

Previously PowerShell began life as a systems administration tool, and could be used to manage CA and SSL certificate issuance, eg Lets Encrypt, connection to databases and other general administrative tasks that are traditionally considered private. What advice do you have when you see your previously private data in logs or the event viewer? The data in question was not in the script, it came from other sources traditionally considered private.

Can PowerShell ever be used for GDPR, PCI-DSS or in similar scenarios given PowerShell shows no control or restraint over broadcasting information?

rhubarb-geek-nz commented 3 weeks ago

Summary of the story so far.

We were looking at an issue of Base64 decoding take much more time and resources than it should.

Take a very simple PowerShell script with method invocation

#!/usr/bin/env pwsh
$env:__PSDumpAMSILogContent='1'
$base64 = [System.Convert]::ToBase64String(([byte]1,[byte]2,[byte]3))
[System.Convert]::FromBase64String($base64) | Format-Hex

The problem with this is it is logging the contents of variables that are supposed to be private to the process.

=== Amsi notification report content ===
<System.Convert>.ToBase64String(<System.Object[]>)
=== Amsi notification report success: True ===

=== Amsi notification report content ===
<System.Convert>.FromBase64String(<AQID>)
=== Amsi notification report success: True ===

So any time you use method invocation, your data is at risk of leak. Eg

#!/usr/bin/env pwsh
$env:__PSDumpAMSILogContent='1'
$ConnectionString = 'Data Source=localhost;Integrated Security=False;Persist Security Info=False;User ID=sa;Password=changeit'
$connection = [System.Data.SqlClient.SqlConnection]::new($ConnectionString)

Results in

=== Amsi notification report content ===
<System.Data.SqlClient.SqlConnection>.new(<Data Source=localhost;Integrated Security=False;Persist Security Info=False;User ID=sa;Password=changeit>)
=== Amsi notification report success: True ===

New-Object does not leak in this manner, you would be far better off using New-Object rather than type::new

So the next step was how can we prevent this leaking. In the case of Base64 the solution was to put the method call we wanted in a cmdlet. Hence rhubarb-geek-nz.Base64String which provides ConvertTo-Base64 and ConvertFrom-Base64 without the values being logged.

Now having access to a large API and having to duplicate cmdlets inorder to safely call each method is not a practical solution. Fortunately, we can simply put the reflection invocation in a cmdlet and use that where ever the standard method invocation would have been used. So we have rhubarb-geek-nz.OnReflection which means we can use a single Invoke-Reflection to pass the arguments ( as list or dictionary ).

$base64 = Invoke-Reflection -Method ToBase64String -Type ([System.Convert]) -ArgumentList @(,[byte[]](1,2,3))
Invoke-Reflection -Method FromBase64String -Type ([System.Convert]) -ArgumentDictionary @{ s = [string]$base64 } | Format-Hex

No logging occurs.

This mean new code can be written that does not leak variables, but this does not solve existing code.

This is easily demonstrated with

#!/usr/bin/env pwsh
$env:__PSDumpAMSILogContent='1'
Invoke-FooBar

From this you get a mountain containing

<System.String>.IsNullOrEmpty(<null>)
<System.String>.Substring(<1>)
<System.String>.Replace(<
<System.String>.Split(<
<System.Text.StringBuilder>.new()
<System.Text.StringBuilder>.Append(<  ....

Heaven forbid that we dare to manipulate strings in PowerShell code. This code that is being logged came with PowerShell, it is reporting on itself, the Stasi would be proud.

So the next step to prevent the leaking from existing code is rhubarb-geek-nz/powershell-amsi.

This prevents this logging by adding an amsi.dll to the PowerShell installation directory.

Path                                   FileVersion SignerCertificate                        StatusMessage
----                                   ----------- -----------------                        -------------
C:\Program Files\PowerShell\7\pwsh.exe 7.4.2.500   F9A7CF9FBE13BAC767F4781061332DA6E8B4E0EE Signature verified.
C:\Program Files\PowerShell\7\amsi.dll 1.0.0.0     601A8B683F791E51F647D34AD102C38DA4DDB65F Signature verified.

In summary, I believe there is a very real problem. The logging of method invocations and the arguments is far beyond what is required. ScriptBlocks are being logged, method invocations did not magically appear, they came from already running scripts. If the scripts came from the file system then PowerShell has no need to scan them on every invocation because Windows Defender already does static file scanning.

If PowerShell is going to continue to log all method invocations then we need guidance from the PowerShell Team on how we are expected to write scripts that protect the data they use.

MartinGC94 commented 3 weeks ago

I don't understand the actual issue here. You don't like the fact that AMSI logs sensitive arguments like base64 encoded secrets, right? But why is that? AMSI is used by anti malware software which tends to have enough privileges to read the memory anyway, so what difference does it make if PowerShell provides the data with AMSI, or the software reads it on its own? I mean if you can't trust the AV to handle the AMSI data correctly then you can't trust having it on your system in the first place.

rhubarb-geek-nz commented 3 weeks ago

I don't understand the actual issue here. You don't like the fact that AMSI logs sensitive arguments like base64 encoded secrets, right? But why is that?

It is not a case of "ooh ooh scary base64 data ooh ooh", it is every string parameter passed to every .NET method call, so that includes database credentials, transaction data with SQL, doing what in the real world we call "real business with real data". I want absolutely no chance of that data going to AV or any logging.

If that means PowerShell is not fit for purpose I am happy to accept that answer and move on to find alternatives.

rhubarb-geek-nz commented 3 weeks ago

I mean if you can't trust the AV to handle the AMSI data correctly then you can't trust having it on your system in the first place.

Yes, I did originally ask how it can be disabled within a PowerShell process.

MartinGC94 commented 3 weeks ago

Let's say the PowerShell team completely removes the AMSI API from the product, what then? Do you think the AV software will just ignore the PowerShell process and let it do whatever it wants? I doubt it, they would most likely try to add hooks or something so they can monitor the code that gets executed.

rhubarb-geek-nz commented 3 weeks ago

I think you and I have very different views on how security works in a computing environment. If I write a Java or C# program, AV does not get an automatic 'in' to see what is going on. If it did then it s game over, and why were we worrying about Spectre and other potential leaks across process boundaries. I had an expectation of privacy within a process that PowerShell does not respect and will deliberately subvert.

Sure if you install some gaming anti-cheat software you give it carte-blanche to inspect your processes, but who in their right mind would install that kind of software in a business environment.

I am not asking for you to stop using AV, I am asking to stop the method by method reporting of arguments. Sure run AV on source scripts used in script blocks if you must, that is no different to AV checking the file system. But not on the data values.

In production environments we have log files that deliberately have all sensitive fields removed. This method argument reporting is completely overriding that, PowerShell has no concept of what is public, private or sensitive. It is just gaily reporting it all.

rhubarb-geek-nz commented 3 weeks ago

I had written a PowerShell module in PowerShell assuming it was the right thing to do,

It turns out that because that uses method invocation it reports everything through the pipeline to AV.

The worst offender is this line

           $pipe.Process($InputObject)

which ends up as

=== Amsi notification report content ===
<System.Management.Automation.SteppablePipeline>.Process(<whatever is going through the pipeline>)
=== Amsi notification report success: True ===

So even if your code is not using method invocation, some other component may, and unless you turn the logging flag on you may have no idea.

So I now need to rewrite that module in C#. And that is how silly this is. If I rewrite the module in C# I don't then have method invocation of arguments logged with AV, which is what I have been asking for. But that will only fix that one module, every other module written in PowerShell in PSGallery using method invocation will still be logging data values to AV.

Update - module is now rewritten in C#.

rhubarb-geek-nz commented 2 weeks ago

In case you still think this

Here is a PowerShell module downloaded 782 times.

Install on 7.4.2

PowerShell 7.4.2
PS> Install-Module gibbels-algorithms

Untrusted repository
You are installing the modules from an untrusted repository. If you trust this repository, change its InstallationPolicy value by running the Set-PSRepository cmdlet. Are you
sure you want to install the modules from 'PSGallery'?
[Y] Yes  [A] Yes to All  [N] No  [L] No to All  [S] Suspend  [?] Help (default is "N"): Y
PS /home/bythesea> Get-Command Test-LuhnValidation

CommandType     Name                                               Version    Source
-----------     ----                                               -------    ------
Function        Test-LuhnValidation                                1.0.4      gibbels-algorithms

Now I ran this in a Debian Bookworm docker because the AMSI logging is still there even though it does not actually call the Windows only AMSI.DLL

In luhn.ps1 with executable bit set

#!/usr/bin/env pwsh
$env:__PSDumpAMSILogContent='1'
Test-LuhnValidation -Number 4024007106418766
Test-LuhnValidation -Number 374519847840029
Test-LuhnValidation -Number 5353760959262719

Run from bash to set __PSDumpAMSILogContent on startup

$ ./luhn.ps1

=== Amsi notification report content ===
<System.String>.ToCharArray()
=== Amsi notification report success: False ===

=== Amsi notification report content ===
<System.Int32>.Parse(<6>)
=== Amsi notification report success: False ===

=== Amsi notification report content ===
<System.Int32>.Parse(<6>)
=== Amsi notification report success: False ===
...
etc

So neatly between the ToCharArray calls we have the credit card digits in reverse order being sent to AMSI. I am sure the PCI-DSS Compliance report would be very interesting explaining that one.

To be fair to the author of the module, in 2016 they had no idea PowerShell was going to pull a stunt like this. The logging only went into 7.3 in 2023.

Now that was not an exhaustive search, it took me 15 seconds in PSGallery to find that example.

And that is before we look for code using StringBuilder.Append to create the payload for the payment gateway.

Update - Might as well recover the credit card numbers from the log.

#!/usr/bin/env pwsh
param($file = 'amsi.log')

Get-Content -LiteralPath $file | Where-Object { $_ -Match 'System.' } | ForEach-Object { $_.Replace('>)','').Replace('<System.Int32>.Parse(<','') } | ForEach-Object {
                $number = ''
        } {
                if ($_ -Match 'ToCharArray')
                {
                        if ($number)
                        {
                                $number
                                $number = ''
                        }
                }
                else
                {
                        $number = "$_$number"
                }
        } {
                if ($number)
                {
                        $number
                }
        }

Here we are

$ ./recover.ps1
4024007106418766
374519847840029
5353760959262719

Now if you want to check that you recovered those number correctly, then good news, there is a Test-LuhnValidation available in the PSGallery.

SydneyhSmith commented 1 week ago

Thanks for the discussion, the information sent is to detect malware... if you don't want this information sent you need to disable your anti-malware...what is sent is determined by this api not by PowerShell... thanks!

rhubarb-geek-nz commented 1 week ago

Thanks for the discussion, the information sent is to detect malware... if you don't want this information sent you need to disable amsi...what is sent is determined by this api not by PowerShell... thanks!

Are you serious? I think my confidence in PowerShell as a serious project has hit zero with this response. What you have implemented is security theatre, as a tragedy, not a comedy.

Can you please provide this advice as a clear instruction, that if you use PowerShell in an environment with confidential information then you must disable anti-virus at a machine level in order to prevent leakage of information from PowerShell scripts. The option POWERSHELL_TELEMETRY_OPTOUT has no effect on this.

Alternatively, use either Linux, Windows Power Shell 5.1, or Power Shell 7.2.

rhubarb-geek-nz commented 1 week ago

I am expecting the 24 hour bot to sweep this under the carpet soon so everyone can pretend to forget about this 'feature'.

I did make the mistake of trying to present technical arguments but after reviewing the PRs 18060 and 18041 I realise this was never about technical merits because there never were any to start with and this was known two years ago. It was about ticking a box that should never have needed to be ticked. Killing performance and opening a gaping backdoor were irrelevant.

However, enabling this logging is an internal requirement on Windows. Same logging has already been enabled to other built-in scripting hosts on Windows, such as CScript and WScript.

Now I did not know that PowerShell Core was part of Windows given that it does not ship with Windows, I thought it was an external project.

As a final worked example to show how ridiculous the whole thing is....

#!/usr/bin/env pwsh
$env:__PSDumpAMSILogContent='1'
[System.Convert]::FromBase64String('AAAA')

gives

<System.Convert>.FromBase64String(<AAAA>)

Well done, you spotted somebody using base64 data, which I don't yet believe is a crime.

So lets rewrite that.

#!/usr/bin/env pwsh
$env:__PSDumpAMSILogContent='1'
$getMethod=([System.Convert]).GetType().GetMethod('GetMethod',[Type[]](([string],([Type[]]))))
$fromBase64String=$getMethod.Invoke(([System.Convert]),[object[]]('FromBase64String', [Type[]](,([string]))))
$fromBase64String.Invoke($Null,[object[]](,'AAAA'))

Now gives you

<System.RuntimeType>.GetType()
<System.RuntimeType>.GetMethod(<GetMethod>, <System.Type[]>)
<System.Reflection.RuntimeMethodInfo>.Invoke(<System.RuntimeType>, <System.Object[]>)
<System.Reflection.RuntimeMethodInfo>.Invoke(<null>, <System.Object[]>)

Oh no, somebody using an object base system is using methods and invoking them.

I don't think any malware writers will be losing any sleep over this 'feature'.

mklement0 commented 1 week ago

@rhubarb-geek-nz, let me attempt a summary of sorts:


Problems with the current implementation:

Disclaimer: I'm neither a security expert nor particularly security-minded.

rhubarb-geek-nz commented 1 week ago

Thank you @mklement0 , an excellent summary.

The matter of leaking of private and confidential data isn't just that it can appear in a log controlled by __PSDumpAMSILogContent, it is that before 7.3 we had no expectation that private and confidential information was going to be deliberately mishandled by this scripting tool. The tool has absolutely no right to take data that had an expectation of privacy and send it to other parties without any notice or consent. I am also saddened by the project's blasé attitude to other peoples expectations of privacy.

mklement0 commented 1 week ago

Glad to hear the summary was helpful, @rhubarb-geek-nz.

we had no expectation that private and confidential information was going to be deliberately mishandled by this scripting tool.

I don't think that mishandling is an apt characterization.

Again, I think that implicit trust is called for with respect to system-level anti-malware features - the same trust I assume you're willing to extend to the .NET methods ultimately called.

To me, both .NET and a ships-with-windows anti-malware feature deserve the same level of trust.

That said:

rhubarb-geek-nz commented 1 week ago

Again, I think that implicit trust is called for with respect to system-level anti-malware features - the same trust I assume you're willing to extend to the .NET methods ultimately called.

To me, both .NET and a ships-with-windows anti-malware feature deserve the same level of trust.

Given what has happened here I certainly don't think it deserves that automatic trust. For example if you are managing credit card details, cryptographic material etc you want that data to be private. The documentation for invoking methods on CLR objects never mentioned that data would be sent to other components in addition to the target method. AMSI is a plug in architecture where providers can register their handlers. I cannot say that none of them are logging any data or passing it to other parties. If you look in the event viewer today you will see entries logged by PowerShell Core regarding scripts it has scanned. That is a fail for PCI-DSS compliance if private data ends up there.

mklement0 commented 1 week ago

Disclaimer: I'm not familiar with what PCI-DSS compliance entails.

The documentation for invoking methods on CLR objects never mentioned that data would be sent to other components in addition to the target method.

Fair point, and I do think this deserves to be documented, as previously noted.

AMSI is a plug in architecture where providers can register their handlers

If you have security concerns about this architecture - which I presume is guarded by needing explicit admin-level opt-in to register plug-ins - you should voice it in the relevant repo / via the Feedback Hub.

If you look in the event viewer today you will see entries logged by PowerShell Core regarding scripts it has scanned.

Are you saying that this reveals runtime values assumed to be private, similar to what setting env. var. __PSDumpAMSILogContent currently reveals? If so, I encourage you to log a specific issue.

rhubarb-geek-nz commented 1 week ago

Absolutely true that registering an AMSI provider requires admin access. ( the old new thing airtight hatch principle ) And yes I don't have a log of private data being sent to event viewer but you can't prove a negative. The mechanism and pathway is there. The simplest way to prevent that happening is to not send private and confidential data to those components rather than just hoping they don't log anything. That would be a reasonable expectation. The AMSI sample provider from Microsoft implementation demonstrates extensive logging during the scans. On the topic of the old new thing I suggest Dubious security vulnerability: Program allows its output to be exfiltrated does not apply here. The script said send data to X and PowerShell also passes it to Y without it's knowledge or consent. If anyone at Microsoft wants to pass this topic to Raymond Chen, I would be glad of his input. If my expectations are unreasonable then I am happy to just let you guys just get on with it.

mklement0 commented 1 week ago

The script said send data to X and PowerShell also passes it to Y without its knowledge or consent.

The following doesn't negate the above critique of how PowerShell chooses to integrate with AMSI:

I think on any platform offering real-time anti-malware scanning it is implied that low-level hooks potentially inspect any activity.

let you guys just get on with it.

To be clear: I'm just a fellow user, arguing what makes sense to me, personally.

rhubarb-geek-nz commented 1 week ago

To be clear: I'm just a fellow user, arguing what makes sense to me, personally.

Thanks @mklement0 , I always value your opinion and thoughtful summaries. The comment was more directed to the decision makers of this project.

I have done further testing and believe that both PowerShell Desktop 5.1 and PowerShell Core 7.2 already support AMSI integration and for example will refuse to run Invoke-Mimikatz. In my test script, both respond with the appropriate error.

Path                                   FileVersion                         SignerCertificate                        StatusMessage
----                                   -----------                         -----------------                        -------------
C:\Program Files\PowerShell\7\pwsh.exe 7.2.19.500                          C2048FB509F1C37A8C3E9EC6648118458AA01780 Signature verified.
C:\Windows\SYSTEM32\amsi.dll           10.0.22621.1 (WinBuild.160101.0800) D8FB0CC66A08061B42D46D03546F0D42CBC49B7C Signature verified.

Command         ScriptContainedMaliciousContent FullyQualifiedErrorId
-------         ------------------------------- ---------------------
Invoke-Mimikatz                            True ScriptContainedMaliciousContent,Microsoft.PowerShell.Commands.InvokeExpressionCommand

More importantly, neither implement PSAMSIMethodInvocationLogging.

So rather than disable antivirus machine wide, a workable temporary solution is to use 7.2.19 and 5.1.

SydneyhSmith commented 2 days ago

@rhubarb-geek-nz if you believe this is a security vulnerability please file an issue here https://github.com/PowerShell/PowerShell/security we do not take security vulnerabilities in GitHub...thanks!

GitHub
Build software better, together
GitHub is where people build software. More than 100 million people use GitHub to discover, fork, and contribute to over 420 million projects.
rhubarb-geek-nz commented 2 days ago

@rhubarb-geek-nz if you believe this is a security vulnerability please file an issue here ...

Thanks, done.