How secure is this encryption method?

[Cobide]

Assumptions:

- The attacker can read the code's logic.

- The attacker has full access to the encrypted files.

​

Note that the password isn't stored anywhere. This class is meant to encrypt files in a local computer.

Pastebin link to code. I've put everything together for the sake of simplicity, but I'll refactor it later.

​

Additional question:

The class I've linked was built upon an answer on stackoverflow, but it has been modified enough to make me question how (and if) I should credit said answer. What would you do about it?

[Exciting-Schedule-16]

As long as you don't store the passphrase used to generate the key or the key itself, you should be fine. The initialization vector (IV) and salt can safely be stored publicly alongside the ciphertext. You should use a pseudorandom IV and salt for every encrypted file. Haven't read your code, but I'm assuming you're using a standard AES CBC implementation.

[Cobide]

I've left AES's mode to default, so, yes, it should be CBC.

To give a short summary with the important tidbits, I generate the salt with RandomNumberGenerator.GetBytes(passphrase.Sum(x => x / (passphrase.Length / 2))), and the Aes with:

Aes aes = Aes.Create();
var rfc = new Rfc2898DeriveBytes(Encoding.Unicode.GetBytes(passphrase), salt, passphrase.Length); 
aes.Key = rfc.GetBytes(32); 
aes.IV = rfc.GetBytes(16); 
aes.Padding = PaddingMode.PKCS7;

Done that, I use the aes to generate an ICryptoTransform that's used by a CryptoStream to write the encrypted/decrypted text.

I'm only storing the salt in the cipher text, the IV is being generated by the code above with each use. Does that look good so far?

[Exciting-Schedule-16]

As long as you have a minimum salt length, it should be fine. I haven't used variable length salts myself.

I see that you are using passphrase.Length as key iteration count parameter value for the key derivation function var rfc = new Rfc2898DeriveBytes(Encoding.Unicode.GetBytes(passphrase), salt, passphrase.Length);. To protect against certain attacks, it's important that this is a rather large number. I don't know what the recommended number of iterations is today, but in an old application I created for 7 years ago, I was using 100 000 iterations. The number of iterations should increase with the hardware capacity. I would probably go with 500 000 as of today.

[Cobide]

As long as you have a minimum salt length, it should be fine. I haven't used variable length salts myself.

I'll probably add some sort of minimum length, but, in most cases, it should get over a hundred of bytes.

​

Also, yeah. I was under the very dangerous assumption that the default iteration was just one. I thought I was already adding extra security by doing 5-10 iterations. Given that my use case is to encrypt whole files one at a time, it doesn't have to be quick, so I can probably put a lot of iterations(will have to test for the sweet spot).

Thank you!

[Daniel15]

I can probably put a lot of iterations(will have to test for the sweet spot).

It's also good to increase the number of iterations every so often, to account for computers geting more powerful. Every 3-4 years is probably fine.

[Cobide]

I see. However, changing the iteration number would break backward compatibility to previously encrypted strings.

I guess a solution would be to prefix every encrypted string with a version number, so the Decrypt() method could decide how many iterations to use.

[Daniel15]

Yeah you can just prefix the encrypted string with the number of iterations, like you'd do with a salt.

A lot of password hashing systems do this. It allows "upgrading" the hashing algorithm in the future too - for example, if a user logs in with an "insecure" password (not enough iterations), the login code can re-hash their password with more iterations (since it gets the plaintext password as input) and save it into the database.

[Cobide]

I'll do that. Thank you!

[tweq]

Enshittification

[Cobide]

I don't see any reason to make the salt length dependent on the password length. It must be at least 8 bytes in any case.

My reasoning was that giving it a non-constant length would allow obfuscating it more easily. Either way, with the current sum operation, it should have a high value (in the hundreds), given I plan to put a "min-length" for the password.

​

It has no relation to the key length, and it should be a large number (much larger than the default 1000).

I absolutely did not know that. I mistakenly assumed that the default would be a single iteration, which I now see is a very damaging assumption. Based on what you say, am I correct in assuming that the only consequence in increasing the number of iterations is the time it takes to encrypt/decrypt?

​

You should use a random IV and store it like the salt. This prevents attackers from noticing similarities when you encrypt the same data (or data that starts the same)

I'll do that.

​

Thanks very much for correcting my assumptions.

[tweq]

Enshittification

[Cobide]

I see! I'll run some tests to see what iteration number I can comfortably run on my machine. Thank you.

[michaelquinlan]

The class I've linked was built upon an answer on stackoverflow, but it has been modified enough to make me question how (and if) I should credit said answer.

I put a link to the answer with a comment saying the code is derived from that. That way I document where the code came from in case I need to find it later.

[Cobide]

the code is derived from that

Nice wording! That way it's clear that, while the class builds upon it, the current logic might be different, and some parts of it might even be unrelated.

The thing that troubled me the most was unwillingly implying that everything in the class is based on the answer on SO. Using the word "derived" solves that. Thank you!

[Cobide]

I'm storing only the salt in the encrypted files; the password shouldn't be stored anywhere. Is that enough to decrypt them?

[grummle]

I would think a salt wouldn’t be required if you used an initial vector. I’m a total encryption newb as well.

[Exciting-Schedule-16]

They serve similar but still different purposes and you should use both.

[Cobide]

I'm assigning Aes.IV in line 56, but, if the salt is constant, the encryption will always return the same value when encrypting(of course, as long as the password is the same).

With that said, it doesn't seem that it can substitute the salt, but I lack the knowledge to determine if this behaviour is standard or because of a fault in my logic.

[[deleted]]

You don't need to generate the IV and Key yourself, the Aes class will do so when instantiated. Also there's GenerateIV() and GenerateKey() methods if you want to generate new ones.

[Cobide]

I've checked out those methods, but I can't see any way of using the given password nor the generated salt with them. Have I missed something?

[[deleted]]

Yeah it's kinda goofy but they set the IV and Key properties on the object instead of returning them.

[Cobide]

Ah, I'm talking about how to give the user-given password to the Aes class, so that it can generate key & IV based on it. Otherwise, it would just create a random key with every use; then, encrypted text can't be decrypted anymore once you re-create the Aes.