Secure client-side encryption of user content

Question

I am working on a pet project of mine which I've recently revived after a year long hiatus. The application is a note-taking application with client-side encryption. If you need an analogy think Evernote meets LastPass.

Before any version of the app hits the first beta testers I would like to have the encryption related parts of the code scrutinized by many more eyes.

For your convenience I've created a small Github Repository that includes all of the code shown here plus a minimal demo application (console) in a solution (C#) for Visual Studio 2013, 2015 (Community Edition will do).

Let me give you a quick conceptual overview of how the encryption in Ciphernote is supposed to work before I dive into the code. While it is not totally necessary to read the overview, it might help understanding the implementation.

User Registration: Client

1. User provides email and a password
2. Client generates a random Content Encryption Key (CEK) using a cryptographic random number generator. This key is used to encrypt all user content including text content and media resources such as images, audio etc. If this key would not exist as intermediate layer, changing a user's password would involve re-encrypting all content.
3. Derive a key for encrypting the CEK using:
   • var input = padToMaxLength(email) + password;
   • var salt = SHA512(input)
   • var contentKeyEncryptionKey = PBKDF2(input, salt, 10000)
4. Encrypt the CEK using contentKeyEncryptionKey derived in the previous step
   • var encryptedContentKey = AES256(CEK, contentKeyEncryptionKey) (prefixed with HMAC-256 over encryptedContentKey)
5. Derive a server authentication token with
   • var input = contentKeyEncryptionKey;
   • var salt = SHA512(padToMaxLength(email) + password)
   • var authToken = PBKDF2(input, salt, 10000)

User Registration: Server

6. Server receives request containing:
   • Email
   • encryptedContentKey
   • authToken

7. Generates a unique 256 Bit per-user salt using a cryptographic random number generator

8. Generates a server-side authentication token using:

   • var serverSideAuthToken = PBKDF2(authToken, salt, 100000)

9. Stores email, serverSideAuthToken and encryptedContentKey in database

Authentication

1. Client computes server authentication token as described above and passes it along with the user's email to the server
2. Server computes PBKDF2(authToken, saltFromDatabase, 100000). User is authenticated if email and derived token matches.

Note: I realize that using email+password for salting is far from ideal.

Update:

CryptoService.cs:

This class implements pretty much everything described in the previous section, except for the server part.

public class CryptoService
{
    public CryptoService(IRandomNumberGenerator rng)
    {
        this.rng = rng;
    }

    private byte[] contentKey;

    // cryptographic RNG (client platform specific)    
    private readonly IRandomNumberGenerator rng;

    protected const int Pbkdf2Iterations = 10000;
    private int IvLength = 16;
    protected const int KeyLength = 32;    // AES-256
    protected const int HmacLength = 32;    // HMAC-SHA-256
    public const int MaxUsernameLength = 256;

    /// <summary>
    /// Pads the supplied username to maxlength
    /// </summary>
    public static string PadUsername(string username, int desiredLength)
    {
        var sb = new StringBuilder(username, desiredLength);
        sb.Append('-', desiredLength - username.Length);
        return sb.ToString();
    }

    /// <summary>
    /// Derives the key for encrypting/decrypting the content key using the supplied credentials
    /// </summary>
    private Task<byte[]> GetContentKeyDecryptionKeyAsync(string username, string password)
    {
        return Task.Run(() =>
        {
            var paddedUsername = PadUsername(username, MaxUsernameLength);
            byte[] salt;

            var input = Encoding.UTF8.GetBytes(paddedUsername + password);

            using (var hasher = SHA512.Create())
                salt = hasher.ComputeHash(input);

            using (var alg = new Rfc2898DeriveBytes(input, salt, Pbkdf2Iterations))
                return alg.GetBytes(KeyLength);
        });
    }

