3
\$\begingroup\$

I developed an application that stores large files. Those files are stored in plain text. Now the project owner wants those files be encrypted on storage, he wants confidentiality, integrity, and authenticity.

The application is written in C# using .NET Framework 4.8. There are no authenticated encryption stream cyphers in .NET Framework 4.8, so I need to create my homemade solution. I'm not a crypto guy, so I'm not sure if the solution is secure enough.

I have a secret RSA key pair acting as the master key. then I do the following to encrypt the source plain text file into the encrypted destination file and to do the opposite:

using HkdfStandard;
using System;
using System.Buffers.Binary;
using System.IO;
using System.Linq;
using System.Security.Cryptography;

namespace StreamEncryption
{
    public sealed class StreamEncryptor : IDisposable
    {
        private readonly RSA rsa;

        const int MAC_LEN = 48;
        const int HMACK_KEY_LEN = 64;
        const int AES_KEY_LEN = 32;
        const int AES_IV_LEN = 16;
        const int SALT_LEN = 32;
        const int INFO_LEN = 8;
        const int IKM_LEN = 64;
        const int PLAIN_BUFFER_SIZE = 1024 * 64 - 1;
        const int CIPHER_BUFFER_SIZE = PLAIN_BUFFER_SIZE + 1 + MAC_LEN + SALT_LEN + INFO_LEN;

        public StreamEncryptor(RSAParameters parameters)
        {
            this.rsa = new RSACng();
            rsa.ImportParameters(parameters);
        }

        public bool IsDisposed { get; private set; }

        public void Dispose()
        {
            if (IsDisposed) return;
            IsDisposed = true;
            rsa.Dispose();
        }

        static Aes CreateAesCbc256(byte[] ikm, byte[] salt, byte[] info)
        {
            var aes = Aes.Create();
            if (aes.KeySize != AES_KEY_LEN * 8) aes.KeySize = AES_KEY_LEN * 8;
            if (aes.Mode != CipherMode.CBC) aes.Mode = CipherMode.CBC;
            if (aes.Padding != PaddingMode.PKCS7) aes.Padding = PaddingMode.PKCS7;
            var keys = Hkdf.DeriveKey(HashAlgorithmName.SHA384, ikm, AES_KEY_LEN + AES_IV_LEN, salt, info);
            var key = new byte[AES_KEY_LEN];
            Array.Copy(keys, 0, key, 0, AES_KEY_LEN);
            var iv = new byte[AES_IV_LEN];
            Array.Copy(keys, AES_KEY_LEN, iv, 0, AES_IV_LEN);
            aes.Key = key;
            aes.IV = iv;
            return aes;
        }

        static byte[] GenerateRadomNumber(int size)
        {
            using (var generator = RandomNumberGenerator.Create())
            {
                var randomNumber = new byte[size];
                generator.GetBytes(randomNumber);
                return randomNumber;
            }
        }

        static byte[] Concatenate(byte[] a, byte[] b)
        {
            var result = new byte[a.Length + b.Length];
            Array.Copy(a, 0, result, 0, a.Length);
            Array.Copy(b, 0, result, a.Length, b.Length);
            return result;
        }


        static int AesEncrypt(Aes aes, byte[] input, int inputOffset, int inputCount, byte[] output, int outputOffset)
        {
            using (var transform = aes.CreateEncryptor())
            {
                int bytesWritten = 0;
                int blocks = inputCount / transform.InputBlockSize;
                int finalInputSize = inputCount - blocks * transform.InputBlockSize;
                if (blocks > 0)
                {
                    bytesWritten = transform.TransformBlock(input, inputOffset, blocks * transform.InputBlockSize, output, outputOffset);
                }
                var finalOutputBlock = transform.TransformFinalBlock(input, inputOffset + bytesWritten, finalInputSize);
                Array.Copy(finalOutputBlock, 0, output, outputOffset + bytesWritten, finalOutputBlock.Length);
                bytesWritten += finalOutputBlock.Length;
                return bytesWritten;
            }
        }

        static int AesDecrypt(Aes aes, byte[] input, int inputOffset, int inputCount, byte[] output, int outputOffset)
        {
            using (var transform = aes.CreateDecryptor(aes.Key, aes.IV))
            {
                int blocks = inputCount / transform.InputBlockSize;
                int blockBytes = (blocks - 1) * transform.InputBlockSize;
                int bytesWritten = 0;
                if (blockBytes > 0)
                {
                    bytesWritten = transform.TransformBlock(input, inputOffset, blockBytes, output, outputOffset);
                }
                var finalOuputBlock = transform.TransformFinalBlock(input, inputOffset + (blocks - 1) * transform.InputBlockSize, inputCount - blockBytes);
                Array.Copy(finalOuputBlock, 0, output, bytesWritten, finalOuputBlock.Length);
                bytesWritten += finalOuputBlock.Length;
                return bytesWritten;
            }
        }

