feat: Encryption security hardening #6668
Conversation
…note for future testing
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughImplements server-versioned E2EE (v1/v2) across JS/TS and native layers, adds prefixed‑base64 payloads, BIP‑39 passphrase generation, per‑room refactor and async push decryption, updates message/content models and chat.update to accept Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as RN UI
participant Room as EncryptionRoom (TS)
participant Native as Native Crypto
participant Server as Server
UI->>Room: encryptText(plaintext)
Room->>Room: determine algorithm (server/version)
alt v2 (rc.v2.aes-sha2)
Room->>Native: aesGcmEncrypt(key, iv, plaintext)
Native-->>Room: {ciphertext, iv}
Room-->>UI: content {algorithm, ciphertext, kid, iv}
else v1 (rc.v1.aes-sha2)
Room->>Native: legacy AES‑CBC-like encrypt
Native-->>Room: ciphertext
Room-->>UI: content {algorithm, ciphertext}
end
UI->>Server: sendMessage({ content, msg? })
sequenceDiagram
autonumber
participant Push as Push Receiver
participant Proc as E2ENotificationProcessor (Android)
participant Ctx as React Context Provider
participant Enc as Native Encryption
participant Sys as System Notification
Push->>Proc: processAsync(bundle, ejson)
loop poll React context until available or timeout
Proc->>Ctx: getReactContext()
end
Proc->>Enc: decryptRoomKey(e2eKey)
Enc-->>Proc: RoomKeyResult { decryptedKey, algorithm }
Proc->>Enc: decryptContent(content/msg, decryptedKey, algorithm)
Enc-->>Proc: plaintext
Proc->>Sys: showNotification(plaintext)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas to focus review on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…note for future testing
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/lib/encryption/room.ts (2)
477-498: Encryption failure silently returns unencrypted message.If
encryptTextthrows (line 483), the catch block logs the error but returns the original unencrypted message (line 497). This fail-open behavior could allow sensitive content to be sent unencrypted. Consider either throwing the error to prevent sending or marking the message with an error state.Apply this diff to prevent unencrypted sends:
} catch (e) { - // Do nothing console.error(e); + throw new Error('Failed to encrypt message: ' + (e instanceof Error ? e.message : String(e))); }
501-528: Same fail-open behavior in upload encryption.Like the
encryptmethod,encryptUploadreturns the original unencrypted upload if encryption fails (line 527). This creates the same security risk of unencrypted content being sent.Consider applying the same fix as suggested for the
encryptmethod.
♻️ Duplicate comments (2)
app/lib/encryption/room.ts (2)
196-222: Back-compat risk: decodePrefixedBase64 assumes fixed RSA ciphertext length.
decodePrefixedBase64at line 203 enforces a fixed 256-byte Base64 length (2048-bit RSA). Older v1 keys using 12-char prefix + variable-length base64, or users with 4096-bit RSA keys, will throwRangeErrorand break key imports.Wrap the call in a try-catch with fallback to legacy parsing:
): Promise<{ sessionKeyExportedString: string; roomKey: ArrayBuffer; keyID: string; algorithm: TAlgorithm }> => { try { - // Parse the encrypted key using prefixed base64 - const [kid, encryptedData] = decodePrefixedBase64(E2EKey); + let kid: string; + let encryptedData: ArrayBuffer; + try { + [kid, encryptedData] = decodePrefixedBase64(E2EKey); + } catch (err) { + // Fallback: legacy v1 uses 12-char keyID prefix + base64 ciphertext of variable length + if (err instanceof RangeError) { + kid = E2EKey.slice(0, 12); + encryptedData = b64ToBuffer(E2EKey.slice(12)); + } else { + throw err; + } + } // Decrypt the session key const decryptedKey = await rsaDecrypt(bufferToB64(encryptedData), privateKey);
352-361: Handle >2048-bit RSA ciphertext when exporting room keys.
encodePrefixedBase64at line 357 requires exactly 256 bytes. If a recipient has a 4096-bit RSA key (or any non-2048-bit key),rsaEncryptproduces a different-sized ciphertext,encodePrefixedBase64throws, and you cannot share the room key with that user.Add fallback to legacy format on size mismatch:
const encryptedUserKey = await rsaEncrypt(this.sessionKeyExportedString as string, userKey); const encryptedBuffer = b64ToBuffer(encryptedUserKey as string); - return encodePrefixedBase64(this.keyID, encryptedBuffer); + try { + return encodePrefixedBase64(this.keyID, encryptedBuffer); + } catch (error) { + if (error instanceof RangeError) { + // Fallback to legacy format for non-2048-bit keys + return `${this.keyID}${encryptedUserKey}`; + } + throw error; + }
🧹 Nitpick comments (2)
app/lib/encryption/room.ts (2)
447-474: Consider explicit algorithm validation to prevent silent v1 fallback.When
this.algorithm === ''(uninitialized) or has an unexpected value, execution silently falls through to the v1 encryption path at line 467. Whilehandshake()should setthis.algorithmviacreateNewRoomKeyorimportRoomKey, explicit validation would catch edge cases where the room is not properly initialized.Add validation before branching:
): Promise< | { algorithm: 'rc.v2.aes-sha2'; kid: string; iv: string; ciphertext: string } | { algorithm: 'rc.v1.aes-sha2'; ciphertext: string } > => { const textBuffer = utf8ToBuffer(text); + if (!this.algorithm) { + throw new Error('Encryption algorithm not initialized. Room handshake may have failed.'); + } if (this.algorithm === 'A256GCM') {
776-821: Well-structured subscription decryption with smart caching.The method includes appropriate guard clauses for ready state, message type, and encryption status. The optimization at lines 803-814 avoids re-decrypting identical messages by comparing
_updatedAttimestamps, which is a good performance enhancement for frequently accessed subscriptions.Consider adding a type guard for lastMessage to improve type safety:
decryptSubscription = async (subscription: Partial<ISubscription>) => { if (!this.ready) { return subscription; } // If the subscription doesn't have a lastMessage just return const { rid, lastMessage } = subscription; - if (!lastMessage) { + if (!lastMessage || typeof lastMessage !== 'object') { return subscription; }
📜 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)
app/lib/encryption/room.ts(15 hunks)app/lib/methods/subscriptions/rooms.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/methods/subscriptions/rooms.ts
🧰 Additional context used
🧬 Code graph analysis (1)
app/lib/encryption/room.ts (4)
app/lib/encryption/helpers/deferred.ts (1)
Deferred(1-39)android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
Encryption(108-454)app/lib/services/restApi.ts (2)
e2eAcceptSuggestedGroupKey(84-86)e2eRejectSuggestedGroupKey(88-90)app/lib/encryption/utils.ts (9)
decodePrefixedBase64(200-219)bufferToB64(17-17)b64ToBuffer(15-15)bufferToB64URI(21-40)encodePrefixedBase64(221-237)utf8ToBuffer(42-59)joinVectorData(73-78)splitVectorData(67-71)bufferToUtf8(61-65)
🔇 Additional comments (7)
app/lib/encryption/room.ts (7)
226-272: Version-aware key creation implemented correctly.The method properly branches by server version (>= 7.13.0 for v2) and sets
this.algorithmto'A256GCM'(line 245) for v2 or'A128CBC'(line 265) for v1. Key sizes, key ID generation, and session key formats are appropriate for each version.
646-663: Parse method correctly handles v1/v2 payload formats.The method properly distinguishes v2 structured payloads (with
algorithm: 'rc.v2.aes-sha2') from v1 string-based payloads. The v1 path correctly extracts the 12-character key ID and splits the vector from ciphertext. Defensive handling withpayload?.ciphertext || ''prevents null reference errors.
665-678: Clean algorithm-aware decryption dispatcher.The method correctly routes decryption to
aesGcmDecryptfor v2 (A256GCM) oraesDecryptfor v1 (A128CBC), gracefully handles null decryption results, and parses the final EJSON payload. Acceptingalgorithmas a parameter rather than usingthis.algorithmprovides flexibility for decrypting old keys.
680-702: Robust v1/v2 decryption with key rotation support.The method correctly handles both encryption versions via the
parsehelper, supports key rotation by searchingoldRoomKeyswhen the key ID doesn't match, and uses the appropriate algorithm for each key. Error handling returns null rather than throwing, which aligns with the graceful degradation pattern used elsewhere.
705-741: Message decryption properly handles v1/v2 formats and nested content.The flow correctly decrypts message content (with v1 fallback to
msgfield), spreads the decrypted structured data (attachments, files, text) into the message, and processes quote attachments. Marking attachments ase2e: 'pending'appropriately signals that file content requires separate decryption.
530-636: File encryption consistently uses structured encryption results.The
encryptFilemethod properly returns structured encryption results fromencryptTextfor both thegetContentcallback (line 607-608) andfileContent(line 624). This maintains consistency with the versioned encryption approach across message and file flows.
638-644: LGTM!File content decryption correctly checks for v1/v2 algorithm markers and delegates to the versioned
decryptContentmethod.
| if (E2ESuggestedKey) { | ||
| try { | ||
| this.establishing = true; | ||
| const { keyID, roomKey, sessionKeyExportedString, algorithm } = await this.importRoomKey( | ||
| E2ESuggestedKey, | ||
| Encryption.privateKey | ||
| ); | ||
| this.keyID = keyID; | ||
| this.roomKey = roomKey; | ||
| this.sessionKeyExportedString = sessionKeyExportedString; | ||
| this.algorithm = algorithm; | ||
| try { | ||
| this.establishing = true; | ||
| const { keyID, roomKey, sessionKeyExportedString } = await this.importRoomKey(E2ESuggestedKey, Encryption.privateKey); | ||
| this.keyID = keyID; | ||
| this.roomKey = roomKey; | ||
| this.sessionKeyExportedString = sessionKeyExportedString; | ||
| await e2eAcceptSuggestedGroupKey(this.roomId); | ||
| Encryption.deleteRoomInstance(this.roomId); | ||
| return; | ||
| } catch (error) { | ||
| await e2eRejectSuggestedGroupKey(this.roomId); | ||
| } | ||
| await e2eAcceptSuggestedGroupKey(this.roomId); | ||
| this.readyPromise.resolve(); | ||
| return; | ||
| } catch (e) { | ||
| log(e); | ||
| } | ||
| } | ||
|
|
||
| // If this room has a E2EKey, we import it | ||
| if (E2EKey && Encryption.privateKey) { | ||
| this.establishing = true; | ||
| const { keyID, roomKey, sessionKeyExportedString } = await this.importRoomKey(E2EKey, Encryption.privateKey); | ||
| this.keyID = keyID; | ||
| this.roomKey = roomKey; | ||
| this.sessionKeyExportedString = sessionKeyExportedString; | ||
| this.readyPromise.resolve(); | ||
| return; | ||
| if (E2EKey) { | ||
| try { | ||
| this.establishing = true; | ||
| const { keyID, roomKey, sessionKeyExportedString, algorithm } = await this.importRoomKey(E2EKey, Encryption.privateKey); | ||
| this.keyID = keyID; | ||
| this.roomKey = roomKey; | ||
| this.sessionKeyExportedString = sessionKeyExportedString; | ||
| this.algorithm = algorithm; | ||
| this.readyPromise.resolve(); | ||
| return; | ||
| } catch (error) { | ||
| this.establishing = false; | ||
| log(error); | ||
| // Fall through to try other options | ||
| } | ||
| } |
There was a problem hiding this comment.
Ready promise not rejected on key import failure, can leave room stuck.
When importRoomKey fails for E2ESuggestedKey (lines 153-155) or E2EKey (lines 169-173), the code sets establishing = false and logs the error but never rejects this.readyPromise. Callers awaiting handshake() will hang indefinitely. The room remains in a not-ready state with no path to recovery until the subscription updates externally.
Consider rejecting the promise to signal failure:
} catch (error) {
await e2eRejectSuggestedGroupKey(this.roomId);
+ this.establishing = false;
+ this.readyPromise.reject(error);
+ return;
} } catch (error) {
this.establishing = false;
+ this.readyPromise.reject(error);
log(error);
- // Fall through to try other options
+ return;
}
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.67.0.107687 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTUEipTZV9RaVOUtE0NSFeLqHqbTyUPzqNTigUDW2gt8bFHzkXPaE7_bzXE8x90LmybFQI5C99zM-BjerKo |
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
iOS Build Available Rocket.Chat Experimental 4.67.0.107690 |
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
3 participants
Proposed changes
Issue(s)
Depends on:
https://rocketchat.atlassian.net/browse/ESH-23
https://rocketchat.atlassian.net/browse/ESH-24
https://rocketchat.atlassian.net/browse/ESH-30
https://rocketchat.atlassian.net/browse/ESH-26
https://rocketchat.atlassian.net/browse/ESH-28
https://rocketchat.atlassian.net/browse/ESH-44
https://rocketchat.atlassian.net/browse/ESH-47
How to test or reproduce
Screenshots
UI improvements on change e2ee password
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Improvements
UI/Style
Localization
Tests
Chores