Security Considerations and Potential Improvements for the Cipher Class

[us254]

1. Key size:

   static async generateKeyPair() {
   const keyPair = await forge.pki.rsa.generateKeyPair({ bits: 512, workers: 2 });
   const publicKey = forge.pki.publicKeyToPem(keyPair.publicKey);
   const privateKey = forge.pki.privateKeyToPem(keyPair.privateKey);
   
   return { privateKey, publicKey };
   }

   The generateKeyPair method generates RSA keys with a key size of 512 bits, which is considered relatively weak. Increasing the bits parameter to at least 2048 would provide better security.

2. Lack of proper error handling:

   public async resolveDRSAP(packet: string) {
   // ...
   let key;
   try {
   key = ownPrivateKey.decrypt(forge.util.hexToBytes(r1));
   } catch (error) {}
   if (!key) {
   try {
     key = ownPrivateKey.decrypt(forge.util.hexToBytes(r2));
   } catch (error) {}
   }
   if (!key) return null;
   // ...
   }

   In the resolveDRSAP method, if the decryption of the secret key fails, the code silently continues without returning an error or logging the failure. Adding proper error handling and logging would help detect and respond to failures.

3. Timestamp verification:

   public async resolveDRSAPHandshake(packet: string, forId: string) {
   // ...
   if (+timestamp < +(oldContact.timestamp || 0)) return logger.debug(`Handshake ${toId} is old`);
   // ...
   }

   The code checks the timestamp of the handshake to determine if it's old, but it doesn't verify the authenticity or integrity of the timestamp. Implementing a more robust timestamp verification mechanism, such as using digital signatures, would enhance security.

4. Lack of perfect forward secrecy: The code uses a static key pair for each user, which is generated and stored in the browser storage. If a private key is compromised, all previous messages encrypted with the corresponding public key can be decrypted. Implementing perfect forward secrecy would involve generating ephemeral key pairs for each session.

5. Potential side-channel attacks: The code uses the forge library for cryptographic operations, but it's not clear if the library is resistant to side-channel attacks. This would require further analysis and testing of the library's implementation.

6. Lack of authentication: The code focuses on encryption and decryption but doesn't include explicit authentication mechanisms to verify the identity of the communicating parties. Adding authentication, such as digital signatures or authenticated key exchange protocols, would help prevent impersonation attacks.

[mostafa-kheibary]

Thank you for your feedback. I've made efforts to enhance the security of the cipher class to meet the standard requirements. I've also taken your recommendations into account for implementation.

[H-MAli]

First off, great work!

I advise against using AES-CBC. Use AES-GCM or something secure against padding oracle attacks. Read more here.

[mostafa-kheibary]

Currently, I'm working on implementing a new algorithm that uses Elliptic Curve 25519. I have switched the encryption library from Node Forge to PGP.js for better community support and a more standard implementation.

You can see the new implementation in the "develop" branch. It will be merged soon for the v1 release.