Hot answers tagged

27

Since there are only three valid key sizes for AES, it makes sense to not even let the AES class be instantiated with any uint16 value. I would introduce an enum similar to this: enum class AesKeyLen { Aes128, Aes192, Aes256 }; And then change the uint16 constructor to this: explicit AES (AesKeyLen keyLen); Sure, they could still pass bad ...


23

I'm looking to improve the code to make it not "DIY-crypto-bad", if at all possible I work in security. This is not my area, but I have a non-zero amount of knowledge on implementing secure cryptographic primitives, and from a cursory look I found some issues. For starters: keyOut[i] = AES::S_BOX[keyOut[i]]; and state[i][j] = AES::S_BOX[state[i][j]]; ...


19

Some general tips: The runner should use argparse to parse arguments. It most definitely should not hardcode passwords. (object) is redundant in Python 3 class definitions. I'd recommend running any Python code through Black, flake8 and mypy with a strict configuration like this one: [flake8] doctests = true exclude = .git max-complexity = 5 max-line-...


15

Interface Design: void encrypt(const uint8 input[], const uint8 key[], uint8 output[]) const override; void decrypt(const uint8 input[], const uint8 key[], uint8 output[]) const override; Your encrypt and decrypt is very limiting. This means you need to load the whole of your input into memory before you can start any operations. It would ...


13

I think the header could be trimmed down a lot. The constant tables belong in the implementation file since they are not needed for the definition of the class. Since the AES class does not hold any state (except for that inherited from BlockCipher), I would not declare the private functions in the header, but only keep them in the implementation file in an ...


12

Since I'm more familiar with cryptography than with C#, I'm going to mainly focus on the high-level cryptographic aspects of your code (mostly, key management) rather than on coding style. You should not be using Rfc2898DeriveBytes on every call to Decrypt or Encrypt. The PBKDF2 algorithm used by Rfc2898DeriveBytes is (deliberately) very slow, so you ...


12

You're obtaining more than 20 bytes from PBKDF2-HMAC-SHA-1 and the attacker doesn't need the data from the second block (12 IV bytes), so your code slows down defenders by a factor 2 without affecting attackers. generate random 16 bit salt using RNG 16 bits is very short. You should use 16 bytes or 128 bits. I suspect this is a typo in the comment, since ...


11

