Skip to content
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

Use ArrayPool in PasswordHasher #17532

Closed
Kahbazi opened this issue Dec 2, 2019 · 2 comments
Closed

Use ArrayPool in PasswordHasher #17532

Kahbazi opened this issue Dec 2, 2019 · 2 comments
Labels

Comments

@Kahbazi
Copy link
Contributor

@Kahbazi Kahbazi commented Dec 2, 2019

Can ArrayPool be used in PasswordHasher to improve memory allocation? or it does not have such an impact since the array size of the bytes are low

@GrabYourPitchforks

This comment has been minimized.

Copy link
Contributor

@GrabYourPitchforks GrabYourPitchforks commented Dec 3, 2019

I strongly recommend against using ArrayPool for security-sensitive operations such as password hashing. The reason for this is that if there's a bug in a completely unrelated part of the system that causes a double-return to the pool (say, a race condition in the JSON stack or in the networking stack), the resulting memory corruption could result in password leakage. If you look at the recent commits which have been made to the System.Security.Cryptography project, you'll see that we generally avoid ArrayPool in most of those code paths for this same reason.

If you absolutely need to avoid allocations (and in this particular code path it's probably not absolutely necessary), you could consider using stackalloc, falling back to a new byte[] (instead of ArrayPool<byte>.Shared.Rent if the allocation is too large. The DataProtection layer does something similar to this and could be used as a template. But do be cautioned that stackalloc introduces its own risks, so folks shouldn't generally shouldn't be encouraged to go down this path unless there's profiling evidence that such a change is worth the risk.

@blowdart

This comment has been minimized.

Copy link
Member

@blowdart blowdart commented Dec 3, 2019

What @GrabYourPitchforks said. There's too much risk for little to no benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.