    /// <summary>
    /// Initializes the content key from the supplied encrypted version and credentials
    /// </summary>
    public Task SetContentKeyAsync(string username, string password, Stream encryptedContentKey)
    {
        return Task.Run(async () =>
        {
            var key = await GetContentKeyDecryptionKeyAsync(username, password);

            var result = new MemoryStream();
            await Decrypt(encryptedContentKey, result, key);

            contentKey = result.ToArray();
        });
    }

    /// <summary>
    /// Returns the decrypted content key
    /// </summary>
    public byte[] GetContentKey()
    {
        return contentKey;
    }

    /// <summary>
    /// Generates a virgin content key (used during new user registration)
    /// </summary>
    public void GenerateAndSetContentKey()
    {
        contentKey = rng.GenerateRandomBytes(KeyLength);
    }

    /// <summary>
    /// Returns the content key encrypted using the provided credentials
    /// </summary>
    public Task<byte[]> GetEncryptedContentKeyAsync(string username, string password)
    {
        return Task.Run(async () =>
        {
            var key = await GetContentKeyDecryptionKeyAsync(username, password);

            return await Encrypt(contentKey, key);
        });
    }

    /// <summary>
    /// Computes an access token for the backend using the supplied credentials
    /// </summary>
    public async Task<byte[]> GetAccessTokenAsync(string username, string password)
    {
        return await Task.Run(async () =>
        {
            var paddedUsername = PadUsername(username, MaxUsernameLength);
            byte[] salt;

            using (var hasher = SHA512.Create())
                salt = hasher.ComputeHash(Encoding.UTF8.GetBytes(paddedUsername + password));

            var input = await GetContentKeyDecryptionKeyAsync(username, password);

            // request two Blocks of 20 Bytes since Rfc2898DeriveBytes uses HMAC-SHA1 internally
            using (var alg = new Rfc2898DeriveBytes(input, salt, Pbkdf2Iterations))
                return alg.GetBytes(40);
        });
    }

    public async Task Encrypt(Stream source, Stream destination, byte[] key)
    {
        Debug.Assert(key.Length == KeyLength);

        // Create Random IV
        var iv = rng.GenerateRandomBytes(IvLength);

        // Reserve space for MAC (SHA256)
        destination.SetLength(HmacLength);
        destination.Seek(0, SeekOrigin.End);

        // Prefix stream with IV
        await destination.WriteAsync(iv, 0, iv.Length);

        // Encrypt
        using (var symmetricKey = Aes.Create())
        {
            symmetricKey.KeySize = KeyLength * 8;
            symmetricKey.Mode = CipherMode.CBC;
            symmetricKey.Padding = PaddingMode.PKCS7;

            using (var encryptor = symmetricKey.CreateEncryptor(key, iv))
            {
                var cs = new CryptoStream(destination, encryptor, CryptoStreamMode.Write);

                await source.CopyToAsync(cs);

                if (!cs.HasFlushedFinalBlock)
                    cs.FlushFinalBlock();
            }
        }

        // Compute HMAC
        using (var hasher = new HMACSHA256(key))
        {
            destination.Seek(HmacLength, SeekOrigin.Begin);
            var hmac = hasher.ComputeHash(destination);
            Debug.Assert(hmac.Length == HmacLength);

            // seek to begin of IV
            destination.Seek(0, SeekOrigin.Begin);

            // write it
            destination.Write(hmac, 0, hmac.Length);
        }
    }

    public async Task Decrypt(Stream source, Stream destination, byte[] key)
    {
        Debug.Assert(key.Length == KeyLength);

        var hmac = new byte[HmacLength];
        var iv = new byte[IvLength];

        // Read HMAC
        await source.ReadAsync(hmac, 0, hmac.Length);

        // Verify HMAC
        using (var hasher = new HMACSHA256(key))
        {
            var hmacActual = hasher.ComputeHash(source);

            // compare
            if (!hmac.ConstantTimeAreEqual(hmacActual))
                throw new CryptoServiceException(CryptoServiceExceptionType.HmacMismatch);
        }

        // Read IV
        source.Seek(HmacLength, SeekOrigin.Begin);
        await source.ReadAsync(iv, 0, iv.Length);

        // Decrypt
        using (var alg = Aes.Create())
        {
            alg.KeySize = KeyLength * 8;
            alg.Mode = CipherMode.CBC;
            alg.Padding = PaddingMode.PKCS7;

            using (var decryptor = alg.CreateDecryptor(key, iv))
            {
                var cs = new CryptoStream(source, decryptor, CryptoStreamMode.Read);

                await cs.CopyToAsync(destination);

                if (!cs.HasFlushedFinalBlock)
                    cs.FlushFinalBlock();
            }
        }
    }

