JavaScript/Python/Ruby: Improve alert message for */weak-cryptographic-algorithm.
#14603
Conversation
…thm`. Specifically, we add a link to the location where the cryptographic algorithm is configured, which can be far away from its use.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from Cryptography::CryptographicOperation operation, string msgPrefix | ||
| where | ||
| algorithm = operation.getAlgorithm() and | ||
| // `Cryptography::HashingAlgorithm` and `Cryptography::PasswordHashingAlgorithm` are | ||
| // handled by `py/weak-sensitive-data-hashing` | ||
| algorithm instanceof Cryptography::EncryptionAlgorithm and | ||
| ( | ||
| exists(Cryptography::EncryptionAlgorithm algorithm | algorithm = operation.getAlgorithm() | | ||
| algorithm.isWeak() and | ||
| msgPrefix = "The cryptographic algorithm " + operation.getAlgorithm().getName() | ||
| msgPrefix = "The cryptographic algorithm " + algorithm.getName() | ||
| ) | ||
| or | ||
| operation.getBlockMode().isWeak() and msgPrefix = "The block mode " + operation.getBlockMode() | ||
| select operation, msgPrefix + " is broken or weak, and should not be used." | ||
| select operation, "$@ is broken or weak, and should not be used.", operation.getInitialization(), | ||
| msgPrefix |
Check warning
Code scanning / CodeQL
Consistent alert message
max-schaefer
force-pushed
the
max-schaefer/broken-crypto-algorithm-link
branch
from
de2b219 to
104700f
Compare
|
sorry, I got away from reviewing this PR, and shall have a proper look today 👍 |
There was a problem hiding this comment.
Thanks for this PR. I'm currently thinking about a next generation of Concepts for cryptography, so wanted to give proper thought to this addition. Sorry for taking so long to review, I got wound up in other work 😐
Overall I think the additions in this PR makes sense right now, so I think we should just accept them. Thanks for making this 🎉 💪 The only thing holding me back is to confirm there are no performance regressions, I've just started a DCA run for all languages, so once that finishes we should be able to merge 👍
In the long term, we might run into more complicated scenarios, where a combination of properties of the configuration makes the operation weak... but no need to worry too much about that for the queries and models with have right now 😊
|
Performance looks good |
The alert message now contains a link to the location where the algorithm was configured, which may be in a different file from the alert location.
The library changes needed to make this work are not conceptually difficult, but quite spread out. I don't understand the affected libraries very well, so I may well have botched a few of the updates. Feedback from the respective maintainers is highly welcome!
I had a quick look at the corresponding queries for Java and C++, and they already seem to link to the right location, so I didn't change anything there. I'm not sure what the query is called for C#, Go and Swift.