        private static byte[] emptyBlock = new byte[0];

        static void ComputeHash(HMACSHA384 hmac, byte[] input, int inputOffset, int inputCount, byte[] output, int outputOffset)
        {
            hmac.TransformBlock(input, inputOffset, inputCount, null, 0);
            hmac.TransformFinalBlock(emptyBlock, 0, 0);
            Array.Copy(hmac.Hash, 0, output, outputOffset, hmac.Hash.Length);
        }

        private static (byte[] aesikm, byte[] hmacKey) CreateKeys()
        {
            var ikm = GenerateRadomNumber(IKM_LEN);
            var salt = GenerateRadomNumber(SALT_LEN);
            var info = GenerateRadomNumber(INFO_LEN);
            var keys = Hkdf.DeriveKey(HashAlgorithmName.SHA384, ikm, IKM_LEN + HMACK_KEY_LEN, salt, info);
            var aesikm = new byte[IKM_LEN];
            Array.Copy(keys, 0, aesikm, 0, IKM_LEN);
            var hmacKey = new byte[HMACK_KEY_LEN];
            Array.Copy(keys, IKM_LEN, hmacKey, 0, HMACK_KEY_LEN);
            return (aesikm, hmacKey);
        }

        public void Encrypt(Stream clearSource, Stream cipherDestination)
        {
            var cipherBuffer = new byte[CIPHER_BUFFER_SIZE];
            var clearBuffer = new byte[PLAIN_BUFFER_SIZE];
            var (aesikm, hmacKey) = CreateKeys();
            using (var hmac = new HMACSHA384(hmacKey))
            {
                var header = Concatenate(hmacKey, aesikm);

                var cipherHeader = rsa.Encrypt(header, RSAEncryptionPadding.OaepSHA384);
                cipherDestination.Write(cipherHeader, 0, cipherHeader.Length);

                var headerSignature = rsa.SignData(header, HashAlgorithmName.SHA384, RSASignaturePadding.Pss);
                var signatureChunk = new byte[rsa.KeySize / 16];

                Array.Copy(headerSignature, 0, signatureChunk, 0, signatureChunk.Length);
                var encryptedSignatureChunk = rsa.Encrypt(signatureChunk, RSAEncryptionPadding.OaepSHA384);
                cipherDestination.Write(encryptedSignatureChunk, 0, encryptedSignatureChunk.Length);

                Array.Copy(headerSignature, signatureChunk.Length, signatureChunk, 0, signatureChunk.Length);
                encryptedSignatureChunk = rsa.Encrypt(signatureChunk, RSAEncryptionPadding.OaepSHA384);
                cipherDestination.Write(encryptedSignatureChunk, 0, encryptedSignatureChunk.Length);

                long chunkNumber = 1;

                while (true)
                {
                    var clearBytesRead = Read(clearSource, clearBuffer, 0, clearBuffer.Length);
                    if (clearBytesRead < clearBuffer.Length) chunkNumber = -chunkNumber;
                    var salt = GenerateRadomNumber(SALT_LEN);
                    byte[] info = new byte[INFO_LEN];
                    BinaryPrimitives.WriteInt64BigEndian(info, chunkNumber);
                    using (var aes = CreateAesCbc256(aesikm, salt, info))
                    {
                        var cipherBytes = AesEncrypt(aes, clearBuffer, 0, clearBytesRead, cipherBuffer, MAC_LEN + SALT_LEN + INFO_LEN);
                        Array.Copy(salt, 0, cipherBuffer, MAC_LEN, SALT_LEN);
                        Array.Copy(info, 0, cipherBuffer, MAC_LEN + SALT_LEN, INFO_LEN);
                        ComputeHash(hmac, cipherBuffer, MAC_LEN, cipherBytes + SALT_LEN + INFO_LEN, cipherBuffer, 0);
                        cipherDestination.Write(cipherBuffer, 0, cipherBytes + MAC_LEN + SALT_LEN + INFO_LEN);
                    }
                    if (chunkNumber < 0) return;
                    chunkNumber++;
                }
            }
        }

