-
Notifications
You must be signed in to change notification settings - Fork 12.6k
feat: e2ee security hardening #36942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/meteor/client/lib/e2ee/rocketchat.e2e.ts (2)
808-809: Fix key distribution predicate: usesomeinstead ofevery.The predicate at line 809 uses
every((user) => user.userId !== this.getUserId()), which excludes rooms where the current user is among the waiters. This prevents key distribution to rooms where both the current user and other users are waiting for keys.The intent is to find rooms with at least one waiter who is NOT the current user (so we can distribute keys to them). Use
someto check for the presence of such users.const predicate = (record: IRoom) => - Boolean('usersWaitingForE2EKeys' in record && record.usersWaitingForE2EKeys?.every((user) => user.userId !== this.getUserId())); + Boolean('usersWaitingForE2EKeys' in record && record.usersWaitingForE2EKeys?.some((user) => user.userId !== this.getUserId()));
657-660: Add safe fallback for decrypted pinned message content.Line 660 directly assigns
data.msgto the attachment text without verifying the property exists. The decrypted data structure might havetextinstead ofmsgdepending on the encryption version or message type, which would cause the assignment to fail silently or setundefined.Apply a safe fallback that checks both possible properties:
const data = await e2eRoom.decrypt(pinnedMessage.content); // TODO(@cardoso): review backward compatibility - message.attachments[0].text = data.msg; + const text = data.msg ?? data.text; + if (text) { + message.attachments[0].text = text; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/client/lib/e2ee/rocketchat.e2e.ts(22 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#37138
File: apps/meteor/client/lib/chats/data.ts:181-194
Timestamp: 2025-10-10T12:34:36.867Z
Learning: Currently, only v1 encrypted message format ('rc.v1.aes-sha2') is supported in the codebase. v2 format ('rc.v2.aes-sha2') mentioned in some documentation is not yet implemented.
🧬 Code graph analysis (1)
apps/meteor/client/lib/e2ee/rocketchat.e2e.ts (8)
apps/meteor/client/lib/e2ee/logger.ts (5)
log(70-75)createLogger(98-100)span(35-37)error(23-25)error(90-95)apps/meteor/client/lib/e2ee/keychain.ts (1)
Keychain(129-163)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts (1)
E2ERoom(68-766)apps/meteor/client/lib/e2ee/crypto/rsa.ts (1)
PrivateKey(18-18)apps/meteor/client/stores/Subscriptions.ts (1)
Subscriptions(7-7)apps/meteor/client/lib/e2ee/helper.ts (1)
generatePassphrase(17-41)packages/core-typings/src/IMessage/IMessage.ts (1)
IE2EEPinnedMessage(438-441)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/client/lib/e2ee/crypto/pbkdf2.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-07T15:08:37.419Z
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#36942
File: apps/meteor/client/lib/e2ee/pbkdf2.ts:13-45
Timestamp: 2025-10-07T15:08:37.419Z
Learning: In apps/meteor/client/lib/e2ee/pbkdf2.ts, the team has decided to use Latin-1 encoding (via Binary.toArrayBuffer and Binary.toString) for password encoding and decrypt output instead of UTF-8 encoding. This is a deliberate choice for E2EE password/key material handling.
Applied to files:
apps/meteor/client/lib/e2ee/crypto/pbkdf2.ts
🧬 Code graph analysis (1)
apps/meteor/client/lib/e2ee/crypto/pbkdf2.ts (2)
apps/meteor/client/lib/e2ee/crypto/shared.ts (6)
IKey(67-77)importRaw(195-207)deriveBits(52-52)decryptBuffer(47-51)getRandomValues(12-12)encryptBuffer(42-46)apps/meteor/client/lib/e2ee/crypto/aes.ts (3)
importKey(36-38)decrypt(48-55)encrypt(57-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/client/lib/e2ee/crypto/pbkdf2.ts (4)
11-16: Excellent type-safe usage restriction.The conditional usage types correctly enforce that CBC-derived keys are decrypt-only while GCM-derived keys support both operations. This aligns perfectly with the PR objective of maintaining v1 backward compatibility (CBC for decryption) while enabling v2 writes (GCM for encryption/decryption).
61-74: LGTM: Solid key derivation and import.The PBKDF2 derivation with SHA-256 and the key import logic are correctly implemented:
- 256-bit derivation matches AES-256 requirements
- Non-extractable keys enhance security
- Usage restrictions properly enforce CBC decrypt-only vs. GCM encrypt/decrypt
76-83: LGTM: Correct dual-mode decryption.The decrypt function properly supports both AES-CBC and AES-GCM by dynamically constructing the algorithm parameters from the key's algorithm. The
satisfiestype annotation ensures type safety across both modes.
85-93: LGTM: Correct GCM-only encryption.The encrypt function correctly:
- Restricts to AES-GCM keys only via type parameter (line 86)
- Uses the correct 12-byte IV for GCM (line 90)
- Implements the v2 requirement that all new encryption uses AES-GCM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/client/lib/e2ee/crypto/shared.ts (2)
29-31: Consider adding a comment explaining the HasUsage error pattern.The type-level error message using
TKey & stringis clever but may produce confusing TypeScript errors. A brief comment explaining this pattern would help maintainers understand the intent.
113-147: Consider adding type-level tests for KeyToJwk.The
KeyToJwkconditional type is sophisticated. While the logic appears sound, type-level tests (using TypeScript's type assertions or a library liketsd) would help ensure the type mappings remain correct as the code evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/client/lib/e2ee/crypto/shared.ts(1 hunks)packages/jest-presets/src/client/jest-setup.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
packages/jest-presets/src/client/jest-setup.ts (1)
1-1: LGTM! Essential setup for E2EE cryptographic testing.The import and global assignment of
webcryptocorrectly provide the Web Crypto API in the Jest/Node environment, which is necessary for testing the new E2EE v2 cryptographic primitives (AES-256-GCM, PBKDF2, etc.) introduced in this PR.Also applies to: 14-16
apps/meteor/client/lib/e2ee/crypto/shared.ts (6)
5-19: LGTM!The algorithm-specific parameter interfaces provide clean type narrowing with explicit name literals, enabling precise type discrimination in conditional types.
33-43: LGTM!The usage-gated encrypt/decrypt wrappers provide excellent compile-time safety, ensuring keys can only be used for their designated operations.
70-77: LGTM!The key generation functions use const type parameters effectively to preserve precise algorithm and usage information through the type system.
Also applies to: 104-111
186-198: LGTM!The
importRawfunction correctly handles raw key import for symmetric keys with appropriate type constraints.
1-3: No changes required—jest environment is already properly configured for crypto support.The concern raised about jsdom/Node.js compatibility is already addressed by the existing jest setup in
packages/jest-presets/src/client/jest-setup.ts, which explicitly assigns Node.js'swebcrypto(which includescrypto.subtle) toglobalThis.crypto. The project uses Node.js 22.16.0, which has full Web Crypto API support includingSubtleCrypto. The code will work correctly in test environments without modification.
52-67: Permutations type limitation is real but not currently impactful.The
Permutationstype correctly resolves toneverfor arrays with 3+ elements, and Web Crypto API does support keys with more than 2 usages. However, all current E2EE use cases in the codebase employ only 1–2 usages per key (e.g.,['encrypt', 'decrypt']for AES,'deriveBits'for PBKDF2). The code compiles and tests pass without issue.This is a forward-looking type system constraint worth considering if E2EE implementations expand to use 3+ usages on a single key in the future. For now, verify whether this limitation aligns with your intended key design or if you plan to support keys with more than 2 usages.
Proposed changes (including videos or screenshots)
This PR makes several improvements to the end-to-end encryption flow using stronger parameters and algorithms while maintaining backward compatibility and introducing facilities for easier upgrades in the future.
1. Key Recovery
A mnemonic is randomly generated and passed through PBKDF2 (with a salt and high iteration count) to create a strong, memorable master key.
Current (v1)
Proposed (v2)
Backwards compatibility
If the stored key structure resembles v1, we are able to decrypt it by defaulting to the old parameters when deriving the key. This derived key is only used for decryption. When the passphrase is changed, the backup will automatically be upgraded to the v2 structure.
Although the same AES key of a given size could be used in both CBC and GCM mode, the WebCrypto API enforces the distinction when calling
deriveKeyorimportKey. It also enforces usage, so we take advantage of that to ensure CBC is only used for decryption while in GCM mode we can encrypt and decrypt.2. Long-Term Identity Keys
A long-term RSA key pair is generated randomly. The private key is then encrypted with the master key for secure server backup and recovery. The public key is shared with others.
Current
Already strong enough. No changes needed.
3. Message Encryption
For any given chat, a symmetric key is generated. This key is used to encrypt the actual messages because it's extremely fast and efficient.
Current (v1)
Proposed (v2)
Backwards compatibility
Fairly straight-forward, we branch on the
algorithmfield.4. Key Distribution
When a member needs the AES key (either to start a chat or join a group), an existing member encrypts it using the recipient's Public RSA key.
Current (v1)
The key id is 12 characters.
Sender:
Recipient:
Proposed (v2)
The key id is no longer assumed to be 12 characters.
Sender:
Recipient:
Backwards compatibility / explanation
When encrypted with RSA-OAEP 2048 bits, both v1 and v2 JWKs become a 256 byte blob, which encoded in Base64 always results in 344 characters. We now take the last 344 characters and treat the previous characters as a the key id. Old keys are still imported and can transparently decrypt content based on its iv length (16 for AES-128-CBC and 12 for AES-256-GCM).
Issue(s)
ESH-1
ESH-2
ESH-3
ESH-4
ESH-5
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Improvements