Random Number Generation in ASP .NET

I wrote a previous article about Generating a Random Password using the ASP .NET provider.

Thanks to Joakim Uddholm for the comment that the Random() function was not really secure.
In this case, I think it was secure enough, but that doesn't change the fact that it is certainly a weak point in the system, which could be exploited under certain conditions.

In the previous article I tackled the main problem with Random(), which is that it returns random numbers in a fixed repeatable sequence for a given seed value, and that when created with the default (empty) constructor it is seeded with the current time. This means that if multiple instances of Random() are instantiated in a short period of time (before the time changes), they will return precisely the same sequence of 'random' numbers.

Googling around, I see comments on StackOverflow that agree with Joakim's position that System.Random() simply isn't meant for any security related purpose. This is probably true, however I'm equally certain that people for a variety of reasons will inevitably attempt to use it for security functions.

For this reason I'm posting first an improved version of the static Random() wrapper using a seed that isn't able to be associated with the current time. I'm still using a periodic re-seeding behaviour because I'm concerned that someone may be able to recognise a section of  the fixed 'random' sequence and use that to predict new values. This just emphasises that we shouldn't really be using System.Random() at all.
Anyway, I think this one is probably as good as we'll get to secure random values using Random().

Secondly, I'm posting a method to return a random int up to a given value using System.Security.Cryptography.RNGCryptoServiceProvider
I'm definitely recommending using the second method for any security related purposes.

The improved System.Random wrapper:

private static Random randomNumGenerator = new Random();
private static DateTime lastRandomNumGeneratorSeedTime = DateTime.Now;

public static Random RandomNumGenerator {
  get {
    lock (typeof(Random)) {
      if (randomNumGenerator == null) {
        randomNumGenerator = new Random();
      } else {
        if (DateTime.Now > lastRandomNumGeneratorSeedTime.AddSeconds(1)) {

          randomNumGenerator = new Random(randomNumGenerator.Next(Int16.MaxValue) * DateTime.Now.Millisecond);
          lastRandomNumGeneratorSeedTime = DateTime.Now;
      }
    }
    return randomNumGenerator;
   }
  }
}

And the better, crypto derived method. I've read comments about the performance when retrieving multiple bytes, so I've split it to only retrieve as many random bytes as it needs. The basic code is mainly from this MSDN page.

  private static System.Security.Cryptography.RNGCryptoServiceProvider rngCsp
    = new System.Security.Cryptography.RNGCryptoServiceProvider();

  public static int CryptoRandomNumber(int maxRndValue) {
    // deal with byte and UInt16 values separately for performance reasons
    if (maxRndValue <= Byte.MaxValue) {
      byte[] randomNumber = new byte[1];
      do {
        rngCsp.GetBytes(randomNumber);
      }
      while (!IsFairRoll(randomNumber[0], maxRndValue, Byte.MaxValue));

      return (int)(randomNumber[0] % maxRndValue);
    }

    if (maxRndValue <= UInt16.MaxValue) {
      byte[] randomNumber = new byte[2];
      int rnd = 0;
      do {
        rngCsp.GetBytes(randomNumber);
        rnd = (int)(randomNumber[0] + randomNumber[1] * 256);
      }
      while (!IsFairRoll(rnd, maxRndValue, UInt16.MaxValue));

      return (rnd % maxRndValue);
    }

    int rnd1 = 0;
    byte[] randomNumber1 = new byte[4];
    do {
      rngCsp.GetBytes(randomNumber1);
      rnd1 = (int)(randomNumber1[0] + randomNumber1[1] * 256 
        + randomNumber1[2] * 256 * 256 + randomNumber1[3] * 256 * 256 * 256);
      if (rnd1 < 0) { rnd1 = (rnd1 + 1) * -1; } 
    }
    while (!IsFairRoll(rnd1, maxRndValue, int.MaxValue));

    return (rnd1 % maxRndValue);
  }

  private static bool IsFairRoll(int result, int maxRndValue, int arrayMaxValue) {
    // There are MaxValue / numSides full sets of numbers that can come up 
    // in a single byte.  For instance, if we have a 6 sided die, there are 
    // 42 full sets of 1-6 that come up.  The 43rd set is incomplete. 
    int fullSetsOfValues = arrayMaxValue / maxRndValue;

    // If the roll is within this range of fair values, then we let it continue. 
    // In the 6 sided die case, a roll between 0 and 251 is allowed.  (We use 
    // < rather than <= since the = portion allows through an extra 0 value). 
    // 252 through 255 would provide an extra 0, 1, 2, 3 so they are not fair 
    // to use. 
    return result < maxRndValue * fullSetsOfValues;
  }

The new password generation function from the previous post now looks like this:

public static string GenerateFriendlyPassword(int length) {
    string chars = "abcdefghijkmnpqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ0123456789";
    var password = new StringBuilder(length);

    for (int i = 0; i < length; i++) {
        password.Append(chars[Gbl.CryptoRandomNumber(chars.Length)]);
    }
    return password.ToString();
}

Thanks!