        static void RequiredRead(Stream stream, byte[] data)
        {
            int remainingBytes = data.Length;
            while (remainingBytes > 0)
            {
                var bytesRead = stream.Read(data, data.Length - remainingBytes, remainingBytes);
                if (bytesRead == 0)
                {
                    throw new EndOfStreamException("Unexpected end of stream");
                }
                remainingBytes -= bytesRead;
            }
        }

        static int Read(Stream stream, byte[] buffer, int offset, int count)
        {
            int remainingBytesToRead = count;
            int totalBytesRead = 0;
            while (remainingBytesToRead > 0)
            {
                var bytesRead = stream.Read(buffer, offset + totalBytesRead, remainingBytesToRead);
                remainingBytesToRead -= bytesRead;
                totalBytesRead += bytesRead;
                if (bytesRead == 0)  break; 
            }
            return totalBytesRead;
        }

        public void Decrypt(Stream encryptedSource, Stream clearDestination)
        {

            var cipherMsgLength = this.rsa.KeySize / 8;

            var encHeader = new byte[cipherMsgLength];
            RequiredRead(encryptedSource, encHeader);
            var header = this.rsa.Decrypt(encHeader, RSAEncryptionPadding.OaepSHA384);

            var signedData = new byte[cipherMsgLength];

            var cipherSignatureChunk = new byte[cipherMsgLength];
            RequiredRead(encryptedSource, cipherSignatureChunk);
            var signatureChunk = this.rsa.Decrypt(cipherSignatureChunk, RSAEncryptionPadding.OaepSHA384);
            Array.Copy(signatureChunk, 0, signedData, 0, signatureChunk.Length);

            RequiredRead(encryptedSource, cipherSignatureChunk);
            signatureChunk = this.rsa.Decrypt(cipherSignatureChunk, RSAEncryptionPadding.OaepSHA384);
            Array.Copy(signatureChunk, 0, signedData, signatureChunk.Length, signatureChunk.Length);

            if (rsa.VerifyData(header, signedData, HashAlgorithmName.SHA384, RSASignaturePadding.Pss) == false)
            {
                throw new CryptographicException("Header signature validation failed");
            }

            var hmacKey = new byte[HMACK_KEY_LEN];
            var aesikm = new byte[IKM_LEN];

            Array.Copy(header, 0, hmacKey, 0, HMACK_KEY_LEN);
            Array.Copy(header, HMACK_KEY_LEN, aesikm, 0, IKM_LEN);

            var cipherBuffer = new byte[CIPHER_BUFFER_SIZE];
            var plainBuffer = new byte[PLAIN_BUFFER_SIZE];
            var storedHmac = new byte[MAC_LEN];
            var calculatedHmac = new byte[MAC_LEN];
            var salt = new byte[SALT_LEN];
            var info = new byte[INFO_LEN];

            long chunkNumber = 1;
            bool isCompleted = false;

            using (var hmac = new HMACSHA384(hmacKey))
            {
                while (true)
                {
                    int bytesRead = Read(encryptedSource, cipherBuffer, 0, CIPHER_BUFFER_SIZE);
                    if (bytesRead == 0)
                    {
                        if (isCompleted == false)
                        {
                            throw new EndOfStreamException("Unexpected end of stream");
                        }
                        return;
                    }
                    else if (isCompleted)
                    {
                        throw new CryptographicException("Some data has been externally appended to the file");
                    }
                    
                    if (bytesRead < MAC_LEN + SALT_LEN + INFO_LEN) throw new EndOfStreamException("Unexpected end of stream");
                    Array.Copy(cipherBuffer, 0, storedHmac, 0, MAC_LEN);
                    ComputeHash(hmac, cipherBuffer, MAC_LEN, bytesRead - MAC_LEN, calculatedHmac, 0);
                    if (storedHmac.SequenceEqual(calculatedHmac) == false)
                    {
                        throw new CryptographicException("Message authentication code validation failed");
                    }
                    Array.Copy(cipherBuffer, MAC_LEN, salt, 0, SALT_LEN);
                    Array.Copy(cipherBuffer, MAC_LEN + SALT_LEN, info, 0, INFO_LEN);
                    var storedChunkNumber = BinaryPrimitives.ReadInt64BigEndian(info);
                    if (storedChunkNumber < 0)
                    {
                        storedChunkNumber = -storedChunkNumber;
                        isCompleted = true;
                    }
                    if (storedChunkNumber != chunkNumber)
                    {
                        throw new CryptographicException("File chunks are not in proper sequence");
                    }
                    using (var aes = CreateAesCbc256(aesikm, salt, info))
                    {
                        var plainBytes = AesDecrypt(aes, cipherBuffer, MAC_LEN + SALT_LEN + INFO_LEN, bytesRead - MAC_LEN - SALT_LEN - INFO_LEN, plainBuffer, 0);
                        clearDestination.Write(plainBuffer, 0, plainBytes);
                    }
                    chunkNumber++;
                }
            }
        }
    }
}

