Skip to content

Commit 1280763

Browse files
daxian-dbwdaxian-dbw
authored
Make the default sensitive history scrubbing function a little smarter (#2921)
1 parent 878b670 commit 1280763

File tree

2 files changed

+263
-4
lines changed

2 files changed

+263
-4
lines changed

‎PSReadLine/History.cs

Lines changed: 176 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
using System.Text;
1313
using System.Text.RegularExpressions;
1414
using System.Threading;
15+
using System.Management.Automation.Language;
1516
using Microsoft.PowerShell.PSReadLine;
1617

1718
namespace Microsoft.PowerShell
@@ -116,6 +117,20 @@ public class HistoryItem
116117
"password|asplaintext|token|apikey|secret",
117118
RegexOptions.Compiled | RegexOptions.IgnoreCase);
118119

120+
private static readonly HashSet<string> s_SecretMgmtCommands = new(StringComparer.OrdinalIgnoreCase)
121+
{
122+
"Get-Secret",
123+
"Get-SecretInfo",
124+
"Get-SecretVault",
125+
"Register-SecretVault",
126+
"Remove-Secret",
127+
"Set-SecretInfo",
128+
"Set-SecretVaultDefault",
129+
"Test-SecretVault",
130+
"Unlock-SecretVault",
131+
"Unregister-SecretVault"
132+
};
133+
119134
private void ClearSavedCurrentLine()
120135
{
121136
_savedCurrentLine.CommandLine = null;
@@ -474,11 +489,169 @@ void UpdateHistoryFromFile(IEnumerable<string> historyLines, bool fromDifferentS
474489
}
475490
}
476491

492+
private static bool IsOnLeftSideOfAnAssignment(Ast ast, out Ast rhs)
493+
{
494+
bool result = false;
495+
rhs = null;
496+
497+
do
498+
{
499+
if (ast.Parent is AssignmentStatementAst assignment)
500+
{
501+
rhs = assignment.Right;
502+
result = ReferenceEquals(assignment.Left, ast);
503+
504+
break;
505+
}
506+
507+
ast = ast.Parent;
508+
}
509+
while (ast.Parent is not null);
510+
511+
return result;
512+
}
513+
514+
private static bool IsSecretMgmtCommand(StringConstantExpressionAst strConst, out CommandAst command)
515+
{
516+
bool result = false;
517+
command = strConst.Parent as CommandAst;
518+
519+
if (command is not null)
520+
{
521+
result = ReferenceEquals(command.CommandElements[0], strConst)
522+
&& s_SecretMgmtCommands.Contains(strConst.Value);
523+
}
524+
525+
return result;
526+
}
527+
528+
private static ExpressionAst GetArgumentForParameter(CommandParameterAst param)
529+
{
530+
if (param.Argument is not null)
531+
{
532+
return param.Argument;
533+
}
534+
535+
var command = (CommandAst)param.Parent;
536+
int index = 1;
537+
for (; index < command.CommandElements.Count; index++)
538+
{
539+
if (ReferenceEquals(command.CommandElements[index], param))
540+
{
541+
break;
542+
}
543+
}
544+
545+
int argIndex = index + 1;
546+
if (argIndex < command.CommandElements.Count
547+
&& command.CommandElements[argIndex] is ExpressionAst arg)
548+
{
549+
return arg;
550+
}
551+
552+
return null;
553+
}
554+
477555
public static AddToHistoryOption GetDefaultAddToHistoryOption(string line)
478556
{
479-
return s_sensitivePattern.IsMatch(line)
480-
? AddToHistoryOption.MemoryOnly
481-
: AddToHistoryOption.MemoryAndFile;
557+
if (string.IsNullOrEmpty(line))
558+
{
559+
return AddToHistoryOption.SkipAdding;
560+
}
561+
562+
Match match = s_sensitivePattern.Match(line);
563+
if (ReferenceEquals(match, Match.Empty))
564+
{
565+
return AddToHistoryOption.MemoryAndFile;
566+
}
567+
568+
bool isSensitive = false;
569+
Ast ast = string.Equals(_singleton._ast?.Extent.Text, line)
570+
? _singleton._ast
571+
: Parser.ParseInput(line, out _, out _);
572+
573+
do
574+
{
575+
int start = match.Index;
576+
int end = start + match.Length;
577+
578+
IEnumerable<Ast> asts = ast.FindAll(
579+
ast => ast.Extent.StartOffset <= start && ast.Extent.EndOffset >= end,
580+
searchNestedScriptBlocks: true);
581+
582+
Ast innerAst = asts.Last();
583+
switch (innerAst)
584+
{
585+
case VariableExpressionAst:
586+
// It's a variable with sensitive name. Using the variable is fine, but assigning to
587+
// the variable could potentially expose sensitive content.
588+
// If it appears on the left-hand-side of an assignment, and the right-hand-side is
589+
// not a command invocation, we consider it sensitive.
590+
// e.g. `$token = Get-Secret` vs. `$token = 'token-text'` or `$token, $url = ...`
591+
isSensitive = IsOnLeftSideOfAnAssignment(innerAst, out Ast rhs)
592+
&& rhs is not PipelineAst;
593+
594+
if (!isSensitive)
595+
{
596+
match = match.NextMatch();
597+
}
598+
break;
599+
600+
case StringConstantExpressionAst strConst:
601+
// If it's not a command name, or it's not one of the secret management commands that
602+
// we can ignore, we consider it sensitive.
603+
isSensitive = !IsSecretMgmtCommand(strConst, out CommandAst command);
604+
605+
if (!isSensitive)
606+
{
607+
// We can safely skip the whole command text.
608+
match = s_sensitivePattern.Match(line, command.Extent.EndOffset);
609+
}
610+
break;
611+
612+
case CommandParameterAst param:
613+
// Special-case the '-AsPlainText' parameter.
614+
if (string.Equals(param.ParameterName, "AsPlainText"))
615+
{
616+
isSensitive = true;
617+
break;
618+
}
619+
620+
ExpressionAst arg = GetArgumentForParameter(param);
621+
if (arg is null)
622+
{
623+
// If no argument is found following the parameter, then it could be a switching parameter
624+
// such as '-UseDefaultPassword' or '-SaveToken', which we assume will not expose sensitive information.
625+
match = match.NextMatch();
626+
}
627+
else if (arg is VariableExpressionAst)
628+
{
629+
// Argument is a variable. It's fine to use a variable for a senstive parameter.
630+
// e.g. `Invoke-WebRequest -Token $token`
631+
match = s_sensitivePattern.Match(line, arg.Extent.EndOffset);
632+
}
633+
else if (arg is ParenExpressionAst paren
634+
&& paren.Pipeline is PipelineAst pipeline
635+
&& pipeline.PipelineElements[0] is not CommandExpressionAst)
636+
{
637+
// Argument is a command invocation, such as `Invoke-WebRequest -Token (Get-Secret)`.
638+
match = match.NextMatch();
639+
}
640+
else
641+
{
642+
// We consider all other arguments sensitive.
643+
isSensitive = true;
644+
}
645+
break;
646+
647+
default:
648+
isSensitive = true;
649+
break;
650+
}
651+
}
652+
while (!isSensitive && !ReferenceEquals(match, Match.Empty));
653+
654+
return isSensitive ? AddToHistoryOption.MemoryOnly : AddToHistoryOption.MemoryAndFile;
482655
}
483656

