Skip to content

Commit

Permalink
Improvements and bug fixes after a personal code review, as well as i…
Browse files Browse the repository at this point in the history 
…ndependent professional audit (Cure53 team).

-AesCtrCryptoTransform improvements.
-CryptoRandom bug fixes.
  "NextDouble()" now generates 52 bits of entropy.
  "NextBytes()" is rewritten to avoid a possible serious concurrency flaw.
-Minor improvements and fixes throughout the codebase.
-Version bump to 1.4.
  • Loading branch information
@sdrapkin
sdrapkin committed Oct 5, 2016
1 parent 78fae76 commit adc5dc0
Show file tree
Hide file tree
Showing 19 changed files with 153 additions and 126 deletions.
2 changes: 2 additions & 0 deletions .gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,3 +238,5 @@ _Doc/*
/Mac/HMAC2*.cs
/.vs/config/applicationhost.config
/Inferno.1.0.0.nuspec
/COMMITS.md
*.snk
57 changes: 40 additions & 17 deletions Cipher/AesCtrCryptoTransform.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,16 @@ internal static class AesConstants

public class AesCtrCryptoTransform : ICryptoTransform
{
byte[] counterBuffer = new byte[AesConstants.AES_BLOCK_SIZE];
byte[] counterBuffer_KeyStreamBuffer = new byte[AesConstants.AES_BLOCK_SIZE * 2];
int keyStreamBytesRemaining;

Aes aes;
readonly ICryptoTransform cryptoTransform;

public bool CanReuseTransform { get { return false; } }
public bool CanTransformMultipleBlocks { get { return true; } }
public int InputBlockSize { get { return AesConstants.AES_BLOCK_SIZE; } }
public int OutputBlockSize { get { return AesConstants.AES_BLOCK_SIZE; } }
public int InputBlockSize { get { return 1; } }
public int OutputBlockSize { get { return 1; } }

/// <summary>ctor</summary>
public AesCtrCryptoTransform(byte[] key, ArraySegment<byte> counterBufferSegment, Func<Aes> aesFactory = null)
Expand All @@ -33,39 +34,61 @@ public AesCtrCryptoTransform(byte[] key, ArraySegment<byte> counterBufferSegment
this.aes.Mode = CipherMode.ECB;
this.aes.Padding = PaddingMode.None;

Utils.BlockCopy(counterBufferSegment.Array, counterBufferSegment.Offset, counterBuffer, 0, AesConstants.AES_BLOCK_SIZE);
Utils.BlockCopy(counterBufferSegment.Array, counterBufferSegment.Offset, counterBuffer_KeyStreamBuffer, 0, AesConstants.AES_BLOCK_SIZE);
this.cryptoTransform = aes.CreateEncryptor(rgbKey: key, rgbIV: null);
}// ctor

#region public
public int TransformBlock(byte[] inputBuffer, int inputOffset, int inputCount, byte[] outputBuffer, int outputOffset)
{
int partialBlockSize = inputCount % AesConstants.AES_BLOCK_SIZE;
int fullBlockSize = inputCount & (-AesConstants.AES_BLOCK_SIZE);//inputCount - partialBlockSize;
int i, j;
byte[] counterBuffer = this.counterBuffer; // looks dumb, but local-access is faster than field-access
if (inputCount == 0) return 0;

for (i = outputOffset, /* reusing inputCount as iMax */ inputCount = outputOffset + fullBlockSize; i < inputCount; i += AesConstants.AES_BLOCK_SIZE)
int i, j, k, remainingInputCount = inputCount;
byte[] counterBuffer_KeyStreamBuffer = this.counterBuffer_KeyStreamBuffer; // looks dumb, but local-access is faster than field-access

