Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ Measured on 103 pinned projects (24 new open-source ones of kinds not tried befo
- Injection: a consider that rests on the function's parameters ("a caller passing outside input would make it exploitable") is asked, with the functions that call it, what the values it places can hold. It becomes a note when text the program fixes (literals its callers pass, numbers, names from a fixed list), values it creates or a local tool's own arguments lead; text another party can set, or callers that are not shown, keep it. Asked where the values come from, the recheck answered "the function's parameters" at 0.9 even for a Rust helper whose four callers pass literal SQL fragments. On the labeled projects, such considers went from 30 right and 51 wrong to 30 right and 32 wrong. Only these follow-ups are asked.
- Hardcoded values: the Choice that names a finding's value lists, with each value, the other lines of its file that write it. A consider that rests only on a value's name, whose file writes that value again, is then asked what the value is: copies that must change together, or a value nothing near it explains, keep it; a value that the field or argument it fills or a comment beside it explains, an idiom such as a tolerance near zero or a unit conversion, or a hand-tuned number, clearly chosen, make it a note. On the labeled projects, such considers went from 53 right and 77 wrong (41%) to 32 right and 28 wrong (53%); the others are notes. Offered instead as the locate's "no value stands out", those kinds also took values with copies that must stay equal. Only these follow-ups are asked.
- Hardcoded values: a review or consider that rests only on a value changing between environments is asked, once its value or constant is named and with the function or the lines that use the constant, where that value would differ. A value each installation must set, or the author's own account or domain, keeps it; the same value in every copy on purpose (the program's own service or registered client id, a provider's fixed address, a path the platform fixes), a fallback used only when configuration gives none, or code no deployment runs, at 0.80, make it a note. The Choice that names a file's constant lists the lines that use each one. On the labeled projects, such findings went from 17 right and 36 wrong to 15 right and 19 wrong: a desktop app's own license server, a provider's registered redirect and fallbacks behind environment variables are notes; a frontend's API host edited in code three times and a template author's domain as a fallback are the right ones lowered. Only these follow-ups are asked.
- Injection: outside PHP, a path check left undecided after its trace and recheck is asked, with the functions that call it, where the paths come from: the program's own directories or configuration, the command line or settings of the person running a local program, a whole path its caller gives, or another party's input (a request, an uploaded file or archive entry, a record users can edit). The program's own or the local user's clear it. A path, URL or redirect check still undecided on the function's parameters, whose Choice does not lean toward another party's input, is a note naming the path, URL or redirect, as a found one already was. Undecided injection units on the corpus went from 813 to 289, and no review or consider changed; the new Choice costs about $0.02 on the corpus.
- Tests: a test whose "checks only its mocks" or "recomputes its expected value" answer stays undecided after its recheck leans: below 0.50 it is clear. Labeled from the code, 4 of 43 such tests below 0.50 were hollow, against 10 of 35 at 0.50 or more. Undecided tests on the corpus went from 792 to 156, and undecided units overall from 1.39% to 0.83% of judged units. Nothing is asked again.
- Unsafe settings: outside C#, PHP and Django, the TLS check asks whether code accepts a server's certificate, host key or host name without verifying it, and names a connection made without TLS (gRPC's insecure credentials, an insecure OpenTelemetry exporter, `sslmode=disable`) and verification skipped only when a caller or the operator asks for it as not turning it off. Asked whether code turns verification off, Online Boutique's gRPC clients and telemetry exporters inside its cluster, and its database reached through the AlloyDB connector, were seven reviews whatever the examples said, and httpx's `verify=False` branch and vaultwarden's opt-in setting for invalid SMTP certificates were reviews too. The escape check names i18next's `escapeValue: false` in a React app, whose components escape what they render, as escaping kept. Labeled unsafe-settings reviews went from 54 right and 24 wrong to 54 right and 14 wrong; pgweb defaulting a bookmark's `sslmode` to `disable`, which turns off TLS rather than verification, is no longer one. Only unsafe-settings traces are asked again.
- Tests: Deno tests are test cases, in each of their forms: `Deno.test("name", fn)`, `Deno.test({ name: "name", fn() {…} })` and `Deno.test(function name() {…})`, with `.only` and `.ignore`. oak writes its 266 tests in the object form, and none of them was judged: its test files got a file-purpose request each and the test rules found nothing to ask. Only Deno projects' requests change.

Expand Down
13 changes: 11 additions & 2 deletions docs/classification-cascade.md
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,8 @@ signatures, or one candidate pair.
its mocks is asked again with the bodies of the functions it calls and its
file's imports, mocks and setup hooks (a part too long is left out, never
cut); each answer replaces the first unless only the first is decisive.
One still undecided then leans: below 0.50 it is clear. Labeled from the
code, 4 of 43 such tests below 0.50 were hollow, against 10 of 35 above.
A Ruby test is sent with the groups it is declared in, since an RSpec
example reads as a sentence continuing them and the outer group often names
the class under test. Its recheck shows, instead of every hook of the
Expand Down Expand Up @@ -372,8 +374,15 @@ signatures, or one candidate pair.
numbers, or values handed to it) was tried for considers on parameters
and dropped: it cleared a sort column taken from the request as readily
as clauses with placeholders. The same question about paths cleared real
traversals, reading names stored in an index as the program's own, so path
checks stay undecided until callers show more. The SQL check counts
traversals, reading names stored in an index as the program's own. Outside
PHP, an undecided path check is now asked, with its callers, where the
paths come from, naming a record users can edit as another party's input
and offering the local user's command line or settings: the program's own
paths or the local user's at 0.80 clear it (74 units on the corpus, among
them httprouter serving a directory through `http.Dir`). A path, URL or
redirect check still undecided on the function's parameters, whose Choice
does not lean toward another party's input, is a note, as a found one
already was: 450 injection units had stayed uncertain that way. The SQL check counts
identifiers quoted by doubling embedded quotes as handled (identifiers
cannot be bound), and the URL check excludes requests a web page sends from
the user's browser; on fresh repositories both had flagged such code, while
Expand Down
13 changes: 11 additions & 2 deletions site/src/how-it-works.md
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@ signatures, or one candidate pair.
its mocks is asked again with the bodies of the functions it calls and its
file's imports, mocks and setup hooks (a part too long is left out, never
cut); each answer replaces the first unless only the first is decisive.
One still undecided then leans: below 0.50 it is clear. Labeled from the
code, 4 of 43 such tests below 0.50 were hollow, against 10 of 35 above.
A Ruby test is sent with the groups it is declared in, since an RSpec
example reads as a sentence continuing them and the outer group often names
the class under test. Its recheck shows, instead of every hook of the
Expand Down Expand Up @@ -421,8 +423,15 @@ signatures, or one candidate pair.
numbers, or values handed to it) was tried for considers on parameters
and dropped: it cleared a sort column taken from the request as readily
as clauses with placeholders. The same question about paths cleared real
traversals, reading names stored in an index as the program's own, so path
checks stay undecided until callers show more. The SQL check counts
traversals, reading names stored in an index as the program's own. Outside
PHP, an undecided path check is now asked, with its callers, where the
paths come from, naming a record users can edit as another party's input
and offering the local user's command line or settings: the program's own
paths or the local user's at 0.80 clear it (74 units on the corpus, among
them httprouter serving a directory through `http.Dir`). A path, URL or
redirect check still undecided on the function's parameters, whose Choice
does not lean toward another party's input, is a note, as a found one
already was: 450 injection units had stayed uncertain that way. The SQL check counts
identifiers quoted by doubling embedded quotes as handled (identifiers
cannot be bound), and the URL check excludes requests a web page sends from
the user's browser; on fresh repositories both had flagged such code, while
Expand Down
4 changes: 2 additions & 2 deletions src/catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,9 @@ pub fn rule_version(key: &str) -> &'static str {
FILE_ORGANIZATION => "19",
FUNCTION_SIMPLIFICATION => "14",
SHARED_LOGIC => "20",
TEST_VALUE => "6",
TEST_VALUE => "7",
TEST_REDUNDANCY => "4",
INJECTION => "9",
INJECTION => "10",
SENSITIVE_DATA => "7",
HARDCODED_VALUES => "7",
UNSAFE_SETTINGS => "6",
Expand Down
24 changes: 23 additions & 1 deletion src/units/compose.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,9 @@ fn resolved<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> (Outcome, Answers
} else if unit.rule == catalog::COMMENTS {
comment_answers(unit, judgments)
} else if unit.rule == catalog::TEST_VALUE {
test_value_answers(unit, judgments)
let merged = test_value_answers(unit, judgments);
let outcome = leaning_test(unit, judgments, unit_outcome(unit, &merged));
return (outcome, merged);
} else if unit.rule == catalog::TEST_REDUNDANCY {
// Whether each test checks something the other does not, asked of a
// pair that reached a review, sits beside its answers.
Expand All @@ -135,6 +137,26 @@ fn resolved<'a>(unit: &UnitPlan, judgments: &'a [Judgment]) -> (Outcome, Answers
(unit_outcome(unit, &merged), merged)
}

