diff --git a/Rules/AvoidSecretDisclosure.cs b/Rules/AvoidSecretDisclosure.cs new file mode 100644 index 000000000..6ac67b247 --- /dev/null +++ b/Rules/AvoidSecretDisclosure.cs @@ -0,0 +1,180 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +#if !CORECLR +using System.ComponentModel.Composition; +#endif +using System.Globalization; +using System.Management.Automation.Language; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ + /// + /// AvoidSecretDisclosure: Checks whether a plaintext secret is being retrieved which can lead to + /// security vulnerabilities such as memory trails or logging trails. + /// The general approach of dealing with credentials is to avoid them and instead rely on other means + /// to authenticate, such as certificates or Windows authentication. + /// +#if !CORECLR + [Export(typeof(IScriptRule))] +#endif + public class AvoidSecretDisclosure : ConfigurableRule + { + + /// + /// Construct an object of AvoidSecretDisclosure type. + /// + public AvoidSecretDisclosure() { + Enable = false; + } + + /// + /// Analyzes the given ast to find the [violation] + /// + /// AST to be analyzed. This should be non-null + /// Name of file that corresponds to the input AST. + /// A an enumerable type containing the violations + public override IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); + + // Check for ConvertFrom-SecureString with -AsPlainText parameter + IEnumerable convertFromSecureStringAsts = ast.FindAll(testAst => + testAst is CommandAst cmdAst && + cmdAst.GetCommandName() != null && + cmdAst.GetCommandName().Equals("ConvertFrom-SecureString", StringComparison.OrdinalIgnoreCase), + true + ); + + foreach (CommandAst cmdAst in convertFromSecureStringAsts) + { + // Use StaticParameterBinder to reliably get parameter values + var bindingResult = StaticParameterBinder.BindCommand(cmdAst, true); + + // Check for -AsPlainText parameter + // The constantValue appears true even the value is a variable + // This is ok because the rule should still trigger in that case since the value of the + // variable could be true at runtime, and we want to catch all potential violations + if ( + bindingResult.BoundParameters.ContainsKey("AsPlainText") && + bindingResult.BoundParameters["AsPlainText"].ConstantValue is bool constantValue && + constantValue == true + ) { + yield return GetDiagnosticRecord(cmdAst.Extent, fileName, "AsPlainText"); + } + } + + // Check for any invocation of a method that starts with "SecureStringTo" + // (e.g. SecureStringToBSTR, SecureStringToCoTaskMemAnsi, etc.) + IEnumerable secureStringToAsts = ast.FindAll(testAst => + testAst is InvokeMemberExpressionAst invokeAst && + invokeAst.Member != null && + invokeAst.Member.ToString().StartsWith("SecureStringTo", StringComparison.OrdinalIgnoreCase), + true + ); + + foreach (InvokeMemberExpressionAst secureStringToAst in secureStringToAsts) { + yield return GetDiagnosticRecord(secureStringToAst.Extent, fileName, secureStringToAst.Member.ToString()); + } + + // Check for any member access of a property named "Password". + // Note that this is a heuristic and may lead to false positives, + // as not all properties named "Password" necessarily contain secrets, + // and there may be secrets stored in properties with other names. + // However, it is too complex to reliably determine whether a Password + // property is a result of e.g. a PSCredential.GetNetworkCredential() call. + // Anyways, this is still a useful common check to have. + IEnumerable passwordAsts = ast.FindAll(testAst => + testAst is MemberExpressionAst memberAst && + memberAst.Member != null && + string.Equals(memberAst.Member.ToString(), "Password", StringComparison.OrdinalIgnoreCase), + true + ); + + foreach (MemberExpressionAst passwordAst in passwordAsts) { + yield return GetDiagnosticRecord(passwordAst.Extent, fileName, passwordAst.Member.ToString()); + } + + } + + /// + /// Helper function to create a DiagnosticRecord for a given violation + /// + private DiagnosticRecord GetDiagnosticRecord(IScriptExtent Extent, string fileName, string suppressionId) + { + return new DiagnosticRecord( + Strings.AvoidSecretDisclosureError, + Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + suppressionId + ); + } + + /// + /// Retrieves the common name of this rule. + /// + public override string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidSecretDisclosureCommonName); + } + + /// + /// Retrieves the description of this rule. + /// + public override string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidSecretDisclosureDescription); + } + + /// + /// Retrieves the name of this rule. + /// + public override string GetName() + { + return string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.AvoidSecretDisclosureName); + } + + /// + /// Retrieves the severity of the rule: error, warning or information. + /// + public override RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + /// + /// Gets the severity of the returned diagnostic record: error, warning, or information. + /// + /// + public DiagnosticSeverity GetDiagnosticSeverity() + { + return DiagnosticSeverity.Warning; + } + + /// + /// Retrieves the name of the module/assembly the rule is from. + /// + public override string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + + /// + /// Retrieves the type of the rule, Builtin, Managed or Module. + /// + public override SourceType GetSourceType() + { + return SourceType.Builtin; + } + } +} + diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 2a04fd759..61c4f2b08 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -921,6 +921,18 @@ Line exceeds the configured maximum length of {0} characters + + AvoidSecretDisclosure + + + Avoid secret disclosure + + + Disclosing a secret might result in security vulnerabilities such as memory trails or logging trails + + + Avoid disclosing a secret + AvoidSemicolonsAsLineTerminators diff --git a/Tests/Rules/AvoidSecretDisclosure.tests.ps1 b/Tests/Rules/AvoidSecretDisclosure.tests.ps1 new file mode 100644 index 000000000..95d59c704 --- /dev/null +++ b/Tests/Rules/AvoidSecretDisclosure.tests.ps1 @@ -0,0 +1,266 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +[Diagnostics.CodeAnalysis.SuppressMessage('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'False positive')] +param() + +BeforeAll { + $ruleName = "PSAvoidSecretDisclosure" + $ruleMessage = "Avoid disclosing a secret" +} + +Describe "AvoidSecretDisclosure" { + + # Secret disclosure examples we like to discourage: + # https://stackoverflow.com/questions/28352141/convert-a-secure-string-to-plain-text + # https://stackoverflow.com/questions/7468389/powershell-decode-system-security-securestring-to-readable-password + + BeforeAll { + $Settings = @{ + IncludeRules = @($ruleName) + Rules = @{ $ruleName = @{ Enable = $true } } + } + } + + Context "Violates" { + It "ConvertFrom-SecureString -AsPlainText" { + $scriptDefinition = { + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $Null = $SecureString | ConvertFrom-SecureString -AsPlainText + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {ConvertFrom-SecureString -AsPlainText}.ToString() + $violations.Message | Should -Be $ruleMessage + $violations.RuleSuppressionID | Should -Be 'AsPlainText' + } + + It "ConvertFrom-SecureString -AsPlainText:$true" { + $scriptDefinition = { + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $Null = $SecureString | ConvertFrom-SecureString -AsPlainText:$true + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {ConvertFrom-SecureString -AsPlainText:$true}.ToString() + $violations.Message | Should -Be $ruleMessage + $violations.RuleSuppressionID | Should -Be 'AsPlainText' + } + + It "SecureStringToBSTR()" { + $scriptDefinition = { + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $BSTR = [System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($SecureString) + $Password = [System.Runtime.InteropServices.Marshal]::PtrToStringAuto($BSTR) + [Runtime.InteropServices.Marshal]::ZeroFreeBSTR($BSTR) + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {[System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($SecureString)}.ToString() + $violations.Message | Should -Be $ruleMessage + $violations.RuleSuppressionID | Should -Be 'SecureStringToBSTR' + } + + It "SecureStringToCoTaskMemUnicode()" { + $scriptDefinition = { + $password = ConvertTo-SecureString 'P@ssw0rd' -AsPlainText -Force + $Ptr = [System.Runtime.InteropServices.Marshal]::SecureStringToCoTaskMemUnicode($password) + $result = [System.Runtime.InteropServices.Marshal]::PtrToStringUni($Ptr) + [System.Runtime.InteropServices.Marshal]::ZeroFreeCoTaskMemUnicode($Ptr) + $result + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {[System.Runtime.InteropServices.Marshal]::SecureStringToCoTaskMemUnicode($password)}.ToString() + $violations.Message | Should -Be $ruleMessage + $violations.RuleSuppressionID | Should -Be 'SecureStringToCoTaskMemUnicode' + } + + It "GetNetworkCredential().Password" { + $scriptDefinition = { + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $PSCredential = [PSCredential]::new(0, $SecureString) + $Credential = $PSCredential.GetNetworkCredential() + $Password = $Credential.Password + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {$Credential.Password}.ToString() + $violations.Message | Should -Be $ruleMessage + $violations.RuleSuppressionID | Should -Be 'Password' + } + + It "Custom password property" { + $scriptDefinition = { + $Cred = ConvertFrom-Json ' + { + "Account": { + "password": "Welcome123!", + "username": "JohnDoe" + } + }' + schtasks /change /s $env:COMPUTERNAME /tn $myTask /ru $Cred.username /rp $Cred.password + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {$Cred.password}.ToString() + $violations.Message | Should -Be $ruleMessage + $violations.RuleSuppressionID | Should -Be 'Password' + } + } + + Context "Compliant" { + It "Correct" { + $scriptDefinition = { + $credential = Get-Credential + $url = "https://server.contoso.com:8089/services/search/jobs/export" + $body = @{ + search = "search index=_internal | reverse | table index,host,source,sourceType,_raw" + output_mode = "csv" + earliest_time = "-2d@d" + latest_time = "-1d@d" + } + Invoke-RestMethod -Method 'Post' -Uri $url -Credential $credential -Body $body -OutFile output.csv + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + + It "Write-Host" { + $scriptDefinition = { + Write-Host AsPlainText SecureStringToBSTR Password + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + } + + Context "Suppressed" { + It "AsPlainText" { + $scriptDefinition = { + [Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidSecretDisclosure', 'AsPlainText', Justification = 'Test')] + Param() + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $Null = $SecureString | ConvertFrom-SecureString -AsPlainText + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + + It "SecureStringToBSTR" { + $scriptDefinition = { + [Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidSecretDisclosure', 'SecureStringToBSTR', Justification = 'Test')] + Param() + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $BSTR = [System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($SecureString) + $Password = [System.Runtime.InteropServices.Marshal]::PtrToStringAuto($BSTR) + [Runtime.InteropServices.Marshal]::ZeroFreeBSTR($BSTR) + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + + It "SecureStringToCoTaskMemUnicode" { + $scriptDefinition = { + [Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidSecretDisclosure', 'SecureStringToCoTaskMemUnicode', Justification = 'Test')] + Param() + $password = ConvertTo-SecureString 'P@ssw0rd' -AsPlainText -Force + $Ptr = [System.Runtime.InteropServices.Marshal]::SecureStringToCoTaskMemUnicode($password) + $result = [System.Runtime.InteropServices.Marshal]::PtrToStringUni($Ptr) + [System.Runtime.InteropServices.Marshal]::ZeroFreeCoTaskMemUnicode($Ptr) + $result + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + + It "Password" { + $scriptDefinition = { + [Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidSecretDisclosure', 'Password', Justification = 'Test')] + Param() + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $PSCredential = [PSCredential]::new(0, $SecureString) + $Password = $PSCredential.GetNetworkCredential().Password + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + + It "All" { + $scriptDefinition = { + [Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidSecretDisclosure', '', Justification = 'Test')] + Param() + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $Null = $SecureString | ConvertFrom-SecureString -AsPlainText + + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $BSTR = [System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($SecureString) + $Password = [System.Runtime.InteropServices.Marshal]::PtrToStringAuto($BSTR) + [Runtime.InteropServices.Marshal]::ZeroFreeBSTR($BSTR) + + $password = ConvertTo-SecureString 'P@ssw0rd' -AsPlainText -Force + $Ptr = [System.Runtime.InteropServices.Marshal]::SecureStringToCoTaskMemUnicode($password) + $result = [System.Runtime.InteropServices.Marshal]::PtrToStringUni($Ptr) + [System.Runtime.InteropServices.Marshal]::ZeroFreeCoTaskMemUnicode($Ptr) + $result + + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $PSCredential = [PSCredential]::new(0, $SecureString) + $Password = $PSCredential.GetNetworkCredential().Password + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + } + + Context "Disabled" { + + BeforeAll { + $Settings = @{ + IncludeRules = @($ruleName) + Rules = @{ $ruleName = @{ Enable = $false } } + } + } + + It "ConvertFrom-SecureString -AsPlainText" { + $scriptDefinition = { + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $Null = $SecureString | ConvertFrom-SecureString -AsPlainText + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + } + + Context "should not crash" { + + It "-AsPlainText:$false" { + $scriptDefinition = { + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $Null = $SecureString | ConvertFrom-SecureString -AsPlainText:$false + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations | Should -BeNullOrEmpty + } + + It "-AsPlainText:$someVar" { + $scriptDefinition = { + param ([bool] $someVar) + $SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText + $Null = $SecureString | ConvertFrom-SecureString -AsPlainText:$someVar + }.ToString() + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings + $violations.Count | Should -Be 1 + $violations.Severity | Should -Be Warning + $violations.Extent.Text | Should -Be {ConvertFrom-SecureString -AsPlainText:$someVar}.ToString() + $violations.Message | Should -Be $ruleMessage + $violations.RuleSuppressionID | Should -Be 'AsPlainText' + } + } +} \ No newline at end of file diff --git a/docs/Rules/AvoidSecretDisclosure.md b/docs/Rules/AvoidSecretDisclosure.md new file mode 100644 index 000000000..21a967d4e --- /dev/null +++ b/docs/Rules/AvoidSecretDisclosure.md @@ -0,0 +1,72 @@ +--- +description: Avoid secret disclosure +ms.date: 05/03/2026 +ms.topic: reference +title: AvoidSecretDisclosure +--- +# AvoidSecretDisclosure + +**Severity Level: Warning** + +## Description + +Disclosing a secret might result in security vulnerabilities such as memory trails or logging trails +that could be exploited by attackers. This rule identifies instances where a secret is being +converted to plain text, which can lead to unintended exposure of sensitive information. + +> [!IMPORTANT] +> The general approach of dealing with credentials is to avoid them and instead rely on other means +> to authenticate, such as certificates or Windows authentication. + +## How to Fix + +In general, avoid any code pattern that involves converting secrets to plaintext or accessing +plaintext secrets. + +- For `ConvertFrom-SecureString -AsPlainText`: Use `-Credential` parameter instead +- For `SecureStringTo*` methods: Avoid converting to plaintext +- For `Password` properties: Use secure credential objects directly or the SecureString equivalent + `SecurePassword` instead of accessing plaintext passwords. + +> [!NOTE] +> For custom properties named "Password", it is recommended to rename them to something that does +> not imply they contain secrets, or to ensure that they do not actually contain secrets. If +> renaming is not possible, consider suppressing the warning for those specific cases. + +## Configuration + +### Parameters + +- `Enable` - Enables or disables the rule. Default value is `$false`. + +## Example + +### Wrong + +```powershell +$credential = Get-Credential +$url = "https://server.contoso.com:8089/services/search/jobs/export" +$body = @{ + search = "search index=_internal | reverse | table index,host,source,sourceType,_raw" + output_mode = "csv" + earliest_time = "-2d@d" + latest_time = "-1d@d" + username = $credential.UserName + password = $credential.GetNetworkCredential().Password +} +Invoke-RestMethod -Method 'Post' -Uri $url -Body $body -OutFile output.csv +``` + +### Correct + +```powershell +$credential = Get-Credential +$url = "https://server.contoso.com:8089/services/search/jobs/export" +$body = @{ + search = "search index=_internal | reverse | table index,host,source,sourceType,_raw" + output_mode = "csv" + earliest_time = "-2d@d" + latest_time = "-1d@d" +} +Invoke-RestMethod -Method 'Post' -Uri $url -Credential $credential -Body $body -OutFile output.csv +``` diff --git a/docs/Rules/README.md b/docs/Rules/README.md index fca031e33..8f8a17c06 100644 --- a/docs/Rules/README.md +++ b/docs/Rules/README.md @@ -24,6 +24,7 @@ The PSScriptAnalyzer contains the following rule definitions. | [AvoidNullOrEmptyHelpMessageAttribute](./AvoidNullOrEmptyHelpMessageAttribute.md) | Warning | Yes | | | [AvoidOverwritingBuiltInCmdlets](./AvoidOverwritingBuiltInCmdlets.md) | Warning | Yes | Yes | | [AvoidReservedWordsAsFunctionNames](./AvoidReservedWordsAsFunctionNames.md) | Warning | Yes | | +| [AvoidSecretDisclosure](./AvoidSecretDisclosure.md) | Warning | No | Yes | | [AvoidSemicolonsAsLineTerminators](./AvoidSemicolonsAsLineTerminators.md) | Warning | No | | | [AvoidShouldContinueWithoutForce](./AvoidShouldContinueWithoutForce.md) | Warning | Yes | | | [AvoidTrailingWhitespace](./AvoidTrailingWhitespace.md) | Warning | Yes | |