// process any available key stream first
if (this.keyStreamBytesRemaining > 0)
{
Utils.BlockCopy(counterBuffer, 0, outputBuffer, i, AesConstants.AES_BLOCK_SIZE);
for (j = AesConstants.AES_BLOCK_SIZE - 1; j >= AesConstants.AES_BLOCK_SIZE - AesConstants.COUNTER_SIZE; --j)
if (++counterBuffer[j] != 0) break;
j = inputCount > this.keyStreamBytesRemaining ? this.keyStreamBytesRemaining : inputCount;
for (i = 0; i < j; ++i)
outputBuffer[outputOffset + i] = (byte)(counterBuffer_KeyStreamBuffer[AesConstants.AES_BLOCK_SIZE * 2 - this.keyStreamBytesRemaining + i] ^ inputBuffer[inputOffset + i]);

inputOffset += j;
outputOffset += j;
this.keyStreamBytesRemaining -= j;
remainingInputCount -= j;

if (remainingInputCount == 0) return inputCount;
}

int partialBlockSize = remainingInputCount % AesConstants.AES_BLOCK_SIZE;
int fullBlockSize = remainingInputCount & (-AesConstants.AES_BLOCK_SIZE); // remainingInputCount - partialBlockSize;

// process full blocks, if any
if (fullBlockSize > 0)
{
for (i = outputOffset, /* reusing k as iMax */ k = outputOffset + fullBlockSize; i < k; i += AesConstants.AES_BLOCK_SIZE)
{
Utils.BlockCopy(counterBuffer_KeyStreamBuffer, 0, outputBuffer, i, AesConstants.AES_BLOCK_SIZE);
for (j = AesConstants.AES_BLOCK_SIZE - 1; j >= AesConstants.AES_BLOCK_SIZE - AesConstants.COUNTER_SIZE; --j) if (++counterBuffer_KeyStreamBuffer[j] != 0) break;
}

fullBlockSize = this.cryptoTransform.TransformBlock(outputBuffer, outputOffset, fullBlockSize, outputBuffer, outputOffset);
//for (i = 0; i < fullBlockSize; ++i) outputBuffer[outputOffset + i] ^= inputBuffer[inputOffset + i];
Utils.Xor(outputBuffer, outputOffset, inputBuffer, inputOffset, fullBlockSize);
}

// process the remaining partial block, if any
if (partialBlockSize > 0)
{
outputOffset += fullBlockSize;
inputOffset += fullBlockSize;
this.cryptoTransform.TransformBlock(counterBuffer, 0, AesConstants.AES_BLOCK_SIZE, counterBuffer, 0);
for (i = 0; i < partialBlockSize; ++i) outputBuffer[outputOffset + i] = (byte)(counterBuffer[i] ^ inputBuffer[inputOffset + i]);
outputOffset += fullBlockSize;

this.cryptoTransform.TransformBlock(counterBuffer_KeyStreamBuffer, 0, AesConstants.AES_BLOCK_SIZE, counterBuffer_KeyStreamBuffer, AesConstants.AES_BLOCK_SIZE);
for (j = AesConstants.AES_BLOCK_SIZE - 1; j >= AesConstants.AES_BLOCK_SIZE - AesConstants.COUNTER_SIZE; --j) if (++counterBuffer_KeyStreamBuffer[j] != 0) break;
for (i = 0; i < partialBlockSize; ++i) outputBuffer[outputOffset + i] = (byte)(counterBuffer_KeyStreamBuffer[AesConstants.AES_BLOCK_SIZE + i] ^ inputBuffer[inputOffset + i]);
this.keyStreamBytesRemaining = AesConstants.AES_BLOCK_SIZE - partialBlockSize;
}

return inputCount;
}// TransformBlock()

Expand All @@ -88,7 +111,7 @@ public void Dispose()
}
finally
{
Array.Clear(counterBuffer, 0, AesConstants.AES_BLOCK_SIZE);
Array.Clear(this.counterBuffer_KeyStreamBuffer, 0, AesConstants.AES_BLOCK_SIZE * 2);
this.aes = null;
}
}
Expand Down
125 changes: 50 additions & 75 deletions CryptoRandom.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public long NextLong()
public long NextLong(long maxValue)
{
if (maxValue < 0)
throw new ArgumentOutOfRangeException("maxValue");
throw new ArgumentOutOfRangeException(nameof(maxValue));

return NextLong(0, maxValue);
}//NextLong()
Expand All @@ -96,7 +96,7 @@ public long NextLong(long minValue, long maxValue)
return minValue;

