Merge pull request #31 from SecurityCze/modulo_bias

Author: nitz

Diceware.cs

@@ -165,7 +165,7 @@ private static void ApplyWordCasing(string[] words, WordCasingType wordCasing, C
 						break;
 					case WordCasingType.Random:
 						char[] randomized = (from c in words[scan].ToCharArray()
-											 select ((random.GetRandomUInt64() & 1) == 0
+											 select (random.CoinToss()
 												 ? char.ToUpper(c)
 												 : char.ToLower(c))
 											)
@@ -194,7 +194,7 @@ private static void ApplyL33tSpeak(string[] words, L33tSpeakType l33tSpeak, Cryp
 			for (int scan = 0; scan < words.Length; ++scan)
 			{
 				bool mutateWord = l33tSpeak.HasFlag(L33tSpeakType.AllWords)
-					|| (random.GetRandomUInt64() & 1) == 0;
+					|| random.CoinToss();
 
 				if (mutateWord == false)
 				{
@@ -237,13 +237,12 @@ private static void ApplySalt(string[] words, SaltType salt, IEnumerable<SaltSou
 			{
 				for (int scan = 0; scan < words.Length; ++scan)
 				{
-					bool skipWord = (random.GetRandomUInt64() & 1) == 0;
-					if (skipWord)
+					if (random.CoinToss()) // skip word
 					{
 						continue;
 					}
 
-					int insertAt = random.Range(0, words[scan].Length - 1);
+					int insertAt = random.AtMost(words[scan].Length);
 
 					words[scan] = words[scan].Insert(insertAt, GenerateSalt(sources, random));
 				}
@@ -283,15 +282,10 @@ private static void ApplySalt(string[] words, SaltType salt, IEnumerable<SaltSou
 						// invalid case: can't insert between a single word.
 						throw new ArgumentOutOfRangeException(nameof(words.Length), "Salt cannot be inserted between a single word.");
 					}
-					// get a random index *between* the first and last word (e.g.: 2nd to 2nd from last)
-					int targetIndex = random.Range(1, words.Length - 2);
-					// choose if the salt should go before or after the word at the selected index
-					bool before = (random.GetRandomUInt64() & 1) == 0;
-
-					words[targetIndex] = before
-						? $"{singleSalt}{separator}{words[targetIndex]}"
-						: $"{words[targetIndex]}{separator}{singleSalt}";
+					// get a random index word after which to place salt
+					int targetIndex = random.AtMost(words.Length - 2);
 
+					words[targetIndex] = $"{words[targetIndex]}{separator}{singleSalt}";
 					break;
 				default:
 					throw new ArgumentOutOfRangeException(nameof(salt));

Extensions.cs

@@ -1,20 +1,109 @@
 
+using System;
+
 using KeePassLib.Cryptography;
 
 namespace KeePassDiceware
 {
 	internal static class Extensions
 	{
+		/// <summary>
+		/// Generates a pseudorandom value in range [0, <paramref name="maxInclusive"/>] (both ends are inclusive).
+		/// </summary>
+		/// <param name="maxInclusive"> maximal value (inclusive) of random number.</param>
+		/// <returns>
+		/// Pseudorandom value from [0, <paramref name="maxInclusive"/>] range.
+		/// </returns>
+		public static ulong AtMost(this CryptoRandomStream random, ulong maxInclusive)
+		{
+			const ulong RANDOM_MAX = ulong.MaxValue; // GetRandomUInt64 max return value
+			if (maxInclusive == RANDOM_MAX)
+			{
+				return random.GetRandomUInt64();
+			}
+
+			ulong modulus = maxInclusive + 1;
+			ulong ceil = RANDOM_MAX - (RANDOM_MAX % modulus);
+
+			ulong generated;
+			do
+			{
+				generated = random.GetRandomUInt64();
+			} while (generated >= ceil);
+
+			return generated % modulus;
+		}
+
+		/// <summary>
+		/// Generates a pseudorandom value in range [0, <paramref name="maxInclusive"/>] (both ends are inclusive).
+		/// </summary>
+		/// <param name="maxInclusive"> maximal value (inclusive) of random number.</param>
+		/// <returns>
+		/// Pseudorandom value from [0, <paramref name="maxInclusive"/>] range.
+		/// </returns>
+		public static int AtMost(this CryptoRandomStream random, int maxInclusive)
+		{
+			if (maxInclusive < 0)
+			{
+				throw new ArgumentOutOfRangeException(nameof(maxInclusive), $"Must be positive");
+			}
+
+			ulong val = random.AtMost(Convert.ToUInt64(maxInclusive));
+
+			return checked((int)val); // maxInclusive <= int.MaxValue -> wont throw
+		}
+
+		/// <summary>
+		/// Generates a pseudorandom value in range [<paramref name="minInclusive"/>, <paramref name="maxInclusive"/>] (both ends are inclusive).
+		/// </summary>
+		/// <param name="minInclusive"> minimal value (inclusive) of random number.</param>
+		/// <param name="maxInclusive"> maximal value (inclusive) of random number.</param>
+		/// <returns>
+		/// Pseudorandom value from [<paramref name="minInclusive"/>, <paramref name="maxInclusive"/>] range.
+		/// </returns>
+		/// <exception cref="ArgumentOutOfRangeException">
+		/// Thrown when the <paramref name="minInclusive"/> is larger than <paramref name="maxInclusive"/>.
+		/// </exception>
 		public static int Range(this CryptoRandomStream random, int minInclusive, int maxInclusive)
-			// #todo: avoid modulo bias
-			=> (int)(random.GetRandomUInt64() % (ulong)(maxInclusive - minInclusive + 1)) + minInclusive;
+		{
+			if (minInclusive > maxInclusive)
+			{
+				throw new ArgumentOutOfRangeException(nameof(minInclusive), $"Must be smaller or equal to {nameof(maxInclusive)}");
+			}
 
+			int randomValue = AtMost(random, maxInclusive - minInclusive);
+			
+			return randomValue + minInclusive;
+		}
+
+		/// <summary>
+		/// Selects a pseudorandom element from <paramref name="array"/>.
+		/// </summary>
+		/// <param name="array"> array from which to select the random element.</param>
+		/// <returns>
+		/// A pseudorandom element from <paramref name="array"/>.
+		/// </returns>
+		/// <exception cref="ArgumentException">
+		/// Thrown when <paramref name="array"/> is empty.
+		/// </exception>
 		public static T SelectRandom<T>(this T[] array, CryptoRandomStream random)
 		{
-			int choice = random.Range(0, array.Length - 1);
+			if (array.Length <= 0)
+			{
+				throw new ArgumentException(nameof(array), $"Unable to select a random member of empty object.");
+			}
+
+			int choice = random.AtMost(array.Length - 1);
+
 			return array[choice];
 		}
 
-		public static bool CoinToss(this CryptoRandomStream random) => (random.GetRandomUInt64() & 1) == 0;
+		/// <summary>
+		/// Return TRUE with 50% probability. Simulates a perfect coin toss.
+		/// </summary>
+		/// <returns>
+		/// TRUE / FALSE with equal probabilities.
+		/// </returns>
+		public static bool CoinToss(this CryptoRandomStream random) => (random.GetRandomBytes(1)[0] & 1) == 0;
 	}
 }