The complete working solution is in GitHub.

Is this code secure enough? What best practices am I missing?

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Just a few quick finds:

  • static byte[] GenerateRadomNumber(int size)

    You have a typo here.

  • static Aes CreateAesCbc256(byte[] ikm, byte[] salt, byte[] info)

    You are using Aes.Create() which returns the desired cryptographic object as expected. What you didn't took into account is that there are some properties which are set to a default value, like Mode and Padding. Well guess what, these default values are just two of the values you are checking for.

    Speaking of checking some values, the code would be much easier to read if you just set these properties to your desired values without bothering about their former values. Removing the checking and also the setting of the default values again will lead to

      static Aes CreateAesCbc256(byte[] ikm, byte[] salt, byte[] info)
      {
          var aes = Aes.Create();
          aes.KeySize = AES_KEY_LEN * 8;
    
          var keys = Hkdf.DeriveKey(HashAlgorithmName.SHA384, ikm, AES_KEY_LEN + AES_IV_LEN, salt, info);
          var key = new byte[AES_KEY_LEN];
          Array.Copy(keys, 0, key, 0, AES_KEY_LEN);
    
          var iv = new byte[AES_IV_LEN];
          Array.Copy(keys, AES_KEY_LEN, iv, 0, AES_IV_LEN);
    
          aes.Key = key;
          aes.IV = iv;
          return aes;
      } 
    

    As you see, with a little bit vertical spacing inserted as well, the code is now much easier to read.

\$\endgroup\$
2
  • \$\begingroup\$ I'm checking for CBC mode, KeyLength and Padding because the documentation doesn't tell anything about the default values. May be I'm a bit paranoid. learn.microsoft.com/en-us/dotnet/api/… \$\endgroup\$ Commented Jan 5 at 10:14
  • 1
    \$\begingroup\$ Which is why, as indicated in the answer, it is much better to set them to specific values. It makes any auditing process easier as well, and it shows a deliberate choice. \$\endgroup\$ Commented Jan 9 at 14:13
3
+50
\$\begingroup\$

crypto primitives

The review context mentions "confidentiality" and a lack of specialized expertise.

using HkdfStandard;

This led me immediately to reading the 2010 rfc5869 description of this building block. It speaks of speed, of stretching low-entropy inputs, and of increasing attacker's cost via iterating with for example the 2000 rfc2898 PKCS #5 spec.

For client applications that need a PBKDF to stretch a low-entropy human memorable pass phrase, I will usually recommend the 2015 contest winner argon2id. It deliberately imposes both cycle and RAM costs on an attacker. Here, we appear to have "enough" source entropy, so perhaps such concerns aren't relevant. I am sad there's no comment motivating the choice of one particular primitive over another, and nothing explicitly calling out the input entropy.

I do see that we're tacking on per-file salt of 256 bits, which is an excellent match for the "confidentiality" requirement. Good.

key management

In your world-readable RSA key pair file I see you are publishing the public key parameters:

  <Modulus>
  <Exponent>

Alas, I see that you reveal these, as well:

  <P>
  <Q>
  <DP>
  <DQ>
  <InverseQ>
  <D>

Please understand that "pair" in this context refers to "public + private" key parameters. To achieve the stated "confidentiality" requirement, it is of the utmost importance that you not disclose private key parameters to unintended individuals.

Roll a new key, and re-encrypt any existing files.

key handling procedures

I am sad to find no advice, in OP or in the GitHub repo, on recommended procedures for handling keying material, including periodically expiring and re-rolling new keys. This tends to be the Achilles heel of any deployed crypto system. Pay careful attention to user education, so folks understand what they should and shouldn't do.

typo

        const int HMACK_KEY_LEN = 64;

Sorry, not following there.

We probably want to rename this 512-bit quantity as HMAC_KEY_LEN.

The consistent _LEN suffix is very nice; I thank you.

buffer length

        const int PLAIN_BUFFER_SIZE = 1024 * 64 - 1;

Ok, sixty-four KiB minus 1.

Why the minus 1?

I'm not getting the motivation. On some systems I/O is able to take advantage of power-of-two sizes. A kernel buffer might receive a 64 KiB prefix of giant file, and then kernel re-maps buffer pages in the MMU to give them away to a user-space process, with zero copy. But the minus 1 would disturb such optimizations.

