Report cryptographic usage that the noise filter was silently dropping - #3
Merged
Merged
Conversation
The scanner decided whether a match was low-value by testing the entire
source line against roughly a hundred substrings and dropping the finding
on any hit. Three of those entries matched ordinary code:
".md" intended for Markdown filenames, matched every ".md5("
"cipher =" matched Cipher cipher = Cipher.getInstance("DES")
"const " matched const cipher = crypto.createCipheriv(...)
So a.md5(b), hashlib.md5(b"a"), const rc4 = CryptoJS.RC4.encrypt(d, k) and
the conventional Java cipher variable all reported zero findings, while
md5(b) reported one. The tool's own crypto-samples tree contained
deliberately vulnerable code it could not see. None of the include flags
recovered them, because the match was dropped rather than downgraded.
Replace the substring blocklist with an evidence classifier anchored to the
position of the match. A match that is part of a call, a member access, an
import or an argument to a crypto factory is reported whatever else appears
on the line. Text mentions (prose, log output, comments, URLs, and the
values of documentation and configuration keys) are still held back, but
the count is reported and --include-narrative shows them. Detected key
material is exempt, since a private key in a comment is still exposed.
Also in this change:
- des.NewCipher and des.NewTripleDESCipher had no pattern at all, so Go DES
usage was undetectable. This was a coverage gap, not suppression.
- cryptoscan:ignore suppressed the following line as well as its own. A
directive now applies to its own line unless it says -next-line.
- The version command, SARIF, CBOM and JSON reported 1.4.0, 1.0.0, 1.1.0
and nothing respectively for the same binary. A CBOM naming the wrong
producer is a false provenance record. All four now read pkg/version.
- Findings were ordered by Go map iteration, so repeated scans of an
unchanged tree emitted a different order each time.
Every regression test here was confirmed to fail against the pre-fix code.
The emitted CBOM still validates against the official CycloneDX 1.6 schema.
Found by the pre-push walkthrough against the new release-smoke checklist. - Colour and the in-place progress line were emitted even when stdout was redirected, so a piped or captured scan collected 189 lines carrying ANSI escape and erase-line sequences. Colour is now off when stdout is not a terminal, NO_COLOR is honoured, and an explicit --no-color still wins. - Replaced the colour emoji in scan output with text markers. The geometric bullets and box drawing that make up the existing layout are unchanged; only characters with emoji presentation were substituted. They previously survived --no-color. - Replaced em-dashes in scan output. - Added credential and key-material patterns to .gitignore. - Added a property test over random lines and random match spans, since the evidence classifier does manual byte indexing and a panic there would abort an entire scan. - Rewrote docs/testing/release-smoke.md, which was still the unfilled web-app template telling a tester to run npm ci and check a 375px viewport for a Go CLI. It now covers the detection, suppression, provenance and determinism properties this release depends on.
An adversarial review of the previous two commits found that the evidence
classifier ran its narrative rules ahead of the operational check, which
reintroduced the same false-negative class the change set out to remove.
Seventeen confirmed regressions against main, all reproduced by hand:
outputs = Cipher.getInstance("RC4") "puts " matched inside "outputs "
inputs = crypto.createHash("md5") "puts " matched inside "inputs "
int z=0; z--; Cipher.getInstance("DES") "--" read as a comment in Java
gpg --cipher-algo 3DES --symmetric "--" read as a comment in shell
cipher: DES-CBC config key treated as a label
MACs hmac-md5 ... hmac-sha1-96 whole-line prose test on sshd_config
SSLProtocol -all +SSLv3 +TLSv1 same, on httpd.conf
ssh-keygen -t rsa -b 1024 -f id_rsa same, on a shell script
exec.Command("sh","-c","openssl dgst -md5 f") command read as prose
Nodes["crypto/rsa.GenerateKey"] Go selector read as a file path
The sshd case was not monotonic: adding a fifth cipher to a MACs line took
the file from three findings to zero. The Apache case flipped an SSLv3
configuration from exit 1 to exit 0 under --fail-on critical. Every one of
these is reachable by an author of the scanned code with a semantics-
preserving edit, which made the scanner steerable by its own input.
Corrections:
- operationalUse runs first, before the log, comment and path rules. This
ordering is the safety property and is documented as such.
- Call prefixes are matched on identifier boundaries rather than as
substrings, so "puts" no longer matches "outputs".
- Comment markers are language-specific. "#" is a comment in Python and a
private field in JavaScript; "--" is a comment in SQL and a decrement in
Java. An unknown language gets no markers.
- Whole-line prose detection applies only to documentation files.
- Configuration keys naming an algorithm are no longer narrative. For a
cryptographic inventory a declared algorithm is the inventory.
- A string carrying command flags is a command, not prose.
- Member access does not fire inside a path-like token, so a filename such
as legacy-md5.md no longer reads as a member call.
- Over-generic factory callees removed; "new" was matching errors.New().
Also corrected: the withheld count was incremented at the point of
suppression, before deduplication and the severity filter, so it overstated
what --include-narrative returns by more than 70% on a documentation tree.
The evidence check now runs last among the filters and the count is computed
against the deduplicated result, so the number the user is shown is exactly
the number the recommended command returns.
All fifteen new fixtures fail against the previous commit and pass now; the
original thirteen still fail against main.
A second adversarial pass found that six of the narrative labels are ordinary
configuration keys, and that the line-anchored branch hands a leading label
the whole rest of the line. Renaming a key by one word hid a CRITICAL finding
that main reported:
note: DES-CBC main 1 finding (CRITICAL), this build 0
remarks: DES-CBC main 1, this build 0
comment: DES-CBC main 1, this build 0
help/usage/example main 1, this build 0
`cryptoscan scan --fail-on critical` on `note: DES-CBC` returned exit 0 where
main returned exit 1. comment: is a real key in Kubernetes annotations and GPG
batch files; note: and remarks: are real in inventory YAML. The previous
commit's own header claimed configuration keys were not narrative, and these
six contradicted it.
Also from the same pass:
- A comma was accepted as a key/value separator, so a neighbouring list
element ending in a label word suppressed the next one:
`[]string{"key usage", "DES-CBC"}` lost the DES finding. This was already
firing on this repo's own pattern tests, on `extendedKeyUsage`.
- `--include-narrative` was not a superset. Deduplication keys on
file:line:category and kept the highest priority, so a withheld match could
REPLACE a reported one rather than add to it: 4 to 19 findings per tree were
in the default report but absent from the flag's output, while the summary
said nothing was withheld. Deduplication now prefers an operational match
over a narrative one at the same location. Verified as a strict subset on
four trees, with the count exact on each.
- Nine unreachable entries in commentMarkers removed; detectLanguage cannot
return sql, lua, haskell, ada, perl, r, makefile, scala or html. The header
comment justified the design with an example that never executed.
- isDocumentationFile no longer claims html (unreachable) or xml (every .xml,
including pom.xml and Spring configs, is configuration rather than prose).
- Removed a narrativeCount clamp that could not fire.
Test gaps that let this through, now closed: no case covered a config key
other than `cipher:`, and the withheld-count test compared lengths, which
cannot see a replacement. Added eleven config-key and list-neighbour fixtures
and a set-containment assertion on file:line:column:match:category. The
`url` case in the withheld suite was vacuous, asserting on a line the pattern
matcher never fires on at all; replaced.
The operational suite now fails against all three earlier revisions: 13
subtests against main, 23 against the first attempt, 8 against the second.
A third adversarial pass found 20 more regressions against main, twelve of
them from one rule. Both remaining word-based heuristics are deleted rather
than tuned, because each had already been narrowed twice and kept losing
findings to inputs nobody had thought of.
Quoted-string prose (>=3 words, no dash-flag, one English word) hid real
configuration, since a quoted string is exactly where algorithm names live:
db.Exec("ALTER SYSTEM SET password_encryption TO 'md5'") 1 -> 0
props.put("jdk.tls.client.cipherSuites", "RC4 and DES ...") 1 -> 0
conf.set("hive.ssl.ciphers", "DES-CBC3-SHA and RC4-MD5 ...") 2 -> 0
q := "SELECT alg FROM certs WHERE alg = 'RSA-1024' AND ..." 2 -> 0
os.system("openvpn config.ovpn cipher DES-CBC auth SHA1 ...") 2 -> 0
Leading documentation labels owned the rest of the line. The word list was
never the problem; handing a label the rest of the line is. It lost a
CRITICAL finding to `"title": "RSA-1024"`, to `"description" : "DES-CBC"`
with a space before the colon, to `:title => "DES-CBC"`, and to any key
merely ending in a label such as `ssl-title:`.
Both are gone. What is left is structural: a string owned by a logging or
error call, a comment in a language that has that comment marker, and a URL
or document path. None of them look at which words a value contains. The
practical cost is that `description: uses SHA-1 ...` is now reported, which
is the cheaper error.
Also fixed from the same pass:
- "#" opened a comment mid-token, so a URL fragment ended a YAML value:
`jdbcUrl: jdbc:mysql://h/db?cipher=DES-CBC#legacy` lost its finding. It now
requires the start of a line or preceding whitespace.
- An assignment did not end a token, so `CHECKSUM=md5.txt` read as a document
path. "=" is now a token boundary, and txt and pdf are no longer treated as
document extensions.
- Removed isProseLine, isDocumentationFile and stripMarkup as dead code:
shouldScanFile skips documentation files unless IncludeDocs is set, and no
flag or config key ever sets it, so the branch could not run.
Verified across 44 realistic lines spanning nginx, sshd, httpd, my.cnf,
php.ini, properties, ini, toml, yaml, json, pom.xml, web.config, Dockerfile,
Terraform, Ansible, systemd, SQL and eight languages: zero regressions
against main, and ten cases where this build finds crypto main missed
entirely. The operational suite now fails against all four earlier revisions
(14 subtests against main, 36, 21 and 13 against the three attempts).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The v1.4.0 release test failed with five P1 defects. All five are fixed here, together with four P2s from the same pass.
The detection defect
isLowValueContextdecided whether a match was low-value by testing the whole source line against about a hundred unanchored substrings, then dropping the finding. Three entries matched ordinary code:a.md5(b),hashlib.md5(b"a"),return hashlib.md5(d).hexdigest()".md", meant for Markdown filenamesCipher cipher = Cipher.getInstance("DES")"cipher ="const cipher = crypto.createCipheriv(...)"const "".md"matching every.md5(is the whole explanation for why the bug looked dot-related and MD5-specific:hashlib.sha1survived only because it contains no.mdsubstring. None of the include flags recovered these, because the match was dropped rather than downgraded. The tool's owncrypto-samples/tree contained deliberately vulnerable code it could not see.Separately,
des.NewCipherproduced zero regex matches — a pattern coverage gap, not suppression.\bDES\.(new|encrypt|decrypt)\bcannot matchdes.NewCipher, because\bafternewrequires a non-word character.The replacement
A position-aware evidence classifier (
pkg/scanner/evidence.go). It asks whether the matched token is part of an operation — a call, a member access, an import, an argument to a crypto factory — before applying any heuristic, and never suppresses a match that answers yes.Only three narrative rules remain, and all three are structural: the match belongs to a logging or error call, sits in a comment in a language that has that comment marker, or is inside a URL or document path. Nothing classifies by the words around a match.
Suppression is counted in the summary and recoverable with
--include-narrative. The count is computed after deduplication, so it is exactly what the flag adds, and deduplication prefers an operational match over a narrative one so the default report is a strict subset. Detected key material is never withheld.Also fixed
1.4.0,1.0.0(SARIF),1.1.0(CBOM) and nothing (JSON). A CBOM naming the wrong producer is a false provenance record. All surfaces now readpkg/version; JSON gained atoolobject.cryptoscan:ignoreblinded the following line. A directive now applies to its own line unless it says-next-line.docs/testing/release-smoke.mdwas the unfilled web-app template telling a tester to runnpm ciand check a 375px viewport for a Go CLI.Behaviour change worth reviewing
Declared configuration is now reported:
cipher: DES-CBC,MACs hmac-md5 ...,SSLProtocol +SSLv3,"algorithm": "RSA-2048". For a cryptographic inventory a declared algorithm is the inventory, and an Apache config enabling SSLv3 must fail--fail-on critical. Scanning a repo that ships crypto reference data will report a finding per row; use--exclude.Verification
Three adversarial review passes were run against this branch. Each found blocking false negatives in the previous revision, and every one is now a regression test:
"puts "inside a variable namedoutputsand--in Java hid CRITICAL findings. Detection was non-monotonic: a fifth cipher on ansshd_config MACsline took the file from 3 findings to 0. An SSLv3 Apache config flipped--fail-on criticalfrom exit 1 to exit 0.note: DES-CBClost a CRITICAL finding.--include-narrativewas deleting findings rather than adding them.password_encryption TO 'md5'and eleven other real settings. Both vocabulary-based rules were deleted rather than tuned.Every regression test was confirmed to fail against the code it guards. The operational suite fails against all four earlier revisions (14 subtests against
main; 36, 21 and 13 against the three intermediate attempts) and passes here. A 44-line sweep across nginx, sshd, httpd, my.cnf, php.ini, properties, ini, toml, yaml, json, pom.xml, web.config, Dockerfile, Terraform, Ansible, systemd, SQL and eight languages shows zero regressions againstmainand ten cases where this build finds cryptomainmissed entirely. A property test over 200k random line/span pairs found no panic. The emitted CBOM still validates against the upstream CycloneDX 1.6 schema.Closes the release blockers tracked in
todo/roadmap/qramm-cryptoscan-detection-suppression.md.