Possible vulnerabilities in key generation, IV and MITM

[AlienDwarf]

After using the plugins for some tests I have noticed a few things that are not standard practice and are possible security vulnerabilities.

Missing key exchange

The plugin lacks an key exchange algorithm like DH.

Key generation

export const encrypt = (msg: string, channelData: UserData["global"]) => {
  const key = crypto.SHA512(channelData.password);
// ...

Why do you use 512 as key and not 256 (since we use AES-256)? You probably think more bits = more security but it's not working like that. Indeed OpenPGP RFC 4880 section 3.7.1.1 states:

If the hash size is greater than the session key size, the high-order (leftmost) octets of the hash are used as the key.

So if crypto-js implements this correctly you basically use only half of your hash. Solution: const key = crypto.SHA256(channelData.password);

However this approach has the following problems:

1. Using the same key for different user will produce the same hash
2. SHA256 is extremely fast (SHA512 is even faster), there is no additional brute-force protection
3. No protection against ASIC

Better solution: Use a key derivation function (pbkdf2, argon2, scrypt). This algorithms are hardened against attacks and the same input wont generate the same key.

Static IV

Using a static IV is not recommended. The code is probably vulnerable to Padding-Oracle attacks (assuming crypto-js uses AES in CBC mode since you use an IV but the code uses no auth tag).

• Using a static IV will generate the same ciphertext which gives an attacker information

Solution: Use random bytes as IV. The other side must know the IV so you have to tell them. AES follows Kerckhoffs's principle

The principle holds that a cryptosystem should be secure, even if everything about the system, except the key, is public knowledge

This means it's completely fine to send the IV in plaintext. It is not possible to break AES for an attacker knowing the IV. Example message: IV/ciphertext or IV%ciphertext or IV@ciphertext

MITM

The code does not verify the authenticity and integrity of the ciphertexts. This results in treating every received message as if it definitely came from the sender. However, an attacker could intercept and manipulate the message on it's way to the receiver. Solution: Use AES in GCM or CCM mode. This will produce an auth tag which behaves like a signature. The receiver can check that the message has not been manipulated.

Right know in it's current state I won't recommend using this plugin for encryption since it violates standard principles.

[hmerritt]

I appreciate the commitment to security in pointing all these issues out, however, this plugin is experimental, if you are looking for a genuinely secure messaging I would recommend Signal.

In addition to being experimental, this plugin is also open-source, if you are eager to see those features, you are more than welcome to submit a PR 🏋️‍♀️.

[AlienDwarf]

Thanks I will fork. What license does this project run under? I have not found a license

[hmerritt]

I have just added the Apache 2.0 License

[AlienDwarf]

Ah btw if you try to install and start the plugin it isn't working because it expects some JSON which hasn't been created yet.

[encryptionPlugin] Error parsing local storage SyntaxError: "undefined" is not valid JSON
    at JSON.parse (<anonymous>)
    at getUserData (encryption.plugin.js:17976:25)
    at new Encryption (encryption.plugin.js:18411:37)
    at M.initializeAddon (renderer.js:4:34235)
    at M.loadAddon (renderer.js:4:8506)
    at M.loadAddon (renderer.js:4:33490)
    at M.reloadAddon (renderer.js:4:9211)
    at _t.reload (renderer.js:4:360780)
    at <anonymous>:1:22

I had to create dummy data manually and then set a real password to make this work :D

It's this line in storage.ts

const getUserData = window?.BdApi?.getData(config.name, config.name);

Just add a sanity check if !getUserData then leave the trycatch block and return the default data