Plus, 63 out of 64 reads of giant file will be mis-aligned w.r.t. cache lines.

mode of operation

            if (aes.Mode != CipherMode.CBC) aes.Mode = CipherMode.CBC;

We don't like GCM?

It offers the possibility of a pipelined and parallelized implementation. In contrast CBC forces sequential dependencies which prevent you from burning all cores if that's what you'd rather.

As a separate item, I have no idea why we first test the mode, the key size, and the padding prior to assigning them. Just jam it in there, already! Assign unconditionally.

SHA-3

            var keys = Hkdf.DeriveKey(HashAlgorithmName.SHA384, ...

This appears to be a form of SHA-2, which is starting to show its age.

Consider preferring SHA-3 here, if feasible.

Also, the plural keys is confusing, given that we just derived a single key. It is especially confusing when we .Copy(keys, 0, key, 0, ... The IV and salt are their own concepts; neither of those is a "key".

negative buffer length

You have tested this, and found it to work, at least on some inputs. So in AesDecrypt there is something going on which I do not yet understand.

                int blockBytes = (blocks - 1) * transform.InputBlockSize;

Why the minus 1?

Suppose inputCount was less than the block size, so initial blocks is zero. Then we compute a negative number of bytes here?!? What?

Recommend you make an automated test suite methodically sweep through a range of cleartext input sizes, verifying that input files of those sizes will each be successfully roundtripped through {encrypt, deccrypt}.

info field

In CreateKeys, this is weird:

            var info = GenerateRadomNumber(INFO_LEN);

There is literally no way for the decrypting end to make sense of this.

Prefer to store a constant, and verify it upon decryption. Or more usefully, maybe store the plaintext length, and later verify it.

versioned file signature

In Encrypt, please give a name to the magic number 16:

                var signatureChunk = new byte[rsa.KeySize / 16];

It is less intuitive than the cipherMsgLength expression.

I recommend that you roll a random GUID and use that as an initial magic number file signature at the start of each encrypted file you generate.

The next one or several bytes after that should mention a version number, so you will have the flexibility to make changes to this evolving codebase in a way that doesn't break decryption backward compatibility.

salt once

Within the encrypt loop we keep re-rolling a new salt, which seems odd. It forces us to write out a new salt with each block, which is needless overhead. Outputting a pseudo chunk number and a MAC also seems unnecessary. We can verify a single overall MAC after processing entire file. Length of ciphertext file should always be K + plaintext length, for some fixed K representing initial header and maybe trailer bytes. Length should not become e.g. K + 1.01 × plaintextLength, that is, 1% bigger.

The negative chunkNumber test is less natural than just asking if we're processing a (final!) partial block. And I'm a little nervous about the "zero leftover bytes" case. It's not clear that such corner cases have been thoroughly explored by an automated test suite.

decrypt

I like the RequiredRead / Read pair, nice and simple. Each does One Thing well.


compression

Consider performing LZMA compression prior to encryption.

By the time AES has scrambled the bits, there's no opportunity in future to compress down English sentences within the plaintext.

integrity

You are using some sophisticated crypto libraries that each come with the their own assumptions, interactions, and complex options. You had to make many choices. Most of them appear to be good ones. But it is hard to get crypto applications right, because so much of the output is supposed to "look random", so mistakes won't leap out at you like they would with other apps.

Consider adopting this belt-n-suspenders approach, which is amenable to unit testing, operates entirely on plaintext and views the crypto layer as a black box. Read plaintext bytes from the input file, and treat

{plaintext, plaintext.length, SHA3(plaintext)}

as the output triple you will send down to be encrypted.

Upon decrypting the crypto layer performs its various checks and maybe throws a fatal error. Else it updelivers such a triple, and at that higher layer you verify the length and the hash. Now you can be confident of integrity, even if there was some weird bug or interaction in a lower layer.

zero the IVs

You properly dispose of the RSACng and its key material -- good.

You allocate storage for several additional sensitive data structures, such as IV. Consider using a routine like ZeroMemory to securely zero them after use. That prevents secret bytes from showing up in pagefile.sys, where they can hang around and be visible to attackers.

automated testing

I didn't notice a test suite in the GitHub repo.

This code would benefit from both unit and integration tests. I would especially want to see methodical sweeps of input sizes, verifying that corner cases work.


This codebase appears to achieve many of its design goals.

I would be willing to delegate or accept maintenance tasks on it.

\$\endgroup\$
1
  • \$\begingroup\$ Appreciate the mention of compression here. Important. \$\endgroup\$
    – Joop Eggen
    Commented Jan 10 at 20:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.