From 1aaf9dd5debf56107904bbdebe840b88a685b531 Mon Sep 17 00:00:00 2001 From: PiX <69745008+pixincreate@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:53:44 +0530 Subject: [PATCH] fix(detectors): validate structure instead of matching shape alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four detectors matched far more than they should, or nothing at all. CreditCardDetector accepted any 13-16 digit run with arbitrary spaces, so commit hashes, timestamps and ids were reported HIGH — 1611 of them in one payments repository, and one match even spanned the gap between two unrelated numbers. Detectors can now declare a structural check (validate = "luhn"), and the pattern additionally requires an issuer prefix in 4-digit groups. Luhn alone was not enough: about one in ten random digit runs satisfies it. HighEntropyDetector required entropy 4.0 from hex, whose 16-symbol alphabet caps entropy at exactly 4.0 — reachable only by a perfectly uniform string, so it had never fired (measured max over 20k random 64-char samples: 3.97). Lowering it to 3.6 alone produced 758 hits in a hashing-heavy repository, because a bare 64-char hex string is a SHA-256 digest as often as a 32-byte token and no threshold separates them. It now relies on the keyword prefilter for credential context. PKCS#8 headers matched nothing: the pattern's algorithm group was effectively mandatory, so 'BEGIN PRIVATE KEY' and 'BEGIN ENCRYPTED PRIVATE KEY' — what openssl genpkey and service-account JSON emit — were invisible. PhoneNumberDetector matched any 10-digit run, making every unix timestamp a phone number. Measured across seven working repositories: -22%, -28%, -11%, -6%, -5% findings, with detection of real cards, keys and tokens verified by test. --- .keywatch-baseline.json | 119 ++++++++++++++++++++++++++++++++++++++++ CHANGELOG.md | 5 ++ detectors.toml | 28 ++++++++-- src/detector.rs | 95 ++++++++++++++++++++++++++++++++ src/scanner.rs | 2 + tests/detector_tests.rs | 89 ++++++++++++++++++++++++++++++ 6 files changed, 332 insertions(+), 6 deletions(-) diff --git a/.keywatch-baseline.json b/.keywatch-baseline.json index 2c82f2d..0147f07 100644 --- a/.keywatch-baseline.json +++ b/.keywatch-baseline.json @@ -1491,6 +1491,125 @@ "finding_type": "Generic Key/Secret", "matched_content_hash": "650dfe7ddce0129182c60029ba66733efa412ba0824523dd6b096482c4eb8ac9", "plugin_name": "GenericKeyValueDetector" + }, + { + "file_path": "./detectors.toml", + "line_number": 275, + "finding_type": "Private Key Content", + "matched_content_hash": "f59444f5756fc6dd701c599452c1752e64a97ecb0447ce41a1e19eb15293ef9a", + "plugin_name": "PrivateKeyDetector" + }, + { + "file_path": "./src/scanner.rs", + "line_number": 1014, + "finding_type": "Private Key Content", + "matched_content_hash": "f91082d1cbd2032b5ea19f2bbf6b3f88e02c12ae320fc9e08e6dc0f73d82bdd1", + "plugin_name": "PrivateKeyDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 300, + "finding_type": "Credit Card Number", + "matched_content_hash": "4541206d542811878a9374508fe296fa321a8b56c2902736a02f388836f6e108", + "plugin_name": "CreditCardDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 301, + "finding_type": "Credit Card Number", + "matched_content_hash": "e2e5b50c7d336fb0ed238a9c1dd7520b847fe55b990c8f90ecb810a398854d52", + "plugin_name": "CreditCardDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 301, + "finding_type": "Aadhaar Card Number", + "matched_content_hash": "14270b9138e72cd99e52cab8f9df9ab6cfcec3d2d8d743ca83774ae0d6f1faed", + "plugin_name": "AadhaarCardDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 302, + "finding_type": "Credit Card Number", + "matched_content_hash": "13ae894eedbfba2dbd06400ba5b215ffd661885646ab86e050fb1a0d192c1c5b", + "plugin_name": "CreditCardDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 302, + "finding_type": "Aadhaar Card Number", + "matched_content_hash": "f326bb251646469912e5296e014c0c226ff0e95d866381c1b0d060e7a8bf30ab", + "plugin_name": "AadhaarCardDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 303, + "finding_type": "Credit Card Number", + "matched_content_hash": "7eafa99f1c4d8c35d2a84d390f9e1a9806fb518aaad3bd0c6dde7c9669e1ab97", + "plugin_name": "CreditCardDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 326, + "finding_type": "Phone Number", + "matched_content_hash": "33360227b18134e6e594a3df05104457bac9713999bb6374dc9bd74fd54c49ee", + "plugin_name": "PhoneNumberDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 326, + "finding_type": "Phone Number", + "matched_content_hash": "cc8546d7c697eb4e2aecde0bfa6ece05bcd1323722b22e34f7dba70c97c706fd", + "plugin_name": "PhoneNumberDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 326, + "finding_type": "Phone Number", + "matched_content_hash": "c7730d0abc47a6b7fddd39f1c461ca7e582e5b27b3e38ee4b2dfdec1558d4023", + "plugin_name": "PhoneNumberDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 343, + "finding_type": "SSH Private Key", + "matched_content_hash": "1b01887f477e98dd56ec542c12433dee8c323176bd532c05163823079263ba31", + "plugin_name": "SSHPrivateKeyDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 344, + "finding_type": "SSH Private Key", + "matched_content_hash": "32006da0b4e4851aa7946ffa4f040f364cac7c317f312b5d742ca5833dd8760f", + "plugin_name": "SSHPrivateKeyDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 345, + "finding_type": "SSH Private Key", + "matched_content_hash": "678a65e8968aabff441076ae306e13d4fc85b1d36d8036a10d2100c1dc40d251", + "plugin_name": "SSHPrivateKeyDetector" + }, + { + "file_path": "./tests/detector_tests.rs", + "line_number": 356, + "finding_type": "Random String", + "matched_content_hash": "e51298df0e431de2bfdf6180e3a7b9f3f092c3e9a912facd350e8c7179936e75", + "plugin_name": "RandomString" + }, + { + "file_path": "./tests/scanner_tests.rs", + "line_number": 210, + "finding_type": "Private Key Content", + "matched_content_hash": "b94c156724c086556087773aad998c0791dc79172caa4b3184a7330cfa7c1d76", + "plugin_name": "PrivateKeyDetector" + }, + { + "file_path": "./tests/scanner_tests.rs", + "line_number": 212, + "finding_type": "Private Key Content", + "matched_content_hash": "43531827dd6142886cfeb10207f046021a4eb6c575828583ad2cb20d9430c72f", + "plugin_name": "PrivateKeyDetector" } ] } \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a7ca7c..63706a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,11 @@ All notable changes to this project will be documented in this file. ### Fixed +- `CreditCardDetector` requires an issuer prefix and a valid Luhn checksum, instead of matching any 13-16 digit run +- `HighEntropyDetector` could never fire (its 4.0 threshold is the ceiling for hex) and now runs, restricted to lines naming a credential +- PKCS#8 private key headers (`BEGIN PRIVATE KEY`, `BEGIN ENCRYPTED PRIVATE KEY`) are detected +- `PhoneNumberDetector` needs punctuation or a country code, so unix timestamps are not phone numbers +- Detectors can require a structural check via `validate = "luhn"` - Hooks use built-in detectors, so a `detectors.toml` committed to a scanned repository can no longer replace the detector set and disable its own scan - Files git renders as binary (including text marked `-diff` in `.gitattributes`) are read from the index instead of being reported clean - `Base64Detector` matches from 28 characters, the length where entropy can actually separate base64 from identifiers diff --git a/detectors.toml b/detectors.toml index 5079d4e..d16e767 100644 --- a/detectors.toml +++ b/detectors.toml @@ -36,14 +36,14 @@ keywords = ["eyJ"] [[detectors]] name = "SSHPrivateKeyDetector" -pattern = "-----BEGIN (RSA|DSA|EC|OPENSSH) PRIVATE KEY-----" +pattern = "-----BEGIN (?:(?:RSA|DSA|EC|OPENSSH|ENCRYPTED) )?PRIVATE KEY-----" finding_type = "SSH Private Key" severity = "HIGH" -keywords = ["BEGIN RSA PRIVATE KEY", "BEGIN DSA PRIVATE KEY", "BEGIN EC PRIVATE KEY", "BEGIN OPENSSH PRIVATE KEY"] +keywords = ["PRIVATE KEY"] [[detectors]] name = "PrivateKeyDetector" -pattern = "(?s)-----BEGIN (RSA|DSA|EC|PGP)? PRIVATE KEY-----\\n[\\s\\S]*?-----END (RSA|DSA|EC|PGP)? PRIVATE KEY-----" +pattern = "(?s)-----BEGIN (?:(?:RSA|DSA|EC|PGP|OPENSSH|ENCRYPTED) )?PRIVATE KEY-----[\\s\\S]*?-----END (?:(?:RSA|DSA|EC|PGP|OPENSSH|ENCRYPTED) )?PRIVATE KEY-----" finding_type = "Private Key Content" severity = "HIGH" @@ -71,13 +71,21 @@ severity = "LOW" [[detectors]] name = "PhoneNumberDetector" -pattern = "\\b(\\+?1\\s*)?(\\(\\d{3}\\)|\\d{3})[-.\\s]?\\d{3}[-.\\s]?\\d{4}\\b" +# Requires punctuation or a country code: an unseparated 10-digit run is a +# unix timestamp or numeric id far more often than a phone number. +pattern = "(?:\\+1[-.\\s]?)?(?:\\(\\d{3}\\)[-.\\s]*|\\b\\d{3}[-.\\s])\\d{3}[-.\\s]?\\d{4}\\b" finding_type = "Phone Number" severity = "LOW" [[detectors]] name = "CreditCardDetector" -pattern = "\\b(?:\\d[ -]*?){13,16}\\b" +# Issuer prefixes (Visa/Mastercard/Amex/Discover/UnionPay) in 4-digit groups. +# The old "any 13-16 digits with arbitrary spaces" shape matched commit +# hashes, timestamps and ids, and even spanned the gap between two unrelated +# numbers ("index aabbcc0..1111111 100644"). Luhn on its own does not fix +# that: roughly one in ten random digit runs satisfies it. +pattern = "\\b(?:4\\d{3}|5[1-5]\\d{2}|2[2-7]\\d{2}|6011|3[47]\\d{2})[ -]?\\d{4}[ -]?\\d{4}[ -]?\\d{1,4}\\b" +validate = "luhn" finding_type = "Credit Card Number" severity = "HIGH" @@ -134,7 +142,15 @@ name = "HighEntropyDetector" pattern = "\\b[a-f0-9]{48,}\\b|\\b[A-Fa-f0-9]{48,}\\b" finding_type = "High Entropy String" severity = "MEDIUM" -entropy = 4.0 +# Hex has 16 symbols, so entropy cannot exceed log2(16) = 4.0 and reaches it +# only on a perfectly uniform string: at 4.0 this detector could never fire +# (measured max over 20k random 64-char hex strings: 3.97). +entropy = 3.6 +# A bare 64-char hex string is a SHA-256 digest as often as it is a 32-byte +# token, and nothing about the content tells them apart — enabling this +# detector without context produced 758 hits in one hashing-heavy repository. +# The keyword prefilter restricts it to lines that name a credential. +keywords = ["key", "secret", "token", "password", "credential", "auth", "bearer"] [[detectors]] name = "StripeAPIKeyDetector" diff --git a/src/detector.rs b/src/detector.rs index 9f80418..7e6a15d 100644 --- a/src/detector.rs +++ b/src/detector.rs @@ -23,6 +23,10 @@ pub enum DetectorError { detector: String, source: ParseSeverityError, }, + InvalidValidator { + detector: String, + source: ParseValidatorError, + }, } impl fmt::Display for DetectorError { @@ -49,6 +53,13 @@ impl fmt::Display for DetectorError { detector, source ) } + DetectorError::InvalidValidator { detector, source } => { + write!( + formatter, + "invalid validator in detector '{}': {}", + detector, source + ) + } } } } @@ -60,10 +71,66 @@ impl std::error::Error for DetectorError { Some(source) } Self::InvalidSeverity { source, .. } => Some(source), + Self::InvalidValidator { source, .. } => Some(source), } } } +/// Extra structural check a detector can require of its matches, for +/// patterns whose shape alone is too permissive. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum ContentValidator { + /// Payment card numbers carry a Luhn check digit. Without it, a 13-16 + /// digit pattern matches every commit hash fragment, timestamp and + /// numeric id in a codebase. + Luhn, +} + +impl FromStr for ContentValidator { + type Err = ParseValidatorError; + + fn from_str(value: &str) -> Result { + match value.trim().to_lowercase().as_str() { + "luhn" => Ok(Self::Luhn), + other => Err(ParseValidatorError { + value: other.to_string(), + }), + } + } +} + +#[derive(Debug)] +pub struct ParseValidatorError { + value: String, +} + +impl fmt::Display for ParseValidatorError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(formatter, "unknown validator '{}'", self.value) + } +} + +impl std::error::Error for ParseValidatorError {} + +/// Luhn checksum, ignoring embedded separators. +fn passes_luhn(matched: &str) -> bool { + let digits: Vec = matched.chars().filter_map(|c| c.to_digit(10)).collect(); + if !(13..=19).contains(&digits.len()) { + return false; + } + let sum: u32 = digits + .iter() + .rev() + .enumerate() + .map(|(index, digit)| match index % 2 { + 1 if *digit > 4 => digit * 2 - 9, + 1 => digit * 2, + _ => *digit, + }) + .sum(); + sum % 10 == 0 +} + pub struct Detector { pub name: String, pub regex: Regex, @@ -72,6 +139,7 @@ pub struct Detector { pub allowlist: Vec, pub keywords: Vec, pub entropy_threshold: Option, + pub validator: Option, } impl Detector { @@ -116,9 +184,25 @@ impl Detector { .map(|keyword| keyword.to_lowercase()) .collect(), entropy_threshold, + validator: None, }) } + /// Attaches a structural validator. Kept separate from `new` so adding a + /// check does not touch every construction site. + pub fn with_validator(mut self, validator: Option) -> Self { + self.validator = validator; + self + } + + /// Whether a match satisfies the detector's structural validator. + pub fn passes_validation(&self, matched: &str) -> bool { + match self.validator { + Some(ContentValidator::Luhn) => passes_luhn(matched), + None => true, + } + } + /// `lowercase_content` must already be lowercased. Keywords are stored /// lowercased at construction so callers can lowercase once per line /// instead of once per detector. @@ -168,6 +252,7 @@ struct DetectorConfig { allowlist: Option>, keywords: Option>, entropy: Option, + validate: Option, } fn find_detectors_config(include_repository_config: bool) -> Option { @@ -239,6 +324,15 @@ fn initialize_detectors_from_config( .map(|detector_config| { let allowlist = detector_config.allowlist.as_deref().unwrap_or_default(); let keywords = detector_config.keywords.as_deref().unwrap_or_default(); + let validator = detector_config + .validate + .as_deref() + .map(ContentValidator::from_str) + .transpose() + .map_err(|source| DetectorError::InvalidValidator { + detector: detector_config.name.clone(), + source, + })?; Detector::new( &detector_config.name, &detector_config.pattern, @@ -248,6 +342,7 @@ fn initialize_detectors_from_config( keywords, detector_config.entropy, ) + .map(|detector| detector.with_validator(validator)) }) .collect::, _>>() .map_err(|source| DetectorInitError::InvalidDetector { source }) diff --git a/src/scanner.rs b/src/scanner.rs index 6dc5591..d0e33b9 100644 --- a/src/scanner.rs +++ b/src/scanner.rs @@ -149,6 +149,7 @@ fn scan_line_detectors( for mat in detector.regex.find_iter(line) { if !is_allowlisted(mat.as_str(), detector) && detector.has_sufficient_entropy(mat.as_str()) + && detector.passes_validation(mat.as_str()) { findings.push(Finding { file_path: path.to_string(), @@ -187,6 +188,7 @@ fn scan_multiline_chunk( if !line_is_suppressed && !is_allowlisted(mat.as_str(), detector) && detector.has_sufficient_entropy(mat.as_str()) + && detector.passes_validation(mat.as_str()) { findings.push(Finding { file_path: path.to_string(), diff --git a/tests/detector_tests.rs b/tests/detector_tests.rs index 5cc1b07..039358b 100644 --- a/tests/detector_tests.rs +++ b/tests/detector_tests.rs @@ -275,3 +275,92 @@ fn test_generic_key_value_ignores_unquoted_identifier_assignments() { assert!(is_reported(secret), "should flag credential: {secret}"); } } + +/// Helper: does any built-in detector report this line? +fn reported_by(line: &str) -> Vec { + let detectors = key_watch::detector::initialize_detectors().expect("load detectors"); + let lowered = line.to_lowercase(); + detectors + .iter() + .filter(|d| d.has_keywords(&lowered)) + .filter(|d| { + d.regex.find_iter(line).any(|m| { + !d.allowlist.iter().any(|a| a.is_match(m.as_str())) + && d.has_sufficient_entropy(m.as_str()) + && d.passes_validation(m.as_str()) + }) + }) + .map(|d| d.name.clone()) + .collect() +} + +#[test] +fn test_credit_card_requires_issuer_prefix_and_luhn() { + for card in [ + "4111111111111111", // Visa + "5500 0000 0000 0004", // Mastercard, space separated + "4111-1111-1111-1111", // dash separated + "378282246310005", // Amex + ] { + assert!( + reported_by(card).contains(&"CreditCardDetector".to_string()), + "should detect card: {card}" + ); + } + + for not_a_card in [ + "4111111111111112", // Visa prefix, fails Luhn + "1234567890123456", // no issuer prefix + "index aabbcc0..1111111 100644", // spans two unrelated numbers + "timestamp = 1700000000123", + ] { + assert!( + !reported_by(not_a_card).contains(&"CreditCardDetector".to_string()), + "should not detect card in: {not_a_card}" + ); + } +} + +#[test] +fn test_phone_number_requires_separator_or_country_code() { + for phone in ["call 555-123-4567", "(555) 123-4567", "+1 555 123 4567"] { + assert!( + reported_by(phone).contains(&"PhoneNumberDetector".to_string()), + "should detect phone: {phone}" + ); + } + assert!( + !reported_by("ts 1700000000").contains(&"PhoneNumberDetector".to_string()), + "a bare 10-digit run is a timestamp, not a phone number" + ); +} + +#[test] +fn test_pkcs8_private_key_headers_are_detected() { + // openssl genpkey and GCP/Azure service-account JSON emit these; before + // the pattern required an algorithm word and matched neither. + for header in [ + "-----BEGIN PRIVATE KEY-----", + "-----BEGIN ENCRYPTED PRIVATE KEY-----", + "-----BEGIN RSA PRIVATE KEY-----", + ] { + assert!( + !reported_by(header).is_empty(), + "should detect private key header: {header}" + ); + } +} + +#[test] +fn test_high_entropy_hex_needs_credential_context() { + let hex = "8b0e7153bf7c3706d85c524e440066559a6656c90bd5482a90a29b9fa5ff5180"; + assert!( + reported_by(&format!("api_token = {hex}")).contains(&"HighEntropyDetector".to_string()), + "hex assigned to a credential-named field should be reported" + ); + assert!( + !reported_by(&format!("let digest = compute({hex});")) + .contains(&"HighEntropyDetector".to_string()), + "a bare hex digest is indistinguishable from a hash and must not fire" + ); +}