    public async Task<Stream> GetDecryptedStream(Stream source, byte[] key)
    {
        var hmac = new byte[HmacLength];
        var iv = new byte[IvLength];

        // Read HMAC
        await source.ReadAsync(hmac, 0, hmac.Length);

        // Verify HMAC
        using (var hasher = new HMACSHA256(key))
        {
            var hmacActual = hasher.ComputeHash(source);

            // compare
            if (!hmac.ConstantTimeAreEqual(hmacActual))
                throw new CryptoServiceException(CryptoServiceExceptionType.HmacMismatch);
        }

        // Read IV
        source.Seek(HmacLength, SeekOrigin.Begin);
        await source.ReadAsync(iv, 0, iv.Length);

        // Decrypt
        var alg = Aes.Create();
        alg.KeySize = KeyLength * 8;
        alg.Mode = CipherMode.CBC;
        alg.Padding = PaddingMode.PKCS7;

        var decryptor = alg.CreateDecryptor(key, iv);

        return new CryptoStreamWithResources(source, decryptor, CryptoStreamMode.Read,
            new IDisposable[] { alg, decryptor });
    }

    public async Task<byte[]> Encrypt(byte[] sourceBytes, byte[] key)
    {
        var source = new MemoryStream(sourceBytes);
        var destination = new MemoryStream();

        await Encrypt(source, destination, key);
        return destination.ToArray();
    }

    public async Task<byte[]> Decrypt(byte[] sourceBytes, byte[] key)
    {
        var source = new MemoryStream(sourceBytes);
        var destination = new MemoryStream();

        await Decrypt(source, destination, key);
        return destination.ToArray();
    }

    public Task EncryptContent(Stream source, Stream destination)
    {
        if(contentKey == null)
            throw new CryptoServiceException(CryptoServiceExceptionType.ContentKeyNotSet);

        return Encrypt(source, destination, contentKey);
    }

    public Task DecryptContent(Stream source, Stream destination)
    {
        if (contentKey == null)
            throw new CryptoServiceException(CryptoServiceExceptionType.ContentKeyNotSet);

        return Decrypt(source, destination, contentKey);
    }

    public Task<Stream> GetDecryptedContentStream(Stream source)
    {
        if (contentKey == null)
            throw new CryptoServiceException(CryptoServiceExceptionType.ContentKeyNotSet);

        return GetDecryptedStream(source, contentKey);
    }

    public async Task<byte[]> ComputeContentHmac(Stream source)
    {
        if (contentKey == null)
            throw new CryptoServiceException(CryptoServiceExceptionType.ContentKeyNotSet);

        return await Task.Run(() =>
        {
            using (var hasher = new HMACSHA256(contentKey))
            {
                var hmac = hasher.ComputeHash(source);
                return hmac;
            }
        });
    }
}

Answer 1

First the good news

• Your code looks clean
• Your methods are mostly short ones
• You are mostly disposing disposable objects by using the using statement
• You name your things mostly well

---

Validation

You don't validate your input parameters which is a bad habit if the methods are public. By not validating the parameters your code will throw exceptions with stacktraces which are exposing the implementation details of your code. This is something you don't want not only because you are dealing with security here.

Some hints:

• before you call Seek() on a Stream you should check if the stream is seekable.
• null checks
• range of arguments like integers etc.

---

Naming

If a method operates asynchron, using the async keyword, it should be postfixed with Async.

---

That beeing said let's dig into your code...

