Skip to content

Add password confirmation in Get-Credential #12782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iSazonov merged 13 commits into from
Jul 22, 2020
+323 −73
Merged

Add password confirmation in Get-Credential #12782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
103219a
Get-Credential: Add parameter -ReEnterPassword to prompt user to re-e…
Shriram0908 May 25, 2020
d637929
Add new overload function PromptForCredential
Shriram0908 May 26, 2020
360489b
Get-Credential: Prompt user to re-enter password if mismatch while us…
Shriram0908 May 26, 2020
c5e19c1
Improve code readability
Shriram0908 May 27, 2020
2cf47f0
Get-Credential: Add Pester test case
Shriram0908 May 27, 2020
b8916a8
Fix code formating
Shriram0908 May 28, 2020
77331ed
Remove confirm password loop
Shriram0908 May 29, 2020
deb354b
Use bit comparison for secure sting
Shriram0908 May 29, 2020
0e00f4f
Fix code formating
Shriram0908 May 29, 2020
91361d8
Add early return if comparison fails
Shriram0908 Jun 2, 2020
6688841
Fix code formating
Shriram0908 Jun 2, 2020
65af943
Rename from ReenterPassword to ConfirmPassword
Shriram0908 Jun 4, 2020
7bef04a
Fix code factor issue
Shriram0908 Jun 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public override Dictionary<string, PSObject> Prompt(string caption, string messa
}

/// <summary>
/// PromptForChoice.
/// Prompt for choice.
/// </summary>
/// <param name="caption"></param>
/// <param name="message"></param>
Expand All @@ -60,7 +60,7 @@ public override int PromptForChoice(string caption, string message, Collection<C
}

/// <summary>
/// PromptForCredential.
/// Prompt for credential.
/// </summary>
/// <param name="caption"></param>
/// <param name="message"></param>
Expand All @@ -73,7 +73,7 @@ public override PSCredential PromptForCredential(string caption, string message,
}

/// <summary>
/// PromptForCredential.
/// Prompt for credential.
/// </summary>
/// <param name="caption"></param>
/// <param name="message"></param>
Expand All @@ -88,7 +88,23 @@ public override PSCredential PromptForCredential(string caption, string message,
}

/// <summary>
/// ReadLine.
/// Prompt for credential.
/// </summary>
/// <param name="caption"></param>
/// <param name="message"></param>
/// <param name="userName"></param>
/// <param name="confirmPassword"></param>
/// <param name="targetName"></param>
/// <param name="allowedCredentialTypes"></param>
/// <param name="options"></param>
/// <returns></returns>
public override PSCredential PromptForCredential(string caption, string message, string userName, bool confirmPassword, string targetName, PSCredentialTypes allowedCredentialTypes, PSCredentialUIOptions options)
{
throw new PSNotImplementedException();
}

/// <summary>
/// Read line.
/// </summary>
/// <returns></returns>
public override string ReadLine()
Expand Down
8 changes: 4 additions & 4 deletions src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfacePrompt.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,10 +303,10 @@ out object convertedObj
PSCredential credential = null;
credential =
PromptForCredential(
null, // caption already written
null, // message already written
null,
string.Empty);
caption: null, // caption already written
message: null, // message already written
userName: null,
targetName: string.Empty);
convertedObj = credential;
cancelInput = (convertedObj == null);
if ((credential != null) && (credential.Password.Length == 0) && listInput)
Expand Down
154 changes: 125 additions & 29 deletions src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceSecurity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@

using System;
using System.Globalization;
using System.Linq;
using System.Management.Automation;
using System.Management.Automation.Internal;
using System.Runtime.InteropServices;
using System.Security;

using Microsoft.Win32;

namespace Microsoft.PowerShell
Expand All @@ -24,21 +25,17 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt
/// this function will be modified to prompt using secure-path
/// if so configured.
/// </summary>
/// <param name="userName">Name of the user whose creds are to be prompted for. If set to null or empty string, the function will prompt for user name first.</param>
/// <param name="targetName">Name of the target for which creds are being collected.</param>
/// <param name="message">Message to be displayed.</param>
/// <param name="caption">Caption for the message.</param>
/// <param name="message">Message to be displayed.</param>
/// <param name="userName">Name of the user whose credentials are to be prompted for. If set to null or empty string, the function will prompt for user name first.</param>
/// <param name="targetName">Name of the target for which credentials are being collected.</param>
/// <returns>PSCredential object.</returns>

