fix(secret): only match secrets of meaningful length, allow example strings to not be matched #8602
Conversation
Require a matching secret text to be at least 64 characters long, as that would be the typical length of the smallest possible library-supported RSA private key. Everyone should be using 2048-bit or greater keys; any key smaller than this matching rule is so trivially small that it can't really be considered a practical "secret." It would be trivially crackable and also result in unusably small block sizies.
|
|
PT-GD
changed the title
|
Can anyone familiar with the test framework help write updates to https://github.com/aquasecurity/trivy/blob/main/pkg/fanal/secret/scanner_test.go#L528 and the other tests related to AsymmetricPrivateKey? I don't see any negative test assertions in the suite. I'd think we want tests which continue to find matches for longer strings but now don't find matches for shorter strings. |
pkg/fanal/secret/builtin-rules.go
Outdated
| @@ -185,7 +185,7 @@ var builtinRules = []Rule{ | |||
| Category: CategoryAsymmetricPrivateKey, | |||
| Title: "Asymmetric Private Key", | |||
| Severity: "HIGH", | |||
| Regex: MustCompile(`(?i)-----\s*?BEGIN[ A-Z0-9_-]*?PRIVATE KEY( BLOCK)?\s*?-----[\s]*?(?P<secret>[A-Za-z0-9=+/\\\r\n][A-Za-z0-9=+/\\\s]+)[\s]*?-----\s*?END[ A-Z0-9_-]*? PRIVATE KEY( BLOCK)?\s*?-----`), | |||
| Regex: MustCompile(`(?i)-----\s*?BEGIN[ A-Z0-9_-]*?PRIVATE KEY( BLOCK)?\s*?-----[\s]*?(?P<secret>[A-Za-z0-9=+/\\\r\n][A-Za-z0-9=+/\\\s]+)[\s]{64,}?-----\s*?END[ A-Z0-9_-]*? PRIVATE KEY( BLOCK)?\s*?-----`), | |||
There was a problem hiding this comment.
As this regex matches other keys such as DSA, we might want to consider using a smaller minimum size. 22 characters would suffice for 128-bit keys. This might increase false positives.
Looking at this, the regex already wouldn't match typical "redaction indicators" like "..." as they aren't part of the Base64 "alphabet." The reason those are still matching is because zero-length match is allowed by "*"
Whoops! The * I changed applies to white space, not the secret. Don't merge this quite yet!
There was a problem hiding this comment.
Whoops! The * I changed applies to white space, not the secret. Don't merge this quite yet!
Just FYI - you can convert this PR to draft, so that it would be clear that PR is not ready yet or requires revision
|
Hello @PT-GD
You can add one more key with lower lenght in asymmetric-private-key.txt (like i added key in line 6). |
|
This PR is stale because it has been labeled with inactivity. |
github-actions
bot
added
the
lifecycle/stale
DmitriyLewen
removed
the
lifecycle/stale
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
DmitriyLewen
changed the title
Description
This change requires a potentially matching secret Base64 text to be at least 64 characters long. This would be the typical encoded length of the smallest possible library-supported RSA private key, 384 bits. (n.b. 384*1.33/8 = 63.8)
Any actual key smaller than this matching rule is not usable with existing library implementations and is also small that it can't really be considered a practical "secret." It would be trivially crackable and also result in unusably small block sizes.
Related issues
Checklist