Skip to content
This repository was archived by the owner on Dec 14, 2018. It is now read-only.

Moving to RandomNumberGenerator as CryptRandom is not supported in Mono #857

Closed
wants to merge 1 commit into from
Closed

Moving to RandomNumberGenerator as CryptRandom is not supported in Mono #857

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion src/Microsoft.AspNet.Mvc.Core/AntiForgery/BinaryBlob.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Diagnostics.Contracts;
using System.Globalization;
using System.Runtime.CompilerServices;
using System.Security.Cryptography;
using System.Text;
using Microsoft.AspNet.Security.DataProtection;

Expand All @@ -17,6 +18,7 @@ namespace Microsoft.AspNet.Mvc
[DebuggerDisplay("{DebuggerString}")]
internal sealed class BinaryBlob : IEquatable<BinaryBlob>
{
private static readonly RandomNumberGenerator _randomNumberGenerator = RandomNumberGenerator.Create();
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for caching the generator. Is there a significant cost to create the generator?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does mention that members aren't thread safe - http://msdn.microsoft.com/en-us/library/system.security.cryptography.randomnumbergenerator(v=vs.110).aspx. So invoking GetBytes concurrently might be bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well AFAIK RandomNumberGenerator.Create does some dictionary lookups inorder to figure out the default cryptographic random number generator ( and returns with RNGCryptoProvider instance), however actual perf would need to be measured. But as @pranavkm pointed out that GetBytes is not thread safe ( even though create is ) I think I will move out the cached and create a RandomNumberGenerator everytime a token is generated ( and cache it later if we see it to be the bottleneck).
(Unfortunately we don't have RNGCryptoProviderService in core clr, as that is thread safe).

Copy link
Contributor

Choose a reason for hiding this comment

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

All implementations of RandomNumberGenerator are required to be thread-safe, so it's safe to use a cached instance.

private readonly byte[] _data;

// Generates a new token using a specified bit length.
Expand Down Expand Up @@ -93,7 +95,7 @@ public override int GetHashCode()
private static byte[] GenerateNewToken(int bitLength)
{
var data = new byte[bitLength / 8];
CryptRand.FillBuffer(new ArraySegment<byte>(data));
_randomNumberGenerator.GetBytes(data);
return data;
}

Expand Down