public override PSCredential PromptForCredential(
string caption,
string message,
string userName,
string targetName)
public override PSCredential PromptForCredential(string caption, string message, string userName, string targetName)
{
return PromptForCredential(caption,
message,
userName,
confirmPassword: false,
targetName,
PSCredentialTypes.Default,
PSCredentialUIOptions.Default);
Expand All @@ -47,31 +44,62 @@ public override PSCredential PromptForCredential(
/// <summary>
/// Prompt for credentials.
/// </summary>
/// <param name="userName">Name of the user whose creds are to be prompted for. If set to null or empty string, the function will prompt for user name first.</param>
/// <param name="targetName">Name of the target for which creds are being collected.</param>
/// <param name="message">Message to be displayed.</param>
/// <param name="caption">Caption for the message.</param>
/// <param name="allowedCredentialTypes">What type of creds can be supplied by the user.</param>
/// <param name="options">Options that control the cred gathering UI behavior.</param>
/// <param name="message">Message to be displayed.</param>
/// <param name="userName">Name of the user whose credentials are to be prompted for. If set to null or empty string, the function will prompt for user name first.</param>
/// <param name="targetName">Name of the target for which credentials are being collected.</param>
/// <param name="allowedCredentialTypes">What type of credentials can be supplied by the user.</param>
/// <param name="options">Options that control the credential gathering UI behavior.</param>
/// <returns>PSCredential object, or null if input was cancelled (or if reading from stdin and stdin at EOF).</returns>
public override PSCredential PromptForCredential(
string caption,
string message,
string userName,
string targetName,
PSCredentialTypes allowedCredentialTypes,
PSCredentialUIOptions options)
{
return PromptForCredential(
caption,
message,
userName,
confirmPassword: false,
targetName,
allowedCredentialTypes,
options);
}

/// <summary>
/// Prompt for credentials.
/// </summary>
/// <param name="caption">Caption for the message.</param>
/// <param name="message">Message to be displayed.</param>
/// <param name="userName">Name of the user whose credentials are to be prompted for. If set to null or empty string, the function will prompt for user name first.</param>
/// <param name="confirmPassword">Prompts user to re-enter the password for confirmation.</param>
/// <param name="targetName">Name of the target for which credentials are being collected.</param>
/// <param name="allowedCredentialTypes">What type of credentials can be supplied by the user.</param>
/// <param name="options">Options that control the credential gathering UI behavior.</param>
/// <returns>PSCredential object, or null if input was cancelled (or if reading from stdin and stdin at EOF).</returns>
public override PSCredential PromptForCredential(
string caption,
string message,
string userName,
bool confirmPassword,
string targetName,
PSCredentialTypes allowedCredentialTypes,
PSCredentialUIOptions options)
{
PSCredential cred = null;
SecureString password = null;
SecureString reenterPassword = null;
string userPrompt = null;
string passwordPrompt = null;
string confirmPasswordPrompt = null;
string passwordMismatch = null;

if (!string.IsNullOrEmpty(caption))
{
// Should be a skin lookup

WriteLineToConsole();
WriteLineToConsole(PromptColor, RawUI.BackgroundColor, WrapToCurrentWindowWidth(caption));
}
Expand All @@ -85,9 +113,7 @@ public override PSCredential PromptForCredential(
{
userPrompt = ConsoleHostUserInterfaceSecurityResources.PromptForCredential_User;

//
// need to prompt for user name first
//
do
{
WriteToConsole(userPrompt, true);
Expand All @@ -100,25 +126,95 @@ public override PSCredential PromptForCredential(
while (userName.Length == 0);
}

passwordPrompt = StringUtil.Format(ConsoleHostUserInterfaceSecurityResources.PromptForCredential_Password, userName
);
passwordPrompt = StringUtil.Format(ConsoleHostUserInterfaceSecurityResources.PromptForCredential_Password, userName);

//
// now, prompt for the password
//
WriteToConsole(passwordPrompt, true);
password = ReadLineAsSecureString();
if (password == null)
do
{
return null;
WriteToConsole(passwordPrompt, true);
password = ReadLineAsSecureString();
if (password == null)
{
return null;
}
}
while (password.Length == 0);

WriteLineToConsole();
if (confirmPassword)
{
confirmPasswordPrompt = StringUtil.Format(ConsoleHostUserInterfaceSecurityResources.PromptForCredential_ReenterPassword, userName);
passwordMismatch = StringUtil.Format(ConsoleHostUserInterfaceSecurityResources.PromptForCredential_PasswordMismatch);

cred = new PSCredential(userName, password);
// now, prompt to re-enter the password.
WriteToConsole(confirmPasswordPrompt, true);
reenterPassword = ReadLineAsSecureString();
if (reenterPassword == null)
{
return null;
}

if (!SecureStringEquals(password, reenterPassword))
{
WriteToConsole(ConsoleColor.Red, ConsoleColor.Black, passwordMismatch, false);
return null;
}
}

WriteLineToConsole();
cred = new PSCredential(userName, password);
return cred;
}

private static bool SecureStringEquals(SecureString password, SecureString confirmPassword)
{
if (password.Length != confirmPassword.Length)
{
return false;
}

IntPtr pwd_ptr = IntPtr.Zero;
IntPtr confirmPwd_ptr = IntPtr.Zero;
try
{
pwd_ptr = Marshal.SecureStringToBSTR(password);
Copy link
Contributor

@PaulHigin PaulHigin Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me. However, Marshal.SecureStringToBSTR() can return a IntPtr.Zero value. I think we should check for that and return false if either pointer is that value.

if (pwd_ptr == IntPtr.Zero)
{
return false;
}

confirmPwd_ptr = Marshal.SecureStringToBSTR(confirmPassword);
if (confirmPwd_ptr == IntPtr.Zero)
{
return false;
}

int pwdLength = Marshal.ReadInt32(pwd_ptr, -4);
int equal = 0;
Copy link
Contributor

@PaulHigin PaulHigin May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear to me that doing multiple PInvoke reads is that performant, or that we really need to worry about performance at this level. For readability and maintainability I am inclined to simply copy the byte array and compare bytes (zero out array in finally block). Also note that this loop can be exited as soon as one byte mismatch is found.

Copy link
Contributor Author

@Shriram0908 Shriram0908 May 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PaulHigin Is the below snippet as per requirements.

            int size = password.Length * 2;
            IntPtr pwd_ptr = IntPtr.Zero;
            IntPtr confirmPwd_ptr = IntPtr.Zero;
            byte[] pwd = new byte[size];
            byte[] confirmPwd = new byte[size];
            try
            {
                pwd_ptr = Marshal.SecureStringToBSTR(password);
                Marshal.Copy(pwd_ptr, pwd, 0, size);

                confirmPwd_ptr = Marshal.SecureStringToBSTR(confirmPassword);
                Marshal.Copy(confirmPwd_ptr, confirmPwd, 0, size);

                return pwd.SequenceEqual(confirmPwd);
            }
            finally
            {
                if (pwd_ptr != IntPtr.Zero)
                {
                    Marshal.ZeroFreeBSTR(pwd_ptr);
                }

                if (confirmPwd_ptr != IntPtr.Zero)
                {
                    Marshal.ZeroFreeBSTR(confirmPwd_ptr);
                }

                Array.Clear(pwd, 0, size);
                Array.Clear(confirmPwd, 0, size);
                size = 0;
            }

Copy link
Collaborator

@iSazonov iSazonov May 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern with Marshal.ReadInt32 comes from StackOverflow site and could is used broadly. It look good for me.
@PaulHigin for your conclusion.

Copy link
Contributor

@PaulHigin PaulHigin Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for the churn. There are various ways to do this and I feel that perf differences are not significant. I am fine with it either way and will leave it up to the author, however I do want to see an early out for first mismatch in the loop (which was missing before).

Copy link
Contributor Author

@Shriram0908 Shriram0908 Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iSazonov @PaulHigin
I have made the changes. Please review and let me know your feedback.

for (int i = 0; i < pwdLength; i++)
{
byte c1 = Marshal.ReadByte(pwd_ptr, i);
byte c2 = Marshal.ReadByte(confirmPwd_ptr, i);
equal = c1 ^ c2;
if (equal != 0)
{
return false;
}
}

return true;
}
finally
{
if (pwd_ptr != IntPtr.Zero)
{
Marshal.ZeroFreeBSTR(pwd_ptr);
}

if (confirmPwd_ptr != IntPtr.Zero)
{
Marshal.ZeroFreeBSTR(confirmPwd_ptr);
}
}
}
}
}

6 changes: 6 additions & 0 deletions ...Microsoft.PowerShell.ConsoleHost/resources/ConsoleHostUserInterfaceSecurityResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,10 @@
<data name="PromptForCredential_Password" xml:space="preserve">
<value>Password for user {0}: </value>
</data>
<data name="PromptForCredential_ReenterPassword" xml:space="preserve">
<value>Re-enter password for user {0}:</value>
</data>
<data name="PromptForCredential_PasswordMismatch" xml:space="preserve">
<value>Passwords do not match.</value>
</data>
</root>
15 changes: 14 additions & 1 deletion src/Microsoft.PowerShell.Security/security/CredentialCommands.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ public string Title

private string _title = UtilsStrings.PromptForCredential_DefaultCaption;

/// <summary>
/// Gets or sets the confirm password prompt.
/// </summary>
[Parameter(ParameterSetName = messageSet)]
public SwitchParameter ConfirmPassword { get; set; }

/// <summary>
/// Initializes a new instance of the GetCredentialCommand
/// class.
Expand All @@ -100,7 +106,14 @@ protected override void BeginProcessing()

try
{
Credential = this.Host.UI.PromptForCredential(_title, _message, _userName, string.Empty);
Credential = this.Host.UI.PromptForCredential(
_title,
_message,
_userName,
ConfirmPassword,
string.Empty,
PSCredentialTypes.Default,
PSCredentialUIOptions.Default);
}
catch (ArgumentException exception)
{
Expand Down
Loading