-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Crypto: Adding bad decrypt then mac order query. #20696
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
…MacOnEncryptPlaintext as well.
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.
Pull Request Overview
This PR adds detection capability for the "decrypt-then-MAC" vulnerability pattern, where decryption occurs before MAC verification. The changes introduce a new query to identify this security issue and refactor existing code to improve reuse of plaintext tracking for both MAC and encryption operations.
Key changes:
- Adds new query
BadMacOrderDecryptThenMac.qlto detect unsafe decrypt-then-MAC ordering - Refactors wrapper argument tracking logic with new predicates (
encryptWrapperArg,decryptWrapperArg,macWrapperArg) - Renames
InterimArgtoEncryptOrMacCallArgandTargetArgto improve code clarity
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| BadMacUse.java | Adds test case decryptThenMac() demonstrating the unsafe decrypt-then-MAC pattern |
| BadMacOrderMacOnEncryptPlaintext.expected | Updates test expectations for plaintext reuse detection with new flow paths |
| BadMacOrderDecryptToMac.expected | Adds test failure expectation for missing source annotation |
| BadMacOrderDecryptThenMac.qlref | Creates query reference file for the new decrypt-then-MAC detection query |
| BadMacOrderDecryptThenMac.expected | Defines expected test results for the new decrypt-then-MAC query |
| BadMacOrderMacOnEncryptPlaintext.ql | Updates variable name from InterimArg to EncryptOrMacCallArg |
| BadMacOrderDecryptThenMac.ql | Implements new query to detect decrypt-then-MAC vulnerability pattern |
| BadMacOrder.qll | Refactors core logic with new wrapper predicates and flow configurations |
| not_included_in_qls.expected | Registers the new query in the excluded queries list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,19 @@ | |||
| /** | |||
| * @name Bad MAC order: decrypt then mac | |||
| * @description Decryption on cipher text, then MAC on ciopher text, is incorrect order | |||
Copilot
AI
Oct 24, 2025
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.
Corrected spelling of 'ciopher' to 'cipher'.
| * @description Decryption on cipher text, then MAC on ciopher text, is incorrect order | |
| * @description Decryption on cipher text, then MAC on cipher text, is incorrect order |
| // /** | ||
| // * An argument of a target sink or a parent call whose parameter flows to a target sink | ||
| // */ | ||
| // class EncryptOrMacPartialFlowArg extends DataFlow::Node { | ||
| // DataFlow::Node targetSink; | ||
| // EncryptOrMacPartialFlowArg() { | ||
| // encryptWrapperArg(this, targetSink) | ||
| // or | ||
| // macWrapperArg(this, targetSink) | ||
| // } | ||
| // DataFlow::Node getTargetSink() { result = targetSink } | ||
| // } |
Copilot
AI
Oct 24, 2025
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.
The commented-out class EncryptOrMacPartialFlowArg should be removed if it's no longer needed. Leaving dead code in the codebase reduces maintainability and can cause confusion.
| // /** | |
| // * An argument of a target sink or a parent call whose parameter flows to a target sink | |
| // */ | |
| // class EncryptOrMacPartialFlowArg extends DataFlow::Node { | |
| // DataFlow::Node targetSink; | |
| // EncryptOrMacPartialFlowArg() { | |
| // encryptWrapperArg(this, targetSink) | |
| // or | |
| // macWrapperArg(this, targetSink) | |
| // } | |
| // DataFlow::Node getTargetSink() { result = targetSink } | |
| // } |
Adds capability to detect decrypt THEN mac. Also fixes to improve reuse of plaintext for mac and encryption.