Skip to content

Commit 635c041

Browse files
committed
Windows PowerShell doesn't have a ConvertFrom-SecureString -AsPlainText
1 parent d62673c commit 635c041

2 files changed

Lines changed: 21 additions & 23 deletions

File tree

Rules/AvoidSecretDisclosure.cs

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,17 @@
77
using System.ComponentModel.Composition;
88
#endif
99
using System.Globalization;
10+
using System.Linq;
1011
using System.Management.Automation.Language;
1112
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
1213

1314
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
1415
{
1516
/// <summary>
16-
/// AvoidSecretDisclosure: Checks whether a plaintext secret is being retrieved which can lead to
17-
/// security vulnerabilities such as memory trails or logging trails.
18-
/// The general approach of dealing with credentials is to avoid them and instead rely on other means
19-
/// to authenticate, such as certificates or Windows authentication.
17+
/// AvoidSecretDisclosure: Checks whether a plaintext secret is being retrieved which can lead
18+
/// to security vulnerabilities such as memory trails or logging trails.
19+
/// The general approach of dealing with credentials is to avoid them and instead rely on other
20+
/// means to authenticate, such as certificates or Windows authentication.
2021
/// </summary>
2122
#if !CORECLR
2223
[Export(typeof(IScriptRule))]
@@ -42,12 +43,12 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
4243
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
4344

4445
// Check for ConvertFrom-SecureString with -AsPlainText parameter
45-
IEnumerable<Ast> convertFromSecureStringAsts = ast.FindAll(testAst =>
46+
IEnumerable<CommandAst> convertFromSecureStringAsts = ast.FindAll(testAst =>
4647
testAst is CommandAst cmdAst &&
4748
cmdAst.GetCommandName() != null &&
4849
cmdAst.GetCommandName().Equals("ConvertFrom-SecureString", StringComparison.OrdinalIgnoreCase),
4950
true
50-
);
51+
).Cast<CommandAst>();
5152

5253
foreach (CommandAst cmdAst in convertFromSecureStringAsts)
5354
{
@@ -59,8 +60,8 @@ testAst is CommandAst cmdAst &&
5960
// This is ok because the rule should still trigger in that case since the value of the
6061
// variable could be true at runtime, and we want to catch all potential violations
6162
if (
62-
bindingResult.BoundParameters.ContainsKey("AsPlainText") &&
63-
bindingResult.BoundParameters["AsPlainText"].ConstantValue is bool constantValue &&
63+
bindingResult.BoundParameters.TryGetValue("AsPlainText", out ParameterBindingResult asPlainTextBinding) &&
64+
asPlainTextBinding.ConstantValue is bool constantValue &&
6465
constantValue == true
6566
) {
6667
yield return GetDiagnosticRecord(cmdAst.Extent, fileName, "AsPlainText");
@@ -69,12 +70,12 @@ testAst is CommandAst cmdAst &&
6970

7071
// Check for any invocation of a method that starts with "SecureStringTo"
7172
// (e.g. SecureStringToBSTR, SecureStringToCoTaskMemAnsi, etc.)
72-
IEnumerable<Ast> secureStringToAsts = ast.FindAll(testAst =>
73+
IEnumerable<InvokeMemberExpressionAst> secureStringToAsts = ast.FindAll(testAst =>
7374
testAst is InvokeMemberExpressionAst invokeAst &&
7475
invokeAst.Member != null &&
7576
invokeAst.Member.ToString().StartsWith("SecureStringTo", StringComparison.OrdinalIgnoreCase),
7677
true
77-
);
78+
).Cast<InvokeMemberExpressionAst>();
7879

7980
foreach (InvokeMemberExpressionAst secureStringToAst in secureStringToAsts) {
8081
yield return GetDiagnosticRecord(secureStringToAst.Extent, fileName, secureStringToAst.Member.ToString());
@@ -87,12 +88,12 @@ testAst is InvokeMemberExpressionAst invokeAst &&
8788
// However, it is too complex to reliably determine whether a Password
8889
// property is a result of e.g. a PSCredential.GetNetworkCredential() call.
8990
// Anyways, this is still a useful common check to have.
90-
IEnumerable<Ast> passwordAsts = ast.FindAll(testAst =>
91+
IEnumerable<MemberExpressionAst> passwordAsts = ast.FindAll(testAst =>
9192
testAst is MemberExpressionAst memberAst &&
9293
memberAst.Member != null &&
9394
string.Equals(memberAst.Member.ToString(), "Password", StringComparison.OrdinalIgnoreCase),
9495
true
95-
);
96+
).Cast<MemberExpressionAst>();
9697

9798
foreach (MemberExpressionAst passwordAst in passwordAsts) {
9899
yield return GetDiagnosticRecord(passwordAst.Extent, fileName, passwordAst.Member.ToString());

Tests/Rules/AvoidSecretDisclosure.tests.ps1

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ Describe "AvoidSecretDisclosure" {
2323
}
2424

2525
Context "Violates" {
26-
It "ConvertFrom-SecureString -AsPlainText" {
26+
It "ConvertFrom-SecureString -AsPlainText" -Skip:($PSVersionTable.PSVersion -le '7.0') {
2727
$scriptDefinition = {
2828
$SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText
2929
$Null = $SecureString | ConvertFrom-SecureString -AsPlainText
@@ -36,7 +36,7 @@ Describe "AvoidSecretDisclosure" {
3636
$violations.RuleSuppressionID | Should -Be 'AsPlainText'
3737
}
3838

39-
It "ConvertFrom-SecureString -AsPlainText:$true" {
39+
It "ConvertFrom-SecureString -AsPlainText:$true" -Skip:($PSVersionTable.PSVersion -le '7.0') {
4040
$scriptDefinition = {
4141
$SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText
4242
$Null = $SecureString | ConvertFrom-SecureString -AsPlainText:$true
@@ -97,13 +97,10 @@ Describe "AvoidSecretDisclosure" {
9797

9898
It "Custom password property" {
9999
$scriptDefinition = {
100-
$Cred = ConvertFrom-Json '
101-
{
102-
"Account": {
103-
"password": "Welcome123!",
104-
"username": "JohnDoe"
105-
}
106-
}'
100+
$Cred = @{
101+
password = 'Welcome123!'
102+
username = 'JohnDoe'
103+
}
107104
schtasks /change /s $env:COMPUTERNAME /tn $myTask /ru $Cred.username /rp $Cred.password
108105
}.ToString()
109106
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $scriptDefinition -Settings $Settings
@@ -240,7 +237,7 @@ Describe "AvoidSecretDisclosure" {
240237

241238
Context "should not crash" {
242239

243-
It "-AsPlainText:$false" {
240+
It "-AsPlainText:$false" -Skip:($PSVersionTable.PSVersion -le '7.0') {
244241
$scriptDefinition = {
245242
$SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText
246243
$Null = $SecureString | ConvertFrom-SecureString -AsPlainText:$false
@@ -249,7 +246,7 @@ Describe "AvoidSecretDisclosure" {
249246
$violations | Should -BeNullOrEmpty
250247
}
251248

252-
It "-AsPlainText:$someVar" {
249+
It "-AsPlainText:$someVar" -Skip:($PSVersionTable.PSVersion -le '7.0') {
253250
$scriptDefinition = {
254251
param ([bool] $someVar)
255252
$SecureString = ConvertTo-SecureString 'P@ssW0rd' -AsPlainText

0 commit comments

Comments
 (0)