/// A test whose hollow checks stay undecided once its recheck is asked (or
/// when it has none) leans: below 0.50 it is clear. Labeled from the code,
/// 4 of 43 such tests below 0.50 checked only their mocks or recomputed
/// their expected value (5 counting a test whose one real check is weak),
/// against 10 of 35 at 0.50 or more; 636 of the 792 undecided tests on the
/// corpus lean below.
fn leaning_test(unit: &UnitPlan, judgments: &[Judgment], outcome: Outcome) -> Outcome {
let rechecked =
unit.recheck.is_none() || !answers(judgments, &unit.id, Pass::Recheck).is_empty();
match outcome {
Outcome::Uncertain(p)
if rechecked
&& !crate::policy::probability_at_least(p, crate::policy::LEADING_PROBABILITY) =>
{
Outcome::Clear
}
other => other,
}
}

/// The pass of the follow-ups whose questions sit beside the first answers
/// under their own ids: document section and pair checks, the kind of a
/// large document, and benign-kind value checks.
Expand Down
38 changes: 34 additions & 4 deletions src/units/outcome/injection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub(in crate::units) fn origin_outcome(answer: &Answer) -> Outcome {
/// Kinds where a variable is a concern only when another party controls it:
/// helpers that build a path, URL or redirect target from their parameters
/// are everywhere.
const RESOURCE_CHECKS: [&str; 3] = ["path", "url", "redirect"];
pub(in crate::units) const RESOURCE_CHECKS: [&str; 3] = ["path", "url", "redirect"];

/// Presence alone never raises an injection: it only decides whether the
/// trace is asked. When every specific check clears the unit, it is clear;
Expand Down Expand Up @@ -96,6 +96,24 @@ fn found_injections<'a>(get: &impl Fn(&str) -> Option<&'a Answer>) -> Vec<&'stat
.collect()
}

