Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

There are three magic numbers in here:

AES.KeySize = 256;
//AES.BlockSize = 128;
var key = new Rfc2898DeriveBytes(passwordBytes, saltBytes, 100);
AES.Key = key.GetBytes(AES.KeySize / 8);

Also, that comment should go. Dead code is dead.


As stated in the comments:

byte[] saltBytes = SHA512.Create().ComputeHash(passwordBytes);

Generating the saltBytes based on the password is a bad idea. The salt should be random and if two users with the exact same password have the same salt, well now either of them can attack the other.

The salt does not need to be a secret. You can keep it in plain-text. Many time, in the case of a database, it will have two (or three) columns: EncryptedPassword and Salt (maybe IV if it's not based on the Salt). (Or in the case of ASP.NET Entity Framework / Simple Membership this is all stored in one column with separators.)

The attacker can know the Salt and IV, you can even use the Salt to generate the IV (or initialize the RNG for it), but you cannot use the password to generate either. That defeats the purpose of the password. If I know Salt is generated from the password, then I don't need the password at all, I can just brute force everything to find that Salt, or I can use a rainbow table lookup, et. al.

There are three magic numbers in here:

AES.KeySize = 256;
//AES.BlockSize = 128;
var key = new Rfc2898DeriveBytes(passwordBytes, saltBytes, 100);
AES.Key = key.GetBytes(AES.KeySize / 8);

Also, that comment should go. Dead code is dead.


As stated in the comments:

byte[] saltBytes = SHA512.Create().ComputeHash(passwordBytes);

Generating the saltBytes based on the password is a bad idea. The salt should be random and if two users with the exact same password have the same salt, well now either of them can attack the other.

The salt does not need to be a secret. You can keep it in plain-text. Many time, in the case of a database, it will have two (or three) columns: EncryptedPassword and Salt (maybe IV if it's not based on the Salt). (Or in the case of ASP.NET Entity Framework / Simple Membership this is all stored in one column with separators.)

The attacker can know the Salt and IV, you can even use the Salt to generate the IV (or initialize the RNG for it), but you cannot use the password to generate either. That defeats the purpose of the password. If I know Salt is generated from the password, then I don't need the password at all, I can just brute force everything to find that Salt, or I can use a rainbow table lookup, et. al.

There are three magic numbers in here:

AES.KeySize = 256;
//AES.BlockSize = 128;
var key = new Rfc2898DeriveBytes(passwordBytes, saltBytes, 100);
AES.Key = key.GetBytes(AES.KeySize / 8);

Also, that comment should go. Dead code is dead.


As stated in the comments:

byte[] saltBytes = SHA512.Create().ComputeHash(passwordBytes);

Generating the saltBytes based on the password is a bad idea. The salt should be random and if two users with the exact same password have the same salt, well now either of them can attack the other.

The salt does not need to be a secret. You can keep it in plain-text. Many time, in the case of a database, it will have two (or three) columns: EncryptedPassword and Salt (maybe IV if it's not based on the Salt). (Or in the case of ASP.NET Entity Framework / Simple Membership this is all stored in one column with separators.)

The attacker can know the Salt and IV, you can even use the Salt to generate the IV (or initialize the RNG for it), but you cannot use the password to generate either. That defeats the purpose of the password. If I know Salt is generated from the password, then I don't need the password at all, I can just brute force everything to find that Salt, or I can use a rainbow table lookup, et. al.

Source Link
Der Kommissar
  • 20.2k
  • 4
  • 70
  • 158

There are three magic numbers in here:

AES.KeySize = 256;
//AES.BlockSize = 128;
var key = new Rfc2898DeriveBytes(passwordBytes, saltBytes, 100);
AES.Key = key.GetBytes(AES.KeySize / 8);

Also, that comment should go. Dead code is dead.


As stated in the comments:

byte[] saltBytes = SHA512.Create().ComputeHash(passwordBytes);

Generating the saltBytes based on the password is a bad idea. The salt should be random and if two users with the exact same password have the same salt, well now either of them can attack the other.

The salt does not need to be a secret. You can keep it in plain-text. Many time, in the case of a database, it will have two (or three) columns: EncryptedPassword and Salt (maybe IV if it's not based on the Salt). (Or in the case of ASP.NET Entity Framework / Simple Membership this is all stored in one column with separators.)

The attacker can know the Salt and IV, you can even use the Salt to generate the IV (or initialize the RNG for it), but you cannot use the password to generate either. That defeats the purpose of the password. If I know Salt is generated from the password, then I don't need the password at all, I can just brute force everything to find that Salt, or I can use a rainbow table lookup, et. al.

lang-cs

AltStyle によって変換されたページ (->オリジナル) /