if (minValue > maxValue)
throw new ArgumentOutOfRangeException("minValue");
throw new ArgumentOutOfRangeException(nameof(minValue));

ulong diff = decimal.ToUInt64((decimal)maxValue - minValue);
ulong upperBound = ulong.MaxValue / diff * diff;
Expand Down Expand Up @@ -136,7 +136,7 @@ public override int Next()
public override int Next(int maxValue)
{
if (maxValue < 0)
throw new ArgumentOutOfRangeException("maxValue");
throw new ArgumentOutOfRangeException(nameof(maxValue));

return Next(0, maxValue);
}//Next()
Expand All @@ -158,7 +158,7 @@ public override int Next(int minValue, int maxValue)
return minValue;

if (minValue > maxValue)
throw new ArgumentOutOfRangeException("minValue");
throw new ArgumentOutOfRangeException(nameof(minValue));

long diff = (long)maxValue - minValue;
long upperBound = uint.MaxValue / diff * diff;
Expand All @@ -180,8 +180,8 @@ public override int Next(int minValue, int maxValue)
/// </returns>
public override double NextDouble()
{
const double max = 1.0 + uint.MaxValue;
return GetRandomUInt() / max;
const double max = 1L << 53; // https://en.wikipedia.org/wiki/Double-precision_floating-point_format
return (GetRandomULong() >> 11) / max;
}//NextDouble()

/// <summary>
Expand Down Expand Up @@ -220,7 +220,7 @@ public void NextBytes(byte[] buffer, int offset, int count)
var checkedBufferSegment = new ArraySegment<byte>(buffer, offset, count); // bounds-validation happens here
if (count == 0) return;
NextBytesInternal(checkedBufferSegment);
}
}//NextBytes()

void NextBytesInternal(ArraySegment<byte> bufferSegment)
{
Expand All @@ -236,68 +236,50 @@ void NextBytesInternal(ArraySegment<byte> bufferSegment)
throw new CryptographicException((int)status);
}

while (true)
lock (_byteCache)
{
int currentByteCachePosition = Interlocked.Add(ref _byteCachePosition, count);
if (currentByteCachePosition <= BYTE_CACHE_SIZE && currentByteCachePosition > 0)
if (_byteCachePosition + count <= BYTE_CACHE_SIZE)
{
Utils.BlockCopy(_byteCache, currentByteCachePosition - count, buffer, 0, count);
Utils.BlockCopy(_byteCache, _byteCachePosition, buffer, offset, count);
_byteCachePosition += count;
return;
}

lock (_byteCache)
status = BCrypt.BCryptGenRandom(_byteCache, BYTE_CACHE_SIZE);
if (status == BCrypt.NTSTATUS.STATUS_SUCCESS)
{
currentByteCachePosition = _byteCachePosition; // atomic read
if (currentByteCachePosition > (BYTE_CACHE_SIZE - count) || currentByteCachePosition <= 0)
{
status = BCrypt.BCryptGenRandom(_byteCache, BYTE_CACHE_SIZE);
if (status == BCrypt.NTSTATUS.STATUS_SUCCESS)
{
_byteCachePosition = count; // atomic write
Utils.BlockCopy(_byteCache, 0, buffer, 0, count);
return;
}

// defensive logic to prevent _byteCachePosition from wrapping into valid range due to BCryptGenRandom failures
if (currentByteCachePosition > BYTE_CACHE_SIZE || currentByteCachePosition < 0) _byteCachePosition = BYTE_CACHE_SIZE;

throw new CryptographicException((int)status);
}// if outside the valid range
}// lock
}// while(true)
}//NextBytes()
_byteCachePosition = count;
Utils.BlockCopy(_byteCache, 0, buffer, offset, count);
return;
}
throw new CryptographicException((int)status);
}// lock
}//NextBytesInternal()

