8
\$\begingroup\$

I'm evaluating the security level of a file encryption app, which implements Scrypt and Inferno, in an ECIES context. The encrypted files are either stored on the user's PC/laptop, either sent with gmail. I must reach a security level appropriate for files containing sensitive data like credit card number, medical infos etc... Nowadays about any hacker can intercept an email, and a laptop can be stolen or seized, so I'm assuming that about anyone could have physical access to the encrypted file(s).

The workflow is: (assuming pre-distribution of public keys)

  1. start app on user PC/laptop(offline, no server).
  2. create/load master key pair to/from file + encrypt/decrypt using hashed password.
  3. create/load a file to work on.
  4. ETM with receiver's public key.
  5. send as gmail attachment(or store on PC if receiver is sender)

The current implementation works well on the surface, but in the given context, what should I improve/change for even better security?

DHM key storage/retrieval:

    const int _saltSize = 48; // 16 is often recommended, better safe than sorry
    const int _keySize = 48;
    const int _iterations = 1048576; // maybe randomize + store?

    private static byte[] Hash(string password, int iterations, byte[] salt = null)
    {            
        var pass = password.ToBytes();
        var saltAndHash = new byte[0];

        if (salt == null) // if encrypting, store salt
        {
            salt = new CryptoRandom().NextBytes(_saltSize);
            saltAndHash = Utils.Combine(saltAndHash, salt);
        }

        int blockSize = 8; // maybe 16 with less iterations considering modern GPUs? 
        int parallel = 1;
        return Utils.Combine(saltAndHash, SCrypt.ComputeDerivedKey(pass, salt, iterations, blockSize, parallel, null, _keySize));
    }

    internal static void SaveMasterKey(Keyring k, string password, string file)
    {
        // Keyring is simply holding the session keys in CngKey props

        var hashed = Hash(password, _iterations);
        var saltSegment = new ArraySegment<byte>(hashed, 0, _saltSize);
        var passSegment = new ArraySegment<byte>(hashed, _saltSize, _keySize);
        var blob = k.SenderDHM.GetPrivateBlob();

        using (var fs = new FileStream(file, FileMode.Create, FileAccess.Write))
        {
            fs.Write(saltSegment.ToArray(), 0, _saltSize);
            using (var etm = new EtM_EncryptTransform(passSegment.ToArray(), saltSegment))
            using (var cs = new CryptoStream(fs, etm, CryptoStreamMode.Write))
                cs.Write(blob, 0, blob.Length);
        }
    }

    internal static Keyring LoadMasterKey(string password, string file)
    {
        var salt = new byte[_saltSize];
        Keyring k = new Keyring();                       

        using (var fs = new FileStream(file, FileMode.Open, FileAccess.Read))
        {
            fs.Read(salt, 0, _saltSize);
            var hashed = Hash(password, _iterations, salt);
            if (Authenticate(file, hashed, salt))
            {                    
                using (var etm = new EtM_DecryptTransform(hashed, salt.AsArraySegment()))
                using (var cs = new CryptoStream(fs, etm, CryptoStreamMode.Read))
                {
                    byte[] blob = new byte[152];
                    cs.Read(blob, 0, blob.Length);
                    k.SenderDHM = blob.ToPrivateKeyFromBlob();
                }
            }
        }
        return k;
    }

    private static bool Authenticate(string file, byte[] key, byte[] salt)
    {
        using (var fs = new FileStream(file, FileMode.Open, FileAccess.Read))
        using (var etm = new EtM_DecryptTransform(key, salt.AsArraySegment(), authenticateOnly: true))
        {
            fs.Position = salt.Length;
            using (var cs = new CryptoStream(fs, etm, CryptoStreamMode.Read)) cs.CopyTo(Stream.Null);
            if (!etm.IsComplete) throw new Exception("Authentication failed.");
        }
        return true;
    }

File Encryption/Decryption:

    internal static void EncryptText(string text, Keyring k, string file, bool forSender)
    {
        SharedEphemeralBundle ephemeralBundle;
        if (forSender) ephemeralBundle = k.SenderDHM.GetSharedEphemeralDhmSecret();
        else ephemeralBundle = k.ReceiverDHM.GetSharedEphemeralDhmSecret();

        var ephemeralPublic = ephemeralBundle.EphemeralDhmPublicKeyBlob;
        var ephemeralSymmetric = ephemeralBundle.SharedSecret;
        var textBytes = text.ToBytes();                       

        using (var fs = new FileStream(file, FileMode.Create, FileAccess.Write))
        {                
            fs.Write(ephemeralPublic, 0, ephemeralPublic.Length);
            using (var etm = new EtM_EncryptTransform(ephemeralSymmetric))
            using (var cs = new CryptoStream(fs, etm, CryptoStreamMode.Write))
                cs.Write(textBytes, 0, textBytes.Length);                                                                                       
        }            
    }

    internal static string DecryptText(string file, Keyring k)
    {
        string decrypted = null;            
        var ephemeralPublic = new byte[104];

        using (var fs = new FileStream(file, FileMode.Open, FileAccess.Read))
        {                
            fs.Read(ephemeralPublic, 0, 104);
            var ephemeralSymmetric = k.SenderDHM.GetSharedDhmSecret(ephemeralPublic.ToPublicKeyFromBlob());
            if (Authenticate(file, ephemeralSymmetric, 104))
            {                    
                using (var etm = new EtM_DecryptTransform(ephemeralSymmetric))
                using (var cs = new CryptoStream(fs, etm, CryptoStreamMode.Read))
                {
                    var decrypt = new byte[fs.Length - 104];
                    cs.Read(decrypt, 0, decrypt.Length);
                    decrypted = decrypt.FromBytes();
                }
            }                                                                                                          
        }
        return decrypted;
    }

    // overloading the above similar
    private static bool Authenticate(string file, byte[] key, int offset)
    {
        using (var fs = new FileStream(file, FileMode.Open, FileAccess.Read))
        using (var etm = new EtM_DecryptTransform(key, authenticateOnly: true))
        {
            fs.Position = offset; 
            using (var cs = new CryptoStream(fs, etm, CryptoStreamMode.Read)) cs.CopyTo(Stream.Null);
            if (!etm.IsComplete) throw new Exception("Authentication failed.");
        }
        return true;
    }
\$\endgroup\$
8
\$\begingroup\$

The biggest issues I see are some conformity/readability issues.

For example, the following if/else structure:

if (forSender) ephemeralBundle = k.SenderDHM.GetSharedEphemeralDhmSecret();
else ephemeralBundle = k.ReceiverDHM.GetSharedEphemeralDhmSecret();

Please don't do that. It makes it significantly harder to follow. If you want to omit braces I understand but don't inline them like that.


var ephemeralPublic = new byte[104];

using (var fs = new FileStream(file, FileMode.Open, FileAccess.Read))
{                
    fs.Read(ephemeralPublic, 0, 104);
    var ephemeralSymmetric = k.SenderDHM.GetSharedDhmSecret(ephemeralPublic.ToPublicKeyFromBlob());
    if (Authenticate(file, ephemeralSymmetric, 104))
    {                    
        using (var etm = new EtM_DecryptTransform(ephemeralSymmetric))
        using (var cs = new CryptoStream(fs, etm, CryptoStreamMode.Read))
        {
            var decrypt = new byte[fs.Length - 104];

I don't know what 104 means, but you use it a lot and now to me it's very magic. Try to get it in a const that reflects the meaning of it.

\$\endgroup\$
  • \$\begingroup\$ Okay thank you. The code has changed a bit since then but I'm making a good note of your advice :) \$\endgroup\$ – Frank Jan 10 '17 at 18:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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