The first thing I noticed was your PadUsername() method. This method is a little bit too much. You could simply use PadRight(int, char) which does the same thing but in a cleaner way like so

public static string PadUsername(string username, int desiredLength)
{
    return username.PadRight(desiredLength, '-');
}  

The changed method behaves different than the former implementation this means if you pass a username with Length > desiredLength it will simply return the username. The former method would throw an ArgumentOutOfRangeException at sb.Append().

But you have another problem here which is the Xml documentation which states

/// Pads the supplied username to maxlength  

This comment is lying ! It doesn't pad to maxlength but to desired length. If a comment is not true, either change it or remove it.

---

/// <summary>
/// Returns the decrypted content key
/// </summary>
public byte[] GetContentKey()
{
    return contentKey;
}  

why is this a method ?

You should change it to a property with a private setter like so

public byte[] ContentKey 
{
    get;
    private set;
}  

which would make the backing field contentKey superflous as well.

---

public async Task Decrypt(Stream source, Stream destination, byte[] key) and public async Task<Stream> GetDecryptedStream(Stream source, byte[] key)

The verifycation of the HMAC should be extracted to a private static method. This has the advantage that you don't need a comment, the code duplication is removed and also that both methods becomes shorter.

I would change it like so

private static byte[] ComputeHash(Stream content, byte[] key)
{
    using (var hasher = new HMACSHA256(key))
    {
        return hasher.ComputeHash(content);
    }
}

and the VerifyHMAC() like so

private static void VerifyHMAC(Stream content, byte[] key)
{
    var hmacActual = ComputeHash(source, key);

    if (!hmac.ConstantTimeAreEqual(hmacActual))
    {
        throw new CryptoServiceException(CryptoServiceExceptionType.HmacMismatch);
    }
}

So each of this blocks

// Verify HMAC
using (var hasher = new HMACSHA256(key))
{
    var hmacActual = hasher.ComputeHash(source);

    // compare
    if (!hmac.ConstantTimeAreEqual(hmacActual))
        throw new CryptoServiceException(CryptoServiceExceptionType.HmacMismatch);
}  

can be replaced by

VerifyHMAC(source, key);  

---

You have a construct like the following

    byte[] salt;

    var input = Encoding.UTF8.GetBytes(paddedUsername + password);

    using (var hasher = SHA512.Create())
        salt = hasher.ComputeHash(input);

two times which should be extracted in the same way.

This

if(contentKey == null)
    throw new CryptoServiceException(CryptoServiceExceptionType.ContentKeyNotSet);  

appeary 4 times so place it into a method like

private void ValidateContentKey()
{
    if(contentKey == null)
    {
        throw new CryptoServiceException(CryptoServiceExceptionType.ContentKeyNotSet);
    }
}

---

public async Task Encrypt(Stream source, Stream destination, byte[] key)

CryptoStream is implementing IDisposable hence you should enclose its usage in a using block as well.

---

Braces {}

Omiting braces although they might be optional, like for single statment if, using etc. can lead to hidden bugs, which one dealing with security won't want. Hidden bugs are very hard to track. They can be introduced simply by mistake.

I would like to encourage you to always use them which helps to make your code less error prone and better structured (IMO).

---

Comments

Some of your comments are good like

// request two Blocks of 20 Bytes since Rfc2898DeriveBytes uses HMAC-SHA1 internally
using (var alg = new Rfc2898DeriveBytes(input, salt, Pbkdf2Iterations))
    return alg.GetBytes(40);

and some are bad like

// seek to begin of IV
destination.Seek(0, SeekOrigin.Begin);

// write it
destination.Write(hmac, 0, hmac.Length);

Comments should tell the reader of the code (which may be you or Sam the maintainer) why something is done in the way it is done. Let the code itself tell what is done by using meaningful named variables, methods and classes.

Sure it would be good to know why the Seek() from above is taking place, but this could be achieved by having and using a constant like so

private const int IVBeginning = 0;

destination.Seek(IVBeginning, SeekOrigin.Begin);

making the comment superflous.