From 55c4509cf757bc28237b40d16cae493892407395 Mon Sep 17 00:00:00 2001 From: Abdel Fane Date: Mon, 27 Jul 2026 17:41:03 -0600 Subject: [PATCH 1/5] Report crypto that the line-wide noise filter was dropping 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. --- CHANGELOG.md | 61 ++++ README.md | 23 +- internal/cli/root.go | 10 +- internal/cli/scan.go | 5 +- internal/cli/scan_test.go | 5 +- pkg/patterns/matcher.go | 10 +- pkg/reporter/cbom.go | 3 +- pkg/reporter/sarif.go | 5 +- pkg/reporter/text.go | 10 + pkg/reporter/version_test.go | 131 ++++++++ pkg/scanner/determinism_test.go | 60 ++++ pkg/scanner/evidence.go | 537 ++++++++++++++++++++++++++++++++ pkg/scanner/evidence_test.go | 203 ++++++++++++ pkg/scanner/scanner.go | 377 +++++++--------------- pkg/version/version.go | 29 ++ 15 files changed, 1195 insertions(+), 274 deletions(-) create mode 100644 CHANGELOG.md create mode 100644 pkg/reporter/version_test.go create mode 100644 pkg/scanner/determinism_test.go create mode 100644 pkg/scanner/evidence.go create mode 100644 pkg/scanner/evidence_test.go create mode 100644 pkg/version/version.go diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..793397a --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,61 @@ +# Changelog + +All notable changes to CryptoScan are recorded here. This project follows +[Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [Unreleased] + +### Fixed + +- **Operational crypto was silently dropped by the noise filter.** The scanner + decided whether a match was low-value by testing the whole source line + against a list of about a hundred substrings, and dropped the finding on any + hit. Three of those entries matched ordinary code: + - `".md"`, intended for Markdown filenames, matched every `.md5(` in the + corpus. `a.md5(b)`, `hashlib.md5(b"a")` and + `return hashlib.md5(data).hexdigest()` all reported zero findings. + - `"cipher ="` matched `Cipher cipher = Cipher.getInstance("DES")`, the + conventional way to name a `javax.crypto.Cipher`. + - `"const "` matched `const cipher = crypto.createCipheriv(...)`, which + `prefer-const` makes the idiomatic form in JavaScript. + + The heuristic has been replaced by 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 now always reported, whatever + else appears on the line. + +- **Suppression is now counted and recoverable.** Matches held back as prose, + log output, comments, URLs or documentation and configuration values are + reported as a count in the scan summary, and `--include-narrative` (also + covered by `--verbose`) shows them. Detected key material is never held back, + since a private key in a comment is still an exposed private key. + +- **`cryptoscan:ignore` no longer blinds the following line.** A trailing + directive suppressed both its own line and the next one. A directive now + applies to its own line unless it says `cryptoscan:ignore-next-line`. + +- **Go DES usage was undetectable.** `des.NewCipher` and + `des.NewTripleDESCipher` are the Go standard library's only DES constructors + and no pattern matched them. + +- **Every output surface reported a different version.** One binary reported + `1.4.0` from the `version` command, `1.0.0` in SARIF, `1.1.0` in CBOM, and no + version at all in JSON. A CBOM naming the wrong producer is a false + provenance record. All surfaces now read `pkg/version`, and JSON carries a + `tool` object. + +- **Finding order was nondeterministic.** Results were built by ranging over a + map, so two scans of an unchanged tree emitted the same findings in a + different order. This broke diffable CI output, golden-file tests and + reproducible CBOMs. + +### Added + +- `--include-narrative` flag to show algorithm mentions held back as text. +- `tool` object in JSON output, carrying the scanner name and version. +- `narrativeSuppressedCount` in the scan summary. + +## [1.3.0] + +See the [release notes](https://github.com/csnp/cryptoscan/releases) for +versions 1.3.0 and earlier. diff --git a/README.md b/README.md index 74e9718..94241db 100644 --- a/README.md +++ b/README.md @@ -301,6 +301,10 @@ Flags: -c, --context int Lines of source context to show (default 3) -p, --progress Show scan progress indicator --min-severity string Minimum severity to report: info, low, medium, high, critical + --include-imports Include library import findings + --include-quantum-safe Include quantum-safe algorithm findings (SHA-256, AES-256) + --include-narrative Include algorithms named in prose, logs, docs and config keys + -v, --verbose Show all findings, including all three categories above --no-color Disable colored output --pretty Pretty print JSON output -h, --help Show help @@ -332,6 +336,23 @@ cryptoscan scan . --exclude "vendor/*,node_modules/*,*_test.go" cryptoscan scan . --min-severity critical --format json | jq '.findings | length' ``` +### What is reported, and what is held back + +CryptoScan reports an algorithm when the match is part of an operation: a call, +an instantiation, a member access, or an algorithm name passed to a crypto +factory. `hashlib.md5(data)`, `des.NewCipher(key)`, `const cipher = +crypto.createCipheriv('des-ede3-cbc', k, iv)` and +`Cipher.getInstance("RC4")` are all reported. + +Mentions in text are held back: prose, log messages, comments, URLs, and the +values of documentation or configuration keys. The scan summary always says how +many were held back, and `--include-narrative` shows them: + +``` + 14 mention(s) withheld as documentation, log or configuration text. + Show them with: cryptoscan scan . --include-narrative +``` + ### Suppressing False Positives Use inline comments to suppress findings that are intentional or not applicable: @@ -352,7 +373,7 @@ legacyKey := oldCrypto.NewKey() ``` Supported directives: -- `cryptoscan:ignore` — Ignore all findings on this line +- `cryptoscan:ignore` — Ignore all findings on this line, and only this line - `cryptoscan:ignore RSA-001` — Ignore specific pattern ID - `cryptoscan:ignore RSA-*` — Ignore pattern family (wildcard) - `cryptoscan:ignore-next-line` — Ignore finding on the following line diff --git a/internal/cli/root.go b/internal/cli/root.go index d5fd977..6dd94c7 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -7,10 +7,11 @@ import ( "fmt" "github.com/spf13/cobra" + + "github.com/csnp/cryptoscan/pkg/version" ) var ( - version = "dev" commit = "none" buildDate = "unknown" ) @@ -47,8 +48,11 @@ Examples: Learn more at https://qramm.org`, } +// SetVersionInfo records the build metadata injected by GoReleaser. The version +// string is stored in pkg/version so that every output surface reports the same +// value; commit and build date are only shown by the version command. func SetVersionInfo(v, c, d string) { - version = v + version.Set(v) commit = c buildDate = d } @@ -66,7 +70,7 @@ var versionCmd = &cobra.Command{ Use: "version", Short: "Print version information", Run: func(cmd *cobra.Command, args []string) { - fmt.Printf("cryptoscan %s\n", version) + fmt.Printf("cryptoscan %s\n", version.Get()) if commit != "none" { fmt.Printf(" commit: %s\n", commit) } diff --git a/internal/cli/scan.go b/internal/cli/scan.go index 72c2b2b..e14627d 100644 --- a/internal/cli/scan.go +++ b/internal/cli/scan.go @@ -35,6 +35,7 @@ var ( streamFindings bool includeImports bool includeQuantumSafe bool + includeNarrative bool verbose bool // CI/CD flexibility flags @@ -108,7 +109,8 @@ func init() { scanCmd.Flags().BoolVar(&streamFindings, "stream", true, "Show findings as they are discovered") scanCmd.Flags().BoolVar(&includeImports, "include-imports", false, "Include library import findings (normally suppressed as low-value)") scanCmd.Flags().BoolVar(&includeQuantumSafe, "include-quantum-safe", false, "Include quantum-safe algorithm findings (SHA-256, AES-256)") - scanCmd.Flags().BoolVarP(&verbose, "verbose", "v", false, "Show all findings including imports and quantum-safe algorithms") + scanCmd.Flags().BoolVar(&includeNarrative, "include-narrative", false, "Include algorithms named in prose, log messages, documentation and config keys") + scanCmd.Flags().BoolVarP(&verbose, "verbose", "v", false, "Show all findings including imports, quantum-safe algorithms and narrative mentions") // CI/CD flexibility flags scanCmd.Flags().StringVar(&ignorePatterns, "ignore", "", "Pattern IDs to ignore (comma-separated, e.g., \"RSA-001,CERT-*\")") @@ -222,6 +224,7 @@ func runScan(cmd *cobra.Command, args []string) error { MinSeverity: parseSeverity(effectiveMinSeverity), IncludeImports: includeImports || verbose, // Include if explicitly set or verbose mode IncludeQuantumSafe: includeQuantumSafe || verbose, // Include if explicitly set or verbose mode + IncludeNarrative: includeNarrative || verbose, // Include if explicitly set or verbose mode IgnoreIDs: ignoreIDs, IgnoreCategories: ignoreCats, } diff --git a/internal/cli/scan_test.go b/internal/cli/scan_test.go index ecdad99..c45c04d 100644 --- a/internal/cli/scan_test.go +++ b/internal/cli/scan_test.go @@ -12,6 +12,7 @@ import ( "time" "github.com/csnp/cryptoscan/pkg/scanner" + "github.com/csnp/cryptoscan/pkg/version" "github.com/spf13/cobra" ) @@ -89,8 +90,8 @@ func TestRootCmdExists(t *testing.T) { func TestSetVersionInfo(t *testing.T) { SetVersionInfo("1.0.0", "abc123", "2025-01-01") - if version != "1.0.0" { - t.Errorf("version = %q, want %q", version, "1.0.0") + if got := version.Get(); got != "1.0.0" { + t.Errorf("version.Get() = %q, want %q", got, "1.0.0") } if commit != "abc123" { t.Errorf("commit = %q, want %q", commit, "abc123") diff --git a/pkg/patterns/matcher.go b/pkg/patterns/matcher.go index b67027b..fa61a0b 100644 --- a/pkg/patterns/matcher.go +++ b/pkg/patterns/matcher.go @@ -423,7 +423,11 @@ func (m *Matcher) loadPatterns() { ID: "DES-001", Name: "DES Algorithm", Category: "Deprecated Algorithm", - Regex: regexp.MustCompile(`(?i)\bDES[-_](CBC|ECB|CFB|OFB)\b|\bDESede\b|Cipher\.getInstance\s*\(\s*["']DES["']|createCipher\s*\(\s*["']des|crypto\.createCipher.*["']des|\bDES\.(new|encrypt|decrypt)\b|\bDES\.MODE_`), + // des.NewCipher is the Go standard library's only way to construct a + // DES cipher, and it was not covered: \bDES\.(new|...)\b cannot match + // "des.NewCipher" because the \b after "new" requires a non-word + // character. Go DES usage was therefore invisible to the scanner. + Regex: regexp.MustCompile(`(?i)\bDES[-_](CBC|ECB|CFB|OFB)\b|\bDESede\b|Cipher\.getInstance\s*\(\s*["']DES["']|createCipher\s*\(\s*["']des|crypto\.createCipher.*["']des|\bDES\.(new|encrypt|decrypt)\b|\bDES\.MODE_|\bdes\.NewCipher\s*\(`), Severity: types.SeverityCritical, Quantum: types.QuantumVulnerable, Algorithm: "DES", @@ -436,7 +440,9 @@ func (m *Matcher) loadPatterns() { ID: "3DES-001", Name: "Triple DES Algorithm", Category: "Deprecated Algorithm", - Regex: regexp.MustCompile(`(?i)\b(3DES|Triple[-_]?DES|DESede|TDEA)\b`), + // des.NewTripleDESCipher is the Go standard library constructor; the + // \b(3DES|...)\b alternation cannot reach it. + Regex: regexp.MustCompile(`(?i)\b(3DES|Triple[-_]?DES|DESede|TDEA)\b|\bdes\.NewTripleDESCipher\s*\(`), Severity: types.SeverityHigh, Quantum: types.QuantumVulnerable, Algorithm: "3DES", diff --git a/pkg/reporter/cbom.go b/pkg/reporter/cbom.go index a46f3e3..5ddc22b 100644 --- a/pkg/reporter/cbom.go +++ b/pkg/reporter/cbom.go @@ -15,6 +15,7 @@ import ( "github.com/csnp/cryptoscan/pkg/analyzer" "github.com/csnp/cryptoscan/pkg/scanner" "github.com/csnp/cryptoscan/pkg/types" + "github.com/csnp/cryptoscan/pkg/version" ) // CBOMReporter generates Cryptographic Bill of Materials output @@ -529,7 +530,7 @@ func (r *CBOMReporter) Generate(results *scanner.Results) (string, error) { { Vendor: "CSNP", Name: "CryptoScan", - Version: "1.1.0", + Version: version.Get(), }, }, Component: summaryComponent, diff --git a/pkg/reporter/sarif.go b/pkg/reporter/sarif.go index c0306e4..be297b3 100644 --- a/pkg/reporter/sarif.go +++ b/pkg/reporter/sarif.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/csnp/cryptoscan/pkg/scanner" + "github.com/csnp/cryptoscan/pkg/version" ) // SARIFReporter generates SARIF 2.1.0 format output @@ -196,8 +197,8 @@ func (r *SARIFReporter) Generate(results *scanner.Results) (string, error) { Tool: sarifTool{ Driver: sarifDriver{ Name: "CryptoScan", - Version: "1.0.0", - SemanticVersion: "1.0.0", + Version: version.Get(), + SemanticVersion: version.Get(), InformationURI: "https://qramm.org", Rules: rules, }, diff --git a/pkg/reporter/text.go b/pkg/reporter/text.go index 4ed93aa..0421a36 100644 --- a/pkg/reporter/text.go +++ b/pkg/reporter/text.go @@ -131,6 +131,16 @@ func (r *TextReporter) Generate(results *scanner.Results) (string, error) { r.color(sev.color, bar))) } } + + // Report what was withheld. A scanner may reduce noise, but it must never + // reduce it invisibly: the reader has to be able to tell the difference + // between "nothing was found" and "something was found and not shown". + if results.Summary.NarrativeSuppressed > 0 { + b.WriteString(r.color(colorCyan, fmt.Sprintf( + "\n %d mention(s) withheld as documentation, log or configuration text.\n"+ + " Show them with: cryptoscan scan %s --include-narrative\n", + results.Summary.NarrativeSuppressed, results.ScanTarget))) + } b.WriteString("\n") // Quantum Risk Assessment with visual emphasis diff --git a/pkg/reporter/version_test.go b/pkg/reporter/version_test.go new file mode 100644 index 0000000..040a630 --- /dev/null +++ b/pkg/reporter/version_test.go @@ -0,0 +1,131 @@ +// Copyright 2025 CyberSecurity NonProfit (CSNP) +// SPDX-License-Identifier: Apache-2.0 + +package reporter + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/csnp/cryptoscan/pkg/scanner" + "github.com/csnp/cryptoscan/pkg/version" +) + +// TestEveryEmitterReportsTheSameVersion guards the provenance defect found by +// the v1.4.0 release test: one binary reported four different versions. The +// version command said 1.4.0, SARIF said 1.0.0, CBOM said 1.1.0 and JSON +// carried no version at all. +// +// SARIF and CBOM are evidence artifacts. A CBOM claiming it was produced by +// CryptoScan 1.1.0 is a false provenance record, and a SARIF consumer cannot +// attribute a result to a scanner build. Every surface must read pkg/version. +func TestEveryEmitterReportsTheSameVersion(t *testing.T) { + const want = "9.8.7-test" + + original := version.Get() + version.Set(want) + t.Cleanup(func() { version.Set(original) }) + + results := &scanner.Results{ + Tool: scanner.ToolInfo{Name: "CryptoScan", Version: version.Get()}, + ScanTarget: ".", + Findings: []scanner.Finding{}, + Summary: scanner.Summary{}, + } + + t.Run("json", func(t *testing.T) { + out := generate(t, NewJSONReporter(false), results) + var doc struct { + Tool struct { + Version string `json:"version"` + } `json:"tool"` + } + decode(t, out, &doc) + if doc.Tool.Version != want { + t.Errorf("JSON tool.version = %q, want %q", doc.Tool.Version, want) + } + }) + + t.Run("sarif", func(t *testing.T) { + out := generate(t, NewSARIFReporter(), results) + var doc struct { + Runs []struct { + Tool struct { + Driver struct { + Version string `json:"version"` + SemanticVersion string `json:"semanticVersion"` + } `json:"driver"` + } `json:"tool"` + } `json:"runs"` + } + decode(t, out, &doc) + if len(doc.Runs) == 0 { + t.Fatal("SARIF report has no runs") + } + d := doc.Runs[0].Tool.Driver + if d.Version != want { + t.Errorf("SARIF driver.version = %q, want %q", d.Version, want) + } + if d.SemanticVersion != want { + t.Errorf("SARIF driver.semanticVersion = %q, want %q", d.SemanticVersion, want) + } + }) + + t.Run("cbom", func(t *testing.T) { + out := generate(t, NewCBOMReporter(), results) + var doc struct { + Metadata struct { + Tools []struct { + Version string `json:"version"` + } `json:"tools"` + } `json:"metadata"` + } + decode(t, out, &doc) + if len(doc.Metadata.Tools) == 0 { + t.Fatal("CBOM has no metadata.tools entry") + } + if got := doc.Metadata.Tools[0].Version; got != want { + t.Errorf("CBOM metadata.tools[0].version = %q, want %q", got, want) + } + }) + + // A literal left behind anywhere in the emitters would defeat the single + // source of truth without failing the assertions above, so check that no + // output still carries the old hardcoded values. + t.Run("no stale literals", func(t *testing.T) { + for name, out := range map[string]string{ + "json": generate(t, NewJSONReporter(false), results), + "sarif": generate(t, NewSARIFReporter(), results), + "cbom": generate(t, NewCBOMReporter(), results), + } { + for _, stale := range []string{`"1.0.0"`, `"1.1.0"`} { + // SARIF's own spec version is legitimately 2.1.0; only the + // tool version fields are checked here. + if strings.Contains(out, `"version": `+stale) { + t.Errorf("%s still emits a hardcoded tool version %s", name, stale) + } + } + } + }) +} + +type reportGenerator interface { + Generate(*scanner.Results) (string, error) +} + +func generate(t *testing.T, r reportGenerator, results *scanner.Results) string { + t.Helper() + out, err := r.Generate(results) + if err != nil { + t.Fatalf("generate: %v", err) + } + return out +} + +func decode(t *testing.T, out string, into any) { + t.Helper() + if err := json.Unmarshal([]byte(out), into); err != nil { + t.Fatalf("decode report: %v\n%s", err, out) + } +} diff --git a/pkg/scanner/determinism_test.go b/pkg/scanner/determinism_test.go new file mode 100644 index 0000000..b9ea34f --- /dev/null +++ b/pkg/scanner/determinism_test.go @@ -0,0 +1,60 @@ +// Copyright 2025 CyberSecurity NonProfit (CSNP) +// SPDX-License-Identifier: Apache-2.0 + +package scanner + +import ( + "fmt" + "path/filepath" + "testing" + + "github.com/csnp/cryptoscan/pkg/types" +) + +// TestFindingOrderIsDeterministic guards against the map-iteration ordering +// found by the v1.4.0 release test. deduplicateFindings built its result by +// ranging over a map, and Go randomises map iteration, so repeated scans of an +// unchanged tree produced the same findings in a different order every time. +// +// That breaks diffable CI output, golden-file tests and reproducible CBOMs. +// Several files with several findings each are needed to make the old +// behaviour fail reliably rather than by luck. +func TestFindingOrderIsDeterministic(t *testing.T) { + dir := t.TempDir() + for i := 0; i < 6; i++ { + write(t, filepath.Join(dir, fmt.Sprintf("f%d.py", i)), `import hashlib +a = hashlib.md5(b"x") +b = hashlib.sha1(b"y") +c = hashlib.new("md5") +`) + } + + var first []string + for run := 0; run < 5; run++ { + results, err := New(Config{Target: dir, MinSeverity: types.SeverityInfo}).Scan() + if err != nil { + t.Fatalf("scan: %v", err) + } + + order := make([]string, 0, len(results.Findings)) + for _, f := range results.Findings { + order = append(order, fmt.Sprintf("%s:%d:%d:%s", f.File, f.Line, f.Column, f.ID)) + } + + if run == 0 { + if len(order) < 6 { + t.Fatalf("fixture produced only %d findings; too few to detect an ordering change", len(order)) + } + first = order + continue + } + if len(order) != len(first) { + t.Fatalf("run %d returned %d findings, run 0 returned %d", run, len(order), len(first)) + } + for i := range order { + if order[i] != first[i] { + t.Fatalf("run %d differs from run 0 at position %d: %q vs %q", run, i, order[i], first[i]) + } + } + } +} diff --git a/pkg/scanner/evidence.go b/pkg/scanner/evidence.go new file mode 100644 index 0000000..691e4f0 --- /dev/null +++ b/pkg/scanner/evidence.go @@ -0,0 +1,537 @@ +// Copyright 2025 CyberSecurity NonProfit (CSNP) +// SPDX-License-Identifier: Apache-2.0 + +package scanner + +import "strings" + +// Evidence classification for a single pattern match. +// +// The scanner needs to separate "this codebase uses MD5" from "this line of +// prose mentions MD5". The previous implementation answered that question by +// testing the whole source line against a list of about a hundred substrings +// and dropping the finding on any hit. That is unsound in a way that produced +// silent false negatives: +// +// a.md5(b) suppressed by ".md" +// return hashlib.md5(b"data").hexdigest() suppressed by ".md" +// Cipher cipher = Cipher.getInstance("DES") suppressed by "cipher =" +// const rc4 = CryptoJS.RC4.encrypt(d, k) suppressed by "const " +// +// None of those substrings had anything to do with the matched token. ".md" +// was meant to catch Markdown filenames and instead matched every ".md5(" in +// the corpus; "const " and "cipher =" matched the most idiomatic way to write +// crypto in JavaScript and Java respectively. The tool's own crypto-samples +// tree contained deliberately vulnerable code that the scanner could not see. +// +// The replacement asks two better-posed questions, both anchored to where the +// match actually sits on the line: +// +// 1. Is the matched token part of an operational construct (a call, a member +// access, an argument to a crypto factory)? If so it is evidence of use and +// is never suppressed, whatever else appears on the line. +// 2. Otherwise, is the token inside narrative text (prose, a log message, a +// documentation or configuration key's value, a URL or path)? Only then is +// it treated as low-value. +// +// Anything that is neither is reported. The default is to report, because a +// missed finding in a security scanner costs more than an extra line of output. + +// evidenceClass describes what a matched token tells us about the codebase. +type evidenceClass int + +const ( + // evidenceOperational means the token participates in a cryptographic + // operation: a call, an instantiation, a member access, or an algorithm + // name handed to a crypto factory. + evidenceOperational evidenceClass = iota + + // evidenceNarrative means the token appears in text rather than in code: + // prose, a log message, a documentation or config key's value, a URL, or + // a file path. + evidenceNarrative +) + +// classifyMatch reports what kind of evidence the token at [start,end) in line +// represents, along with the rule that decided it. The reason string is used +// for diagnostics and tests; callers should not parse it. +// +// start and end are byte offsets into line. Out-of-range offsets classify as +// operational, since an unclassifiable match must not be silently dropped. +func classifyMatch(line string, start, end int) (evidenceClass, string) { + if start < 0 || end > len(line) || start >= end { + return evidenceOperational, "unclassifiable" + } + + quoteStart, quoteEnd, inString := stringLiteralAt(line, start, end) + + // An import names a library the codebase actually depends on, so it is + // evidence of use even though the module path looks like a file path. + // This is checked before the path rule for exactly that reason. + if isImportLine(line) { + return evidenceOperational, "import" + } + + // A crypto call whose result is logged is still a crypto call, so the log + // test only applies when the match is inside the logged string itself. + if inString && isLoggingCall(line[:quoteStart]) { + return evidenceNarrative, "log-message" + } + + if commentStartsAt(line) >= 0 && commentStartsAt(line) < start { + return evidenceNarrative, "comment" + } + + if isPathLikeToken(tokenAround(line, start, end)) { + return evidenceNarrative, "url-or-path" + } + + if reason, ok := operationalUse(line, start, end, quoteStart, inString); ok { + return evidenceOperational, reason + } + + // A comparison against an algorithm name is code that handles the + // algorithm, not code that uses it: `if alg == "RSA"` tells you the + // program can recognise RSA, not that it encrypts with it. + if inString && isComparisonOperand(line, quoteStart, quoteEnd) { + return evidenceNarrative, "comparison" + } + + // Prose: an algorithm named inside a sentence rather than used. Three or + // more whitespace-separated words is the threshold, which keeps single + // tokens such as "RC4", "aes-256-gcm" and "AES/GCM/NoPadding" operational + // while catching "publicKey must be a valid Ed25519 public key". + if inString && wordCount(line[quoteStart+1:quoteEnd]) >= 3 { + return evidenceNarrative, "prose" + } + + if key, ok := precedingKey(line, start); ok { + return evidenceNarrative, "key-value:" + key + } + + // Prose outside a string literal: body text in HTML and Markdown, where + // the sentence is the line rather than a quoted argument. + if !inString && isProseLine(line) { + return evidenceNarrative, "prose-line" + } + + return evidenceOperational, "default" +} + +// operationalUse reports whether the token at [start,end) participates in a +// cryptographic operation. +func operationalUse(line string, start, end, quoteStart int, inString bool) (string, bool) { + // A call: md5(...), MD5(...), or the whole matched span ending in a call + // such as DES.encrypt(...). + if next := skipSpaces(line, end); next < len(line) && line[next] == '(' { + return "call", true + } + + // The matched span is itself a call. Several patterns match a whole + // construct rather than a bare token, for example + // `crypto.createCipheriv('des` or `Cipher.getInstance("DES")`. Those spans + // straddle the opening quote, so they are not "inside" a string literal and + // the factory rule below cannot see them. + if strings.Contains(line[start:end], "(") { + return "call-in-match", true + } + + // A member access or member call on the matched token: md5.New(), + // des.NewCipher(k), CryptoJS.MD5.encrypt(d). + if end < len(line) && line[end] == '.' && end+1 < len(line) && isIdentChar(line[end+1]) { + return "member-call", true + } + + // The token is a member of something: hashlib.md5, a.md5, CryptoJS.MD5. + // Requires an identifier before the dot so that ".md5" inside a filename + // or a sentence does not qualify. + if start > 0 && line[start-1] == '.' && start >= 2 && isIdentChar(line[start-2]) { + return "member-access", true + } + + // An algorithm name passed to a crypto factory: + // Cipher.getInstance("RC4"), crypto.createHash('md5'), + // crypto.createCipheriv('aes-256-gcm', key, iv). + if inString && isCryptoFactoryCall(line[:quoteStart]) { + return "factory-argument", true + } + + return "", false +} + +// loggingCallees are the call prefixes whose string arguments are program +// output rather than program behaviour. +var loggingCallees = []string{ + "log.", "logger.", "logging.", "logrus.", "slog.", "zap.", + "console.log", "console.error", "console.warn", "console.info", "console.debug", + "fmt.print", "fmt.sprint", "fmt.fprint", "fmt.errorf", + "print(", "println(", "puts ", "echo ", + "errors.new", "fmt.errorf", +} + +// isLoggingCall reports whether prefix, the text preceding a string literal on +// the same line, ends in a call to a logging, printing or error-construction +// function. +func isLoggingCall(prefix string) bool { + lower := strings.ToLower(prefix) + // Only the tail matters: the call that owns this string literal is the + // nearest one to its left. Anything further away is unrelated code on the + // same line. + if len(lower) > 64 { + lower = lower[len(lower)-64:] + } + for _, callee := range loggingCallees { + if strings.Contains(lower, callee) { + return true + } + } + return false +} + +// cryptoFactoryCallees are call prefixes whose string argument names the +// algorithm being instantiated. A match inside one of these is a use, not a +// mention. +var cryptoFactoryCallees = []string{ + "getinstance(", "createcipher", "createdecipher", "createhash(", "createhmac(", + "createsign(", "createverify(", "creatediffiehellman(", + "messagedigest", "keypairgenerator", "keygenerator", "keyfactory", + "secretkeyspec", "ivparameterspec", "mac.getinstance", "signature.getinstance", + "new cipher", "cipher(", "digest(", "hmac(", "hashlib.new(", "hmac.new(", + "algorithms.", "crypto.subtle", "subtle.digest(", "subtle.importkey(", + "evp_get_cipherbyname(", "openssl_encrypt(", "openssl_digest(", +} + +// isCryptoFactoryCall reports whether prefix, the text preceding a string +// literal, ends in a call that takes an algorithm name as an argument. +func isCryptoFactoryCall(prefix string) bool { + lower := strings.ToLower(prefix) + if len(lower) > 64 { + lower = lower[len(lower)-64:] + } + for _, callee := range cryptoFactoryCallees { + if strings.Contains(lower, callee) { + return true + } + } + return false +} + +// narrativeKeys are field names whose value is descriptive text or declared +// configuration data rather than an executed operation. +// +// The key has to sit immediately before the match and be followed by ':' or +// ',', so this recognises `"algorithm": "RSA-2048"`, +// `description: "uses SHA-256"` and `c.Locals("authenticated_via", "ed25519")` +// without matching a line that merely contains the word somewhere. +var narrativeKeys = []string{ + // Documentation and presentation. + "description", "summary", "title", "label", "placeholder", "tooltip", + "help", "note", "remarks", "usage", "example", "comment", "display_name", + // Declared configuration data. + // "type" and "method" are deliberately absent: they are too generic, and + // their values (`type: 'MLDSA65-ECDSA-P256'`) are exactly the algorithm + // inventory this tool exists to collect. + "algorithm", "algorithms", "cipher", "ciphers", "hash", "digest", + "key_type", "crypto_type", "auth_method", + "authenticated_via", "authentication_type", "signing_algorithm", + "encryption_algorithm", "algorithm_name", "supported_algorithms", + "allowed_algorithms", "preferred_algorithm", + // Test data. + "test_vector", "test_data", "test_case", "test_input", "test_output", + "expected_hash", "expected_signature", +} + +// precedingKey reports whether the match at start is the value of a narrative +// key, returning the key that matched. +func precedingKey(line string, start int) (string, bool) { + prefix := strings.ToLower(line[:start]) + for _, key := range narrativeKeys { + idx := strings.LastIndex(prefix, key) + if idx == -1 { + continue + } + // Whole word only: "algorithm" must not match inside "myalgorithms". + if idx > 0 && isIdentChar(prefix[idx-1]) { + continue + } + rest := prefix[idx+len(key):] + // Allow a closing quote and whitespace between the key and its + // separator, covering `"algorithm": x` and `'algorithm' : x`. + rest = strings.TrimLeft(rest, "\"' \t") + if strings.HasPrefix(rest, ":") || strings.HasPrefix(rest, ",") || + strings.HasPrefix(rest, "=>") { + return key, true + } + } + return "", false +} + +// isComparisonOperand reports whether the string literal at [quoteStart, +// quoteEnd] is an operand of an equality comparison. +func isComparisonOperand(line string, quoteStart, quoteEnd int) bool { + before := strings.TrimRight(line[:quoteStart], " \t") + for _, op := range []string{"==", "!=", "===", "!==", "<>"} { + if strings.HasSuffix(before, op) { + return true + } + } + for _, call := range []string{".equals(", ".equalsignorecase(", "strcmp(", "strcasecmp(", "string.compare(", ".startswith(", ".endswith("} { + if strings.HasSuffix(strings.ToLower(before), call) { + return true + } + } + + after := strings.TrimLeft(line[quoteEnd+1:], " \t") + for _, op := range []string{"==", "!=", "===", "!==", "<>"} { + if strings.HasPrefix(after, op) { + return true + } + } + return false +} + +// isProseLine reports whether a line is natural-language text rather than code. +// +// Body text in HTML and Markdown puts the algorithm name in a sentence with no +// enclosing string literal, so the quoted-prose rule cannot see it. The test is +// word count against code-punctuation density: a sentence has many words and +// few braces, semicolons and operators. +func isProseLine(line string) bool { + stripped := stripMarkup(line) + if wordCount(stripped) < 6 { + return false + } + + code := 0 + for i := 0; i < len(stripped); i++ { + switch stripped[i] { + case '{', '}', ';', '=', '[', ']', '|', '&', '(', ')': + code++ + } + } + return code <= 2 +} + +// stripMarkup removes HTML tags so that the words of a sentence can be counted +// without the surrounding markup. +func stripMarkup(line string) string { + var b strings.Builder + depth := 0 + for i := 0; i < len(line); i++ { + switch line[i] { + case '<': + depth++ + case '>': + if depth > 0 { + depth-- + continue + } + b.WriteByte(line[i]) + default: + if depth == 0 { + b.WriteByte(line[i]) + } + } + } + return b.String() +} + +// isImportLine reports whether a line brings a module into scope. +// +// A Go import block lists bare quoted paths one per line, so a line whose only +// content is a quoted string is treated as an import entry. Without this, +// `"golang.org/x/crypto/bcrypt"` would be classified as a URL and the +// dependency would go unreported. +func isImportLine(line string) bool { + trimmed := strings.TrimSpace(line) + lower := strings.ToLower(trimmed) + + for _, prefix := range []string{"import ", "import(", "import\t", "from ", "#include", "using ", "require ", "use "} { + if strings.HasPrefix(lower, prefix) { + return true + } + } + if strings.Contains(lower, "require(") || strings.Contains(lower, "import(") { + return true + } + + // A bare quoted path on its own line: a Go import-block entry, optionally + // with an alias or a trailing comma. + body := strings.TrimSuffix(strings.TrimSpace(trimmed), ",") + if i := strings.IndexAny(body, `"`+"`"); i != -1 { + head := strings.TrimSpace(body[:i]) + if head == "" || head == "_" || head == "." || isIdentifier(head) { + rest := body[i:] + if len(rest) >= 2 && (rest[0] == '"' || rest[0] == '`') && + rest[len(rest)-1] == rest[0] && + !strings.Contains(rest[1:len(rest)-1], string(rest[0])) { + return true + } + } + } + return false +} + +// commentStartsAt returns the offset where a line comment begins, or -1. Only +// comment markers outside string literals count, so a "//" inside a URL string +// does not turn the rest of the line into a comment. +func commentStartsAt(line string) int { + inQuote := byte(0) + for i := 0; i < len(line); i++ { + c := line[i] + if inQuote != 0 { + if c == inQuote && (i == 0 || line[i-1] != '\\') { + inQuote = 0 + } + continue + } + switch { + case c == '"' || c == '\'' || c == '`': + inQuote = c + case c == '/' && i+1 < len(line) && line[i+1] == '/': + return i + case c == '/' && i+1 < len(line) && line[i+1] == '*': + return i + case c == '#': + return i + case c == '-' && i+1 < len(line) && line[i+1] == '-': + return i + } + } + // A javadoc or block-comment continuation line: leading "*" before any code. + if trimmed := strings.TrimSpace(line); strings.HasPrefix(trimmed, "*") { + return strings.Index(line, "*") + } + return -1 +} + +func isIdentifier(s string) bool { + if s == "" { + return false + } + for i := 0; i < len(s); i++ { + if !isIdentChar(s[i]) { + return false + } + } + return true +} + +// documentExtensions are file suffixes whose contents are prose about code +// rather than code. +var documentExtensions = map[string]bool{ + "md": true, "markdown": true, "html": true, "htm": true, "rst": true, + "txt": true, "adoc": true, "pdf": true, +} + +// isPathLikeToken reports whether a whitespace-delimited token is a URL or a +// file path. +// +// The extension test looks at the token's real suffix rather than doing a +// substring search, which is what stopped ".md" from matching ".md5(". +func isPathLikeToken(token string) bool { + if token == "" { + return false + } + trimmed := strings.Trim(token, `"'`+"`,;()[]{}<>") + if trimmed == "" { + return false + } + lower := strings.ToLower(trimmed) + + if strings.Contains(lower, "://") { + return true + } + + // A path has a separator and either an absolute/relative prefix or a host + // segment. "crypto/rsa" is a Go import path, not a document, and is + // classified by its own Library Import category instead. + if strings.Contains(lower, "/") && !strings.Contains(lower, "(") { + if strings.HasPrefix(lower, "/") || strings.HasPrefix(lower, "./") || + strings.HasPrefix(lower, "../") { + return true + } + if head := lower[:strings.Index(lower, "/")]; strings.Contains(head, ".") { + return true + } + } + + // A bare document filename such as README.md or api.html. + if dot := strings.LastIndex(lower, "."); dot != -1 && dot+1 < len(lower) { + if documentExtensions[lower[dot+1:]] { + return true + } + } + + return false +} + +// tokenAround returns the whitespace-delimited token containing [start,end). +func tokenAround(line string, start, end int) string { + left := start + for left > 0 && !isSpaceByte(line[left-1]) { + left-- + } + right := end + for right < len(line) && !isSpaceByte(line[right]) { + right++ + } + return line[left:right] +} + +// stringLiteralAt reports whether [start,end) falls inside a quoted string on +// this line, returning the offsets of the opening and closing quotes. +// +// This is a single-line scan: it cannot see multi-line strings, and it treats +// an unterminated quote as not-a-string so that an apostrophe in a comment does +// not swallow the rest of the line. +func stringLiteralAt(line string, start, end int) (quoteStart, quoteEnd int, ok bool) { + for i := 0; i < len(line); i++ { + c := line[i] + if c != '"' && c != '\'' && c != '`' { + continue + } + // Skip an escaped quote. + if i > 0 && line[i-1] == '\\' { + continue + } + closing := -1 + for j := i + 1; j < len(line); j++ { + if line[j] == c && line[j-1] != '\\' { + closing = j + break + } + } + if closing == -1 { + return 0, 0, false + } + if start > i && end <= closing { + return i, closing, true + } + i = closing + } + return 0, 0, false +} + +// wordCount counts whitespace-separated words in s. +func wordCount(s string) int { + return len(strings.Fields(s)) +} + +func skipSpaces(line string, i int) int { + for i < len(line) && (line[i] == ' ' || line[i] == '\t') { + i++ + } + return i +} + +func isIdentChar(c byte) bool { + return c == '_' || + (c >= 'a' && c <= 'z') || + (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') +} + +func isSpaceByte(c byte) bool { + return c == ' ' || c == '\t' +} diff --git a/pkg/scanner/evidence_test.go b/pkg/scanner/evidence_test.go new file mode 100644 index 0000000..6ffdda5 --- /dev/null +++ b/pkg/scanner/evidence_test.go @@ -0,0 +1,203 @@ +// Copyright 2025 CyberSecurity NonProfit (CSNP) +// SPDX-License-Identifier: Apache-2.0 + +package scanner + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/csnp/cryptoscan/pkg/types" +) + +// TestOperationalCryptoIsNeverSuppressed covers the detection-suppression +// defects found by the v1.4.0 release test. Each line here is a real way to +// write the algorithm, and each produced zero findings before the evidence +// classifier replaced the substring blocklist: +// +// a.md5(b) dropped by the ".md" entry in urlPatterns +// hashlib.md5(b"a") dropped by ".md" +// cipher = md5(b"a") dropped by "cipher =" +// const x = crypto.createHash dropped by "const " +// +// The fixtures are scanned end to end rather than unit-tested against the +// classifier, so a regression anywhere in the filter chain fails the test. +func TestOperationalCryptoIsNeverSuppressed(t *testing.T) { + cases := []struct { + name string + file string + line string + }{ + {"python bare call", "a.py", `md5(b)`}, + {"python method call", "a.py", `a.md5(b)`}, + {"python assigned method call", "a.py", `x = a.md5(b)`}, + {"python hashlib", "a.py", `hashlib.md5(b"a")`}, + {"python hashlib returned", "a.py", `return hashlib.md5(b"data").hexdigest()`}, + {"python variable named cipher", "a.py", `cipher = md5(b"a")`}, + {"go des constructor", "a.go", `block, err := des.NewCipher(key)`}, + {"go triple des constructor", "a.go", `block, err := des.NewTripleDESCipher(key)`}, + {"js let binding", "a.js", `let x = crypto.createHash('md5')`}, + {"js const binding", "a.js", `const x = crypto.createHash('md5')`}, + {"js const cipher binding", "a.js", `const cipher = crypto.createCipheriv('des-ede3-cbc', k, iv)`}, + {"js library member call", "a.js", `const rc4 = CryptoJS.RC4.encrypt(data, key)`}, + {"java anonymous variable", "a.java", `Cipher q = Cipher.getInstance("RC4");`}, + {"java conventional variable", "a.java", `Cipher cipher = Cipher.getInstance("RC4");`}, + {"java des instance", "a.java", `Cipher cipher = Cipher.getInstance("DES");`}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + n := scanLine(t, tc.file, tc.line) + if n == 0 { + t.Errorf("operational crypto was suppressed: %q produced 0 findings, want at least 1", tc.line) + } + }) + } +} + +// TestNarrativeMentionsAreStillWithheld guards the other direction. If the +// classifier reported everything, the fix would trade false negatives for an +// unusable amount of noise, so the suppression that earns its place has to +// keep working. +func TestNarrativeMentionsAreStillWithheld(t *testing.T) { + cases := []struct { + name string + file string + line string + }{ + {"log message", "a.go", `log.Printf("falling back to md5 for legacy peers")`}, + {"error string", "a.go", `return errors.New("publicKey must be a valid Ed25519 public key")`}, + {"json config key", "a.json", ` "algorithm": "RSA-2048",`}, + {"yaml config key", "a.yaml", ` algorithm: RSA-2048`}, + {"doc key", "a.yaml", ` description: uses SHA-1 for backwards compatibility`}, + {"markdown link", "a.go", `// see docs/legacy-md5.md for the migration plan`}, + {"url", "a.go", `const ref = "https://example.test/wiki/RC4_considered_harmful"`}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if n := scanLine(t, tc.file, tc.line); n != 0 { + t.Errorf("narrative mention was reported: %q produced %d findings, want 0", tc.line, n) + } + }) + } +} + +// TestNarrativeSuppressionIsCountedAndRecoverable asserts the two properties +// that separate a defensible noise filter from a silent false negative: the +// user is told how many mentions were withheld, and a flag brings them back. +func TestNarrativeSuppressionIsCountedAndRecoverable(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "a.go"), `package main +func main() { + log.Printf("falling back to md5 for legacy peers") +} +`) + + withheld := scanDir(t, dir, false) + if withheld.Summary.NarrativeSuppressed == 0 { + t.Error("narrative suppression was not counted; a withheld finding must be visible in the summary") + } + if len(withheld.Findings) != 0 { + t.Errorf("got %d findings by default, want 0", len(withheld.Findings)) + } + + shown := scanDir(t, dir, true) + if len(shown.Findings) == 0 { + t.Error("--include-narrative did not recover the withheld finding") + } +} + +// TestIgnoreCommentAppliesToItsOwnLineOnly covers the off-by-one where a +// trailing cryptoscan:ignore also blinded the following line. Line 3 is not +// annotated and must be reported. +func TestIgnoreCommentAppliesToItsOwnLineOnly(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "a.py"), `import hashlib +A = hashlib.sha1(b"1") # cryptoscan:ignore +B = hashlib.sha1(b"2") +C = hashlib.sha1(b"3") +`) + + results := scanDir(t, dir, false) + + lines := map[int]bool{} + for _, f := range results.Findings { + lines[f.Line] = true + } + if lines[2] { + t.Error("line 2 carries cryptoscan:ignore and must be suppressed") + } + for _, want := range []int{3, 4} { + if !lines[want] { + t.Errorf("line %d has no ignore directive but was suppressed; got findings on lines %v", want, lines) + } + } +} + +// TestIgnoreNextLineStillWorks confirms the explicit next-line form was not +// broken by the fix above. +func TestIgnoreNextLineStillWorks(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "a.py"), `import hashlib +# cryptoscan:ignore-next-line +A = hashlib.sha1(b"1") +B = hashlib.sha1(b"2") +`) + + results := scanDir(t, dir, false) + for _, f := range results.Findings { + if f.Line == 3 { + t.Error("line 3 is covered by cryptoscan:ignore-next-line and must be suppressed") + } + } + if len(results.Findings) == 0 { + t.Error("line 4 is not covered by the directive and must still be reported") + } +} + +// TestClassifyMatchOutOfRangeIsReported guards the fail-open contract: an +// offset the classifier cannot interpret must not drop the finding. +func TestClassifyMatchOutOfRange(t *testing.T) { + for _, tc := range []struct{ start, end int }{{-1, 3}, {0, 999}, {5, 2}} { + if class, _ := classifyMatch("md5(b)", tc.start, tc.end); class != evidenceOperational { + t.Errorf("classifyMatch(%d,%d) suppressed the finding; unclassifiable matches must be reported", tc.start, tc.end) + } + } +} + +// scanLine writes a single source line into a fresh directory and returns the +// number of findings a default scan reports for it. +func scanLine(t *testing.T, name, line string) int { + t.Helper() + dir := t.TempDir() + write(t, filepath.Join(dir, name), line+"\n") + return len(scanDir(t, dir, false).Findings) +} + +func scanDir(t *testing.T, dir string, includeNarrative bool) *Results { + t.Helper() + results, err := New(Config{ + Target: dir, + MinSeverity: types.SeverityInfo, + IncludeNarrative: includeNarrative, + }).Scan() + if err != nil { + t.Fatalf("scan %s: %v", dir, err) + } + return results +} + +func write(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write %s: %v", path, err) + } + // Guard against a vacuously green test: the fixture has to contain the + // algorithm token at all, or the assertions below prove nothing. + if !strings.ContainsAny(strings.ToLower(content), "0123456789abcdefghijklmnopqrstuvwxyz") { + t.Fatalf("fixture %s is empty", path) + } +} diff --git a/pkg/scanner/scanner.go b/pkg/scanner/scanner.go index da8e663..1849a3a 100644 --- a/pkg/scanner/scanner.go +++ b/pkg/scanner/scanner.go @@ -20,6 +20,7 @@ import ( "github.com/csnp/cryptoscan/pkg/config" "github.com/csnp/cryptoscan/pkg/patterns" "github.com/csnp/cryptoscan/pkg/types" + "github.com/csnp/cryptoscan/pkg/version" ) // Re-export types for convenience @@ -60,6 +61,7 @@ type Config struct { IncludeDocs bool // Whether to include documentation files IncludeImports bool // Whether to include library import findings (low-value, default false) IncludeQuantumSafe bool // Whether to include quantum-safe findings like SHA-256, AES-256 (default false) + IncludeNarrative bool // Whether to include algorithms named in prose, logs, docs and config keys (default false) OnFinding func(Finding) // Callback when a finding is discovered (for streaming output) OnFileScanned func(path string) // Callback when a file is scanned (for progress) @@ -69,8 +71,19 @@ type Config struct { IgnoreFiles []string // File patterns to ignore (e.g., "vendor/*", "test/*") } +// ToolInfo identifies the scanner build that produced a set of results. +// +// JSON reports previously carried no tool identity at all, so a stored report +// could not be attributed to a scanner version. The value comes from +// pkg/version, the same source the version command, SARIF and CBOM read. +type ToolInfo struct { + Name string `json:"name"` + Version string `json:"version"` +} + // Results contains all scan results type Results struct { + Tool ToolInfo `json:"tool"` Findings []Finding `json:"findings"` Summary Summary `json:"summary"` MigrationScore *types.MigrationScore `json:"migrationScore,omitempty"` @@ -108,6 +121,11 @@ type Summary struct { QuantumVulnCount int `json:"quantumVulnerableCount"` HighConfidence int `json:"highConfidenceCount"` ActionableCount int `json:"actionableCount"` // High confidence + code files + + // NarrativeSuppressed counts matches withheld because the algorithm was + // named in text rather than used. Reported so that the reduction is + // visible and recoverable with --include-narrative. + NarrativeSuppressed int `json:"narrativeSuppressedCount"` } // HasCritical returns true if any critical findings exist @@ -138,6 +156,11 @@ type Scanner struct { bytesScanned int64 languageStats map[string]int } + + // narrativeSuppressed counts matches withheld because the algorithm was + // named in prose, a log message, a documentation or configuration key, or + // a URL. It is reported so that a suppression is never invisible. + narrativeSuppressed int } // New creates a new Scanner instance @@ -198,9 +221,24 @@ func (s *Scanner) Scan() (*Results, error) { // Deduplicate findings (same file+line+category = keep highest priority) s.findings = s.deduplicateFindings(s.findings) - // Sort findings by priority - sort.Slice(s.findings, func(i, j int) bool { - return s.findings[i].Priority() > s.findings[j].Priority() + // Sort findings by priority, breaking ties on location so that the order + // is fully determined by the scanned content rather than by the order + // files happened to be walked. + sort.SliceStable(s.findings, func(i, j int) bool { + a, b := s.findings[i], s.findings[j] + if a.Priority() != b.Priority() { + return a.Priority() > b.Priority() + } + if a.File != b.File { + return a.File < b.File + } + if a.Line != b.Line { + return a.Line < b.Line + } + if a.Column != b.Column { + return a.Column < b.Column + } + return a.ID < b.ID }) // Calculate migration score and enhance findings with QRAMM mapping @@ -208,6 +246,7 @@ func (s *Scanner) Scan() (*Results, error) { // Build results results := &Results{ + Tool: ToolInfo{Name: "CryptoScan", Version: version.Get()}, Findings: s.findings, MigrationScore: migrationScore, FilesScanned: s.stats.filesScanned, @@ -551,7 +590,7 @@ func (s *Scanner) scanFile(path string) error { } // Apply noise reduction filters - if !s.shouldIncludeFinding(m) { + if !s.shouldIncludeFinding(m, line) { continue } @@ -626,23 +665,31 @@ func (s *Scanner) hasIgnoreComment(line string, allLines []string, lineNum int) } } - // Check previous line for ignore directive + // Check the previous line, but only for the explicit next-line form. + // + // This used to accept any ignore directive on the previous line, so a + // trailing `# cryptoscan:ignore` also blinded the line below it. One + // suppression comment silently hid an adjacent line the author never asked + // to hide, which in a security scanner is a false negative the user cannot + // see. A directive now applies to its own line unless it says -next-line. if lineNum >= 2 && lineNum-2 < len(allLines) { prevLine := allLines[lineNum-2] - prevLower := strings.ToLower(prevLine) - if strings.Contains(prevLower, "cryptoscan:ignore") || - strings.Contains(prevLower, "crypto-scan:ignore") || - strings.Contains(prevLower, "cryptoscan:ignore-next-line") { - // Check if it's a blanket ignore - if s.isBlanketIgnore(prevLine) { - return true - } + if isNextLineDirective(prevLine) && s.isBlanketIgnore(prevLine) { + return true } } return false } +// isNextLineDirective reports whether a line carries an ignore directive that +// explicitly applies to the following line. +func isNextLineDirective(line string) bool { + lower := strings.ToLower(line) + return strings.Contains(lower, "cryptoscan:ignore-next-line") || + strings.Contains(lower, "crypto-scan:ignore-next-line") +} + // isBlanketIgnore checks if an ignore directive is a blanket ignore (no specific pattern) func (s *Scanner) isBlanketIgnore(line string) bool { lowerLine := strings.ToLower(line) @@ -682,13 +729,17 @@ func (s *Scanner) hasPatternSpecificIgnore(line string, allLines []string, lineN } } - // Check previous line + // Check the previous line, but only for the explicit next-line form, for + // the same reason as hasIgnoreComment: an ignore directive applies to the + // line it sits on unless it says otherwise. if lineNum >= 2 && lineNum-2 < len(allLines) { prevLine := allLines[lineNum-2] - if patterns := s.extractIgnorePatterns(prevLine); len(patterns) > 0 { - for _, p := range patterns { - if config.MatchesPattern(patternID, p) { - return true + if isNextLineDirective(prevLine) { + if patterns := s.extractIgnorePatterns(prevLine); len(patterns) > 0 { + for _, p := range patterns { + if config.MatchesPattern(patternID, p) { + return true + } } } } @@ -845,9 +896,12 @@ func (s *Scanner) meetsConfidenceThreshold(conf types.Confidence) bool { } } -// shouldIncludeFinding applies noise reduction filters -// Returns false if the finding should be suppressed based on config -func (s *Scanner) shouldIncludeFinding(f types.Finding) bool { +// shouldIncludeFinding applies noise reduction filters. +// Returns false if the finding should be suppressed based on config. +// +// line is the full source line the match came from; f.Context is truncated for +// display and must not be used for classification. +func (s *Scanner) shouldIncludeFinding(f types.Finding, line string) bool { // Skip library import findings unless explicitly included // These are low-value (just import statements, not actual crypto usage) if !s.config.IncludeImports && f.Category == "Library Import" { @@ -867,10 +921,23 @@ func (s *Scanner) shouldIncludeFinding(f types.Finding) bool { } } - // Skip findings in low-value contexts (logs, labels, error messages, docstrings) - // These mention algorithms but aren't actual cryptographic operations - if isLowValueContext(f.Context) { - return false + // Withhold matches where the algorithm is named in narrative text rather + // than used: prose, log output, a documentation or configuration key's + // value, a URL or a file path. Operational constructs are never withheld, + // however the surrounding line reads. + // + // Detected key material is exempt: a private key pasted into a comment is + // still an exposed private key, and that is the reason the scan loop reads + // comments containing "key" or "secret" in the first place. + if !s.config.IncludeNarrative && f.FindingType != types.FindingTypeSecret { + start := f.Column - 1 + end := start + len(f.Match) + if class, _ := classifyMatch(line, start, end); class == evidenceNarrative { + s.mu.Lock() + s.narrativeSuppressed++ + s.mu.Unlock() + return false + } } // Skip findings in files whose names indicate API docs or documentation @@ -903,234 +970,16 @@ func (s *Scanner) shouldIgnorePattern(id, category string) bool { return false } -// isLowValueContext checks if a line contains algorithm mentions in contexts -// that are not actual cryptographic operations (logs, labels, error messages, docstrings) -func isLowValueContext(line string) bool { - lineLower := strings.ToLower(line) - - // Log/print statements - algorithm mentioned in output, not usage - logPatterns := []string{ - "fmt.print", "fmt.sprint", "fmt.fprint", - "log.", "logger.", "logging.", - "console.log", "console.error", "console.warn", - "print(", "println(", - "debug(", "info(", "warn(", "error(", - } - for _, pattern := range logPatterns { - if strings.Contains(lineLower, pattern) { - return true - } - } - - // String labels/metadata - algorithm name as a value, not usage - // e.g., "auth_method": "ed25519", c.Locals("authenticated_via", "ed25519") - labelPatterns := []string{ - "auth_method", - "authenticated_via", - "authentication_type", - "signing_algorithm", - "encryption_algorithm", - "key_type", - "algorithm_name", - "crypto_type", - `"type":`, - `"method":`, - } - for _, pattern := range labelPatterns { - if strings.Contains(lineLower, pattern) { - return true - } - } - - // Error message strings - validation messages mentioning algorithms - // e.g., "publicKey must be a valid Ed25519 public key" - errorPatterns := []string{ - "must be a valid", - "invalid.*key", - "failed to", - "error:", - "expected.*got", - "cannot be empty", - } - for _, pattern := range errorPatterns { - if strings.Contains(lineLower, pattern) { - return true - } - } - - // Docstrings - documentation inside code - // e.g., """Sign a message using Ed25519""", // Sign using Ed25519 - docstringPatterns := []string{ - `"""`, // Python docstring - "'''", // Python docstring - "sign a message using", - "verify a message using", - "encrypt using", - "decrypt using", - "generated.*key", - } - for _, pattern := range docstringPatterns { - if strings.Contains(lineLower, pattern) { - return true - } - } - - // Common documentation patterns - docPatterns := []string{ - "description:", - "description\":", - "summary:", - "summary\":", - " - ", // Markdown list item - "* ", // Markdown list item - "fingerprint", // UI label like "Public Key Fingerprint (SHA-256)" - "hashed before", // API doc like "SHA-256 hashed before storage" - "uses sha", // Description like "Uses SHA-256 for..." - "uses aes", - "encrypted with", - "algorithm:", - // API documentation object patterns - "auth:", // API auth method description - "auth\":", // JSON style - "authentication:", // Auth description - "verification using", // API doc like "verification using Ed25519" - "signing using", // API doc about signing - "cryptographic", // Description of cryptographic features - "zero-effort", // Feature description - "attestation", // Security attestation descriptions - "capabilities", // Capability descriptions - } - for _, pattern := range docPatterns { - if strings.Contains(lineLower, pattern) { - return true - } - } - - // URL patterns - algorithm names in URLs are typically documentation/API references - urlPatterns := []string{ - "http://", "https://", - "/api/", "/v1/", "/v2/", - ".html", ".htm", ".md", - "github.com", "gitlab.com", "bitbucket.org", - "docs.", "documentation.", - "/docs/", "/wiki/", - } - for _, pattern := range urlPatterns { - if strings.Contains(lineLower, pattern) { - return true - } - } - - // Test data patterns - test vectors, example values - testDataPatterns := []string{ - "test_vector", - "test_data", - "test_case", - "test_input", - "test_output", - "example_", - "sample_", - "mock_", - "fixture", - "golden", - "expected_hash", - "expected_signature", - } - for _, pattern := range testDataPatterns { - if strings.Contains(lineLower, pattern) { - return true - } - } - - // Configuration key patterns - JSON/YAML keys mentioning algorithms - configKeyPatterns := []string{ - `"algorithm":`, - `"cipher":`, - `"hash":`, - `'algorithm':`, - `'cipher':`, - `'hash':`, - "algorithm =", - "cipher =", - "supported_algorithms", - "allowed_algorithms", - "preferred_algorithm", - } - for _, pattern := range configKeyPatterns { - if strings.Contains(lineLower, pattern) { - return true - } - } - - // Constant/enum definition patterns - naming, not usage - constPatterns := []string{ - "algorithm_", - "cipher_", - "_algorithm", - "_cipher", - "alg_", - "_alg", - "const ", - "#define ", - "enum ", - } - for _, pattern := range constPatterns { - if strings.Contains(lineLower, pattern) { - return true - } - } - - // String comparison patterns - checking algorithm names, not using them - comparisonPatterns := []string{ - "== \"rsa", - "== \"aes", - "== \"sha", - "== \"ecdsa", - "== \"ed25519", - "=== \"rsa", - "=== \"aes", - "=== \"sha", - ".equals(\"", - "strcmp(", - "strcasecmp(", - "string.compare", - } - for _, pattern := range comparisonPatterns { - if strings.Contains(lineLower, pattern) { - return true - } - } - - // UI/Display patterns - algorithm names shown to users - uiPatterns := []string{ - "label:", - "title:", - "header:", - "placeholder:", - "tooltip:", - "display_name", - "display name", - "show_algorithm", - "algorithm_display", - } - for _, pattern := range uiPatterns { - if strings.Contains(lineLower, pattern) { - return true - } - } - - return false -} - func (s *Scanner) calculateSummary() Summary { summary := Summary{ - TotalFindings: len(s.findings), - BySeverity: make(map[string]int), - ByCategory: make(map[string]int), - ByQuantumRisk: make(map[string]int), - ByConfidence: make(map[string]int), - ByFileType: make(map[string]int), - ByLanguage: make(map[string]int), + TotalFindings: len(s.findings), + NarrativeSuppressed: s.narrativeSuppressed, + BySeverity: make(map[string]int), + ByCategory: make(map[string]int), + ByQuantumRisk: make(map[string]int), + ByConfidence: make(map[string]int), + ByFileType: make(map[string]int), + ByLanguage: make(map[string]int), } for _, f := range s.findings { @@ -1272,28 +1121,32 @@ func (s *Scanner) deduplicateFindings(findings []Finding) []Finding { return findings } - // Key: file:line:category -> best finding + // Key: file:line:category -> best finding. + // + // The result used to be built by ranging over this map, and Go randomises + // map iteration order, so two scans of an unchanged tree emitted the same + // findings in a different order every time. That breaks diffable CI output, + // golden-file tests and reproducible CBOMs, and it made the text report's + // "#N" numbering meaningless. Insertion order is recorded separately and + // used to rebuild the slice. seen := make(map[string]Finding) + order := make([]string, 0, len(findings)) for _, f := range findings { - // Create a key for grouping similar findings key := fmt.Sprintf("%s:%d:%s", f.File, f.Line, f.Category) existing, exists := seen[key] if !exists { seen[key] = f - } else { - // Keep the higher priority finding - if f.Priority() > existing.Priority() { - seen[key] = f - } + order = append(order, key) + } else if f.Priority() > existing.Priority() { + seen[key] = f } } - // Convert map back to slice - deduped := make([]Finding, 0, len(seen)) - for _, f := range seen { - deduped = append(deduped, f) + deduped := make([]Finding, 0, len(order)) + for _, key := range order { + deduped = append(deduped, seen[key]) } return deduped diff --git a/pkg/version/version.go b/pkg/version/version.go new file mode 100644 index 0000000..c01f192 --- /dev/null +++ b/pkg/version/version.go @@ -0,0 +1,29 @@ +// Copyright 2025 CyberSecurity NonProfit (CSNP) +// SPDX-License-Identifier: Apache-2.0 + +// Package version holds the single source of truth for the CryptoScan version +// string. The release build injects the value into main.version via GoReleaser +// ldflags; cmd/cryptoscan calls Set exactly once at startup, and every surface +// that reports a version (the version command, SARIF, CBOM, JSON) reads it from +// here. +// +// Emitting a version literal anywhere else produces a false provenance record: +// SARIF and CBOM are consumed as evidence of which scanner produced a result. +package version + +// value is the process-wide version string. It is written once from +// cmd/cryptoscan before any command runs and read-only afterwards. +var value = "dev" + +// Set records the version string for the running binary. It is called once from +// cmd/cryptoscan/main.go with the value injected at build time. +func Set(v string) { + if v != "" { + value = v + } +} + +// Get returns the version string reported by every CryptoScan output surface. +func Get() string { + return value +} From 572f8d7f9e0288cc9ced08b4b333c710652c2056 Mon Sep 17 00:00:00 2001 From: Abdel Fane Date: Mon, 27 Jul 2026 17:50:44 -0600 Subject: [PATCH 2/5] Clean up scan output for non-terminal use and CSNP style 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. --- .gitignore | 7 ++ docs/testing/release-smoke.md | 161 ++++++++++++++++++-------- internal/cli/scan.go | 61 +++++++--- pkg/reporter/text.go | 8 +- pkg/scanner/evidence_property_test.go | 74 ++++++++++++ 5 files changed, 246 insertions(+), 65 deletions(-) create mode 100644 pkg/scanner/evidence_property_test.go diff --git a/.gitignore b/.gitignore index 9401f81..f2048d7 100644 --- a/.gitignore +++ b/.gitignore @@ -40,6 +40,13 @@ build/ .env.local config.local.yaml +# Credentials and key material +secrets.json +*.pem +*.key +*.p12 +*.pfx + # AI assistant files CLAUDE.md .cursorrules diff --git a/docs/testing/release-smoke.md b/docs/testing/release-smoke.md index e8a722c..f15e6cf 100644 --- a/docs/testing/release-smoke.md +++ b/docs/testing/release-smoke.md @@ -1,54 +1,121 @@ -# Release Smoke Test: cryptoscan - -Manual pre-release walkthrough. Run this before every deploy or publish. -Use real keyboard and mouse input, not synthetic events. Record the actual -output, not a summary. - -## 1. Build and start clean - -- [ ] Fresh install succeeds (`npm ci` or equivalent) with no errors. -- [ ] Build succeeds (`npm run build`). -- [ ] App starts locally (`npm run dev`) with no console errors. - -## 2. Core user paths - -List the three most important things a real visitor does on this site or -with this tool, then walk each one by hand. - -- [ ] Path 1: -- [ ] Path 2: -- [ ] Path 3: - -## 3. Accessibility and audience fit - -CSNP serves non-technical and vulnerable users. Verify the experience holds -for them. - -- [ ] Plain language: no unexplained jargon in user-facing copy. -- [ ] Keyboard navigable: every interactive element reachable by Tab. -- [ ] Screen-reader labels present on buttons, links, and form fields. -- [ ] Color contrast meets WCAG AA. - -## 4. Responsive parity - -- [ ] Mobile (375px): nav, menus, and layout all work. -- [ ] Desktop (1280px): no overflow or broken grids. -- [ ] Dark mode (if supported) renders correctly. - -## 5. Data and links - -- [ ] Every download link resolves (no 404s). -- [ ] Any displayed number or statistic traces to a real source. -- [ ] No placeholder or fabricated content shipped. - -## 6. Security - -- [ ] No secrets in the bundle or committed files. +# Release smoke test: cryptoscan + +Manual pre-release walkthrough. Run this before every tag push. Record the +actual output, not a summary. CryptoScan is a Go CLI, so build the +CI-equivalent binary rather than a plain `go build`: the release injects the +version, and a plain build carries the `dev` default, which hides every +version-provenance defect. + +## 1. Build the artifact the release will ship + +```bash +go build -ldflags "-X main.version=" -o /tmp/cryptoscan ./cmd/cryptoscan +/tmp/cryptoscan version +``` + +- [ ] `go build ./...` succeeds. +- [ ] `go test -race ./...` passes. +- [ ] `go vet ./...` is clean. +- [ ] `cryptoscan version` prints the version being released, not `dev`. +- [ ] `main.version` is a `var`, not a `const` (a `const` silently ignores `-X`). + +## 2. Detection: operational crypto is reported + +Every line below must produce at least one finding. Each one reported zero +findings at some point in the tool's history, because a noise filter matched a +substring elsewhere on the line. + +```bash +d=$(mktemp -d) +printf 'a.md5(b)\n' > $d/a.py +printf 'return hashlib.md5(data).hexdigest()\n' > $d/b.py +printf 'cipher = md5(b"a")\n' > $d/c.py +printf 'block, err := des.NewCipher(key)\n' > $d/d.go +printf "const x = crypto.createHash('md5')\n" > $d/e.js +printf 'const rc4 = CryptoJS.RC4.encrypt(data, key)\n' > $d/f.js +printf 'Cipher cipher = Cipher.getInstance("RC4");\n' > $d/g.java +/tmp/cryptoscan scan $d --format json | python3 -c 'import sys,json;print(len(json.load(sys.stdin)["findings"]),"findings")' +``` + +- [ ] At least 7 findings (one per file). Zero for any file is a release blocker. +- [ ] Scan the repo's own `crypto-samples/` tree: the deliberately vulnerable + samples are reported, including the DES, 3DES, RC4 and MD5 cases. + +## 3. Detection: mentions in text are withheld, visibly and recoverably + +- [ ] A log line (`log.Printf("falling back to md5")`) produces no finding. +- [ ] The summary states how many mentions were withheld. +- [ ] `--include-narrative` brings them back. +- [ ] Detected key material is never withheld, even inside a comment. + +## 4. Suppression comments + +```bash +printf 'import hashlib\nA=hashlib.sha1(b"1") # cryptoscan:ignore\nB=hashlib.sha1(b"2")\nC=hashlib.sha1(b"3")\n' > $d/ig.py +``` + +- [ ] Findings are reported on lines 3 and 4 only. Line 2 is suppressed and no + other line is affected by it. +- [ ] `# cryptoscan:ignore-next-line` suppresses the following line. +- [ ] `cryptoscan:ignore RSA-001` suppresses only that pattern. + +## 5. Every output surface agrees + +```bash +for f in json sarif cbom; do /tmp/cryptoscan scan $d --format $f; done +``` + +- [ ] JSON `tool.version`, SARIF `runs[0].tool.driver.version` and + `.semanticVersion`, and CBOM `metadata.tools[0].version` all equal the + output of `cryptoscan version`. +- [ ] The CBOM validates against the official CycloneDX 1.6 schema, fetched + from the CycloneDX specification repository, not against the tool's own + tests. Resolve the `jsf-0.82` and `spdx` `$ref`s and validate with + python `jsonschema`. +- [ ] SARIF result locations point at real files. + +## 6. Determinism + +```bash +for i in 1 2 3; do + /tmp/cryptoscan scan crypto-samples --format json \ + | python3 -c 'import sys,json;d=json.load(sys.stdin);print([(f["file"],f["line"],f["id"]) for f in d["findings"]])' \ + | shasum -a 256 +done +``` + +- [ ] All three hashes match. Finding order must not vary between runs of an + unchanged tree, or CI diffs and CBOMs are not reproducible. + +## 7. Output and exit codes + +- [ ] Piped (non-TTY) output contains no ANSI escape sequences. +- [ ] `--no-color` output contains no ANSI escape sequences and no emoji. +- [ ] `--help` lists every flag, and every flag it lists actually parses. +- [ ] An invalid `--format` value is rejected with a clear message and a + non-zero exit code, rather than silently falling back to text. +- [ ] Exit codes match what `--fail-on` documents. + +## 8. Findings read well + +For each finding in a real scan, check all five: + +- [ ] It names the file and line. +- [ ] It says what specifically is wrong. +- [ ] The remediation is a concrete action, not general advice. +- [ ] Severity is coherent against the other findings on the same file. +- [ ] A non-developer security manager would understand what to do. + +## 9. Security and hygiene + +- [ ] No secrets in committed files. - [ ] `.env` and credential files are gitignored and excluded. -- [ ] Forms validate and sanitize input. +- [ ] `npx hackmyagent secure --ci` reports no CRITICAL or HIGH findings, or + each one is a documented and verified false positive. ## Result +- Version: - Date: - Tester: - Verdict: PASS / FAIL diff --git a/internal/cli/scan.go b/internal/cli/scan.go index e14627d..1c1ac57 100644 --- a/internal/cli/scan.go +++ b/internal/cli/scan.go @@ -123,6 +123,16 @@ func init() { func runScan(cmd *cobra.Command, args []string) error { target := args[0] + // Disable colour when the output is not a terminal, so that redirecting to + // a file or piping into another tool produces clean text rather than ANSI + // escape sequences. An explicit --no-color still wins, and NO_COLOR is + // honoured per https://no-color.org. + if !cmd.Flags().Changed("no-color") { + if os.Getenv("NO_COLOR") != "" || !isTerminal(os.Stdout) { + noColor = true + } + } + // Load config file (explicit or auto-detected) var cfgFile *config.Config cfgPath := configFile @@ -239,7 +249,7 @@ func runScan(cmd *cobra.Command, args []string) error { findingCount++ num := findingCount // Clear the progress line before printing finding - fmt.Print("\r\033[K") + clearProgressLine() printStreamFinding(f, num, !noColor) outputMu.Unlock() } @@ -249,13 +259,16 @@ func runScan(cmd *cobra.Command, args []string) error { count := fileCount outputMu.Unlock() // Show progress every 50 files (less frequent for parallel scanning) - if count%50 == 0 { + // The progress line rewrites itself in place, which only makes + // sense on a terminal. Piped output would otherwise collect a + // carriage return and an erase-line sequence per update. + if count%50 == 0 && !noColor { shortPath := path if len(shortPath) > 50 { shortPath = "..." + shortPath[len(shortPath)-47:] } outputMu.Lock() - fmt.Printf("\r\033[K \033[2m📂 %d files scanned | %s\033[0m", count, shortPath) + fmt.Printf("\r\033[K \033[2m%d files scanned | %s\033[0m", count, shortPath) outputMu.Unlock() } } @@ -292,7 +305,7 @@ func runScan(cmd *cobra.Command, args []string) error { // Print streaming footer with summary if streamFindings && outputFormat == "text" { // Clear any remaining progress line - fmt.Print("\r\033[K") + clearProgressLine() printScanningFooter(findingCount, fileCount, duration, !noColor) } @@ -394,7 +407,7 @@ func printBanner() { fmt.Println(" ║ ║") fmt.Println(" ╚═══════════════════════════════════════════════════════════════╝" + colorReset) fmt.Println() - fmt.Println(colorBlue + " Crypto Scan — QRAMM Cryptographic Discovery" + colorReset) + fmt.Println(colorBlue + " Crypto Scan: QRAMM Cryptographic Discovery" + colorReset) fmt.Println(colorDim + " Quantum Readiness Assurance & Migration Tool" + colorReset) fmt.Println() fmt.Println(colorGreen + " ┌─────────────────────────────────────────────────────────────┐") @@ -438,10 +451,10 @@ func printScanningFooter(findingCount, fileCount int, duration time.Duration, us fmt.Println() if useColor { - fmt.Printf("%s%s ✓ Scan complete%s — %d findings in %d files (%s)\n\n", + fmt.Printf("%s%s \u2713 Scan complete%s: %d findings in %d files (%s)\n\n", colorGreen, colorBold, colorReset, findingCount, fileCount, duration.Round(time.Millisecond)) } else { - fmt.Printf(" Scan complete — %d findings in %d files (%s)\n\n", findingCount, fileCount, duration.Round(time.Millisecond)) + fmt.Printf(" Scan complete: %d findings in %d files (%s)\n\n", findingCount, fileCount, duration.Round(time.Millisecond)) } } @@ -462,24 +475,24 @@ func printStreamFinding(f scanner.Finding, num int, useColor bool) { var sevIcon, sevColor string switch f.Severity { case scanner.SeverityCritical: - sevIcon, sevColor = "🔴", colorRed+colorBold + sevIcon, sevColor = "●", colorRed+colorBold case scanner.SeverityHigh: - sevIcon, sevColor = "🟠", colorRed + sevIcon, sevColor = "●", colorRed case scanner.SeverityMedium: - sevIcon, sevColor = "🟡", colorYellow + sevIcon, sevColor = "●", colorYellow case scanner.SeverityLow: - sevIcon, sevColor = "🔵", colorBlue + sevIcon, sevColor = "●", colorBlue default: - sevIcon, sevColor = "⚪", colorCyan + sevIcon, sevColor = "●", colorCyan } // Quantum risk indicator var qIcon string switch f.Quantum { case scanner.QuantumVulnerable: - qIcon = "⚠️ " + qIcon = "[!]" case scanner.QuantumPartial: - qIcon = "⚡" + qIcon = "[~]" default: qIcon = " " } @@ -633,3 +646,23 @@ func calculateSummary(findings []scanner.Finding) scanner.Summary { return summary } + +// clearProgressLine erases the in-place progress line. It is a no-op when +// colour is off, since a redirected stream has no cursor to move. +func clearProgressLine() { + if noColor { + return + } + fmt.Print("\r\033[K") +} + +// isTerminal reports whether f is attached to a terminal. A redirected or +// piped stream is a character device only when it is a terminal, so this +// distinguishes interactive output from output being captured. +func isTerminal(f *os.File) bool { + info, err := f.Stat() + if err != nil { + return false + } + return info.Mode()&os.ModeCharDevice != 0 +} diff --git a/pkg/reporter/text.go b/pkg/reporter/text.go index 0421a36..525a953 100644 --- a/pkg/reporter/text.go +++ b/pkg/reporter/text.go @@ -63,9 +63,9 @@ func (r *TextReporter) severityColor(s scanner.Severity) string { func (r *TextReporter) quantumIcon(q scanner.QuantumRisk) string { switch q { case scanner.QuantumVulnerable: - return "⚠️ QUANTUM VULNERABLE" + return "[!] QUANTUM VULNERABLE" case scanner.QuantumPartial: - return "⚡ QUANTUM WEAKENED" + return "[~] QUANTUM WEAKENED" case scanner.QuantumSafe: return "✓ QUANTUM SAFE" default: @@ -146,7 +146,7 @@ func (r *TextReporter) Generate(results *scanner.Results) (string, error) { // Quantum Risk Assessment with visual emphasis b.WriteString(r.color(colorBold, " Quantum Risk Assessment:\n")) if results.Summary.QuantumVulnCount > 0 { - b.WriteString(r.color(colorRed+colorBold, fmt.Sprintf(" ⚠️ %d quantum-vulnerable findings require migration planning\n", results.Summary.QuantumVulnCount))) + b.WriteString(r.color(colorRed+colorBold, fmt.Sprintf(" [!] %d quantum-vulnerable findings require migration planning\n", results.Summary.QuantumVulnCount))) } quantumRisks := []struct { name string @@ -428,7 +428,7 @@ func (r *TextReporter) writeMigrationScore(b *strings.Builder, score *scanner.Mi // Status breakdown b.WriteString(r.color(colorCyan, "│") + r.color(colorBold, " INVENTORY ") + r.color(colorCyan, "│\n")) b.WriteString(r.color(colorCyan, "│") + fmt.Sprintf(" %s Safe (PQC): %-5d", r.color(colorGreen, "✓"), score.SafeCount) + fmt.Sprintf(" %s Vulnerable: %-5d", r.color(colorRed, "✗"), score.VulnerableCount) + r.color(colorCyan, " │\n")) - b.WriteString(r.color(colorCyan, "│") + fmt.Sprintf(" %s Hybrid: %-5d", r.color(colorGreen, "◐"), score.HybridCount) + fmt.Sprintf(" %s Critical: %-5d", r.color(colorRed+colorBold, "⚠"), score.CriticalCount) + r.color(colorCyan, " │\n")) + b.WriteString(r.color(colorCyan, "│") + fmt.Sprintf(" %s Hybrid: %-5d", r.color(colorGreen, "◐"), score.HybridCount) + fmt.Sprintf(" %s Critical: %-5d", r.color(colorRed+colorBold, "[!]"), score.CriticalCount) + r.color(colorCyan, " │\n")) b.WriteString(r.color(colorCyan, "│") + fmt.Sprintf(" %s Partial: %-5d", r.color(colorYellow, "◑"), score.PartialCount) + fmt.Sprintf(" Total: %-5d", score.TotalCount) + r.color(colorCyan, " │\n")) // QRAMM Readiness diff --git a/pkg/scanner/evidence_property_test.go b/pkg/scanner/evidence_property_test.go new file mode 100644 index 0000000..141e715 --- /dev/null +++ b/pkg/scanner/evidence_property_test.go @@ -0,0 +1,74 @@ +// Copyright 2025 CyberSecurity NonProfit (CSNP) +// SPDX-License-Identifier: Apache-2.0 + +package scanner + +import ( + "math/rand" + "testing" +) + +// TestClassifyMatchNeverPanics is a property check over random lines and +// random match spans. +// +// classifyMatch does a lot of manual byte indexing (line[start-1], +// line[end+1], prefix[idx-1]) because it has to reason about the characters +// immediately around a match. A panic there is a denial of scan: one +// pathological source line would abort the whole run. The alphabet is weighted +// toward the characters the classifier actually branches on, plus multi-byte +// runes, since match offsets are byte offsets. +func TestClassifyMatchNeverPanics(t *testing.T) { + alphabet := []rune{ + 'a', 'M', 'D', '5', '.', '"', '\'', '`', '/', '(', ')', ' ', '\t', + '#', '-', '=', ':', ',', '\\', '<', '>', '*', '{', '}', 'é', '中', + } + r := rand.New(rand.NewSource(1)) + + for i := 0; i < 50000; i++ { + buf := make([]rune, r.Intn(24)) + for j := range buf { + buf[j] = alphabet[r.Intn(len(alphabet))] + } + line := string(buf) + if line == "" { + continue + } + + // Deliberately includes spans that run past the end of the line and + // spans with end before start. + start := r.Intn(len(line) + 2) + end := start + r.Intn(6) + + func() { + defer func() { + if p := recover(); p != nil { + t.Fatalf("panic: line=%q start=%d end=%d: %v", line, start, end, p) + } + }() + classifyMatch(line, start, end) + }() + } +} + +// TestUnclassifiableSpansAreReported pins the fail-open contract. A span the +// classifier cannot interpret must produce a finding, never a silent drop. +func TestUnclassifiableSpansAreReported(t *testing.T) { + cases := []struct { + line string + start, end int + }{ + {"md5(b)", -1, 3}, + {"md5(b)", 0, 999}, + {"md5(b)", 5, 2}, + {"", 0, 1}, + {"md5(b)", 3, 3}, + } + for _, tc := range cases { + if class, reason := classifyMatch(tc.line, tc.start, tc.end); class != evidenceNarrative { + continue + } else { + t.Errorf("classifyMatch(%q, %d, %d) suppressed the finding (reason %q); unclassifiable spans must be reported", + tc.line, tc.start, tc.end, reason) + } + } +} From e678a2cc849583c6530472b5c4a4d641e7d20eac Mon Sep 17 00:00:00 2001 From: Abdel Fane Date: Mon, 27 Jul 2026 18:06:41 -0600 Subject: [PATCH 3/5] Ask the operational question before any noise heuristic 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. --- CHANGELOG.md | 36 +- README.md | 39 +- pkg/reporter/text.go | 2 +- pkg/scanner/evidence.go | 592 ++++++++++++++++---------- pkg/scanner/evidence_property_test.go | 4 +- pkg/scanner/evidence_test.go | 195 +++++---- pkg/scanner/scanner.go | 122 ++++-- 7 files changed, 617 insertions(+), 373 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 793397a..eddc89b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,15 +20,33 @@ All notable changes to CryptoScan are recorded here. This project follows `prefer-const` makes the idiomatic form in JavaScript. The heuristic has been replaced by 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 now always reported, whatever - else appears on the line. - -- **Suppression is now counted and recoverable.** Matches held back as prose, - log output, comments, URLs or documentation and configuration values are - reported as a count in the scan summary, and `--include-narrative` (also - covered by `--verbose`) shows them. Detected key material is never held back, - since a private key in a comment is still an exposed private key. + position of the match. The classifier asks whether the token is part of an + operation *before* it applies any noise heuristic, so a call, a member + access, an import or an argument to a crypto factory is always reported, + whatever else appears on the line. + + That ordering is the safety property, and getting it wrong is subtle. An + intermediate version of this change ran the log, comment and path rules + first, which reintroduced the same defect class through a different door: a + variable named `outputs` (containing `puts `) hid + `Cipher.getInstance("RC4")`, a `z--` decrement hid a DES cipher in Java, + `gpg --cipher-algo 3DES` read as a comment, and adding a fifth entry to an + `sshd_config` `MACs` line took the file from three findings to zero. All of + those are now regression tests. + +- **Declared cryptographic configuration is reported.** `cipher: DES-CBC`, + `MACs hmac-md5 ...`, `SSLProtocol +SSLv3` and `ssh-keygen -t rsa -b 1024` + are findings. For a cryptographic inventory a declared algorithm *is* the + inventory, and an Apache config enabling SSLv3 must fail + `--fail-on critical`. Scanning a repository that ships cryptographic + reference data will now report a finding per row; use `--exclude`. + +- **Suppression is counted and recoverable.** Findings held back as prose, log + output, comments, URLs or documentation labels are reported as a count in the + scan summary, and `--include-narrative` (also covered by `--verbose`) shows + them. The count is computed after deduplication, so it is exactly the number + of findings the flag adds. Detected key material is never held back, since a + private key in a comment is still an exposed private key. - **`cryptoscan:ignore` no longer blinds the following line.** A trailing directive suppressed both its own line and the next one. A directive now diff --git a/README.md b/README.md index 94241db..b8dfbb1 100644 --- a/README.md +++ b/README.md @@ -338,21 +338,42 @@ cryptoscan scan . --min-severity critical --format json | jq '.findings | length ### What is reported, and what is held back -CryptoScan reports an algorithm when the match is part of an operation: a call, -an instantiation, a member access, or an algorithm name passed to a crypto -factory. `hashlib.md5(data)`, `des.NewCipher(key)`, `const cipher = -crypto.createCipheriv('des-ede3-cbc', k, iv)` and -`Cipher.getInstance("RC4")` are all reported. +CryptoScan asks one question first: is the matched token part of an operation? +A call, an instantiation, a member access, an import, or an algorithm name +passed to a crypto factory is always reported, whatever else appears on the +line. So all of these produce findings: -Mentions in text are held back: prose, log messages, comments, URLs, and the -values of documentation or configuration keys. The scan summary always says how -many were held back, and `--include-narrative` shows them: +``` +hashlib.md5(data) +des.NewCipher(key) +const cipher = crypto.createCipheriv('des-ede3-cbc', k, iv) +Cipher.getInstance("RC4") +``` + +Declared configuration is also reported. For a cryptographic inventory, an +algorithm named in a config file is the inventory: + +``` +cipher: DES-CBC +MACs hmac-md5 hmac-sha1 umac-64 +SSLProtocol -all +SSLv3 +TLSv1 +ssh-keygen -t rsa -b 1024 -f id_rsa +``` + +Only mentions in *text* are held back: prose, log and error messages, comments, +URLs, and the values of documentation labels such as `description:` and +`title:`. The scan summary always says how many were held back, and the number +is exactly what the flag returns: ``` - 14 mention(s) withheld as documentation, log or configuration text. + 14 finding(s) withheld as documentation, log or comment text. Show them with: cryptoscan scan . --include-narrative ``` +Scanning a repository that ships cryptographic reference data (a database of +packages and the algorithms they use, for example) will report a finding per +row. Use `--exclude` to skip those paths. + ### Suppressing False Positives Use inline comments to suppress findings that are intentional or not applicable: diff --git a/pkg/reporter/text.go b/pkg/reporter/text.go index 525a953..cc15186 100644 --- a/pkg/reporter/text.go +++ b/pkg/reporter/text.go @@ -137,7 +137,7 @@ func (r *TextReporter) Generate(results *scanner.Results) (string, error) { // between "nothing was found" and "something was found and not shown". if results.Summary.NarrativeSuppressed > 0 { b.WriteString(r.color(colorCyan, fmt.Sprintf( - "\n %d mention(s) withheld as documentation, log or configuration text.\n"+ + "\n %d finding(s) withheld as documentation, log or comment text.\n"+ " Show them with: cryptoscan scan %s --include-narrative\n", results.Summary.NarrativeSuppressed, results.ScanTarget))) } diff --git a/pkg/scanner/evidence.go b/pkg/scanner/evidence.go index 691e4f0..213a2f7 100644 --- a/pkg/scanner/evidence.go +++ b/pkg/scanner/evidence.go @@ -8,7 +8,7 @@ import "strings" // Evidence classification for a single pattern match. // // The scanner needs to separate "this codebase uses MD5" from "this line of -// prose mentions MD5". The previous implementation answered that question by +// prose mentions MD5". The original implementation answered that question by // testing the whole source line against a list of about a hundred substrings // and dropping the finding on any hit. That is unsound in a way that produced // silent false negatives: @@ -20,98 +20,127 @@ import "strings" // // None of those substrings had anything to do with the matched token. ".md" // was meant to catch Markdown filenames and instead matched every ".md5(" in -// the corpus; "const " and "cipher =" matched the most idiomatic way to write -// crypto in JavaScript and Java respectively. The tool's own crypto-samples -// tree contained deliberately vulnerable code that the scanner could not see. +// the corpus. // -// The replacement asks two better-posed questions, both anchored to where the -// match actually sits on the line: +// # Ordering is the safety property // -// 1. Is the matched token part of an operational construct (a call, a member -// access, an argument to a crypto factory)? If so it is evidence of use and -// is never suppressed, whatever else appears on the line. -// 2. Otherwise, is the token inside narrative text (prose, a log message, a -// documentation or configuration key's value, a URL or path)? Only then is -// it treated as low-value. +// The replacement asks the operational question FIRST, and never suppresses a +// match that answers it. This ordering is not cosmetic. An earlier revision of +// this file ran the log-message, comment and URL rules ahead of the +// operational check, and that alone reintroduced the same bug class through a +// different door: // -// Anything that is neither is reported. The default is to report, because a -// missed finding in a security scanner costs more than an extra line of output. +// outputs = Cipher.getInstance("RC4") suppressed: "puts " matched "outputs " +// inputs = crypto.createHash("md5") suppressed: "puts " matched "inputs " +// h = hashlib.md5(load("a.md")) suppressed: ".md" again, via the token +// +// Any narrative rule is a heuristic over weak evidence. Running them after the +// operational check bounds the damage a wrong heuristic can do to lines that +// are not obviously code. +// +// # Suppression is bounded, counted and recoverable +// +// Narrative rules are deliberately narrow, because a missed finding in a +// security scanner costs far more than an extra line of output: +// +// - Whole-line prose detection applies only to documentation files. It used +// to apply everywhere, which deleted crypto out of sshd_config, httpd.conf, +// shell scripts and CI YAML: adding a fifth cipher to an sshd "MACs" line +// took the file from three findings to zero, and an Apache config enabling +// SSLv3 stopped failing `--fail-on critical`. +// - 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. Treating them as universal hid `z--; Cipher.getInstance("DES")` +// and `gpg --cipher-algo 3DES`. +// - Call prefixes are matched on identifier boundaries, never as substrings. +// - Configuration keys naming an algorithm are NOT narrative. `cipher: +// DES-CBC` is exactly the inventory this tool exists to produce. +// +// Everything the classifier does withhold is counted and recoverable with +// --include-narrative. // evidenceClass describes what a matched token tells us about the codebase. type evidenceClass int const ( // evidenceOperational means the token participates in a cryptographic - // operation: a call, an instantiation, a member access, or an algorithm - // name handed to a crypto factory. + // operation: a call, an instantiation, a member access, an import, or an + // algorithm name handed to a crypto factory. evidenceOperational evidenceClass = iota // evidenceNarrative means the token appears in text rather than in code: - // prose, a log message, a documentation or config key's value, a URL, or - // a file path. + // prose, a log message, a comment, a URL, or a documentation label. evidenceNarrative ) -// classifyMatch reports what kind of evidence the token at [start,end) in line -// represents, along with the rule that decided it. The reason string is used -// for diagnostics and tests; callers should not parse it. +// matchContext carries what the classifier knows about where a match came +// from. Language and file type come from the file analyzer and are needed to +// interpret comment markers and to decide whether whole-line prose detection +// applies at all. +type matchContext struct { + Line string + Start int + End int + Language string + FileType string +} + +// classifyMatch reports what kind of evidence a matched token represents, +// along with the rule that decided it. The reason string is for diagnostics +// and tests; callers should not parse it. // -// start and end are byte offsets into line. Out-of-range offsets classify as -// operational, since an unclassifiable match must not be silently dropped. -func classifyMatch(line string, start, end int) (evidenceClass, string) { - if start < 0 || end > len(line) || start >= end { +// Start and End are byte offsets into Line. An offset the classifier cannot +// interpret yields evidenceOperational: an unclassifiable match must be +// reported, never silently dropped. +func classifyMatch(mc matchContext) (evidenceClass, string) { + line := mc.Line + if mc.Start < 0 || mc.End > len(line) || mc.Start >= mc.End { return evidenceOperational, "unclassifiable" } - quoteStart, quoteEnd, inString := stringLiteralAt(line, start, end) + quoteStart, quoteEnd, inString := stringLiteralAt(line, mc.Start, mc.End) - // An import names a library the codebase actually depends on, so it is - // evidence of use even though the module path looks like a file path. - // This is checked before the path rule for exactly that reason. + // An import names a library the codebase depends on. Evidence of use, even + // though a module path looks like a file path. if isImportLine(line) { return evidenceOperational, "import" } - // A crypto call whose result is logged is still a crypto call, so the log - // test only applies when the match is inside the logged string itself. + // The operational question, asked before any heuristic. + if reason, ok := operationalUse(line, mc.Start, mc.End, quoteStart, inString); ok { + return evidenceOperational, reason + } + + // The match sits inside a string that is being logged or turned into an + // error message. The call prefix is matched on identifier boundaries. if inString && isLoggingCall(line[:quoteStart]) { return evidenceNarrative, "log-message" } - if commentStartsAt(line) >= 0 && commentStartsAt(line) < start { + if at := commentStartsAt(line, mc.Language); at >= 0 && at < mc.Start { return evidenceNarrative, "comment" } - if isPathLikeToken(tokenAround(line, start, end)) { + if isPathLikeToken(tokenAround(line, mc.Start, mc.End)) { return evidenceNarrative, "url-or-path" } - if reason, ok := operationalUse(line, start, end, quoteStart, inString); ok { - return evidenceOperational, reason - } - - // A comparison against an algorithm name is code that handles the - // algorithm, not code that uses it: `if alg == "RSA"` tells you the - // program can recognise RSA, not that it encrypts with it. - if inString && isComparisonOperand(line, quoteStart, quoteEnd) { - return evidenceNarrative, "comparison" - } - - // Prose: an algorithm named inside a sentence rather than used. Three or - // more whitespace-separated words is the threshold, which keeps single - // tokens such as "RC4", "aes-256-gcm" and "AES/GCM/NoPadding" operational - // while catching "publicKey must be a valid Ed25519 public key". - if inString && wordCount(line[quoteStart+1:quoteEnd]) >= 3 { + // Prose inside a string literal. Word count alone is not enough: a shelled + // out command such as "openssl dgst -md5 file.bin" is three words and is + // not prose, so at least one ordinary English word is also required. + if inString && isProse(line[quoteStart+1:quoteEnd]) { return evidenceNarrative, "prose" } - if key, ok := precedingKey(line, start); ok { - return evidenceNarrative, "key-value:" + key + if key, ok := precedingLabel(line, mc.Start); ok { + return evidenceNarrative, "label:" + key } - // Prose outside a string literal: body text in HTML and Markdown, where - // the sentence is the line rather than a quoted argument. - if !inString && isProseLine(line) { + // Whole-line prose, for body text in documentation where the sentence is + // the line rather than a quoted argument. Restricted to documentation + // files: in a config file or a shell script, a long line with few braces + // is a directive, not a sentence. + if isDocumentationFile(mc.Language, mc.FileType) && !inString && isProseLine(line) { return evidenceNarrative, "prose-line" } @@ -121,8 +150,8 @@ func classifyMatch(line string, start, end int) (evidenceClass, string) { // operationalUse reports whether the token at [start,end) participates in a // cryptographic operation. func operationalUse(line string, start, end, quoteStart int, inString bool) (string, bool) { - // A call: md5(...), MD5(...), or the whole matched span ending in a call - // such as DES.encrypt(...). + // A call: md5(...), MD5(...), or a matched span ending in a call such as + // DES.encrypt(...). if next := skipSpaces(line, end); next < len(line) && line[next] == '(' { return "call", true } @@ -138,15 +167,21 @@ func operationalUse(line string, start, end, quoteStart int, inString bool) (str // A member access or member call on the matched token: md5.New(), // des.NewCipher(k), CryptoJS.MD5.encrypt(d). - if end < len(line) && line[end] == '.' && end+1 < len(line) && isIdentChar(line[end+1]) { - return "member-call", true - } + // + // Both forms require the surrounding token to be an identifier chain. A + // dot inside a filename looks identical: "legacy-md5.md" would otherwise + // read as a member call on md5. + if !isPathLikeToken(tokenAround(line, start, end)) { + if end < len(line) && line[end] == '.' && end+1 < len(line) && isIdentChar(line[end+1]) { + return "member-call", true + } - // The token is a member of something: hashlib.md5, a.md5, CryptoJS.MD5. - // Requires an identifier before the dot so that ".md5" inside a filename - // or a sentence does not qualify. - if start > 0 && line[start-1] == '.' && start >= 2 && isIdentChar(line[start-2]) { - return "member-access", true + // The token is a member of something: hashlib.md5, a.md5, CryptoJS.MD5. + // Requires an identifier before the dot so that ".md5" inside a + // filename or a sentence does not qualify. + if start > 0 && line[start-1] == '.' && start >= 2 && isIdentChar(line[start-2]) { + return "member-access", true + } } // An algorithm name passed to a crypto factory: @@ -159,146 +194,286 @@ func operationalUse(line string, start, end, quoteStart int, inString bool) (str return "", false } -// loggingCallees are the call prefixes whose string arguments are program -// output rather than program behaviour. +// loggingCallees are call prefixes whose string arguments are program output +// rather than program behaviour. Each is matched on an identifier boundary. var loggingCallees = []string{ - "log.", "logger.", "logging.", "logrus.", "slog.", "zap.", + "log", "logger", "logging", "logrus", "slog", "zap", "console.log", "console.error", "console.warn", "console.info", "console.debug", - "fmt.print", "fmt.sprint", "fmt.fprint", "fmt.errorf", - "print(", "println(", "puts ", "echo ", - "errors.new", "fmt.errorf", + "fmt.print", "fmt.printf", "fmt.println", "fmt.sprint", "fmt.sprintf", + "fmt.fprint", "fmt.fprintf", "fmt.errorf", + "print", "println", "printf", "puts", "echo", "warn", "panic", + "errors.new", "t.logf", "t.errorf", "t.fatalf", } // isLoggingCall reports whether prefix, the text preceding a string literal on // the same line, ends in a call to a logging, printing or error-construction // function. +// +// Matching is identifier-anchored. A substring test here is what made "puts " +// match "outputs = ..." and "log." match "catalog.", hiding real crypto behind +// a variable name. func isLoggingCall(prefix string) bool { - lower := strings.ToLower(prefix) - // Only the tail matters: the call that owns this string literal is the - // nearest one to its left. Anything further away is unrelated code on the - // same line. - if len(lower) > 64 { - lower = lower[len(lower)-64:] - } - for _, callee := range loggingCallees { - if strings.Contains(lower, callee) { - return true - } - } - return false + return hasCalleeSuffix(prefix, loggingCallees) } // cryptoFactoryCallees are call prefixes whose string argument names the -// algorithm being instantiated. A match inside one of these is a use, not a -// mention. +// algorithm being instantiated. A match inside one of these is a use. var cryptoFactoryCallees = []string{ - "getinstance(", "createcipher", "createdecipher", "createhash(", "createhmac(", - "createsign(", "createverify(", "creatediffiehellman(", - "messagedigest", "keypairgenerator", "keygenerator", "keyfactory", - "secretkeyspec", "ivparameterspec", "mac.getinstance", "signature.getinstance", - "new cipher", "cipher(", "digest(", "hmac(", "hashlib.new(", "hmac.new(", - "algorithms.", "crypto.subtle", "subtle.digest(", "subtle.importkey(", - "evp_get_cipherbyname(", "openssl_encrypt(", "openssl_digest(", + "getinstance", "createcipher", "createcipheriv", "createdecipher", + "createdecipheriv", "createhash", "createhmac", "createsign", "createverify", + "creatediffiehellman", "messagedigest", "keypairgenerator", "keygenerator", + "keyfactory", "secretkeyspec", "ivparameterspec", + "hashlib.new", "hmac.new", "cipher.new", "digest.new", } -// isCryptoFactoryCall reports whether prefix, the text preceding a string -// literal, ends in a call that takes an algorithm name as an argument. +// isCryptoFactoryCall reports whether prefix ends in a call that takes an +// algorithm name as an argument. func isCryptoFactoryCall(prefix string) bool { - lower := strings.ToLower(prefix) - if len(lower) > 64 { - lower = lower[len(lower)-64:] + return hasCalleeSuffix(prefix, cryptoFactoryCallees) +} + +// hasCalleeSuffix reports whether prefix ends in a call to one of callees. +// +// The prefix is expected to end at an opening quote, so the shape being +// matched is `... callee ( args? "`. The callee has to end at an identifier +// boundary, which is what stops "puts" from matching inside "outputs". +func hasCalleeSuffix(prefix string, callees []string) bool { + // Walk back over the argument list to the opening parenthesis of the call + // that owns this string literal. + trimmed := strings.TrimRight(prefix, " \t") + depth := 0 + open := -1 + for i := len(trimmed) - 1; i >= 0; i-- { + switch trimmed[i] { + case ')': + depth++ + case '(': + if depth == 0 { + open = i + i = -1 + continue + } + depth-- + } + if open >= 0 { + break + } } - for _, callee := range cryptoFactoryCallees { - if strings.Contains(lower, callee) { + if open < 0 { + return false + } + + name := strings.ToLower(strings.TrimRight(trimmed[:open], " \t")) + for _, callee := range callees { + if !strings.HasSuffix(name, callee) { + continue + } + // Identifier boundary: the character before the callee must not + // continue an identifier. A dot is allowed so that "fmt.print" and + // "console.log" match, and so that "crypto.createHash" matches the + // bare "createhash" entry. + if at := len(name) - len(callee); at == 0 || !isIdentChar(name[at-1]) { return true } } return false } -// narrativeKeys are field names whose value is descriptive text or declared -// configuration data rather than an executed operation. +// narrativeLabels are field names whose value is descriptive text, not +// configuration. // -// The key has to sit immediately before the match and be followed by ':' or -// ',', so this recognises `"algorithm": "RSA-2048"`, -// `description: "uses SHA-256"` and `c.Locals("authenticated_via", "ed25519")` -// without matching a line that merely contains the word somewhere. -var narrativeKeys = []string{ - // Documentation and presentation. +// Keys that name an algorithm as configuration (algorithm, cipher, hash, +// digest, key_type and friends) are deliberately absent. `cipher: DES-CBC` in +// a YAML file and `"algorithm": "RSA-2048"` in a JSON file are precisely the +// cryptographic inventory this tool is asked to produce; suppressing them +// produced a clean verdict that was never earned. +var narrativeLabels = []string{ "description", "summary", "title", "label", "placeholder", "tooltip", "help", "note", "remarks", "usage", "example", "comment", "display_name", - // Declared configuration data. - // "type" and "method" are deliberately absent: they are too generic, and - // their values (`type: 'MLDSA65-ECDSA-P256'`) are exactly the algorithm - // inventory this tool exists to collect. - "algorithm", "algorithms", "cipher", "ciphers", "hash", "digest", - "key_type", "crypto_type", "auth_method", - "authenticated_via", "authentication_type", "signing_algorithm", - "encryption_algorithm", "algorithm_name", "supported_algorithms", - "allowed_algorithms", "preferred_algorithm", - // Test data. "test_vector", "test_data", "test_case", "test_input", "test_output", "expected_hash", "expected_signature", } -// precedingKey reports whether the match at start is the value of a narrative -// key, returning the key that matched. -func precedingKey(line string, start int) (string, bool) { +// precedingLabel reports whether the match at start is the value of a +// descriptive label, returning the label that matched. +// +// The label must sit immediately before the match, separated only by quotes, +// whitespace and one key/value separator. An earlier revision searched the +// whole prefix, so a "label:" anywhere on the line suppressed a match thirty +// characters later. +func precedingLabel(line string, start int) (string, bool) { prefix := strings.ToLower(line[:start]) - for _, key := range narrativeKeys { - idx := strings.LastIndex(prefix, key) - if idx == -1 { + + // A label that opens the line owns the rest of it, so that + // `description: uses SHA-1 for backwards compatibility` is narrative even + // though the label is not adjacent to the match. Anchoring to the start of + // the line is what keeps this from matching a "label:" that happens to + // appear later on a line of code. + opening := strings.TrimLeft(prefix, " \t-\"'") + for _, label := range narrativeLabels { + if !strings.HasPrefix(opening, label) { continue } - // Whole word only: "algorithm" must not match inside "myalgorithms". - if idx > 0 && isIdentChar(prefix[idx-1]) { + rest := strings.TrimLeft(opening[len(label):], "\"' \t") + if strings.HasPrefix(rest, ":") { + return label, true + } + } + + // Step back over the value side: quotes and whitespace only. + head := strings.TrimRight(prefix, " \t\"'") + + // Exactly one separator. + switch { + case strings.HasSuffix(head, "=>"): + head = head[:len(head)-2] + case strings.HasSuffix(head, ":"), strings.HasSuffix(head, ","): + head = head[:len(head)-1] + default: + return "", false + } + + // Step back over the key's own quotes and whitespace. + head = strings.TrimRight(head, " \t\"'") + + for _, label := range narrativeLabels { + if !strings.HasSuffix(head, label) { continue } - rest := prefix[idx+len(key):] - // Allow a closing quote and whitespace between the key and its - // separator, covering `"algorithm": x` and `'algorithm' : x`. - rest = strings.TrimLeft(rest, "\"' \t") - if strings.HasPrefix(rest, ":") || strings.HasPrefix(rest, ",") || - strings.HasPrefix(rest, "=>") { - return key, true + if at := len(head) - len(label); at == 0 || !isIdentChar(head[at-1]) { + return label, true } } return "", false } -// isComparisonOperand reports whether the string literal at [quoteStart, -// quoteEnd] is an operand of an equality comparison. -func isComparisonOperand(line string, quoteStart, quoteEnd int) bool { - before := strings.TrimRight(line[:quoteStart], " \t") - for _, op := range []string{"==", "!=", "===", "!==", "<>"} { - if strings.HasSuffix(before, op) { - return true +// proseWords are ordinary English words that appear in sentences and not in +// command lines or configuration directives. Requiring one of these is what +// separates "publicKey must be a valid Ed25519 public key" from +// "openssl dgst -md5 file.bin". +var proseWords = map[string]bool{ + "a": true, "an": true, "the": true, "is": true, "are": true, "was": true, + "were": true, "be": true, "been": true, "must": true, "should": true, + "can": true, "could": true, "would": true, "will": true, "may": true, + "to": true, "of": true, "for": true, "with": true, "without": true, + "from": true, "in": true, "into": true, "on": true, "at": true, "by": true, + "and": true, "or": true, "not": true, "no": true, "this": true, + "that": true, "these": true, "those": true, "it": true, "its": true, + "you": true, "your": true, "we": true, "our": true, "using": true, + "used": true, "use": true, "uses": true, "before": true, "after": true, + "instead": true, "because": true, "when": true, "while": true, + "which": true, "than": true, "then": true, "but": true, "all": true, + "any": true, "only": true, "also": true, "still": true, "valid": true, + "invalid": true, "supported": true, "deprecated": true, "broken": true, + "failed": true, "unable": true, "please": true, "does": true, "do": true, + "has": true, "have": true, +} + +// isProse reports whether s reads as a sentence rather than as a token, a +// command line or a configuration value. +func isProse(s string) bool { + fields := strings.Fields(s) + if len(fields) < 3 { + return false + } + // A command line carries flags. Sentences do not, and treating + // "openssl enc -rc4 -in a -out b" as prose hid a real invocation. + for _, f := range fields { + if len(f) > 1 && f[0] == '-' { + return false } } - for _, call := range []string{".equals(", ".equalsignorecase(", "strcmp(", "strcasecmp(", "string.compare(", ".startswith(", ".endswith("} { - if strings.HasSuffix(strings.ToLower(before), call) { + for _, f := range fields { + word := strings.ToLower(strings.Trim(f, ".,;:!?()[]{}\"'`")) + if proseWords[word] { return true } } + return false +} - after := strings.TrimLeft(line[quoteEnd+1:], " \t") - for _, op := range []string{"==", "!=", "===", "!==", "<>"} { - if strings.HasPrefix(after, op) { - return true +// isDocumentationFile reports whether whole-line prose detection applies. +// +// Restricting it to documentation is the difference between removing HTML body +// text and removing an sshd_config directive. Config files, shell scripts and +// CI YAML all have long lines with almost no code punctuation, and they are +// where a migration inventory most needs to see weak algorithms. +func isDocumentationFile(language, fileType string) bool { + switch language { + case "markdown", "html", "xml": + return true + } + return fileType == "documentation" +} + +// commentMarkers returns the line-comment and block-comment openers for a +// language. +// +// An unknown language gets no markers rather than a permissive default: the +// cost of missing a comment is one noisy finding, and the cost of guessing +// wrong is a silently hidden one. +func commentMarkers(language string) []string { + switch language { + case "go", "java", "javascript", "typescript", "c", "cpp", "csharp", + "swift", "kotlin", "rust", "php", "scala": + return []string{"//", "/*"} + case "python", "ruby", "shell", "yaml", "toml", "perl", "r", "makefile": + return []string{"#"} + case "sql", "lua", "haskell", "ada": + return []string{"--"} + case "markdown", "html", "xml": + return []string{"