/// Whether the Choice that settles an undecided resource check was asked
/// (where its paths, URLs or redirect targets come from) without leaning
/// toward another party's input, which keeps the check open.
fn settle_asked<'a>(check: &str, get: &impl Fn(&str) -> Option<&'a Answer>) -> bool {
let question = match check {
"path" if get("path_parts").is_some() => "path_parts",
"path" => "path_source",
"url" => "url_parts",
"redirect" => "redirect_target",
_ => return false,
};
choice_mass(get(question), &OUTSIDE_SOURCES)
.is_some_and(|p| !probability_at_least(p, LEADING_PROBABILITY))
}

/// Options of the settle Choices that name another party's input.
const OUTSIDE_SOURCES: [&str; 3] = ["outside", "request", "stored"];

/// The origin's outcome given the checks that found something: with none,
/// another party's values are a note and parameters a note only when a
/// check leans toward a concern; parameters only in paths or URLs are lower.
Expand All @@ -108,14 +126,26 @@ fn by_origin<'a>(
match outcome {
Outcome::Review(p) if found.is_empty() => Outcome::Note(p),
Outcome::Consider(p) if found.is_empty() => {
let leaning = settled_checks(catalog::INJECTION, get)
let open: Vec<(&str, Outcome)> = settled_checks(catalog::INJECTION, get)
.into_iter()
.filter(|(_, o)| *o != Outcome::Clear)
.collect();
let leaning = open
.iter()
.filter_map(|(id, _)| get(id))
.map(lean)
.fold(0.0, f64::max);
if probability_at_least(leaning, LEADING_PROBABILITY) {
Outcome::Note(leaning)
// Parameters in a path, URL or redirect are a note until a caller
// shows another party controls them, found or not: undecided,
// 320 such units on the corpus stayed uncertain while a found one
// was a note. Only once the Choice that settles each check was
// asked and did not clear it.
let resources_open = !open.is_empty()
&& open
.iter()
.all(|(id, _)| RESOURCE_CHECKS.contains(id) && settle_asked(id, get));
if probability_at_least(leaning, LEADING_PROBABILITY) || resources_open {
Outcome::Note(leaning.max(p))
} else {
Outcome::Uncertain(p)
}
Expand Down
2 changes: 1 addition & 1 deletion src/units/outcome/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub(super) use comments::{comment_concern_kind, comment_outcome, comment_signals
use documentation::stale_outcome;
pub(super) use documentation::{document_outcome, document_split, section_signals};
pub(super) use exposure::{Messages, django_settings_outcome, exposure_outcome, messages};
pub(super) use injection::{injection_outcome, origin_outcome};
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,
Expand Down
58 changes: 45 additions & 13 deletions src/units/questions/settle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,315 +6,347 @@
use super::{EVIDENCE, security::CALLERS};
use serde_json::{Value, json};

/// Where a source Choice looks for the variables it asks about, and its
/// note: with `callers`, also in what the functions that call it pass.
fn sources_shown(callers: bool) -> (&'static str, String) {
if callers {
(
", in the function or in what `callers` pass it",
format!("{CALLERS} {EVIDENCE}"),
)
} else {
("", EVIDENCE.to_string())
}
}

/// Options of the path-source Choice that rule a path concern out: the
/// program's own paths, the local user's, or no file.
pub const OWN_PATHS: [&str; 3] = ["own", "local", "none"];

/// Where the file paths a function opens, writes or deletes come from,
/// asked outside PHP when the path check stays undecided (PHP asks its own
/// `path_parts`). Undecided path checks were 348 units across the corpus,
/// mostly command-line tools writing where their user points them and
/// helpers joining a fixed directory with a name.
pub fn security_path_source(code: &str, callers: bool) -> Value {
let (shown, note) = sources_shown(callers);
json!({
"type": "choice",
"instructions": {
"question": format!("Where do the file paths that `{code}` opens, writes or deletes come from?"),
"note": note,
},
"criteria": {
"own": format!("Directories and file names written in the code, the program's own directories, or its configuration or environment, with only names, ids or numbers from variables{shown}."),
"local": "The command line, settings or files of the person running a local program or script, such as an output path they pass.",
"given": "A whole path handed to the function as a parameter or field, whose origin this code does not show.",
"outside": "Partly from outside the program: a network request, message, uploaded file or archive entry, or a record users can edit.",
"none": "It opens, writes or deletes no file.",
},
})
}

/// Options of the URL-parts Choice that rule a URL concern out: a host of the
/// program's own, or no request.
pub const OWN_PARTS: [&str; 2] = ["own", "none"];

/// Where the URLs a function requests come from, asked when the URL check
/// stays undecided: on clients of a fixed or configured service the check
/// split on a variable path or query, while naming the host decided them. A
/// host that is sent another URL to fetch is its own option, since internal
/// proxies fetched what users sent. The same question about paths cleared
/// real traversals, reading names stored in an index as the program's own,
/// so paths are not settled this way.
/// proxies fetched what users sent. The same question about paths once
/// cleared real traversals, reading names stored in an index as the
/// program's own; `security_path_source` names such records as another
/// party's input.
pub fn security_url_parts(code: &str, callers: bool) -> Value {
let shown = if callers {
", in the function or in what `callers` pass it"
} else {
""
};
let note = if callers {
format!("{CALLERS} {EVIDENCE}")
} else {
EVIDENCE.to_string()
};
let (shown, note) = sources_shown(callers);
json!({
"type": "choice",
"instructions": {
"question": format!("Where do the URLs that `{code}` requests come from?"),
"note": note,
},
"criteria": {
"own": format!("A host written in the code or set in the program's configuration or environment, with only ids, names, numbers or search terms from variables in its path or query{shown}."),
"forwards": "A host from the code or configuration, with another URL or host from a variable passed in its path or query for that service to fetch.",
"given": "A whole URL or host handed to the function as a parameter or field.",
"outside": "A URL or host from outside the program, such as a request, message, uploaded file or a record users can edit.",
"none": "It requests no URL.",
},
})
}

/// The option of the runs-in Choice that rules a forged request out.
pub const BROWSER: &str = "browser";

/// Where a function runs, asked when the URL check stays undecided, since a
/// request from the user's browser reaches only what that user can. Offered
/// beside the URL's parts, the browser lost to "a whole URL handed to it"
/// for a client component's fetch helper.
pub fn security_runs_in(code: &str) -> Value {
json!({
"type": "choice",
"instructions": {
"question": format!("Where does `{code}` run once the program is deployed?"),
"note": EVIDENCE,
},
"criteria": {
"browser": "Only in the user's web browser: in a client component, in a web page script, or in a component or hook that only client code uses.",
"server": "On a server or in a backend process: a route handler, server component, Server Action, API, job, or command-line tool.",
"either": "Either side may run it, such as shared code that both server and browser code import, or the code does not show which.",
},
})
}

/// Options of the redirect-target Choice that rule an open redirect out.
pub const OWN_TARGETS: [&str; 3] = ["own", "checked", "none"];

/// Where the targets a function redirects clients to come from, asked when
/// the redirect check stays undecided: client components that navigate to
/// fixed paths or to a checkout URL their server returns, and helpers that
/// build a path their callers name, split on "a URL or path taken from a
/// variable". Offered "a whole path handed to it" beside "what callers
/// pass", helpers whose callers pass fixed paths took the first, which is
/// true as well; with callers shown, that option is only for paths the
/// callers do not explain.
pub fn security_redirect_target(code: &str, callers: bool) -> Value {
let (own, given, note) = if callers {
(
"A path or URL written in the code, built from the program's own origin or configuration, or returned by the program's own server code or a service it calls, such as a payment provider's checkout page, in the function or in what `callers` pass it; variables fill only ids, names, numbers or messages in its segments or query.",
"A whole path or URL handed to the function as a parameter, where `callers` does not show where it comes from.",
format!("{CALLERS} {EVIDENCE}"),
)
} else {
(
"A path or URL written in the code, built from the program's own origin or configuration, or returned by the program's own server code or a service it calls, such as a payment provider's checkout page; variables fill only ids, names, numbers or messages in its segments or query.",
"A whole path or URL handed to the function as a parameter or field.",
EVIDENCE.to_string(),
)
};
json!({
"type": "choice",
"instructions": {
"question": format!("Where do the paths or URLs that `{code}` redirects or navigates the client to come from?"),
"note": note,
},
"criteria": {
"own": own,
"checked": "A path or URL from a variable that is checked before the redirect to be a path on the program's own site or on a host from an allowed list.",
"given": given,
"outside": "A whole path or URL that a request carries, such as a query parameter, form field, header or cookie, or an argument of a function clients call directly, without such a check.",
"none": "It redirects or navigates nowhere; it only builds or returns a path, or it has no redirect.",
},
})
}

/// Options of the markup Choice that rule a markup injection out.
pub const INERT_MARKUP: [&str; 3] = ["escaped", "text", "none"];

/// How the markup a function builds with variables is rendered, asked when
/// the markup check stays undecided: React components with values in
/// attributes, and snippets shown in a text field, split on "a variable put
/// into markup without escaping". A Django view is asked what it sends
/// back: views that only redirect or render a template split on the markup
/// check, since the variables they pass on end up in a page, and a template
/// escapes them unless it writes one with `|safe`. A function elsewhere
/// that renders a template writing values unescaped is asked the same way.
pub fn security_markup_output(code: &str, django: bool, rendered: bool) -> Value {
if rendered && !django {
return json!({
"type": "choice",
"instructions": {
"question": format!("What does `{code}` send back to the client, and how are the variables in it rendered?"),
"note": EVIDENCE,
},
"criteria": {
"escaped": "A page rendered from a template that writes each value it is given with an escaping tag, such as EJS `<%= … %>` or Handlebars `{{ … }}`, or HTML built with an escaping function.",
"text": "It is never rendered as HTML: JSON, a file download or plain text.",
"raw": "HTML it builds from variables as text itself, or a template that writes a value it is given without escaping, such as with EJS `<%- … %>`, Handlebars `{{{ … }}}` or a `|safe` filter.",
"none": "No markup with variables: it only redirects, or sends nothing to a client itself.",
},
});
}
if django {
return json!({
"type": "choice",
"instructions": {
"question": format!("What does `{code}` send back to the client, and how are the variables in it rendered?"),
"note": EVIDENCE,
},
"criteria": {
"escaped": "A page rendered from a template that writes each value it is given without a safe filter or autoescaping off, which Django escapes, or HTML built with format_html or escape.",
"text": "It is never rendered as HTML: JSON, a file download or plain text.",
"raw": "HTML it builds from variables as text itself, text it marks safe with mark_safe, or a template that writes a value it is given with a safe filter or with autoescaping off.",
"none": "No markup with variables: it only redirects, or sends nothing to a client itself.",
},
});
}
json!({
"type": "choice",
"instructions": {
"question": format!("How is the markup that `{code}` builds with variables rendered?"),
"note": EVIDENCE,
},
"criteria": {
"escaped": "By JSX or a template engine that escapes each value: variables appear only as element children, attribute values or component props, or go through an escaping or sanitizing function first.",
"text": "It is never rendered as HTML: it is shown as plain text, such as a code snippet in a text field, or sent as text.",
"raw": "As raw HTML with a variable inside, unescaped: through dangerouslySetInnerHTML, innerHTML, insertAdjacentHTML, document.write, an iframe srcdoc, or an HTML response built as text.",
"none": "It builds no HTML or SVG markup with variables.",
},
})
}

/// Options of the logging Choice that rule a logged secret out.
pub const PLAIN_LOGS: [&str; 4] = ["plain", "identity", "operator", "none"];

/// What a function's log statements write, asked whenever a logging signal
/// is not clear: an error caught from a payment or database call, logged
/// with a message, split on the check for a logged object; and the question
/// whether it logs personal data found an audit line naming who signed in
/// (vaultwarden's "User {email} logged in successfully. IP: {ip}") and a
/// command printing recovery codes for the admin who ran it: 10 of 19
/// labeled logging reviews were such lines.
pub fn security_logged(code: &str) -> Value {
json!({
"type": "choice",
"instructions": {
"question": format!("What do the log and console statements of `{code}` write?"),
"note": EVIDENCE,
},
"criteria": {
"plain": "Only messages, ids, counts, statuses, or an error caught from a failed call, none of which holds a password, token or key.",
"identity": "Who did what: a user's id, name, email address or IP address beside the action they took, as an audit or access log records, and no secret.",
"operator": "Values it shows on purpose to the person running a command-line tool, such as recovery codes or credentials a command prints for that person.",
"secret": "A password, token, API key or other secret, or a whole object, configuration, request or argument list that holds one.",
"personal": "Other personal data about a person, such as a home address, document number, or health or payment details.",
"none": "It logs or prints nothing.",
},
})
}

/// Options of the token Choice that rule an unverified-token concern out.
pub const VERIFIED_TOKENS: [&str; 5] = [
"verifies",
"passes",
"verified_before",
"reads_claims",
"none",
];

/// What a function does with security tokens, asked whenever the token check
/// is not clear: front-end hooks that read their own token to send it and
/// middleware that looks a session up stayed between 0.2 and 0.5 on the
/// check, while naming what the code does with tokens decides. Reading a
/// token's claims is apart from deciding access with them: code that read
/// the expiry of a token its identity provider had just sent, or the
/// character id of an access token, was chosen as trusting it unverified.
pub fn security_token_use(code: &str) -> Value {
json!({
"type": "choice",
"instructions": {
"question": format!("What does `{code}` do with security tokens, such as JSON Web Tokens or session tokens?"),
"note": EVIDENCE,
},
"criteria": {
"verifies": "It verifies each token's signature and expiry, or looks the token up in its own store, before trusting what it holds.",
"passes": "It only creates, signs, stores, sends or forwards tokens, or checks that one is present, while a server verifies them.",
"verified_before": "It reads the claims of a token verified before it runs, such as by middleware, or of a token it has just received from an identity provider over TLS.",
"reads_claims": "It decodes a token only to read or show what it says, such as a user id, a name or its expiry, while other code or a server decides what the caller may do.",
"decides_access": "It decides what the caller may do, such as signing them in, granting a role or accepting a reset, from a token it has not verified.",
"turned_off": "It turns off a check a library makes by default, such as verify_signature=False, verify=False, an algorithm list that allows none, or ignoreExpiration.",
"none": "It handles no security tokens.",
},
})
}

/// Options of the password Choice that rule a weak-password concern out.
pub const HASHED_PASSWORDS: [&str; 2] = ["slow_hash", "none"];

/// How a function treats users' passwords, asked whenever the password
/// check is not clear: HMAC signing, key loading and a demo login form were
/// reviews or stayed between 0.2 and 0.4 on it.
pub fn security_password_handling(code: &str) -> Value {
json!({
"type": "choice",
"instructions": {
"question": format!("How does `{code}` handle users' passwords?"),
"note": EVIDENCE,
},
"criteria": {
"slow_hash": "It hashes them with bcrypt, scrypt, Argon2 or a key derivation function with many iterations, or hands them to a library, framework or model hook that does.",
"plain": "It saves them, or checks a login against saved ones, as plain text.",
"fast_hash": "It hashes them with MD5, SHA-1, a single round of SHA-256 or another fast hash, or derives keys from them with few iterations.",
"none": "It stores and checks no users' passwords: what it hashes, signs or encrypts is other data, such as tokens, messages, files or keys, or it only fills in or sends a password someone types.",
},
})
}

/// Options of the CORS Choice that rule a credentialed-origin concern out.
pub const SAFE_ORIGINS: [&str; 3] = ["unset", "listed", "public"];

/// Which other sites a function lets send credentialed requests, asked when
/// the CORS check stays undecided: route handlers that set cookies or answer
/// preflights with `*` and no credentials split on "any origin allowed".
pub fn security_cors_origins(code: &str) -> Value {
json!({
"type": "choice",
"instructions": {
"question": format!("Which other sites does `{code}` let send requests that carry a user's cookies or credentials?"),
"note": EVIDENCE,
},
"criteria": {
"unset": "None: it sets no CORS header or option.",
"listed": "Only origins written in the code or configuration, or the program's own origin.",
"public": "Any origin, but without allowing credentials: no `Access-Control-Allow-Credentials: true` or credentials option, as for a public or token-authenticated API.",
"any": "Any origin, or whatever origin a request names reflected back, with credentials allowed.",
},
})
}

/// The options of the cookie Choice that clear the cookie check.
pub const FLAGGED_COOKIES: [&str; 2] = ["unset", "flagged"];

/// What a function leaves a session cookie's flags as, asked when the cookie
/// check stays undecided: a SvelteKit form action's `cookies.set` without
/// options, whose defaults set both flags, stayed at 0.21.
pub fn security_cookie_flags(code: &str) -> Value {
json!({
"type": "choice",
"instructions": {
"question": format!("How are the Secure and HttpOnly flags set on the cookies `{code}` sets?"),
"note": EVIDENCE,
},
"criteria": {
"unset": "It sets no cookie, or only cookies that hold no session, token or sign-in state, such as a theme or language preference.",
"flagged": "Session or token cookies get both flags: in the options it passes, or from a framework whose defaults set them, such as SvelteKit's `cookies.set`.",
"missing": "A session or token cookie is set with Secure or HttpOnly turned off, or through an API whose defaults leave them off, such as Express `res.cookie`, `document.cookie` or PHP `setcookie` without them.",
},
})
}

/// The options of the destination Choice that rule error details out: every
/// place but a remote client.
pub const AWAY_FROM_CLIENTS: [&str; 4] = ["local", "logs", "caller", "stored"];

/// Where a function's text goes, asked when an error-detail signal stays
/// undecided. An error or body shaped for a response counts as the client:
/// helpers that format errors for a server's callers return them. A game
/// client that hands the server's error text to its own window over a
/// channel of `Response` messages was answered as sending it to a client,
/// so the local option names the program's own screens.
pub fn security_destination(code: &str) -> Value {
json!({
"type": "choice",
"instructions": {
"question": format!("Where does the text that `{code}` produces or passes on go?"),
"note": EVIDENCE,
},
"criteria": {
"client": "Into a response to a request from another computer: an HTTP, API or RPC response, a message to a connected remote client, or an error, status or body shaped for such a response that it builds or returns.",
"local": "To the person running a local program: a terminal, console or window, the program's own screens that a desktop, game or mobile app reaches through a channel, event or IPC call, or a report or file on their own machine.",
"logs": "To logs, or to the program's own error reporting or monitoring.",
"caller": "Back to the code that called it as an ordinary error or value, such as a parse, lookup or validation failure, not shaped as a response.",
"stored": "Into a database, queue, cache or job record.",
},
})
}

Check warning on line 352 in src/units/questions/settle.rs

View workflow job for this annotation

GitHub Actions / review

JevGate consider [maintainability/file-organization]

This file writes out the same kind of code for several features (0.95); each feature's part would be easier to find in its own module. → Consider moving that set of members into its own module
12 changes: 11 additions & 1 deletion src/units/security.rs
Original file line number Diff line number Diff line change
Expand Up @@ -550,30 +550,30 @@
let php = php
.iter()
.filter(|check| language == questions::PHP && questions::php_mentions(check.id, source));
general
.iter()
.map(|check| {
questions::DJANGO_VARIANTS
.iter()
.find(|variant| django && variant.id == check.id)
.unwrap_or(check)
})
.chain(csharp)
.chain(framework)
.chain(php)
.chain(
(rule == INJECTION && !django)
.then(|| questions::deserializer_check(language, source))
.flatten(),
)
.chain((rule == INJECTION && xml).then_some(&questions::XXE))
.chain(
(rule == UNSAFE_SETTINGS && language != questions::CSHARP && !django)
.then_some(&questions::TOKEN_AND_KEY)
.into_iter()
.flatten(),
)
.collect()

Check warning on line 576 in src/units/security.rs

View workflow job for this annotation

GitHub Actions / review

JevGate consider [maintainability/function-simplification]

`asked_checks` likely mixes separate jobs; splitting it may make it easier to understand (0.82). Lines 553–576 would be most useful as their own function. → Consider extracting the located block into a named function
}

/// The trace follow-up of one unit: which site, the rule's specific checks,
Expand Down Expand Up @@ -855,7 +855,7 @@
/// does with tokens and how it handles passwords settle those checks
/// whenever they are not clear: front ends that send their own token and
/// HMAC signing split on them or were reviews.
pub(in crate::units) const SETTLES: [SettleKind; 13] = [
pub(in crate::units) const SETTLES: [SettleKind; 14] = [
SettleKind {
rule: INJECTION,
question: "url_parts",
Expand Down Expand Up @@ -919,6 +919,15 @@
when: SettleWhen::NotClear,
files: SettleFiles::Only(questions::PHP),
},
SettleKind {
rule: INJECTION,
question: "path_source",
checks: &["path"],
clears: &questions::OWN_PATHS,
callers: true,
when: SettleWhen::Undecided,
files: SettleFiles::Except(questions::PHP),
},
SettleKind {
rule: SENSITIVE_DATA,
question: "destination",
Expand Down Expand Up @@ -1006,6 +1015,7 @@
let callers = kind.callers && !subject.callers.is_empty();
let body = match kind.question {
"url_parts" => questions::security_url_parts(&code, callers),
"path_source" => questions::security_path_source(&code, callers),
"runs_in" => questions::security_runs_in(&code),
"redirect_target" => questions::security_redirect_target(&code, callers),
"markup_output" => {
Expand Down
Loading
Loading