diff --git a/src/models.rs b/src/models.rs index 8f439bb..9de9a76 100644 --- a/src/models.rs +++ b/src/models.rs @@ -183,6 +183,12 @@ pub struct UnifiedRule { #[serde(default)] pub patterns: Option>, + // Exclusions for search mode: a positive match is dropped when any of these matches + // the matched span. Taint rules express the same idea with `sanitizers`. + #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default)] + pub unless: Option>, + // Taint analysis fields (used when mode = "taint") #[serde(skip_serializing_if = "Option::is_none")] #[serde(default)] diff --git a/src/rules.rs b/src/rules.rs index 85fe30b..dfc253d 100644 --- a/src/rules.rs +++ b/src/rules.rs @@ -394,22 +394,87 @@ pub fn match_any_pattern(patterns: &[String], text: &str) -> bool { CommonUtils::matches_any_pattern(patterns, text) } +/// True when the rule matches `text` and no `unless` entry vetoes the match. +/// +/// A veto is *match-scoped*: each `unless` entry is tested against the byte span the +/// matching pattern covers, never against the whole line. Scoping matters — +/// `el.addEventListener('x', h); div.innerHTML = userInput;` must still report the +/// innerHTML sink even though `addEventListener` is an `unless` entry of that rule. pub fn rule_matches_pattern_unified(rule: &UnifiedRule, text: &str) -> bool { - if let Some(pattern) = &rule.pattern - && match_pattern(pattern, text) - { + let mut matched = matching_patterns(rule, text).peekable(); + if matched.peek().is_none() { + return false; + } + + let Some(unless) = rule.unless.as_ref() else { return true; + }; + + // Every pattern that matched gets its span checked: a rule's patterns overlap, and a + // sanitizer sitting outside the narrowest match still belongs to the same expression. + !matched.any(|pattern| { + let scope = unless_scope_range(pattern, text).map_or(text, |range| &text[range]); + match_any_pattern(unless, scope) + }) +} + +/// The rule's patterns that match `text`: `pattern` before `patterns`, in file order. +fn matching_patterns<'a>( + rule: &'a UnifiedRule, + text: &'a str, +) -> impl Iterator + 'a { + rule.pattern + .as_deref() + .into_iter() + .chain(rule.patterns.as_deref().unwrap_or_default().iter().map(String::as_str)) + .filter(move |pattern| match_pattern(pattern, text)) +} + +/// Byte span of `pattern`'s match inside `text`, used *only* to scope `unless`. +/// +/// `None` means "no span recoverable for this pattern form"; the caller then widens the +/// scope to the whole `text`, which is how `unless` behaved before match scoping. +/// +/// Deliberately separate from `pattern_match_range` below: that one feeds markup line +/// attribution and must keep mirroring `matches_unified_pattern`'s "no range for glob" +/// behaviour, or findings move lines. +fn unless_scope_range(pattern: &str, text: &str) -> Option> { + if pattern == text { + return Some(0..text.len()); } + if let Some(stripped) = pattern.strip_prefix("regex:") { + return Regex::new(stripped).ok()?.find(text).map(|m| m.range()); + } + if pattern.contains("\\\\") || pattern.contains("\\.") { + return Regex::new(pattern).ok()?.find(text).map(|m| m.range()); + } + if pattern.contains('\\') { + // Escaped taint form: matched by string surgery, no span to recover. + return None; + } + tightest_segment_span(pattern, text) +} - if let Some(patterns) = &rule.patterns { - for pattern in patterns { - if match_pattern(pattern, text) { - return true; - } - } +/// Tightest in-order span of a glob/substring pattern's literal segments. +/// +/// Anchored right-to-left: the last segment is taken at its *last* occurrence and each +/// earlier segment at its last occurrence before the one that follows it. Leftmost-first +/// would stretch the span across unrelated code — for `document.write(*user*` against +/// `document.write("safe"); document.write(userInput);` it would swallow the quoted first +/// call and let the `document\.write\(['\"]` exclusion veto the real finding. +fn tightest_segment_span(pattern: &str, text: &str) -> Option> { + let mut end = text.len(); + let mut span_end = None; + let mut start = None; + + for segment in pattern.split('*').filter(|segment| !segment.is_empty()).rev() { + let found = text[..end].rfind(segment)?; + span_end.get_or_insert(found + segment.len()); + start = Some(found); + end = found; } - false + Some(start?..span_end?) } /// Byte range of the first match of `pattern` inside `text`, when the pattern form can diff --git a/tests/unit/directory_loading_tests.rs b/tests/unit/directory_loading_tests.rs index f939994..433861a 100644 --- a/tests/unit/directory_loading_tests.rs +++ b/tests/unit/directory_loading_tests.rs @@ -13,6 +13,7 @@ fn make_rule(pattern: &str, finding_type: &str) -> UnifiedRule { mode: "search".to_string(), pattern: Some(pattern.to_string()), patterns: None, + unless: None, sources: None, sinks: None, propagators: None, diff --git a/tests/unit/main.rs b/tests/unit/main.rs index 488e90b..ad8d6a5 100644 --- a/tests/unit/main.rs +++ b/tests/unit/main.rs @@ -13,3 +13,4 @@ mod pattern_matching_tests; mod prefilter_should_scan_tests; mod rule_deserialization_tests; mod semantic_variables_tests; +mod unless_exclusion_tests; diff --git a/tests/unit/pattern_matching_tests.rs b/tests/unit/pattern_matching_tests.rs index 77e9a35..9f99fdc 100644 --- a/tests/unit/pattern_matching_tests.rs +++ b/tests/unit/pattern_matching_tests.rs @@ -16,6 +16,7 @@ fn make_rule(pattern: Option<&str>, patterns: Option>) -> UnifiedRule mode: "search".to_string(), pattern: pattern.map(|s| s.to_string()), patterns: patterns.map(|v| v.into_iter().map(|s| s.to_string()).collect()), + unless: None, sources: None, sinks: None, propagators: None, diff --git a/tests/unit/prefilter_should_scan_tests.rs b/tests/unit/prefilter_should_scan_tests.rs index 2f5c2c4..da4eaca 100644 --- a/tests/unit/prefilter_should_scan_tests.rs +++ b/tests/unit/prefilter_should_scan_tests.rs @@ -22,6 +22,7 @@ mod prefilter_should_scan_tests { mode: "search".to_string(), pattern: None, patterns: None, + unless: None, sources: None, sinks: None, propagators: None, diff --git a/tests/unit/unless_exclusion_tests.rs b/tests/unit/unless_exclusion_tests.rs new file mode 100644 index 0000000..60e8470 --- /dev/null +++ b/tests/unit/unless_exclusion_tests.rs @@ -0,0 +1,160 @@ +use sighthound::models::UnifiedRule; +use sighthound::rules::{Rules, rule_matches_pattern_unified}; + +// UnifiedRule does not derive Default; this fills the fields these tests do not care about. +fn make_rule(patterns: Vec<&str>, unless: Option>) -> UnifiedRule { + UnifiedRule { + id: Some("test-rule".to_string()), + name: None, + description: None, + category: None, + mode: "search".to_string(), + pattern: None, + patterns: Some(patterns.into_iter().map(str::to_string).collect()), + unless: unless.map(|entries| entries.into_iter().map(str::to_string).collect()), + sources: None, + sinks: None, + propagators: None, + sanitizers: None, + finding_type: None, + severity: None, + confidence: None, + file_types: None, + conditions: None, + tags: None, + cwe_id: None, + message: None, + } +} + +#[cfg(test)] +mod unless_exclusion_tests { + use super::*; + + fn frontend_rules() -> Rules { + Rules::load_from_file("rules/javascript/frontend_security.ron") + .expect("Failed to load frontend_security.ron") + } + + fn rule_by_id(rules: &Rules, id: &str) -> UnifiedRule { + rules + .rules + .iter() + .find(|rule| rule.id.as_deref() == Some(id)) + .unwrap_or_else(|| panic!("rule {id} missing from frontend_security.ron")) + .clone() + } + + // The bug this suite guards: `unless:` shipped in the rule files but had no field on + // UnifiedRule, and serde has no deny_unknown_fields — so every exclusion parsed into + // nothing and every scan ignored it. + #[test] + fn frontend_rules_parse_their_unless_lists() { + let rules = frontend_rules(); + + for (id, entries) in [ + ("js-dom-xss-001", 16), + ("js-react-dangerously-set-inner-html-001", 5), + ("js-dom-xss-003", 5), + ] { + let rule = rule_by_id(&rules, id); + let unless = rule.unless.unwrap_or_else(|| panic!("{id} dropped its unless list")); + assert_eq!(unless.len(), entries, "{id} unless entry count"); + } + } + + #[test] + fn innerhtml_rule_reports_unsanitized_assignment() { + let rule = rule_by_id(&frontend_rules(), "js-dom-xss-001"); + assert!(rule_matches_pattern_unified(&rule, "div.innerHTML = userInput")); + } + + #[test] + fn innerhtml_rule_excludes_sanitized_assignment() { + let rule = rule_by_id(&frontend_rules(), "js-dom-xss-001"); + assert!(!rule_matches_pattern_unified( + &rule, + "div.innerHTML = DOMPurify.sanitize(userInput)" + )); + } + + #[test] + fn dangerously_set_inner_html_rule_reports_raw_html() { + let rule = rule_by_id(&frontend_rules(), "js-react-dangerously-set-inner-html-001"); + assert!(rule_matches_pattern_unified( + &rule, + "dangerouslySetInnerHTML={{ __html: userProfile }}" + )); + } + + #[test] + fn dangerously_set_inner_html_rule_excludes_sanitized_html() { + let rule = rule_by_id(&frontend_rules(), "js-react-dangerously-set-inner-html-001"); + assert!(!rule_matches_pattern_unified( + &rule, + "dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(userProfile) }}" + )); + } + + #[test] + fn document_write_rule_reports_dynamic_content() { + let rule = rule_by_id(&frontend_rules(), "js-dom-xss-003"); + assert!(rule_matches_pattern_unified(&rule, "document.write(userInput)")); + } + + #[test] + fn document_write_rule_excludes_static_string() { + let rule = rule_by_id(&frontend_rules(), "js-dom-xss-003"); + assert!(!rule_matches_pattern_unified(&rule, "document.write(\"

data

\")")); + } + + // Regression: exclusions are scoped to the matched span, not the line. An `unless` + // string belonging to an unrelated statement must not swallow a real finding sharing + // the line with it. + #[test] + fn unrelated_unless_text_on_the_same_line_does_not_suppress() { + let rule = rule_by_id(&frontend_rules(), "js-dom-xss-001"); + assert!(rule_matches_pattern_unified( + &rule, + "el.addEventListener('click', h); div.innerHTML = userInput;" + )); + } + + #[test] + fn safe_document_write_earlier_on_the_line_does_not_suppress() { + let rule = rule_by_id(&frontend_rules(), "js-dom-xss-003"); + assert!(rule_matches_pattern_unified( + &rule, + "document.write(\"safe\"); document.write(userInput);" + )); + } + + #[test] + fn sanitized_sibling_expression_does_not_suppress_a_raw_one() { + let rule = rule_by_id(&frontend_rules(), "js-dom-xss-001"); + assert!(rule_matches_pattern_unified( + &rule, + "a.innerHTML = DOMPurify.sanitize(x); b.innerHTML = userInput;" + )); + } + + #[test] + fn rule_without_unless_is_unaffected() { + let rule = make_rule(vec!["*.innerHTML*=*user*"], None); + + assert!(rule.unless.is_none()); + assert!(rule_matches_pattern_unified( + &rule, + "div.innerHTML = DOMPurify.sanitize(userInput)" + )); + assert!(rule_matches_pattern_unified(&rule, "div.innerHTML = userInput")); + assert!(!rule_matches_pattern_unified(&rule, "div.textContent = userInput")); + } + + #[test] + fn empty_unless_list_never_suppresses() { + let rule = make_rule(vec!["*.innerHTML*=*user*"], Some(vec![])); + + assert!(rule_matches_pattern_unified(&rule, "div.innerHTML = userInput")); + } +}