/// <summary>
/// Gets one random unsigned 32bit integer in a thread safe manner.
/// </summary>
uint GetRandomUInt()
{
BCrypt.NTSTATUS status;
while (true)

lock (_byteCache)
{
int currentByteCachePosition = Interlocked.Add(ref _byteCachePosition, sizeof(uint));
if (currentByteCachePosition <= BYTE_CACHE_SIZE && currentByteCachePosition > 0)
return BitConverter.ToUInt32(_byteCache, currentByteCachePosition - sizeof(uint));
if (_byteCachePosition + sizeof(uint) <= BYTE_CACHE_SIZE)
{
var result = BitConverter.ToUInt32(_byteCache, _byteCachePosition);
_byteCachePosition += sizeof(uint);
return result;
}

lock (_byteCache)
status = BCrypt.BCryptGenRandom(_byteCache, BYTE_CACHE_SIZE);
if (status == BCrypt.NTSTATUS.STATUS_SUCCESS)
{
currentByteCachePosition = _byteCachePosition; // atomic read
if (currentByteCachePosition > (BYTE_CACHE_SIZE - sizeof(uint)) || currentByteCachePosition <= 0)
{
status = BCrypt.BCryptGenRandom(_byteCache, BYTE_CACHE_SIZE);
if (status == BCrypt.NTSTATUS.STATUS_SUCCESS)
{
_byteCachePosition = sizeof(uint); // atomic write
return BitConverter.ToUInt32(_byteCache, 0);
}

// defensive logic to prevent _byteCachePosition from wrapping into valid range due to BCryptGenRandom failures
if (currentByteCachePosition > BYTE_CACHE_SIZE || currentByteCachePosition < 0) _byteCachePosition = BYTE_CACHE_SIZE;

throw new CryptographicException((int)status);
}// if outside the valid range
}// lock
}// while(true)
_byteCachePosition = sizeof(uint);
return BitConverter.ToUInt32(_byteCache, 0);
}
throw new CryptographicException((int)status);
}// lock
}//GetRandomUInt()

/// <summary>
Expand All @@ -306,31 +288,24 @@ uint GetRandomUInt()
ulong GetRandomULong()
{
BCrypt.NTSTATUS status;
while (true)

lock (_byteCache)
{
int currentByteCachePosition = Interlocked.Add(ref _byteCachePosition, sizeof(ulong));
if (currentByteCachePosition <= BYTE_CACHE_SIZE && currentByteCachePosition > 0)
return BitConverter.ToUInt64(_byteCache, currentByteCachePosition - sizeof(ulong));
if (_byteCachePosition + sizeof(ulong) <= BYTE_CACHE_SIZE)
{
var result = BitConverter.ToUInt64(_byteCache, _byteCachePosition);
_byteCachePosition += sizeof(ulong);
return result;
}

lock (_byteCache)
status = BCrypt.BCryptGenRandom(_byteCache, BYTE_CACHE_SIZE);
if (status == BCrypt.NTSTATUS.STATUS_SUCCESS)
{
currentByteCachePosition = _byteCachePosition; // atomic read
if (currentByteCachePosition > (BYTE_CACHE_SIZE - sizeof(ulong)) || currentByteCachePosition <= 0)
{
status = BCrypt.BCryptGenRandom(_byteCache, BYTE_CACHE_SIZE);
if (status == BCrypt.NTSTATUS.STATUS_SUCCESS)
{
_byteCachePosition = sizeof(ulong); // atomic write
return BitConverter.ToUInt64(_byteCache, 0);
}

// defensive logic to prevent _byteCachePosition from wrapping into valid range due to BCryptGenRandom failures
if (currentByteCachePosition > BYTE_CACHE_SIZE || currentByteCachePosition < 0) _byteCachePosition = BYTE_CACHE_SIZE;

throw new CryptographicException((int)status);
}// if outside the valid range
}// lock
}// while(true)
_byteCachePosition = sizeof(ulong);
return BitConverter.ToUInt64(_byteCache, 0);
}
throw new CryptographicException((int)status);
}// lock
}//GetRandomULong()
}//class CryptoRandom

