-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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.
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
Add SecureStringHelper.FromPlainTextString #14124
Conversation
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment. |
src/System.Management.Automation/security/SecureStringHelper.cs
Outdated
Show resolved
Hide resolved
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
#nullable enable | ||
internal static unsafe SecureString FromPlainTextString(string plainTextString) | ||
{ | ||
if (plainTextString.Length == 0) | ||
{ | ||
return new SecureString(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nullable annotations do not still work in callsites and I'd add Assert to check null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nullable annotations do not still work in callsites and I'd add Assert to check null.
Apparently so, I was able to compile code with SecureStringHelper.FromPlainTextString(null)
. In that case I would prefer to use string.IsNullOrEmpty
to avoid possible NRE.
I wasn't aware the compiler didn't check nullability at callsites, is this by design? If so that certainly reduces the value of #12631.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It checks if nullable annotations is enabled for callsites. So we get all benefits only after we finish annotations.
I agree with IsNullOrEmpty().
Debug.Assert(plainTextString is not null); | ||
|
||
if (string.IsNullOrEmpty(plainTextString)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use IsNullOrEmpty we can remove Debug.Assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current configuration makes sense; we shouldn't accept null
, but if it's given to us in the product, we'll do the right thing.
Actually, no, I'd prefer that we either return null
or throw a NullReferenceException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the null check and added documentation.
Debug.Assert(plainTextString is not null); | ||
|
||
if (string.IsNullOrEmpty(plainTextString)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current configuration makes sense; we shouldn't accept null
, but if it's given to us in the product, we'll do the right thing.
Actually, no, I'd prefer that we either return null
or throw a NullReferenceException
If we are passed null, we throw a NRE.
🎉 Handy links: |
[7.2.0-preview.5] - 2021-04-14 * Breaking Changes - Make PowerShell Linux deb and RPM packages universal (#15109) - Enforce AppLocker Deny configuration before Execution Policy Bypass configuration (#15035) - Disallow mixed dash and slash in command line parameter prefix (#15142) (Thanks @davidBar-On!) * Experimental Features - `PSNativeCommandArgumentPassing`: Use `ArgumentList` for native executable invocation (breaking change) (#14692) * Engine Updates and Fixes - Add `IArgumentCompleterFactory` for parameterized `ArgumentCompleters` (#12605) (Thanks @powercode!) * General Cmdlet Updates and Fixes - Fix SSH remoting connection never finishing with misconfigured endpoint (#15175) - Respect `TERM` and `NO_COLOR` environment variables for `$PSStyle` rendering (#14969) - Use `ProgressView.Classic` when Virtual Terminal is not supported (#15048) - Fix `Get-Counter` issue with `-Computer` parameter (#15166) (Thanks @krishnayalavarthi!) - Fix redundant iteration while splitting lines (#14851) (Thanks @hez2010!) - Enhance `Remove-Item -Recurse` to work with OneDrive (#14902) (Thanks @iSazonov!) - Change minimum depth to 0 for `ConvertTo-Json` (#14830) (Thanks @kvprasoon!) - Allow `Set-Clipboard` to accept empty string (#14579) - Turn on and off `DECCKM` to modify keyboard mode for Unix native commands to work correctly (#14943) - Fall back to `CopyAndDelete()` when `MoveTo()` fails due to an `IOException` (#15077) * Code Cleanup <details> <summary> <p>We thank the following contributors!</p> <p>@xtqqczze, @iSazonov, @ZhiZe-ZG</p> </summary> <ul> <li>Update .NET to <code>6.0.0-preview.3</code> (#15221)</li> <li>Add space before comma to hosting test to fix error reported by <code>SA1001</code> (#15224)</li> <li>Add <code>SecureStringHelper.FromPlainTextString</code> helper method for efficient secure string creation (#14124) (Thanks @xtqqczze!)</li> <li>Use static lambda keyword (#15154) (Thanks @iSazonov!)</li> <li>Remove unnecessary <code>Array</code> -> <code>List</code> -> <code>Array</code> conversion in <code>ProcessBaseCommand.AllProcesses</code> (#15052) (Thanks @xtqqczze!)</li> <li>Standardize grammar comments in Parser.cs (#15114) (Thanks @ZhiZe-ZG!)</li> <li>Enable <code>SA1001</code>: Commas should be spaced correctly (#14171) (Thanks @xtqqczze!)</li> <li>Refactor <code>MultipleServiceCommandBase.AllServices</code> (#15053) (Thanks @xtqqczze!)</li> </ul> </details> * Tools - Use Unix line endings for shell scripts (#15180) (Thanks @xtqqczze!) * Tests - Add the missing tag in Host Utilities tests (#14983) - Update `copy-props` version in `package.json` (#15124) * Build and Packaging Improvements <details> <summary> <p>We thank the following contributors!</p> <p>@JustinGrote</p> </summary> <ul> <li>Fix <code>yarn-lock</code> for <code>copy-props</code> (#15225)</li> <li>Make package validation regex accept universal Linux packages (#15226)</li> <li>Bump NJsonSchema from 10.4.0 to 10.4.1 (#15190)</li> <li>Make MSI and EXE signing always copy to fix daily build (#15191)</li> <li>Sign internals of EXE package so that it works correctly when signed (#15132)</li> <li>Bump Microsoft.NET.Test.Sdk from 16.9.1 to 16.9.4 (#15141)</li> <li>Update daily release tag format to work with new Microsoft Update work (#15164)</li> <li>Feature: Add Ubuntu 20.04 Support to install-powershell.sh (#15095) (Thanks @JustinGrote!)</li> <li>Treat rebuild branches like release branches (#15099)</li> <li>Update WiX to 3.11.2 (#15097)</li> <li>Bump NJsonSchema from 10.3.11 to 10.4.0 (#15092)</li> <li>Allow patching of preview releases (#15074)</li> <li>Bump Newtonsoft.Json from 12.0.3 to 13.0.1 (#15084, #15085)</li> <li>Update the <code>minSize</code> build package filter to be explicit (#15055)</li> <li>Bump NJsonSchema from 10.3.10 to 10.3.11 (#14965)</li> </ul> </details> * Documentation and Help Content - Merge `7.2.0-preview.4` changes to master (#15056) - Update `README` and `metadata.json` (#15046) - Fix broken links for `dotnet` CLI (#14937) [7.2.0-preview.5]: v7.2.0-preview.4...v7.2.0-preview.5
PR Summary
Add unsafe helper method using
SecureString(Char*, Int32)
to construct aSecureString
.Replace the following pattern (which allocates on every loop iteration):
PR Context
See also: dotnet/runtime#44873.
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.