484657
/// <summary>

‎test/HistoryTest.cs

Lines changed: 87 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ public void ParallelHistorySaving()
8989
}
9090

9191
[SkippableFact]
92-
public void SensitiveHistoryDefaultBehavior()
92+
public void SensitiveHistoryDefaultBehavior_One()
9393
{
9494
TestSetup(KeyMode.Cmd);
9595

@@ -163,6 +163,92 @@ public void SensitiveHistoryDefaultBehavior()
163163
}
164164
}
165165

166+
[SkippableFact]
167+
public void SensitiveHistoryDefaultBehavior_Two()
168+
{
169+
TestSetup(KeyMode.Cmd);
170+
171+
// Clear history
172+
SetHistory();
173+
174+
var options = PSConsoleReadLine.GetOptions();
175+
var oldHistoryFilePath = options.HistorySavePath;
176+
var oldHistorySaveStyle = options.HistorySaveStyle;
177+
178+
// AddToHistoryHandler should be set to the default handler.
179+
Assert.Same(PSConsoleReadLineOptions.DefaultAddToHistoryHandler, options.AddToHistoryHandler);
180+
181+
var newHistoryFilePath = Path.GetTempFileName();
182+
var newHistorySaveStyle = HistorySaveStyle.SaveIncrementally;
183+
184+
string[] expectedHistoryItems = new[] {
185+
"$token = 'abcd'", // Assign expr-value to sensitive variable. Not saved to file.
186+
"Set-Secret abc $mySecret", // 'Set-Secret' will not be save to file.
187+
"ConvertTo-SecureString stringValue -AsPlainText", // '-AsPlainText' is an alert. Not saved to file.
188+
"Get-Secret PSGalleryApiKey -AsPlainText", // For eligible secret-mgmt command, the whole command is skipped, so '-AsPlainText' here is OK.
189+
"$token = Get-Secret -Name github-token -Vault MySecret",
190+
"[MyType]::CallRestAPI($token, $url, $args)",
191+
"$template -f $token",
192+
"Invoke-RestCall $url -UseDefaultToken",
193+
"Publish-Module -NuGetApiKey $apikey",
194+
"Publish-Module -NuGetApiKey (Get-Secret PSGalleryApiKey -AsPlainText)",
195+
"Publish-Module -NuGetApiKey (Get-NewSecret -Name apikey)", // 'Get-NewSecret' is not in our allow-list. Not saved to file.
196+
"Send-HeartBeat -UseDefaultToken",
197+
"Send-HeartBeat -password $pass -SavePassword",
198+
"Invoke-WebRequest -Token xxx", // Expr-value as argument to '-Token'. Not saved to file.
199+
"Invoke-WebRequest -Token (2+2)", // Expr-value as argument to '-Token'. Not saved to file.
200+
"Get-SecretInfo -Name mytoken; Get-SecretVault; Register-SecretVault; Remove-Secret apikey",
201+
"Get-SecretInfo -Name mytoken; Get-SecretVault; Register-SecretVault; Remove-Secret apikey; Set-Secret", // 'Set-Secret' Not saved to file.
202+
"Set-SecretInfo -Name apikey; Set-SecretVaultDefault; Test-SecretVault; Unlock-SecretVault -password $pwd; Unregister-SecretVault -SecretVault vaultInfo",
203+
"Get-ResultFromTwo -Secret1 (Get-Secret -Name blah -AsPlainText) -Secret2 $secret2",
204+
"Get-ResultFromTwo -Secret1 (Get-Secret -Name blah -AsPlainText) -Secret2 sdv87ysdfayf798hfasd8f7ha" // '-Secret2' has expr-value argument. Not saved to file.
205+
};
206+
207+
string[] expectedSavedItems = new[] {
208+
"Get-Secret PSGalleryApiKey -AsPlainText",
209+
"$token = Get-Secret -Name github-token -Vault MySecret",
210+
"[MyType]::CallRestAPI($token, $url, $args)",
211+
"$template -f $token",
212+
"Invoke-RestCall $url -UseDefaultToken",
213+
"Publish-Module -NuGetApiKey $apikey",
214+
"Publish-Module -NuGetApiKey (Get-Secret PSGalleryApiKey -AsPlainText)",
215+
"Send-HeartBeat -UseDefaultToken",
216+
"Send-HeartBeat -password $pass -SavePassword",
217+
"Get-SecretInfo -Name mytoken; Get-SecretVault; Register-SecretVault; Remove-Secret apikey",
218+
"Set-SecretInfo -Name apikey; Set-SecretVaultDefault; Test-SecretVault; Unlock-SecretVault -password $pwd; Unregister-SecretVault -SecretVault vaultInfo",
219+
"Get-ResultFromTwo -Secret1 (Get-Secret -Name blah -AsPlainText) -Secret2 $secret2",
220+
};
221+
222+
try
223+
{
224+
options.HistorySavePath = newHistoryFilePath;
225+
options.HistorySaveStyle = newHistorySaveStyle;
226+
SetHistory(expectedHistoryItems);
227+
228+
// Sensitive input history should be kept in the internal history queue.
229+
var historyItems = PSConsoleReadLine.GetHistoryItems();
230+
Assert.Equal(expectedHistoryItems.Length, historyItems.Length);
231+
for (int i = 0; i < expectedHistoryItems.Length; i++)
232+
{
233+
Assert.Equal(expectedHistoryItems[i], historyItems[i].CommandLine);
234+
}
235+
236+
// Sensitive input history should NOT be saved to the history file.
237+
string[] text = File.ReadAllLines(newHistoryFilePath);
238+
Assert.Equal(expectedSavedItems.Length, text.Length);
239+
for (int i = 0; i < text.Length; i++)
240+
{
241+
Assert.Equal(expectedSavedItems[i], text[i]);
242+
}
243+
}
244+
finally
245+
{
246+
options.HistorySavePath = oldHistoryFilePath;
247+
options.HistorySaveStyle = oldHistorySaveStyle;
248+
File.Delete(newHistoryFilePath);
249+
}
250+
}
251+
166252
[SkippableFact]
167253
public void SensitiveHistoryOptionalBehavior()
168254
{

0 commit comments

Comments
 (0)