Ed25519 and Ed448 generate() rules were matching too broadly#429
Ed25519 and Ed448 generate() rules were matching too broadly#429Arijit429 wants to merge 1 commit into
Conversation
…prevent false positives Signed-off-by: Arijit429 <arijitdeb1203@gmail.com>
eeff0b7 to
d27f2f5
Compare
san-zrl
left a comment
There was a problem hiding this comment.
You claim to fix #300. This is not quite correct.
When scanning https://github.com/docling-project/docling the original version detects 48 assets all of which seem to be false positives (see dockling-cbom-original.json). All detection relates to a "generate" context.
After applying your PR the scan detects 24 assets (see dockling-cbom-with-pr.json). These findings also relate entirely to a "generate" call on some object.
The reduced the number of FPs tells us
- The fix is correct as it solves some of the observed problems.
- Your PR only partially addresses #300. You must have missed some other detection rules with matching conditions which are too general
If you'd like to continue working on this PR please folllow the advice below. I'm happy to review the next version.
- Follow the description in the issue on how to reproduce the problem in the issue. Good issues should contain such a description. Apply your PR and verify if your claim to fix the targeted issue is correct.
- Your PR does not contain any regression tests. In this example it should have test cases for the positive case (finding correctly detected) and the negative case (no false positive).
Thank you for the detailed review and for running the scan yourself. You're right — I only looked at Ed25519 and Ed448 but didn't check the Will push an update shortly. |
Fixes #300
The generate() rules for Ed25519 and Ed448 were using withAnyParameters(),
which caused the scanner to match any .generate() call regardless of parameters.
The actual Ed25519PrivateKey.generate() and Ed448PrivateKey.generate() from
pyca/cryptography take no parameters. So withoutParameters() is the correct matcher.
Reproducer from the issue — this was getting flagged as Edwards448 Key Pair Generation:
Changed withAnyParameters() to withoutParameters() in both SIGN_ED25519
and SIGN_ED448 rules in PycaSign.java.