From b0692eabb3c894e4cb688bf065352e4183a79745 Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sun, 27 Sep 2026 13:05:20 -0300 Subject: [PATCH 1/2] Ask markup and redirect findings what their values hold and where they lead --- src/catalog.rs | 2 +- src/units/compose.rs | 37 +++++++++----- src/units/follow_ups.rs | 4 +- src/units/mod.rs | 7 +-- src/units/outcome/injection.rs | 55 ++++++++++++++------- src/units/outcome/mod.rs | 2 +- src/units/questions/security.rs | 64 ++++++++++++++++++++++++ src/units/security.rs | 40 +++++++-------- src/units/tests/nextjs.rs | 4 ++ src/units/tests/security.rs | 87 +++++++++++++++++++++++++++++++-- src/units/wording/security.rs | 34 +++++++++---- 11 files changed, 268 insertions(+), 68 deletions(-) diff --git a/src/catalog.rs b/src/catalog.rs index fc6a150..fee3786 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -292,7 +292,7 @@ pub fn rule_version(key: &str) -> &'static str { SHARED_LOGIC => "22", TEST_VALUE => "7", TEST_REDUNDANCY => "4", - INJECTION => "11", + INJECTION => "12", SENSITIVE_DATA => "8", HARDCODED_VALUES => "8", UNSAFE_SETTINGS => "6", diff --git a/src/units/compose.rs b/src/units/compose.rs index e9d98ca..992b03a 100644 --- a/src/units/compose.rs +++ b/src/units/compose.rs @@ -3,8 +3,8 @@ use super::{ Access, Block, Detail, FilePlan, Presence, UnitPlan, outcome::{ - Answers, Outcome, at_most_note, benefit, checks, choice, choice_mass, document_split, - logs_found, lowered, noul, open, origin_outcome, rests_on_paths, score, settled_checks, + Answers, Outcome, at_most_note, benefit, checks, choice, choice_mass, confirmable, + document_split, logs_found, lowered, noul, open, origin_outcome, score, settled_checks, several_kind, unit_outcome, value_signals, }, wording::{Wording, comment_reason, comment_wording}, @@ -250,28 +250,44 @@ fn rechecked<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> (Outcome, Answer } /// 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). +/// yet asked: an injection finding whose one concern is a path (what its +/// paths can hold), or markup or a redirect unless its values are asked +/// already (what they hold, where they lead), 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 { + let Detail::Security { + checked, logging, .. + } = &u.detail + else { return false; }; let (outcome, resolved) = resolved(u, judgments); let get = |q: &str| resolved.get(q).copied(); + let kind = confirmable(&get); matches!(outcome, Outcome::Review(_) | Outcome::Consider(_)) - && (paths.is_some() && rests_on_paths(&get) + && (checked.is_some() + && (kind == Some("path") || kind.is_some() && !values_due(outcome, &resolved)) || logging.is_some() && logs_found(&get)) }) .map(|u| u.id.clone()) .collect() } +/// Whether an injection outcome calls for what its values can hold: a +/// consider that rests on the function's parameters, its origin not +/// another party. +fn values_due(outcome: Outcome, resolved: &Answers<'_>) -> bool { + matches!(outcome, Outcome::Consider(_)) + && !resolved + .get("origin") + .is_some_and(|a| matches!(origin_outcome(a), Outcome::Review(_))) +} + /// 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 @@ -308,11 +324,8 @@ fn locate_due(unit: &UnitPlan, judgments: &[Judgment]) -> bool { 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()) + values_due(outcome, &resolved) + && confirmable(&|q| resolved.get(q).copied()) != Some("path") } Detail::Function { locate: Some(_), .. diff --git a/src/units/follow_ups.rs b/src/units/follow_ups.rs index e305edd..bb2f676 100644 --- a/src/units/follow_ups.rs +++ b/src/units/follow_ups.rs @@ -16,7 +16,9 @@ pub fn locates(plan: &Plan, files: &[FileResult]) -> Vec { files, compose::unconfirmed_units, |unit| match &unit.detail { - Detail::Security { paths, logging, .. } => paths.as_ref().or(logging.as_ref()), + Detail::Security { + checked, logging, .. + } => checked.as_ref().or(logging.as_ref()), _ => None, }, ); diff --git a/src/units/mod.rs b/src/units/mod.rs index e7dc9de..e1f61e4 100644 --- a/src/units/mod.rs +++ b/src/units/mod.rs @@ -193,9 +193,10 @@ 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 injection, what the values of a path, markup or redirect + /// finding can hold or where they lead, asked only after a finding + /// whose one concern is one of those. + checked: Option, /// For sensitive data, when its log line runs, asked only after a /// finding its log checks raised. logging: Option, diff --git a/src/units/outcome/injection.rs b/src/units/outcome/injection.rs index feb518f..dc5d185 100644 --- a/src/units/outcome/injection.rs +++ b/src/units/outcome/injection.rs @@ -62,31 +62,50 @@ 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::Review(p) | Outcome::Consider(p) if harmless(get).is_some() => 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 { +/// The kinds of injection whose values a confirm Choice asks about after +/// the finding, with its question and the options that make it a note. +const CONFIRMED: [(&str, &str, &[&str]); 3] = [ + ("path", "paths", &questions::CONFINED_PATHS), + ("markup", "markup_values", &questions::HARMLESS_MARKUP), + ("redirect", "redirect_reach", &questions::OWN_SITE), +]; + +/// The one kind of injection a finding rests on, when every check that +/// found a variable placed unhandled is that kind and a confirm Choice asks +/// about it. +pub(in crate::units) fn confirmable<'a>( + get: &impl Fn(&str) -> Option<&'a Answer>, +) -> Option<&'static str> { let found = found_injections(get); - !found.is_empty() && found.iter().all(|id| *id == "path") + let first = *found.first()?; + CONFIRMED + .iter() + .find(|(kind, ..)| *kind == first && found.iter().all(|id| id == kind)) + .map(|(kind, ..)| *kind) } -/// 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)) +/// The kind of a finding whose confirm Choice leans toward values that can +/// do no harm there: paths that stay inside their directory (a route +/// parameter parsed as a UUID or as Rocket's `PathBuf`, a base name, a +/// checked id), markup values already escaped or encoded, or redirect +/// targets that stay on the site. Such a finding is a note. On the corpus, +/// the 5 path findings labeled right 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, as a type's parsing +/// is shown only by its derive list. +pub(in crate::units) fn harmless<'a>( + get: &impl Fn(&str) -> Option<&'a Answer>, +) -> Option<&'static str> { + let kind = confirmable(get)?; + let (_, question, clears) = CONFIRMED.iter().find(|(k, ..)| *k == kind)?; + choice_mass(get(question), clears) + .is_some_and(|p| probability_at_least(p, LEADING_PROBABILITY)) + .then_some(kind) } /// Whether what a consider's values can hold, asked after it, leans toward diff --git a/src/units/outcome/mod.rs b/src/units/outcome/mod.rs index da199ac..d8e96e5 100644 --- a/src/units/outcome/mod.rs +++ b/src/units/outcome/mod.rs @@ -31,7 +31,7 @@ pub(super) use exposure::{ settings_module_outcome, }; pub(super) use injection::{ - RESOURCE_CHECKS, confined_paths, injection_outcome, origin_outcome, rests_on_paths, + RESOURCE_CHECKS, confirmable, harmless, injection_outcome, origin_outcome, }; pub(super) use maintainability::{ benign_key, function_outcome, organization_outcome, several_kind, shared_outcome, diff --git a/src/units/questions/security.rs b/src/units/questions/security.rs index bb2d876..4b64d28 100644 --- a/src/units/questions/security.rs +++ b/src/units/questions/security.rs @@ -267,6 +267,70 @@ pub fn injection_paths(code: &str, callers: bool, types: bool) -> Value { /// The options of `injection_paths` that keep a path inside its directory. pub const CONFINED_PATHS: [&str; 3] = ["confined", "own", "local"]; +/// What a markup finding's values hold where they enter the markup, asked +/// only after a finding whose one concern is markup. vaultwarden's +/// `hibp_breach` percent-encodes the username before it builds the link, +/// oak's examples write a URL object whose serialization percent-encodes +/// `<` and `>`, and a JSP page runs its own `esc()` first: the markup check +/// reads a variable joined into HTML, whatever it was turned into before. +pub fn markup_values(code: &str, callers: bool) -> Value { + let (by_callers, note) = if callers { + ( + " or by the functions in `callers`", + format!("{CALLERS} {EVIDENCE}"), + ) + } else { + ("", EVIDENCE.to_string()) + }; + json!({ + "type": "choice", + "instructions": { + "question": format!("What do the values that `{code}` places into HTML or SVG markup hold where they enter it?"), + "note": note, + }, + "criteria": { + "encoded": format!("Text already escaped for HTML, percent-encoded or serialized as a URL before it enters the markup, in this code{by_callers}, so it cannot hold `<`, `>`, `&` or quotes."), + "typed": "Numbers, dates, booleans or ids, or names chosen from a fixed list.", + "own": "Text the program writes itself or reads from its configuration.", + "raw": "Text as another party or a caller wrote it, which can hold `<`, `>`, `&` or quotes.", + "unknown": "Values whose origin or handling is not shown.", + }, + }) +} + +/// The options of `markup_values` that cannot open a tag or attribute. +pub const HARMLESS_MARKUP: [&str; 3] = ["encoded", "typed", "own"]; + +/// Where a redirect finding's targets can lead, asked only after a finding +/// whose one concern is a redirect. vaultwarden's admin login redirects to +/// its admin path followed by the form's value, and shiori's to its login +/// page with the current path as a query value: a fixed path before the +/// variable keeps the target on the site, which the redirect check does not +/// ask. +pub fn redirect_reach(code: &str, callers: bool) -> Value { + let note = if callers { + format!("{CALLERS} {EVIDENCE}") + } else { + EVIDENCE.to_string() + }; + json!({ + "type": "choice", + "instructions": { + "question": format!("Where can the targets that `{code}` redirects clients to lead?"), + "note": note, + }, + "criteria": { + "own_site": "Only to the program's own site: every target starts with a fixed path that has a single leading slash, or with the program's own origin and a slash, and variables only follow it or fill its query string.", + "checked": "Only where a check allows: the target is compared with an allowed list of hosts or checked to be a path on the site before the redirect.", + "anywhere": "Anywhere a variable says: a variable starts the target, or follows a fixed scheme and host with no slash between them, so it can name another host.", + "none": "It does not redirect.", + }, + }) +} + +/// The options of `redirect_reach` that keep a redirect on the site. +pub const OWN_SITE: [&str; 3] = ["own_site", "checked", "none"]; + /// 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 diff --git a/src/units/security.rs b/src/units/security.rs index e34c2a2..2b18526 100644 --- a/src/units/security.rs +++ b/src/units/security.rs @@ -354,8 +354,8 @@ fn push_unit( let confirm = (rule == INJECTION) .then(|| confirm(file, subject, id)) .flatten(); - let paths = (rule == INJECTION) - .then(|| confirm_paths(file, subject, id)) + let checked = (rule == INJECTION) + .then(|| confirm_checks(file, subject, id)) .flatten(); let logging = (rule == SENSITIVE_DATA) .then(|| confirm_logging(file, subject, id)) @@ -380,7 +380,7 @@ fn push_unit( trace: trace.map(Into::into), settles, confirm: confirm.map(Into::into), - paths: paths.map(Into::into), + checked: checked.map(Into::into), logging: logging.map(Into::into), django: subject.django, test_path: subject.test_path, @@ -428,7 +428,7 @@ fn send( trace, settles, confirm, - paths, + checked, logging, .. } = &mut unit.detail @@ -436,7 +436,7 @@ fn send( *trace = None; settles.clear(); *confirm = None; - *paths = None; + *checked = None; *logging = None; } } @@ -811,28 +811,28 @@ 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( +/// What the values of a path, markup or redirect finding can hold or where +/// they lead, asked only after a finding whose one concern is one of those; +/// only the question of its kind is read. The function, the functions that +/// call it and the project's types its parameters name. +fn confirm_checks( file: &FileContext<'_>, subject: &Subject<'_>, id: &str, ) -> Option<(Value, Asked)> { let code = subject.code(); + let callers = !subject.callers.is_empty(); let mut questions = Questions::default(); - questions.ask( - "paths".into(), - questions::injection_paths( - &code, - !subject.callers.is_empty(), - !subject.types.is_empty(), + for (question, body) in [ + ( + "paths", + questions::injection_paths(&code, callers, !subject.types.is_empty()), ), - id, - INJECTION, - "paths", - Pass::Locate, - ); + ("markup_values", questions::markup_values(&code, callers)), + ("redirect_reach", questions::redirect_reach(&code, callers)), + ] { + questions.ask(question.into(), body, id, INJECTION, question, Pass::Locate); + } let mut state = with_callers(file, subject); if !subject.types.is_empty() { state["types_named_in_parameters"] = json!(subject.types); diff --git a/src/units/tests/nextjs.rs b/src/units/tests/nextjs.rs index f418ec5..9497a69 100644 --- a/src/units/tests/nextjs.rs +++ b/src/units/tests/nextjs.rs @@ -64,6 +64,10 @@ fn an_unchecked_redirect_target_is_an_open_redirect_review() { ("resource", noul_at(0.95)), ("redirect", noul_at(0.95)), ("origin", spread(0.0, 0.05, 0.95)), + ( + "redirect_reach", + choice_of("anywhere", &["anywhere", "checked", "none", "own_site"]), + ), ]; let report = run(&project, &options, &mut eval); let file = report diff --git a/src/units/tests/security.rs b/src/units/tests/security.rs index aff7121..11ca6c2 100644 --- a/src/units/tests/security.rs +++ b/src/units/tests/security.rs @@ -460,6 +460,7 @@ fn redirects_deserializers_and_uploads_are_checked_kinds_with_their_weakness() { ("resource", noul_at(0.95)), (check, noul_at(0.95)), ("origin", spread(0.0, 0.05, 0.95)), + ("redirect_reach", choice_of("anywhere", &REACH)), ]; let report = run(&project, &options, &mut eval); let finding = &report.files[0].findings[0]; @@ -643,6 +644,7 @@ fn php_settled(markup: &str, path_check: f64, path: &str) -> (Status, u64) { ("origin", spread(0.4, 0.3, 0.3)), ("markup_parts", choice_of(markup, &MARKUP_PARTS)), ("path_parts", choice_of(path, &PATH_PARTS)), + ("markup_values", choice_of("raw", &MARKUP_VALUES)), ]; let report = run(&project, &options, &mut eval); ( @@ -1645,11 +1647,15 @@ fn a_path_finding_is_a_note_when_its_paths_stay_in_their_directory() { .iter() .find_map(|u| match &u.detail { Detail::Security { - paths: Some(paths), .. - } => Some(paths.request()), + checked: Some(checked), + .. + } => Some(checked.request()), _ => None, }) - .expect("an injection unit with a path confirm"); + .expect("an injection unit with a confirm of its checks"); + for question in ["paths", "markup_values", "redirect_reach"] { + assert!(paths["questions"].get(question).is_some(), "{question}"); + } assert!( paths["state"]["types_named_in_parameters"][0] .as_str() @@ -1728,3 +1734,78 @@ fn a_log_line_an_operator_turns_on_to_log_tokens_is_a_note() { "{message}" ); } + +/// A handler that percent-encodes a name before it builds a link, and one +/// that redirects to its admin path followed by a form value. +const ENCODED: &str = "fn breach(username: &str) -> String {\n let name: String = form_urlencoded::byte_serialize(username.as_bytes()).collect();\n format!(\"{name}\")\n}\n"; +const ADMIN: &str = "fn login(form: Form) -> Redirect {\n let target = form.redirect.clone();\n Redirect::to(format!(\"{}{target}\", admin_path()))\n}\n"; + +#[test] +fn markup_and_redirect_findings_are_notes_when_their_values_can_do_no_harm() { + let judged = |source: &str, check: &'static str, question: &'static str, choice: Value| { + let (project, options) = security_project(source); + let mut eval = scripted(0); + let presence = if check == "markup" { + "interpreted" + } else { + "resource" + }; + eval.overrides = vec![ + (presence, noul_at(0.95)), + (check, noul_at(0.95)), + ("origin", spread(0.0, 0.0, 1.0)), + (question, choice), + ]; + let report = run(&project, &options, &mut eval); + report.files[0] + .findings + .iter() + .find(|f| f.rule == "security/injection") + .map(|f| (f.strength, f.message.clone())) + .unwrap() + }; + let markup = MARKUP_VALUES; + assert_eq!( + judged( + ENCODED, + "markup", + "markup_values", + choice_of("raw", &markup) + ) + .0, + Strength::Review + ); + let (strength, message) = judged( + ENCODED, + "markup", + "markup_values", + choice_of("encoded", &markup), + ); + assert_eq!(strength, Strength::Note, "percent-encoded before the link"); + assert!(message.contains("escaped or encoded before"), "{message}"); + let reach = REACH; + assert_eq!( + judged( + ADMIN, + "redirect", + "redirect_reach", + choice_of("anywhere", &reach) + ) + .0, + Strength::Review + ); + let (strength, message) = judged( + ADMIN, + "redirect", + "redirect_reach", + choice_of("own_site", &reach), + ); + assert_eq!(strength, Strength::Note, "the admin path comes first"); + assert!(message.contains("keeps it on the site"), "{message}"); +} + +/// The options of the Choice on what a markup finding's values hold. +const MARKUP_VALUES: [&str; 5] = ["encoded", "own", "raw", "typed", "unknown"]; + +/// The options of the Choice on where a redirect finding's targets lead. +const REACH: [&str; 4] = ["anywhere", "checked", "none", "own_site"]; diff --git a/src/units/wording/security.rs b/src/units/wording/security.rs index 2d73c75..4094119 100644 --- a/src/units/wording/security.rs +++ b/src/units/wording/security.rs @@ -391,16 +391,32 @@ 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", + let harmless = (strength == Strength::Note) + .then(|| crate::units::outcome::harmless(&get)) + .flatten(); + let confirmed = match harmless { + Some("path") => Some(( + 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." ), - category.to_string(), - ); + "Optional: confirm the value cannot hold `..` or a slash where it enters", + )), + Some("markup") => Some(( + format!( + "{subject} places a variable into {noun}, but it was likely escaped or encoded before, so it cannot open a tag or attribute." + ), + "Optional: confirm the value is escaped on every path that reaches the markup", + )), + Some("redirect") => Some(( + format!( + "{subject} redirects clients to a target built from a variable, but a fixed path or check likely keeps it on the site." + ), + "Optional: confirm no target can start with `//` or another host", + )), + _ => None, + }; + if let Some(wording) = confirmed { + return (wording, category.to_string()); } let message = match (strength, outside) { (Strength::Review, _) => format!( From 2a48a8b0ffeaab6154a0925a23d95ca445534460 Mon Sep 17 00:00:00 2001 From: Tauan BF <11513929+tauanbinato@users.noreply.github.com> Date: Sun, 27 Sep 2026 13:10:40 -0300 Subject: [PATCH 2/2] Measure the markup and redirect confirms, name the unsafe redirect shape, split injection wording The redirect options now name the forms: a fixed path such as /admin first stays on the site; origin + next with no slash between them does not. Offered only "origin and a slash", chatbot-ui's origin + next read as staying on the site at 0.63; with the forms it is 0.93 anywhere. On the 38 corpus projects with path, markup or redirect findings: vaultwarden's hibp_breach and admin login redirect and shiori's login redirect, all labeled wrong, are notes; the 26 markup and 6 redirect findings labeled right put at most 0.22 and 0.44 on the harmless options. --- CHANGELOG.md | 2 + src/units/outcome/injection.rs | 8 ++- src/units/questions/security.rs | 8 ++- src/units/wording/security.rs | 105 +++++++++++++++++++------------- 4 files changed, 75 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 964946e..b675f55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ Notable changes to JevGate. Versions follow [Semantic Versioning](https://semver ## [Unreleased] +- Injection: the follow-up that asks a path finding what its paths can hold now also asks a markup finding what its values hold where they enter the markup (already escaped, percent-encoded or serialized as a URL; typed; the program's own; or raw text) and a redirect finding where its targets can lead (a fixed path such as `/admin` first keeps it on the site; `origin + next` with no slash between them does not). Leaning to harmless values, the finding is a note. On the 38 corpus projects with such findings, vaultwarden's `hibp_breach` (a username percent-encoded before the link), its admin login redirect and shiori's login redirect, all labeled wrong, are notes; the 26 markup and 6 redirect findings labeled right put at most 0.22 and 0.44 on the harmless options and stay. About $0.01 on the corpus. + ## [0.23.0] - 2026-09-27 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. diff --git a/src/units/outcome/injection.rs b/src/units/outcome/injection.rs index dc5d185..6588527 100644 --- a/src/units/outcome/injection.rs +++ b/src/units/outcome/injection.rs @@ -96,8 +96,12 @@ pub(in crate::units) fn confirmable<'a>( /// targets that stay on the site. Such a finding is a note. On the corpus, /// the 5 path findings labeled right 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, as a type's parsing -/// is shown only by its derive list. +/// parameters leaned to confined names at 0.65 to 0.84, as a type's parsing +/// is shown only by its derive list. The 26 markup findings labeled right +/// put at most 0.22 on values that cannot open a tag, and vaultwarden's +/// percent-encoded username 0.56; the 6 redirect findings labeled right put +/// at most 0.44 on staying on the site, and vaultwarden's admin path and +/// shiori's login page 0.68 and 0.58. pub(in crate::units) fn harmless<'a>( get: &impl Fn(&str) -> Option<&'a Answer>, ) -> Option<&'static str> { diff --git a/src/units/questions/security.rs b/src/units/questions/security.rs index 4b64d28..125cd41 100644 --- a/src/units/questions/security.rs +++ b/src/units/questions/security.rs @@ -306,7 +306,9 @@ pub const HARMLESS_MARKUP: [&str; 3] = ["encoded", "typed", "own"]; /// its admin path followed by the form's value, and shiori's to its login /// page with the current path as a query value: a fixed path before the /// variable keeps the target on the site, which the redirect check does not -/// ask. +/// ask. Offered "its own origin and a slash" without the form written out, +/// chatbot-ui's `requestUrl.origin + next` read as staying on the site at +/// 0.63, though `next=@evil.com` leaves it. pub fn redirect_reach(code: &str, callers: bool) -> Value { let note = if callers { format!("{CALLERS} {EVIDENCE}") @@ -320,9 +322,9 @@ pub fn redirect_reach(code: &str, callers: bool) -> Value { "note": note, }, "criteria": { - "own_site": "Only to the program's own site: every target starts with a fixed path that has a single leading slash, or with the program's own origin and a slash, and variables only follow it or fill its query string.", + "own_site": "Only to the program's own site: every target starts with a fixed path written in the code, such as `/admin` or `/login?next=`, so it begins with one slash and a path; or with the program's own origin followed by a slash written in the code; variables only follow that fixed part or fill its query string.", "checked": "Only where a check allows: the target is compared with an allowed list of hosts or checked to be a path on the site before the redirect.", - "anywhere": "Anywhere a variable says: a variable starts the target, or follows a fixed scheme and host with no slash between them, so it can name another host.", + "anywhere": "Anywhere a variable says: a variable starts the target, or directly follows the program's own origin or a host with no slash written between them, as in `origin + next`, where `@evil.com` or `.evil.com` in the variable names another host.", "none": "It does not redirect.", }, }) diff --git a/src/units/wording/security.rs b/src/units/wording/security.rs index 4094119..a9f1480 100644 --- a/src/units/wording/security.rs +++ b/src/units/wording/security.rs @@ -371,51 +371,16 @@ fn injection_wording( answers.get("origin").map(|a| origin_outcome(a)), Some(Outcome::Review(_)) ); - // A page script has no parameters: what it does not show the origin of - // is set by the files it includes or returned by the helpers it calls. - if subject == SCRIPT_SUBJECT && !outside && strength != Strength::Review { - let message = if strength == Strength::Consider { - format!( - "{subject} places values whose origin it does not show, such as those an included file sets or a helper returns, into {noun} without binding, escaping or checking them; outside input reaching them would make it exploitable ({p:.2})." - ) - } else { - format!( - "{subject} places a value whose origin it does not show into {noun}; it may already be bound or checked, or hold only the program's own values." - ) - }; - let action = if strength == Strength::Note { - "Optional: bind or check the value where it enters" - } else { - action - }; - return ((message, action), category.to_string()); - } let get = |q: &str| answers.get(q).copied(); - let harmless = (strength == Strength::Note) - .then(|| crate::units::outcome::harmless(&get)) - .flatten(); - let confirmed = match harmless { - Some("path") => Some(( - 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", - )), - Some("markup") => Some(( - format!( - "{subject} places a variable into {noun}, but it was likely escaped or encoded before, so it cannot open a tag or attribute." - ), - "Optional: confirm the value is escaped on every path that reaches the markup", - )), - Some("redirect") => Some(( - format!( - "{subject} redirects clients to a target built from a variable, but a fixed path or check likely keeps it on the site." - ), - "Optional: confirm no target can start with `//` or another host", - )), - _ => None, + let special = if subject == SCRIPT_SUBJECT && !outside && strength != Strength::Review { + Some(script_wording(subject, &noun, strength, p, action)) + } else if strength == Strength::Note { + crate::units::outcome::harmless(&get) + .and_then(|kind| confirmed_wording(kind, subject, &noun)) + } else { + None }; - if let Some(wording) = confirmed { + if let Some(wording) = special { return (wording, category.to_string()); } let message = match (strength, outside) { @@ -443,6 +408,60 @@ fn injection_wording( ((message, action), category.to_string()) } +/// A page script's finding: it has no parameters, so what it does not show +/// the origin of is set by the files it includes or returned by the helpers +/// it calls. +fn script_wording( + subject: &str, + noun: &str, + strength: Strength, + p: f64, + action: &'static str, +) -> Wording { + if strength == Strength::Consider { + ( + format!( + "{subject} places values whose origin it does not show, such as those an included file sets or a helper returns, into {noun} without binding, escaping or checking them; outside input reaching them would make it exploitable ({p:.2})." + ), + action, + ) + } else { + ( + format!( + "{subject} places a value whose origin it does not show into {noun}; it may already be bound or checked, or hold only the program's own values." + ), + "Optional: bind or check the value where it enters", + ) + } +} + +/// A note whose confirm Choice found values that can do no harm where they +/// go: a path that stays in its directory, markup values already escaped or +/// encoded, a redirect that stays on the site. +fn confirmed_wording(kind: &str, subject: &str, noun: &str) -> Option { + Some(match kind { + "path" => ( + 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", + ), + "markup" => ( + format!( + "{subject} places a variable into {noun}, but it was likely escaped or encoded before, so it cannot open a tag or attribute." + ), + "Optional: confirm the value is escaped on every path that reaches the markup", + ), + "redirect" => ( + format!( + "{subject} redirects clients to a target built from a variable, but a fixed path or check likely keeps it on the site." + ), + "Optional: confirm no target can start with `//` or another host", + ), + _ => return None, + }) +} + /// Logging or error details, whichever signal is strongest. fn exposure_kind(answers: &Answers<'_>) -> (&'static str, &'static str, &'static str) { let strongest = |questions: &[&str]| {