Question: Security

[datenwort]

Hi,

I take a look into the encryptionUtils file and find the usage of PBKDF2WithHmacSHA1. Did not found many information specific on the differences of PBKDF2WithHmacSHA1 and PBKDF2WithHmacSHA256 but as far as I know SHA1 is unsecure. I also read more often that CBC algorithms should be used.

SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
KeySpec spec = new PBEKeySpec(password, salt, 65536, 256);
SecretKey tmp = factory.generateSecret(spec);
SecretKey secret = new SecretKeySpec(tmp.getEncoded(), "AES");

Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
cipher.init(Cipher.ENCRYPT_MODE, secret);
AlgorithmParameters params = cipher.getParameters();
byte[] iv = params.getParameterSpec(IvParameterSpec.class).getIV();
byte[] ciphertext = cipher.doFinal("Hello, World!".getBytes("UTF-8"));

Has communote weaknesses in security?

[rwi]

@datenwort sorry for the delayed response, but definitely thanks for filing this issue. One of the benefits we expected from going open source was that other devs could spot problems and weaknesses in our code, especially w.r.t. security related things. So let's have a look at the EncryptionUtils.

When assessing the security of components at least the following 3 things need to be taken into account

1. what do you want to protect
2. how do you use the component
3. what are the attackers you want to protect the data from

For the 1st aspect
In Communote we use the EncryptionUtils to encrypt passwords of configurations like the passwords of users which interact with the SMTP or IMAP mail-servers. So the goal is to prevent that clear text passwords are stored in the database, for example for a scenario where Communote administrator and database admin are different people. Note: the passwords of users are of course not encrypted with the EncryptionUtils. Here we use cryptographic hash functions.

For the 2nd aspect
As you have posted in your example, a password-based-key-derivation-function (PBKDF) like the PBKDF2WithHmacSHA1 is used to generate a key from a password (and some salt) which can later be used as the secret key of the encryption algorithm. The main idea of such a PBKDF is to add some computational cost to a brute force attack, especially for weak passwords. The hash algorithm of the PBKDF hasn't a such a big influence on the cost, in particular the generation of a SHA256 hash code doesn't take much longer than a SHA1. More important are the number of iterations of the PBKDF. Admittedly, this isn't high in the EncryptionUtils, but this needs to be seen in the context of the usage because the password we feed into the PBKDF is a type 4 UUID (pseudo randomly generated 128bit value). So a brute force attack would have to break this.

Regarding the cypher mode you are right that for instance CBC should be used. When requesting a cipher instance without providing the full transformation as we do in EncryptionUtils (this is discouraged and something we've missed) a default of the security provider kicks in. In the case of Sun's provider this is ECB. Using ECB is not recommended since it has a problem with semantic security. So for example if 2 clear texts with the same prefix are encrypted with the same key, the encrypted messages would also have the same prefix. In an encrypted message you would also see if the clear text has common substrings of block size which are aligned at block boundaries. In the context of our usage the first one is not a problem because every clear text is encrypted with another key which is derived from the PBKDF. The second one shouldn't be a problem either because our clear texts (some passwords) are usually smaller than AES block size (128bit).

For the last aspect
As said we wanted to prevent that clear text passwords are stored in the database. Therefore, the attacker who needs to be considered is someone that has or can get selective or full access to the database. Well, here we have a point for improvement because the UUID used as input for the PBKDF is also stored in the db and can be easily found since the source code is now open source. This is something we didn't consider. It would be better to store it on the filesystem (e.g. Communote config directory) to add a bit more security. If then someone only has access to the database only the stored notes would be compromised and not the passwords of the connected 3rd party systems like the SMTP server. However, if the attacker has enough computational power or time then these passwords also have to be considered compromised. And of course if the attacker also has access to the filesystem this modification wouldn't help.

To sum it up

• The EncryptionUtils shouldn't be used for anything else than what we are using them for. If you need encryption for your Communote plugins you should probably implement your own appropriate solution and have the above aspects in mind.
• You will have to decide how likely the described attackers are in your environment and how critical a successful attack is.
• There are things that can be improved
  • add some documentation to the EncryptionUtils about intended use and maybe limitations
  • change the way the UUID is stored. While working on this
    • cipher mode can be changed to CBC or something like this
    • costs of PBKDF can be increased
    • a migration of the currently encrypted data should be supported
    • a way for easily increasing the costs or changing algorithms in a later release should be taken into account
  • pull requests are welcome 😃

Does this help?