Secure client-side encryption of user content

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

• User provides email and a password

• 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.

• Derive a key for encrypting the CEK using:

• var input = padToMaxLength(email) + password;

• var salt = SHA512(input)

• var contentKeyEncryptionKey = PBKDF2(input, salt, 10000)

• Encrypt the CEK using contentKeyEncryptionKey derived in the previous step

• var encryptedContentKey = AES256(CEK, contentKeyEncryptionKey) (prefixed with HMAC-256 over encryptedContentKey)

• Derive a server authentication token with

• var input = contentKeyEncryptionKey;

• var salt = SHA512(padToMaxLength(email) + password)

• var authToken = PBKDF2(input, salt, 10000)

User Registration: Server

• Server receives request containing:

• Email

• encryptedContentKey

• authToken

-
Generates a unique 256 Bit per-user salt using a cryptographic ran

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.

/// 
/// Returns the decrypted content key
/// 
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 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

StackExchange Code Review Q#153511, answer score: 8