Expand Down
2 changes: 1 addition & 1 deletion EtM_CBC.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public static void Encrypt(byte[] masterKey, ArraySegment<byte> plaintext, byte[
int finalBlockLength = plaintext.Count % Cipher.AesConstants.AES_BLOCK_SIZE;
int paddingLength = Cipher.AesConstants.AES_BLOCK_SIZE - finalBlockLength;
int ciphertextLength = CONTEXT_BUFFER_LENGTH + plaintext.Count + paddingLength + MAC_LENGTH;
if (output.Length - outputOffset < ciphertextLength) throw new ArgumentOutOfRangeException("output", "'output' array segment is not big enough for the ciphertext");
if (output.Length - outputOffset < ciphertextLength) throw new ArgumentOutOfRangeException(nameof(output), $"'{nameof(output)}' array segment is not big enough for the ciphertext");

try
{
Expand Down
2 changes: 1 addition & 1 deletion EtM_CTR.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ static void ClearKeyMaterial()
public static void Encrypt(byte[] masterKey, ArraySegment<byte> plaintext, byte[] output, int outputOffset, ArraySegment<byte>? salt = null, uint counter = 1)
{
int ciphertextLength = CONTEXT_BUFFER_LENGTH + plaintext.Count + MAC_LENGTH;
if (output.Length - outputOffset < ciphertextLength) throw new ArgumentOutOfRangeException("output", "'output' array segment is not big enough for the ciphertext");
if (output.Length - outputOffset < ciphertextLength) throw new ArgumentOutOfRangeException(nameof(output), $"'{nameof(output)}' array segment is not big enough for the ciphertext");
try
{
_cryptoRandom.NextBytes(_contextBuffer.Value);
Expand Down
8 changes: 4 additions & 4 deletions EtM_Transforms.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public class EtM_EncryptTransform : ICryptoTransform
/// <summary>ctor</summary>
public EtM_EncryptTransform(byte[] key, ArraySegment<byte>? salt = null, uint chunkNumber = 1)
{
if (key == null) throw new ArgumentNullException("key", "key cannot be null.");
if (key == null) throw new ArgumentNullException(nameof(key), nameof(key) + " cannot be null.");
this.key = key;
this.salt = salt;
this.currentChunkNumber = chunkNumber;
Expand All @@ -53,7 +53,7 @@ public int TransformBlock(byte[] inputBuffer, int inputOffset, int inputCount, b
output: outputBuffer,
outputOffset: outputOffset + j,
salt: this.salt,
counter: this.currentChunkNumber++);
counter: checked(this.currentChunkNumber++));
}
}
return j;
Expand Down Expand Up @@ -104,7 +104,7 @@ public class EtM_DecryptTransform : ICryptoTransform
/// <summary>ctor</summary>
public EtM_DecryptTransform(byte[] key, ArraySegment<byte>? salt = null, uint chunkNumber = 1, bool authenticateOnly = false)
{
if (key == null) throw new ArgumentNullException("key", "key cannot be null.");
if (key == null) throw new ArgumentNullException(nameof(key), nameof(key) + " cannot be null.");
this.key = key;
this.salt = salt;
this.currentChunkNumber = chunkNumber;
Expand Down Expand Up @@ -152,7 +152,7 @@ public int TransformBlock(byte[] inputBuffer, int inputOffset, int inputCount, b
this.key = null;
throw new CryptographicException("Decryption failed for block " + this.currentChunkNumber.ToString() + ".");
}
++this.currentChunkNumber;
this.currentChunkNumber = checked(this.currentChunkNumber + 1);
}
}
return j;
Expand Down
2 changes: 1 addition & 1 deletion Extensions/B64Extensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public static string ToB64(this ArraySegment<byte> inputSegment)
public static byte[] FromB64(this string input)
{
if (input == null)
throw new ArgumentNullException("input");
throw new ArgumentNullException(nameof(input));

int len = input.Length;
if (len < 1)
Expand Down
2 changes: 1 addition & 1 deletion Extensions/Base16Config.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public Base16Config(char[] alphabet = null)
}

if (alphabet.Length != BASE)
throw new ArgumentOutOfRangeException("alphabet", "'alphabet' array must have exactly " + BASE + " characters.");
throw new ArgumentOutOfRangeException(nameof(alphabet), $"'{nameof(alphabet)}' array must have exactly {BASE.ToString()} characters.");

this.Base16table = alphabet;

Expand Down
Loading

0 comments on commit adc5dc0

Please sign in to comment.