4
\$\begingroup\$

Please tell me if I'm doing this right, and what else is needed to make this hardened.

It seems like it's too simple, like there's almost nothing I'm actually doing. For example, there are 4 or 5 stacked using statements!

Here is the code to review:

class Program
{
    static void Main(string[] args)
    {
        var enc = true;
        var fileIn = string.Empty;
        var fileOut = string.Empty;

        /* parse command line to populate enc, fileIn, fileOut */

        Console.Write("key: ");
        var key = Encoding.UTF8.GetBytes(Console.ReadLine());
        Console.Write("IV: ");
        var iv = Encoding.UTF8.GetBytes(Console.ReadLine());

        using (var aes = AesManaged.Create())
        using (var md5 = MD5.Create())
        using (var readStream = File.OpenRead(fileIn))
        using (var writeStream = File.OpenWrite(fileOut))
        using (var cryptoStream = new CryptoStream(
            enc ? writeStream : readStream, 
            enc 
                ? aes.CreateEncryptor(md5.ComputeHash(key), md5.ComputeHash(iv))
                : aes.CreateDecryptor(md5.ComputeHash(key), md5.ComputeHash(iv)),
            enc ? CryptoStreamMode.Write : CryptoStreamMode.Read))
        {
            if (enc) { readStream.CopyToAsync(cryptoStream).Wait(); }
            else { cryptoStream.CopyToAsync(writeStream).Wait(); }
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ CreateEncryptor() and CreateDecryptor both return an IDisposable object which should be wrapped in a using clause before it's passed to the CryptoStream. \$\endgroup\$ – Jesse C. Slicer May 26 '17 at 1:01
  • 5
    \$\begingroup\$ I think to get the best value out of reviews/answers, you should definitely include the /* parse command line to populate enc, fileIn, fileOut */ commented-out part. \$\endgroup\$ – Mathieu Guindon May 26 '17 at 1:47
  • \$\begingroup\$ @jesse good catch! \$\endgroup\$ – pomeroy May 26 '17 at 14:38
  • 1
    \$\begingroup\$ In what context will this program be used? Why do you hash the given key and IV, instead of giving the user full control over the input? Also, there are known attacks against MD5. I don't know how that affects your use of it here, but I'd use a more secure algorithm just to be sure. \$\endgroup\$ – Pieter Witvoet May 26 '17 at 18:26
  • \$\begingroup\$ @mat's mug I can't! The app wouldn't let me submit it with that code. \$\endgroup\$ – pomeroy May 26 '17 at 22:50
3
\$\begingroup\$

I am not an expert in cryptography by any stretch of the imagination but I am pretty sure an IV should be randomly generated.

" In cryptography, an initialization vector (IV) or starting variable (SV)[1] is a fixed-size input to a cryptographic primitive that is typically required to be random or pseudorandom. Randomization is crucial for encryption schemes to achieve semantic security [...] " -

https://en.wikipedia.org/wiki/Initialization_vector

\$\endgroup\$
2
\$\begingroup\$

IV it's intended to be quite long and variant, so yep inserting it manually is not a problem but if take an high entropy long string is better.

MD5: it's the weakest hash possible, consider to use SHA-*

\$\endgroup\$

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.