Follow up from password rework#2740
Conversation
1. Fix an issue when the sum of minLower, minUpper is greater than 128, which circumvents the stackalloc length check. 2. Allow any length of password, don't limit to 128. 3. Add comments describing password recommendations and entropy calculation
ghost
added
the
area-integrations
| var length = Math.Max(minLength, requiredMinLength); | ||
|
|
||
| Span<char> chars = stackalloc char[length]; | ||
| Span<char> chars = length <= 128 ? stackalloc char[length] : new char[length]; |
There was a problem hiding this comment.
ArrayPool? These can be cleared when returned.
There was a problem hiding this comment.
If there's a hypothetical exploit or bug somewhere else in the process that causes the content of a
returned array pool buffer to be visible (simple as code returning the buffer to the pool before copying it to untrusted caller), this would be a new path to observing the secret.
That's likely one reason why the crypto code itself has a private array pool of its own.
There was a problem hiding this comment.
Incidentally is it best practice to zero even this private buffer before releasing the reference? (And maybe even zero the stack allocation?) Reasoning would be less likely exposure in dump files. @GrabYourPitchforks
There was a problem hiding this comment.
@bartonjs is this the reason for the crypto array pool?
There was a problem hiding this comment.
ArrayPool?
I don't think it is worth it. it's not often that someone needs a password of > 128 characters. If it becomes a problem in the future we can add it.
clearing the buffer
I'll wait for the experts to weigh in. But note that we are returning the password as a string, and passing it around in memory.
There was a problem hiding this comment.
The crypto pool was originally supposed to be a completely separate pool, for maximum defense in depth; but for reasons it ended up just being utility wrappers over the shared pool.
The shared pool doesn't clear the array by default when returning it (assuming it's for a blittable type); the crypto pool wrappers have two Return overloads; .Return(array) clears it; and .Return(array, length); clears from 0..length.
Incidentally is it best practice to zero even this private buffer before releasing the reference? (And maybe even zero the stack allocation?) Reasoning would be less likely exposure in dump files.
In System.Security.Cryptography we (try) to do that for anything where the input is considered as a key or a password, or is obviously supposed to be secret (like something being encrypted/decrypted). That's not just for warm, fuzzy feelings; but we've had customers complain that their FIPS/FedRAMP auditors complain that the program is failing a "the key is removed from memory when no longer needed" criterion. (Cue rant about GC compaction clones/ghosts).
The threat from a stackalloc is really low; but in crypto code we'll often do it (and then the conditional pool-return uses .Return(array, 0) with a comment saying it got cleared as the span).
There was a problem hiding this comment.
Ok sounds like it's worth clearing the span/array even though as you say its benefits are dubious given compaction. And as Eric says, probably not worth bothering with pool.
- clear the buffer
|
/azp run dotnet.aspire |
|
Azure Pipelines successfully started running 1 pipeline(s). |
5 participants
Microsoft Reviewers: Open in CodeFlow