You don't show us the BlockCipher base class, but it appears that it imposes a terrible interface on us: void encrypt(const uint8 input[], const uint8 key[], uint8 output[]) const override; void decrypt(const uint8 input[], const uint8 key[], uint8 output[]) const override; I'm assuming that uint8 is a simple typedef of std::uint8_t (though not ...


10

As far as I can tell, this byte[] encryptedData; // ... using (var memoryStream = new MemoryStream()) { using (var cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write)) { using (var binaryWriter = new BinaryWriter(cryptoStream)) { binaryWriter.Write(rij.IV); binaryWriter.Write(...


8

Instead of having private string _sharedSecret; you should consider to use SecureString instead. Each time you need this password you should convert it to a string. Read how-to-convert-securestring-to-system-string and how-to-properly-convert-securestring-to-string Don't create the RijndaelManaged rm; in the constructor. Create it if you need it. Right now ...


8

The code is simple to understand and does what it needs to do. Just some remarks: Whenever you use the ECB cipher mode, you must justify in written form why you use it. This is because this cipher mode can reveal patterns in your plain text. Converting the encrypted bytes to an UTF-8 string doesn't make sense. You should better print them in hex or base64. ...


7

Your code overall is quite easy to read and understand. It's so easy that even I could read it! You know what that means? A+ on readability! But here's a very scary thing: private static byte[] _salt = Encoding.ASCII.GetBytes("SomeConstantSalt"); You shouldn't have the same salt for everything. Salts are meant to be unique per encryption. They don't need ...


7

As far as best practices go, I see a few offhand: static public XXX is idiomatically written as public static XXX. Since all your methods are static, the class should be made static as well to prevent (useless) instances from being created. Are your bit size numbers near the top of the classes intended to be modified? If so, make them private and have ...


6

This is draft 2 of my answer, didn't notice the beginner so I'll try to be gentle: Why do you use a Module instead of a Class? I feel like this would be much more appropriate as an instance class, which would allow you to treat it much more intuitively. On a more important note: this is not encryption. Yes, you are using an AES Encryption library to ...


6

public string AesEncrypt(string clearText) { byte[] clearBytes = Encoding.Unicode.GetBytes(clearText); using (Aes encryptor = Aes.Create()) { Rfc2898DeriveBytes pdb = new Rfc2898DeriveBytes(Passphrase, Salt); encryptor.Key = pdb.GetBytes(32); encryptor.IV = pdb.GetBytes(16); using (MemoryStream ms = new ...


6

You should, in general, avoid using concrete implementation types for cryptographic algorithms in .NET (as of 4.6.1 that should always work). Don't use RijndaelManaged, use Aes. You are both assigning the Key property then reading it to call CreateEncryptor(byte[], byte[]). You only need to set it to call CreateEncryptor(). Pick a paradigm and stick with it....


6

You can simplify it by replacing all those streams with a call to encrypter.TransformFinalBlock. You don't have a MAC, so an attacker who can send a modifier message to the decrypter can probably exploit a padding oracle to decrypt the message Why ask the caller for an IV? You could generate it internally. If you want to keep a deterministic implementation ...


6

Well two things, the Cipher.getInstance is not so good, as you said it's not using an IV; it should be using at least something like "AES/CBC/PKCS5Padding" and probably a longer key, i.e. 256 bits (which needs to be enabled for the JVM because of US export restrictions; OpenJDK will already have that on, otherwise you'll get an exception during setup)Edit: ...


6

try-with-resources FileInputStream fis = null; try { MessageDigest md = MessageDigest.getInstance(HASH_ALG); fis = new FileInputStream(this.filePath); byte[] byteArray = new byte[1024]; int bytesCount = 0; while ((bytesCount = fis.read(byteArray)) >= 0) md.update(...


6

Accessibility With security in mind, I don't think you want this field accessible as public: public const int AES256KeySize = 256; Instead, you may want to change the access modifier to private as I can't see the importance of exposing this int outside of Encryptions class. Like so: private const int AES256KeySize = 256; Keeping the Heap Clean + No ...


6

Ways of improving: Splitting filename Both methods encrypt and decrypt try to split a filename into "root" part and extension. encrypt method makes that even worse and verbose with 2 statements: file_name = "".join(file_.split(".")[:-1]) file_extention = "." + file_.split(".")[-1] Instead, that's easily achievable with os.path.splittext(path) ...


5

It seems you did a good job getting the same functionality using AES. The problem is that Microsoft is not using good practices themselves. They confuse a password with a key, and they do not use a good key derivation function such as PBKDF2 (implemented in .NET by the class RFC2898DeriveBytes). This you should only use if you do not have access to a shared ...


5

Few tiny notes: In the method cryptoExample, the usage of AesCryptoServiceProvider should be wrapped in a using block much like it is in the EncryptStringToBytes_Aes method. Similarly, ICryptoTransform encryptor = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV) in the EncryptStringToBytes_Aes method is an IDisposable resource and should also be wrapped in a ...


5

I have just two minor comments to add to the existing review: csp.KeySize = 256; This is completely unnecessary, because when you set Key it will automatically update KeySize. csp.BlockSize = 128; This is also unnecessary, because AES only supports one block size. However, you could argue that it's future-proofing. I personally ...


5

These are some crypto recommendations which I believe apply to your case (I am not familiar with Python crypto details): The salt should not be hard coded, but should be passed to the code or generated randomly. Reusing the salt makes it possible to generate a rainbow table to recover passwords faster. The salt appears after the third dollar sign in the ...


5

OK, so let's get through this. storing the key in an immutable string is not a good idea, storing it in a byte array and then destroying the key data is a better way of handling the key; calling the method multiple times will reuse the zero IV, which will all but destroy the confidentiality - use a random IV / nonce instead (12 bytes / 96 bits is the right ...


5

I'm not qualified to review the actual crypto, but here are some notes (or nits) on the C++ idioms. class twofish256 final The final qualifier is relevant basically only if you're using classical polymorphism. It tells the compiler that your virtual methods are never going to be overridden, thus allowing extra optimization possibilities. But in your case ...


5

Style private const int _saltSizeBytes = 32; private const int _IVSizeBytes = 16; private const int _PBKDF2Iterations = 10000; Initially, I wanted to comment that consts should be written in all caps, e.g. SALT_SIZE_BYTES. However, this answer disagrees based on Microsoft's StyleCop rules. The recommended naming and capitalization convention is to use ...


5

var AesManaged aesManaged = new AesManaged() You can make you code less verbose by using var instead of explicit type everywhere. Consider this: var aesManaged = new AesManaged() EncryptFile & DecryptFile I find those methods confusing. Their names suggest to encrypt/decrypt files but all they do is to save data as files, they don't read them. ...


Only top voted, non community-wiki answers of a minimum length are eligible