[groups] Fix #38823 - filter group keys by session_id during decrypt #39401
+19
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request addresses a critical issue where group messages were being decrypted against all available keys, instead of filtering by session ID. The changes introduce a mechanism to derive the session ID from the key and only attempt decryption when the session IDs match. This significantly improves efficiency and security. The implementation appears sound, but I have a few suggestions for improvement.
Summary of Findings
- Missing Log Messages: Consider adding log messages for key context not found and session ID mismatch to aid in debugging.
- Potential Null Pointer Dereference: It might be beneficial to add a check to ensure that
keyContextis valid before callingDeriveGroupSessionId. - Inconsistent Logging: The logging statements for decryption attempts are placed inside a conditional compilation block, which might lead to inconsistent logging behavior.
Merge Readiness
The pull request effectively addresses the issue of filtering group keys by session ID during decryption. However, I recommend addressing the comments provided to improve debugging capabilities and ensure consistent logging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the medium severity issue should be addressed before merging.
turon
force-pushed
the
fix/38823/group-key-filter
branch
from
184946e to
ce941b1
Compare
…ring group message decrypt.
turon
force-pushed
the
fix/38823/group-key-filter
branch
from
ce941b1 to
91e99ef
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #38823 by ensuring that decryption for group messages is attempted only when the session ID in the packet header matches the hash of the decryption key.
- Updated header includes to use the appropriate provider and logging constants.
- Added retrieval of key context and a check to compare the key hash against the session ID to filter decryption attempts.
|
PR #39401: Size comparison from b1fc719 to 91e99ef Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #38823
The receive handler for a group message can try to decrypt against multiple keys, but right now it tries every key when it should only try keys where session_id matches hash(key).
This change will extract the hash of the key from CryptoContext and only attempt decryption when that is equal to the session_id in PacketHeader.
Testing