diff --git a/CHANGELOG.md b/CHANGELOG.md index 2447c2c..b75116b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,11 +4,13 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] -Three changes to maintainability findings, each from the findings JevGate's own release check got wrong and measured on the corpus with every changed review and consider labeled by hand (a debatable one counting as not right): considers went from 57% to 59% right on the projects used for tuning, from 47% to 51% on the held-out ones and from 24% to 28% on 23 Bend 2 projects never used for tuning; no review changed but one wrong file-organization review. The self-check's baseline is empty, down from four accepted findings. +Three changes to maintainability findings, each from the findings JevGate's own release check got wrong and measured on the corpus with every changed review and consider labeled by hand (a debatable one counting as not right): considers went from 57% to 59% right on the projects used for tuning, from 47% to 51% on the held-out ones and from 24% to 28% on 23 Bend 2 projects never used for tuning; no review changed but one wrong file-organization review. The self-check's baseline is empty, down from four accepted findings. Two changes to security findings come from the reviews JevGate got wrong on vaultwarden, found while preparing a pull request to it. - Function simplification: splitting a function of 20 lines or fewer is at most a note. 12 of 39 such considers were right on the tuned projects and 5 of 50 on the blind Bend 2 ones: helpers that read in one look, a dispatch over a token's cases, proofs. Function-simplification considers went from 68% to 73% right. Nothing is asked again. - Shared logic: copies of up to twelve lines between test cases in different files are notes, like short copies inside test cases: tests of separate modules or rules repeat the same setup because the code they test is parallel, and a helper shared across test files would couple them. 30 of 87 such considers were right on the tuned projects and 2 of 24 on the held-out ones; shared-logic considers went from 53% to 57% right there and from 43% to 53% on the held-out projects. Nothing is asked again. - File organization: a split the recheck raised from an undecided first answer is asked what kind of file it is, as an undecided one is, and a kind that serves one feature clears it. The first pass and the recheck disagreeing was a weak sign: 4 of the 18 findings it cleared were right. About $0.01 on the corpus. +- Injection: a finding that rests on a file path is asked what the path's variable parts can hold, with its callers and the definitions of the project's types its parameters name (a Rust derive list such as `#[derive(UuidFromParam)]`); leaning toward names that stay inside their directory (an id its type parses, a base name, a Rocket `PathBuf` route segment, which rejects hidden and encoded-slash segments), it is a note. vaultwarden's four path reviews on typed Rocket route parameters were all wrong and are notes; the five path findings labeled right answered another party's input at 0.96 or more and stay. +- Sensitive data: a finding its log checks raised is asked when the log line runs; a line that runs only when an operator turns on a setting meant for logging those values, off by default, is a note. vaultwarden's SSO tokens under `SSO_DEBUG_TOKENS` and pgweb's opt-in request logger, both labeled wrong, are notes; every log finding labeled right answered normal operation or debug level and stays. The two changes cleared 6 wrong security reviews and no right one, for under $0.01 on the corpus. ## [0.22.0] - 2026-09-27 diff --git a/src/catalog.rs b/src/catalog.rs index d0fdd7f..fc6a150 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -292,8 +292,8 @@ pub fn rule_version(key: &str) -> &'static str { SHARED_LOGIC => "22", TEST_VALUE => "7", TEST_REDUNDANCY => "4", - INJECTION => "10", - SENSITIVE_DATA => "7", + INJECTION => "11", + SENSITIVE_DATA => "8", HARDCODED_VALUES => "8", UNSAFE_SETTINGS => "6", AGENT_CONTEXT => "3", diff --git a/src/units/compose.rs b/src/units/compose.rs index da50252..e9d98ca 100644 --- a/src/units/compose.rs +++ b/src/units/compose.rs @@ -4,8 +4,8 @@ use super::{ Access, Block, Detail, FilePlan, Presence, UnitPlan, outcome::{ Answers, Outcome, at_most_note, benefit, checks, choice, choice_mass, document_split, - lowered, noul, open, origin_outcome, score, settled_checks, several_kind, unit_outcome, - value_signals, + logs_found, lowered, noul, open, origin_outcome, rests_on_paths, score, settled_checks, + several_kind, unit_outcome, value_signals, }, wording::{Wording, comment_reason, comment_wording}, wording::{ @@ -249,57 +249,85 @@ fn rechecked<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> (Outcome, Answer (outcome, first) } -/// Functions whose split question raised a review or consider, and whose -/// block has not been located yet; hardcoded-value functions raised to a -/// review or consider whose value has not been named yet. -pub fn unlocated_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { +/// Security units whose finding a confirm Choice of their own follows, not +/// yet asked: an injection finding that rests on a path (what its paths can +/// hold) and a sensitive-data finding its log checks raised (when the log +/// line runs). +pub fn unconfirmed_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { plan.units .iter() .filter(|u| u.presence == Presence::Judged) .filter(|u| answers(judgments, &u.id, Pass::Locate).is_empty()) .filter(|u| { + let Detail::Security { paths, logging, .. } = &u.detail else { + return false; + }; let (outcome, resolved) = resolved(u, judgments); - let raised = - |o: Option| matches!(o, Some(Outcome::Review(_) | Outcome::Consider(_))); - match &u.detail { - Detail::Values { - locate: Some(_), .. - } - | Detail::Constants { - locate: Some(_), .. - } => raised(Some(outcome)), - Detail::TestPair { - confirm: Some(_), .. - } => matches!(outcome, Outcome::Review(_)), - // Only the internal-details check raises a test's consider. - Detail::Test { confirm: Some(_) } => matches!(outcome, Outcome::Consider(_)), - // An injection consider rests on the function's parameters - // unless its origin was another party. - Detail::Security { - confirm: Some(_), .. - } => { - matches!(outcome, Outcome::Consider(_)) - && !resolved - .get("origin") - .is_some_and(|a| matches!(origin_outcome(a), Outcome::Review(_))) - } - Detail::Function { - locate: Some(_), .. - } => raised(resolved.get("split").map(|a| benefit(a))), - Detail::Document { - locate: Some(_), .. - } => raised( - resolved - .get("split") - .map(|a| document_split(a, resolved.get("kind").copied())), - ), - _ => false, - } + let get = |q: &str| resolved.get(q).copied(); + matches!(outcome, Outcome::Review(_) | Outcome::Consider(_)) + && (paths.is_some() && rests_on_paths(&get) + || logging.is_some() && logs_found(&get)) }) .map(|u| u.id.clone()) .collect() } +/// Functions whose split question raised a review or consider, and whose +/// block has not been located yet; hardcoded-value functions raised to a +/// review or consider whose value has not been named yet; and the other +/// units whose confirm `locate_due` calls for. +pub fn unlocated_units(plan: &FilePlan, judgments: &[Judgment]) -> BTreeSet { + plan.units + .iter() + .filter(|u| u.presence == Presence::Judged) + .filter(|u| answers(judgments, &u.id, Pass::Locate).is_empty()) + .filter(|u| locate_due(u, judgments)) + .map(|u| u.id.clone()) + .collect() +} + +/// Whether a unit's outcome calls for its locate or confirm follow-up. +fn locate_due(unit: &UnitPlan, judgments: &[Judgment]) -> bool { + let (outcome, resolved) = resolved(unit, judgments); + let raised = |o: Option| matches!(o, Some(Outcome::Review(_) | Outcome::Consider(_))); + match &unit.detail { + Detail::Values { + locate: Some(_), .. + } + | Detail::Constants { + locate: Some(_), .. + } => raised(Some(outcome)), + Detail::TestPair { + confirm: Some(_), .. + } => matches!(outcome, Outcome::Review(_)), + // Only the internal-details check raises a test's consider. + Detail::Test { confirm: Some(_) } => matches!(outcome, Outcome::Consider(_)), + // An injection consider rests on the function's parameters unless + // its origin was another party; one that rests on a path is asked + // what its paths can hold instead. + Detail::Security { + confirm: Some(_), .. + } => { + matches!(outcome, Outcome::Consider(_)) + && !resolved + .get("origin") + .is_some_and(|a| matches!(origin_outcome(a), Outcome::Review(_))) + && !rests_on_paths(&|q| resolved.get(q).copied()) + } + Detail::Function { + locate: Some(_), .. + } => raised(resolved.get("split").map(|a| benefit(a))), + Detail::Document { + locate: Some(_), .. + } => raised( + resolved + .get("split") + .map(|a| document_split(a, resolved.get("kind").copied())), + ), + _ => false, + } +} + /// Judged units whose first pass stayed undecided, or became a note from a /// torn function or file-organization answer, and that have no recheck yet; /// for injection, units whose traced origin stayed unclear or was the diff --git a/src/units/follow_ups.rs b/src/units/follow_ups.rs index 6b4376f..e305edd 100644 --- a/src/units/follow_ups.rs +++ b/src/units/follow_ups.rs @@ -7,11 +7,24 @@ use std::collections::BTreeSet; /// One locate follow-up per function whose split raised a review or consider, /// per hardcoded-value function raised to a review or consider, per redundant -/// test pair raised to a review, per test that asserts internal details, and -/// per injection consider that rests on its parameters. +/// test pair raised to a review, per test that asserts internal details, per +/// injection consider that rests on its parameters, per injection finding +/// that rests on a path and per logging finding. pub fn locates(plan: &Plan, files: &[FileResult]) -> Vec { - follow_ups(plan, files, compose::unlocated_units, |unit| { - match &unit.detail { + let mut planned = follow_ups( + plan, + files, + compose::unconfirmed_units, + |unit| match &unit.detail { + Detail::Security { paths, logging, .. } => paths.as_ref().or(logging.as_ref()), + _ => None, + }, + ); + planned.extend(follow_ups( + plan, + files, + compose::unlocated_units, + |unit| match &unit.detail { Detail::Function { locate, .. } | Detail::Document { locate, .. } | Detail::Values { locate, .. } @@ -20,8 +33,9 @@ pub fn locates(plan: &Plan, files: &[FileResult]) -> Vec { | Detail::Test { confirm } | Detail::Security { confirm, .. } => confirm.as_ref(), _ => None, - } - }) + }, + )); + planned } /// One question per hardcoded-value consider resting on a value's name diff --git a/src/units/mod.rs b/src/units/mod.rs index f90300d..e7dc9de 100644 --- a/src/units/mod.rs +++ b/src/units/mod.rs @@ -193,6 +193,12 @@ pub enum Detail { /// For injection, what the values it places can hold, asked only /// after a consider that rests on its parameters. confirm: Option, + /// For injection, what the variable parts of its file paths can + /// hold, asked only after a finding that rests on a path. + paths: Option, + /// For sensitive data, when its log line runs, asked only after a + /// finding its log checks raised. + logging: Option, /// Django code, asked the Django checks: a weak setting must be /// named by one of them. django: bool, diff --git a/src/units/outcome/exposure.rs b/src/units/outcome/exposure.rs index b95e747..bfd568b 100644 --- a/src/units/outcome/exposure.rs +++ b/src/units/outcome/exposure.rs @@ -51,6 +51,27 @@ pub(in crate::units) fn messages<'a>( }) } +/// The sensitive-data signals that a function writes a value to a log. +pub(in crate::units) const LOG_SIGNALS: [&str; 2] = ["logs_secret", "logs_object_secret"]; + +/// Whether a log signal reached a finding: its log line is then asked when +/// it runs. +pub(in crate::units) fn logs_found<'a>(get: &impl Fn(&str) -> Option<&'a Answer>) -> bool { + LOG_SIGNALS + .iter() + .any(|q| matches!(get(q).map(noul), Some(Outcome::Review(_)))) +} + +/// Whether the log line runs only when an operator turns on a setting whose +/// purpose is that logging, at the threshold. The operator chose to log the +/// value, so its log signals are at most a note. On the corpus it cleared +/// vaultwarden's SSO tokens under `SSO_DEBUG_TOKENS` and pgweb's request +/// logger, both labeled wrong; every log finding labeled right was asked it +/// and answered normal operation or debug level. +pub(in crate::units) fn opted_in<'a>(get: &impl Fn(&str) -> Option<&'a Answer>) -> bool { + choice_mass(get("logged_when"), &[questions::OPT_IN_LOGGING]).is_some_and(at_least) +} + /// A judged exposure answer and how far it leans toward its concern. type Signal = (Outcome, f64); @@ -91,7 +112,18 @@ fn exposure_signals<'a>( .then(|| messages(get)) .flatten(); let away = rule == catalog::SENSITIVE_DATA && away_from_clients(get); - let judge = |question: &str, answer: &Answer| exposure_signal(question, answer, own, away); + let opted_in = rule == catalog::SENSITIVE_DATA && opted_in(get); + let judge = |question: &str, answer: &Answer| { + let signal = exposure_signal(question, answer, own, away); + match signal { + (Outcome::Review(p) | Outcome::Consider(p), lean) + if opted_in && LOG_SIGNALS.contains(&question) => + { + (Outcome::Note(p), lean) + } + signal => signal, + } + }; // A Choice asked whenever a presence signal is not clear rules it out // too: what a function's logs write clears an audit line that names who // signed in, which the presence question found as personal data. @@ -155,10 +187,20 @@ fn exposure_level(rule: &str, presence: &[Signal], specific: &[Signal]) -> Outco !probability_at_least(*lean, LEADING_PROBABILITY) && !matches!(o, Outcome::Review(_) | Outcome::Consider(_)) }); + // A signal already lowered to a note, such as a log line an operator + // turned on to log that value. + let noted = presence + .iter() + .chain(specific) + .filter(|(o, _)| matches!(o, Outcome::Note(_))) + .map(|(o, _)| o.concern()) + .reduce(f64::max); if unnamed && !found.is_empty() { Outcome::Note(strongest(&found).concern()) } else if !found.is_empty() { strongest(&found) + } else if let Some(p) = noted { + Outcome::Note(p) } else if ruled_out { Outcome::Clear } else if probability_at_least(leaning, LEADING_PROBABILITY) { diff --git a/src/units/outcome/injection.rs b/src/units/outcome/injection.rs index c459c86..feb518f 100644 --- a/src/units/outcome/injection.rs +++ b/src/units/outcome/injection.rs @@ -62,10 +62,33 @@ pub(in crate::units) fn injection_outcome<'a>( }; Some(match by_origin(origin, &found, get) { Outcome::Consider(p) if program_values(get) => Outcome::Note(p), + Outcome::Review(p) | Outcome::Consider(p) if confined_paths(get) => Outcome::Note(p), outcome => outcome, }) } +/// Whether every injection check that found a variable placed unhandled is +/// the path check: such a finding is asked what its paths can hold. +pub(in crate::units) fn rests_on_paths<'a>(get: &impl Fn(&str) -> Option<&'a Answer>) -> bool { + let found = found_injections(get); + !found.is_empty() && found.iter().all(|id| *id == "path") +} + +/// Whether a path finding's paths, asked after it, lean toward names that +/// stay inside their directory, the program's own or the local user's: a +/// route parameter parsed as a UUID or as Rocket's `PathBuf`, a base name or +/// a checked id. Such a finding is a note. On the corpus, the 5 path +/// findings labeled right (request parameters and uploaded names joined to +/// a directory) answered another party's input at 0.96 or more, while +/// vaultwarden's 4 wrong ones on typed Rocket route parameters leaned to +/// confined names at 0.67 to 0.78; none reached the threshold, as a type's +/// parsing is shown only by its derive list. +pub(in crate::units) fn confined_paths<'a>(get: &impl Fn(&str) -> Option<&'a Answer>) -> bool { + rests_on_paths(get) + && choice_mass(get("paths"), &questions::CONFINED_PATHS) + .is_some_and(|p| probability_at_least(p, LEADING_PROBABILITY)) +} + /// Whether what a consider's values can hold, asked after it, leans toward /// the program's own: literals its callers pass, values it creates, or the /// arguments of a local tool. Such a consider, resting on the function's diff --git a/src/units/outcome/mod.rs b/src/units/outcome/mod.rs index bdb0a52..da199ac 100644 --- a/src/units/outcome/mod.rs +++ b/src/units/outcome/mod.rs @@ -27,9 +27,12 @@ pub(super) use comments::{comment_concern_kind, comment_outcome, comment_signals use documentation::staleness_outcome; pub(super) use documentation::{document_outcome, document_split, section_signals}; pub(super) use exposure::{ - Messages, django_settings_outcome, exposure_outcome, messages, settings_module_outcome, + Messages, django_settings_outcome, exposure_outcome, logs_found, messages, opted_in, + settings_module_outcome, +}; +pub(super) use injection::{ + RESOURCE_CHECKS, confined_paths, injection_outcome, origin_outcome, rests_on_paths, }; -pub(super) use injection::{RESOURCE_CHECKS, injection_outcome, origin_outcome}; pub(super) use maintainability::{ benign_key, function_outcome, organization_outcome, several_kind, shared_outcome, value_signals, values_outcome, diff --git a/src/units/plan/security_units.rs b/src/units/plan/security_units.rs index 92b3fe3..19e95fc 100644 --- a/src/units/plan/security_units.rs +++ b/src/units/plan/security_units.rs @@ -111,8 +111,13 @@ fn function_subject<'a>( } else { Vec::new() }; - let mut subject = - security::function_subject(context, unit, callers, &shared.enums, &shared.constants); + let mut subject = security::function_subject( + context, + unit, + callers, + (&shared.enums, &shared.types), + &shared.constants, + ); if rules.contains(&catalog::SENSITIVE_DATA) { subject.callee_errors = callee_errors(scope, &shared.links, context.owner, unit); } diff --git a/src/units/plan/shared.rs b/src/units/plan/shared.rs index ef7faa4..a6975b1 100644 --- a/src/units/plan/shared.rs +++ b/src/units/plan/shared.rs @@ -2,7 +2,7 @@ //! subjects and their sources, routes, helpers, enums, constants and hashes. use super::{ Scope, java, - trace_evidence::{csharp_constants, enums}, + trace_evidence::{csharp_constants, enums, types}, }; use crate::{ analysis::{ @@ -45,6 +45,7 @@ pub(super) struct Shared<'a> { pub(super) cases: BTreeMap>, /// Enum definitions by name, from selected files and context, for security traces. pub(super) enums: BTreeMap, + pub(super) types: BTreeMap, /// C# constants by field name, as `Class.Field = value`, for security traces. pub(super) constants: BTreeMap>, pub(super) hashes: BTreeMap, @@ -117,6 +118,7 @@ impl<'a> Shared<'a> { module_helpers: BTreeMap::new(), cases, enums: BTreeMap::new(), + types: BTreeMap::new(), constants: BTreeMap::new(), hashes: source_hashes(scope), teaching: false, @@ -138,6 +140,7 @@ impl<'a> Shared<'a> { } if shared.enabled(catalog::INJECTION) { shared.enums = enums(scope); + shared.types = types(scope); } if shared.enabled(catalog::UNSAFE_SETTINGS) { shared.constants = csharp_constants(scope); diff --git a/src/units/plan/trace_evidence.rs b/src/units/plan/trace_evidence.rs index 6d7ccc8..e450757 100644 --- a/src/units/plan/trace_evidence.rs +++ b/src/units/plan/trace_evidence.rs @@ -1,6 +1,7 @@ //! Definitions across the scope that a security trace shows beside a site: //! enums the site names and C# constants, often declared in another file. use super::Scope; +use crate::analysis::units::Unit; use std::collections::BTreeMap; /// An enum shown with a security trace is at most this long. @@ -9,6 +10,42 @@ const ENUM_BYTES: usize = 1500; /// Enum definitions in selected files and context by name; a name defined /// twice is left out, since the site could mean either. pub(super) fn enums(scope: &Scope<'_>) -> BTreeMap { + definitions(scope, |unit, source| { + let text = unit.source(source); + (declaration(unit, text).contains("enum ") && text.len() <= ENUM_BYTES) + .then(|| text.to_string()) + }) +} + +/// A type shown with a path confirm is at most this long. +const TYPE_BYTES: usize = 800; + +/// Definitions of the types in selected files and context (structs, +/// classes, records; not enums) by name, with the documentation and +/// attributes above them, such as a Rust derive list that says how a route +/// parameter of that type is parsed; a name defined twice is left out. +pub(super) fn types(scope: &Scope<'_>) -> BTreeMap { + definitions(scope, |unit, source| { + let text = source.get(unit.span.clone())?; + (!declaration(unit, text).contains("enum ") && text.len() <= TYPE_BYTES) + .then(|| text.to_string()) + }) +} + +/// The line of a definition that names it. +fn declaration<'t>(unit: &Unit, text: &'t str) -> &'t str { + text.lines() + .find(|l| l.contains(&unit.short_name)) + .unwrap_or("") +} + +/// The definitions `shown` keeps among the types, enums and other +/// non-callable units of selected files and context, by short name; a name +/// defined twice is left out. +fn definitions( + scope: &Scope<'_>, + shown: impl Fn(&Unit, &str) -> Option, +) -> BTreeMap { let selected = scope.owners.iter().map(|&owner| { ( scope.inputs[owner].source.as_deref().unwrap_or(""), @@ -22,16 +59,11 @@ pub(super) fn enums(scope: &Scope<'_>) -> BTreeMap { let mut found: BTreeMap> = BTreeMap::new(); for (source, units) in selected.chain(context) { for unit in units.iter().filter(|u| !u.callable()) { - let text = unit.source(source); - let declaration = text - .lines() - .find(|l| l.contains(&unit.short_name)) - .unwrap_or(""); - if declaration.contains("enum ") && text.len() <= ENUM_BYTES { + if let Some(text) = shown(unit, source) { found .entry(unit.short_name.clone()) .and_modify(|d| *d = None) - .or_insert_with(|| Some(text.to_string())); + .or_insert(Some(text)); } } } diff --git a/src/units/questions/security.rs b/src/units/questions/security.rs index 7dee56a..bb2d876 100644 --- a/src/units/questions/security.rs +++ b/src/units/questions/security.rs @@ -225,6 +225,72 @@ pub fn injection_values(code: &str, callers: bool) -> Value { /// The options of `injection_values` that hold only the program's own values. pub const PROGRAM_VALUES: [&str; 3] = ["fixed", "own", "local"]; +/// What the variable parts of a path finding's paths can hold, asked only +/// for an injection finding whose check found a path, with its callers and +/// the definitions of the project's types its parameters name. On +/// vaultwarden, Rocket route parameters typed `PathBuf` (which Rocket parses +/// so they cannot climb above where they are joined) and id types whose +/// parsing accepts only a UUID were four wrong path reviews: the path check +/// reads a variable joined to a directory, whatever the variable can hold. +pub fn injection_paths(code: &str, callers: bool, types: bool) -> Value { + let types_note = if types { + " `types_named_in_parameters` holds the definitions of the project's types that its parameters name, with their attributes." + } else { + "" + }; + let lead = if callers { + format!("{CALLERS}{types_note}") + } else { + types_note.trim_start().to_string() + }; + let note = if lead.is_empty() { + EVIDENCE.to_string() + } else { + format!("{lead} {EVIDENCE}") + }; + json!({ + "type": "choice", + "instructions": { + "question": format!("What can the variable parts of the file paths that `{code}` opens, writes or deletes hold?"), + "note": note, + }, + "criteria": { + "confined": "Only names that cannot leave the directory they are joined to: numbers, UUIDs or ids that a type or the web framework parses before the function runs, names reduced to a base name or checked against a pattern, or a path parameter the framework parses so it cannot climb above where it is joined, such as a Rocket `PathBuf` route segment, which rejects hidden and encoded-slash segments and drops `..` at its start.", + "own": "Names the program chooses or keeps for itself, or reads from its configuration.", + "local": "The command line, settings or files of the person running a local program or script.", + "outside": "A name or path another party sets that can hold `..`, a slash or an absolute path, such as a request parameter or field read as text, an uploaded file's name or an archive entry.", + "unknown": "Values from parameters or calls whose origin is not shown, which may hold any of these.", + }, + }) +} + +/// The options of `injection_paths` that keep a path inside its directory. +pub const CONFINED_PATHS: [&str; 3] = ["confined", "own", "local"]; + +/// When a logging finding's log line runs, asked only for a sensitive-data +/// finding raised by its log checks. vaultwarden logs SSO tokens inside +/// `if CONFIG.sso_debug_tokens()`, a setting off by default and documented +/// for logging them while troubleshooting: logging an identifier instead, +/// as the finding says, would remove the feature. +pub fn logged_when(code: &str) -> Value { + json!({ + "type": "choice", + "instructions": { + "question": format!("When does `{code}` write the secret or personal value to a log?"), + "note": EVIDENCE, + }, + "criteria": { + "always": "Whenever that code runs, at a level the program logs at in normal operation, such as info, warning or error.", + "debug": "Only at debug or trace level, which an operator may turn on to troubleshoot.", + "opt_in": "Only when an operator turns on a setting, off by default, whose purpose is to log these values for troubleshooting, such as an option named for logging tokens or request bodies.", + "none": "It writes no secret or personal value to a log.", + }, + }) +} + +/// The option of `logged_when` for a setting whose purpose is the logging. +pub const OPT_IN_LOGGING: &str = "opt_in"; + /// Asked in the sensitive-data trace: whether every error message is the /// program's own. It can only clear the error-detail signals; functions that /// throw the program's typed errors otherwise stayed undecided, since the diff --git a/src/units/security.rs b/src/units/security.rs index f135e4a..e34c2a2 100644 --- a/src/units/security.rs +++ b/src/units/security.rs @@ -43,6 +43,10 @@ pub(super) struct Subject<'a> { /// defined in this or another selected file: fixed choices, not /// parameters, which the trace otherwise could not tell apart. pub enums: Vec, + /// Definitions of the project's types its parameters name, shown when a + /// path finding is confirmed: how a route parameter of that type is + /// parsed decides what it can hold. + pub types: Vec, /// C# constants it names, as `Class.Field = value`: a key written in /// the code or a value read from configuration. pub constants: Vec, @@ -90,7 +94,7 @@ pub(super) fn function_subject<'a>( file: &FileContext<'_>, unit: &'a Unit, callers: Vec<(String, String)>, - enums: &BTreeMap, + (enums, types): (&BTreeMap, &BTreeMap), constants: &BTreeMap>, ) -> Subject<'a> { let source = unit.source(file.source).to_string(); @@ -104,6 +108,7 @@ pub(super) fn function_subject<'a>( lines: (unit.line, unit.end_line), callers, enums: named_enums(&unit.sites, enums), + types: named_types(&unit.signature, types), evidence: serde_json::Map::new(), django: false, callee_errors: Vec::new(), @@ -139,6 +144,25 @@ fn named_constants( found } +/// Type definitions shown with one subject, at most. +const TYPES: usize = 3; + +/// The definitions of the project's types named as words in a signature. +fn named_types(signature: &str, types: &BTreeMap) -> Vec { + let word = |c: Option| c.is_some_and(|c| c.is_alphanumeric() || c == '_'); + types + .iter() + .filter(|(name, _)| { + signature.match_indices(name.as_str()).any(|(at, _)| { + !word(signature[..at].chars().next_back()) + && !word(signature[at + name.len()..].chars().next()) + }) + }) + .map(|(_, definition)| definition.clone()) + .take(TYPES) + .collect() +} + /// Enum definitions shown with one subject, at most. const ENUMS: usize = 3; @@ -191,6 +215,7 @@ pub(super) fn setup_subject<'a>( lines: (first.1, last.2), callers: Vec::new(), enums: Vec::new(), + types: Vec::new(), evidence: serde_json::Map::new(), django: false, callee_errors: Vec::new(), @@ -220,6 +245,7 @@ pub(super) fn template_subject<'a>( lines: (first.1, last.2), callers: Vec::new(), enums: Vec::new(), + types: Vec::new(), constants: Vec::new(), evidence: serde_json::Map::new(), django: false, @@ -328,6 +354,12 @@ fn push_unit( let confirm = (rule == INJECTION) .then(|| confirm(file, subject, id)) .flatten(); + let paths = (rule == INJECTION) + .then(|| confirm_paths(file, subject, id)) + .flatten(); + let logging = (rule == SENSITIVE_DATA) + .then(|| confirm_logging(file, subject, id)) + .flatten(); let settles = settles(file, subject, rule, id); out.units.push(UnitPlan { rule, @@ -348,6 +380,8 @@ fn push_unit( trace: trace.map(Into::into), settles, confirm: confirm.map(Into::into), + paths: paths.map(Into::into), + logging: logging.map(Into::into), django: subject.django, test_path: subject.test_path, }, @@ -394,12 +428,16 @@ fn send( trace, settles, confirm, + paths, + logging, .. } = &mut unit.detail { *trace = None; settles.clear(); *confirm = None; + *paths = None; + *logging = None; } } } @@ -773,6 +811,61 @@ fn confirm(file: &FileContext<'_>, subject: &Subject<'_>, id: &str) -> Option<(V file.budget.fits(&request).then_some((request, asked)) } +/// What the variable parts of the paths a path finding rests on can hold, +/// asked only after such a finding: the function, the functions that call +/// it and the project's types its parameters name. +fn confirm_paths( + file: &FileContext<'_>, + subject: &Subject<'_>, + id: &str, +) -> Option<(Value, Asked)> { + let code = subject.code(); + let mut questions = Questions::default(); + questions.ask( + "paths".into(), + questions::injection_paths( + &code, + !subject.callers.is_empty(), + !subject.types.is_empty(), + ), + id, + INJECTION, + "paths", + Pass::Locate, + ); + let mut state = with_callers(file, subject); + if !subject.types.is_empty() { + state["types_named_in_parameters"] = json!(subject.types); + } + let (request, asked) = file.request("locate", state, questions); + file.budget.fits(&request).then_some((request, asked)) +} + +/// When the log line of a logging finding runs, asked only after such a +/// finding: the function alone. +fn confirm_logging( + file: &FileContext<'_>, + subject: &Subject<'_>, + id: &str, +) -> Option<(Value, Asked)> { + let code = subject.code(); + let mut questions = Questions::default(); + questions.ask( + "logged_when".into(), + questions::logged_when(&code), + id, + SENSITIVE_DATA, + "logged_when", + Pass::Locate, + ); + let state = json!({ + "file": file.file_state(), + subject.kind: subject.state(), + }); + let (request, asked) = file.request("locate", state, questions); + file.budget.fits(&request).then_some((request, asked)) +} + /// A Choice asked when one of a rule's checks stays undecided after the /// trace and recheck; it can only clear the checks it settles, so it is /// asked apart from them. diff --git a/src/units/tests/security.rs b/src/units/tests/security.rs index 7d0f184..aff7121 100644 --- a/src/units/tests/security.rs +++ b/src/units/tests/security.rs @@ -1629,3 +1629,102 @@ fn a_template_writing_client_data_unescaped_is_judged_as_template_code() { assert_eq!(code["source"], "<%= raw cookies[:font] %>"); assert_eq!(plan.files[&0].units[0].locations[0].start_line, 2); } + +/// A Rocket route joining an id its type parses as a UUID to a directory. +const DOWNLOAD: &str = "#[derive(Clone, UuidFromParam)]\npub struct FileId(String);\n\n#[get(\"/files/\")]\nasync fn download(id: FileId) -> Option {\n let path = Path::new(\"data\").join(id.as_ref());\n NamedFile::open(path).await.ok()\n}\n"; + +/// The options of the Choice on what a path finding's paths can hold. +const PATHS: [&str; 5] = ["confined", "local", "outside", "own", "unknown"]; + +#[test] +fn a_path_finding_is_a_note_when_its_paths_stay_in_their_directory() { + let (project, mut options) = security_project(DOWNLOAD); + let (_, plan) = planned(&project, &options); + let paths = plan.files[&0] + .units + .iter() + .find_map(|u| match &u.detail { + Detail::Security { + paths: Some(paths), .. + } => Some(paths.request()), + _ => None, + }) + .expect("an injection unit with a path confirm"); + assert!( + paths["state"]["types_named_in_parameters"][0] + .as_str() + .unwrap() + .contains("UuidFromParam"), + "the parameter's type is shown with its derive list: {paths}" + ); + let mut judged = |choice: Value| { + let mut eval = scripted(0); + eval.overrides = vec![ + ("resource", noul_at(0.95)), + ("path", noul_at(0.95)), + ("origin", spread(0.0, 0.0, 1.0)), + ("paths", choice), + ]; + let report = run(&project, &options, &mut eval); + options.refresh = true; + report.files[0] + .findings + .iter() + .find(|f| f.rule == "security/injection") + .map(|f| (f.strength, f.message.clone())) + }; + assert_eq!( + judged(choice_of("outside", &PATHS)).map(|f| f.0), + Some(Strength::Review) + ); + let (strength, message) = judged(choice_of("confined", &PATHS)).unwrap(); + assert_eq!( + strength, + Strength::Note, + "a UUID cannot climb out of the directory" + ); + assert!( + message.contains("likely keeps the path inside"), + "{message}" + ); +} + +/// Tokens logged only under a setting that exists to log them. +const TOKENS: &str = "fn exchange(token: &str) -> String {\n if CONFIG.sso_debug_tokens() {\n debug!(\"Access token {token}\");\n }\n token.to_string()\n}\n"; + +#[test] +fn a_log_line_an_operator_turns_on_to_log_tokens_is_a_note() { + let (project, mut options) = security_project(TOKENS); + let logs = [ + "plain", "identity", "operator", "secret", "personal", "none", + ]; + let when = ["always", "debug", "none", "opt_in"]; + let mut judged = |chosen: &str| { + let mut eval = scripted(0); + eval.overrides = vec![ + ("logs_secret", noul_at(0.95)), + ("logs_object_secret", noul_at(0.95)), + ("logged", choice_of("secret", &logs)), + ("logged_when", choice_of(chosen, &when)), + ]; + let report = run(&project, &options, &mut eval); + options.refresh = true; + let file = &report.files[0]; + let messages: String = file.findings.iter().map(|f| f.message.clone()).collect(); + ( + file.dimensions[catalog::SENSITIVE_DATA].status.clone(), + messages, + ) + }; + assert_eq!( + judged("debug").0, + Status::Review, + "debug level is still a log" + ); + let (status, message) = judged("opt_in"); + assert_eq!(status, Status::Note); + assert!( + message.contains("only when an operator turns on"), + "{message}" + ); +} diff --git a/src/units/wording/security.rs b/src/units/wording/security.rs index aedfb9f..2d73c75 100644 --- a/src/units/wording/security.rs +++ b/src/units/wording/security.rs @@ -390,6 +390,18 @@ fn injection_wording( }; return ((message, action), category.to_string()); } + let get = |q: &str| answers.get(q).copied(); + if strength == Strength::Note && crate::units::outcome::confined_paths(&get) { + return ( + ( + format!( + "{subject} builds {noun} from a variable, but what it can hold, such as an id its type parses, likely keeps the path inside its directory." + ), + "Optional: confirm the value cannot hold `..` or a slash where it enters", + ), + category.to_string(), + ); + } let message = match (strength, outside) { (Strength::Review, _) => format!( "{subject} places values from another party into {noun} without binding, escaping or checking them ({p:.2})." @@ -476,6 +488,18 @@ fn exposure_wording( Some(crate::units::outcome::Messages::Foreign) ); let decided = crate::policy::probability_at_least(p, crate::policy::REVIEW_PROBABILITY); + let opted_in = category.starts_with("CWE-532") && crate::units::outcome::opted_in(&get); + if strength == Strength::Note && opted_in { + return ( + ( + format!( + "{subject} {what} only when an operator turns on a setting meant for logging it." + ), + "Optional: keep that setting off by default and document that it logs secrets", + ), + category.to_string(), + ); + } let message = match strength { // A decided finding lowered because the code runs only in // development or tests; its answer was not split.