URL-safe pseudorandom string generator in C#

Question

public static class SimpleToken
{
 const string TOKENALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-.";
 static string NewToken(int length = 16)
 {
 var rnd = new RNGCryptoServiceProvider();
 var tokenBytes = new byte[length];
 rnd.GetBytes(tokenBytes);
 var token =
 Enumerable
 .Range(0, length)
 .Select(i => TOKENALPHABET[tokenBytes[i] % TOKENALPHABET.Length])
 .ToArray();
 return new String(token);
 }
}

I needed a quick and dirty(?) way to generate long urls for onetime use. It's a simple login-scheme where a user enters his e-mail and gets a one-time URL for logging in. The URL is discarded after one use.

I.e. http://example.com/tokenlogin/3cuzLkh8GcANjqnWcijEeJIHphHx6ZDwfj-2XTR4bfkkqmzmmFYAY2tWsZWST1.5

• Brad M: RNGCryptoServiceProvider implements IDisposable, so put it in a using block.
• kuskmen: What are your concerns? Maybe give us some hints so we can try to be more precise in our suggestions :)
• Feras: Encode the token in base64. see: Why base64?
• rossum: If you use Base64 then you will need the URL-safe version of Base64. See RFC 4648 Setion 5.
• Christian Wattengård: But why would I want to use Base64? I don't have a source value to encode. I only want to generate a random string for use as a one-time token. And I want it to be a certain length because longer looks more secure (client thing). My real concerns was just that it looked so simple that I guessed I might have overlooked something :)

Answer 1

 const string TOKENALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-.";

I learnt the hard way that the problem with using . in a URL token sent by e-mail is that certain mail clients (Outlook in particular) will attempt to auto-detect URLs in a plain text email, but will exclude a trailing . from the inferred URL, so when your user clicks on the auto-generated link they send an invalid token. I suggest that you change . to _.

---

 static string NewToken(int length = 16)

Length in what units? Generally with cryptographic stuff it's clearer to explicitly use bits as the unit of length.

---

 var rnd = new RNGCryptoServiceProvider();

As already noted in comments, this is IDisposable and the standard using pattern is preferred.

---

 var token =
 Enumerable
 .Range(0, length)
 .Select(i => TOKENALPHABET[tokenBytes[i] % TOKENALPHABET.Length])
 .ToArray();

Firstly, this could be simplified to

 var token =
 tokenBytes
 .Select(b => TOKENALPHABET[b % TOKENALPHABET.Length])
 .ToArray();

But secondly, by only using 6 bits per byte you're throwing away 25% of the entropy which the system just produced for you. On busy servers, cryptographic-grade entropy is a valuable resource and you should only request as much as you need.

• Christian Wattengård: Thank you. That looks nice. I think I'll keep the length parameter as it is, this function isn't primarily of a cryptographic nature, it's more just to generate a pseudo-random string of length n, hence the length specified in bytes.

Answer 2

If you just want to have one-time use random URL, i change your code and now

• It,s cleaer
• Much faster than your code
• Really random string

• URL safe

  public static string NewToken()
  {
   using (RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider())
   {
   byte[] randomBuffer = new byte[16];
   rng.GetBytes(randomBuffer);
   using (MD5 md5 = MD5.Create())
   {
   byte[] hashBytes = md5.ComputeHash(randomBuffer);
   StringBuilder sBuilder = new StringBuilder();
   foreach (byte byt in hashBytes)
   {
   sBuilder.Append(byt.ToString("x2"));
   }
   return sBuilder.ToString();
   }
   }
  }
  

You can change the method to combine random generated buffer with user's email address and then calculate hash of theme, it's more reliable.

• Dan Oberlam: You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process.
• Dan Oberlam: It's not that I don't understand what you did. It's that you haven't actually reviewed his code, you've just put up your own version without explaining in your answer why it is an improvement, hence my downvote. I'd happily retract once that is added to the question
• Peter Taylor: "hash functions guarantee that the result is unique if the input is unique" is not only wrong but obviously wrong. Every standard hash function has collisions because its output space is smaller than its input space. It may be hard to find a collision, but that's not the same thing.
• Roland Illig: md5(random16bytes) can never be safer than random16bytes. If any, it is less random and therefore less safe.