Python fp and docs updtes (#58)

* refactor: Update comments for clarity and add expectations.json files for performance metrics

* feat: Implement FP guard for JS/TS local-collection receivers to suppress missing ownership checks

* feat: Enhance Rust parameter handling to classify local collections and prevent false ownership checks

* refactor: Simplify code formatting for better readability in multiple files

* refactor: Improve UTF-8 sequence length handling and enhance clarity in loop iteration

* feat: Update Java and Python patterns to include new security rules

* refactor: Improve comment clarity and consistency across multiple Rust files

* refactor: Simplify code formatting for improved readability in integration tests and module files

* refactor: Improve comment formatting and enhance clarity in assertions across multiple files
This commit is contained in:
Eli Peter 2026-04-29 19:53:34 -04:00 committed by GitHub
parent 4db0805de6
commit a438886217
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
291 changed files with 9485 additions and 3851 deletions

View file

@ -107,11 +107,11 @@ fn has_web_handler_params(ctx: &AnalysisContext, func_name: &str) -> bool {
/// Determine if a function qualifies as a web entrypoint (not just any entrypoint).
///
/// A web entrypoint must:
/// 1. Match entrypoint naming rules (handle_*, route_*, api_*, etc.) but NOT bare `main`
/// 1. Match entrypoint naming rules (handle_*, route_*, api_*, etc.), but NOT bare `main`
/// unless it has web-like parameters
/// 2. Have parameters resembling HTTP handler signatures
fn is_web_entrypoint(ctx: &AnalysisContext, func_name: &str) -> bool {
// "main" without web params is a CLI entrypoint skip
// "main" without web params is a CLI entrypoint, skip
if func_name == "main" {
return has_web_handler_params(ctx, func_name);
}
@ -163,7 +163,7 @@ impl CfgAnalysis for AuthGap {
fn run(&self, ctx: &AnalysisContext) -> Vec<CfgFinding> {
// Decorator/annotation/attribute auth on the body declaration
// already gates every sink in the body skip the
// already gates every sink in the body, skip the
// structural-call dominance check entirely when the framework
// enforces auth at the declaration level. Mirrors the
// `classify_auth_decorators` lookup the state engine uses to

View file

@ -14,7 +14,7 @@ use petgraph::visit::EdgeRef;
/// Returns true if the identifier is exactly `err` / `error` or a
/// snake-case error name (`err_x`, `error_x`, `x_err`, `x_error`).
/// CamelCase names (`isErrorEnabled`, `getError`, `errorMsg`) are
/// rejected the cost is occasional FNs on Java-style error fields,
/// rejected, the cost is occasional FNs on Java-style error fields,
/// which is acceptable for a precision fix.
fn is_error_var_ident(name: &str) -> bool {
let lower = name.to_ascii_lowercase();
@ -36,7 +36,7 @@ fn is_error_var_ident(name: &str) -> bool {
/// Used by the error-fallthrough rule to skip happy-path checks
/// like `if (!data.error && Array.isArray(results))` whose TRUE branch
/// is the success path and is not expected to return. The original
/// rule fires on `if (err) { warn(); } sink_after()` a positive
/// rule fires on `if (err) { warn(); } sink_after()`, a positive
/// error check whose body forgets to early-return.
fn contains_negated_err_identifier(text: &str) -> bool {
let bytes = text.as_bytes();
@ -46,7 +46,7 @@ fn contains_negated_err_identifier(text: &str) -> bool {
i += 1;
continue;
}
// Skip the `!=` / `!==` operators those are comparisons, not
// Skip the `!=` / `!==` operators, those are comparisons, not
// logical-not. Only treat a `!` followed by whitespace or an
// identifier-leading char as logical negation.
if i + 1 < bytes.len() && bytes[i + 1] == b'=' {
@ -57,7 +57,7 @@ fn contains_negated_err_identifier(text: &str) -> bool {
while j < bytes.len() && (bytes[j] == b' ' || bytes[j] == b'\t') {
j += 1;
}
// Allow a leading `(` for `!(expr)` shapes peek past one open
// Allow a leading `(` for `!(expr)` shapes, peek past one open
// paren and continue capturing the identifier chain.
if j < bytes.len() && bytes[j] == b'(' {
j += 1;
@ -118,7 +118,95 @@ fn branch_terminates(cfg: &crate::cfg::Cfg, if_node: NodeIndex) -> bool {
false
}
/// Check if all paths from `node` reach a Return/Break/Continue before exiting scope.
/// Recognise calls that never return on the success path.
///
/// `cfg-error-fallthrough` looks for `if err != nil { … }` whose body
/// fails to terminate. A `return`/`break`/`continue`/`throw` is the
/// canonical terminator and already produces a `StmtKind::Return` /
/// `Throw` / `Break` / `Continue` node. But a large class of real
/// terminators arrives as a *call* whose callee is documented to abort
/// the goroutine, process, or test:
///
/// * Go testing, `t.Fatal`, `t.Fatalf`, `t.Fatalln`, `b.Fatal*`,
/// `*Helper()` chains ending in `Fatal*`, also third-party
/// `require.NoError(t, …)` (asserts and aborts on err) which the
/// common `c.Fatalf("...")` pattern in minio's table tests reduces
/// to. All `Fatal*` methods on a `testing.T`/`B`/`F` call
/// `runtime.Goexit()` which is documented as never returning to the
/// caller.
/// * Go std-library, `os.Exit`, `syscall.Exit`, `runtime.Goexit`,
/// `log.Fatal`, `log.Fatalf`, `log.Fatalln`, `log.Panic*`.
/// * Go builtin, bare `panic(…)`.
/// * Rust, `panic!`, `unreachable!`, `unimplemented!`, `todo!`,
/// `process::exit`, `std::process::exit`, `process::abort`,
/// `std::process::abort` (the macros currently lower to
/// `StmtKind::Throw` via tree-sitter's macro arm; the function
/// forms need explicit recognition).
/// * Python, `sys.exit`, `os._exit`, `os.abort`.
///
/// The recogniser looks at the bare method name (last segment after
/// `.` or `::`) and, where the receiver is a closed token, the
/// receiver's first segment. Bare `panic` / `exit` callees are
/// recognised only when the namespace context matches (callee equals
/// the literal string, no other receiver). This keeps the recogniser
/// from claiming arbitrary user-defined `Exit(...)` / `Panic(...)` as
/// terminators.
///
/// Closes the minio test-file cluster (49 in `xl-storage_test.go`
/// alone, 176 across the repo) where every `if err != nil { c.Fatalf(...) }`
/// fired `cfg-error-fallthrough`: the `Fatalf` aborts the goroutine
/// and the post-if code never executes, but the rule classified it as
/// fall-through. Conservative: only adds new terminators; never
/// removes the existing `Return`/`Throw`/`Break`/`Continue` recognition.
fn call_never_returns(info: &crate::cfg::NodeInfo) -> bool {
if info.kind != StmtKind::Call {
return false;
}
let Some(callee) = info.call.callee.as_deref() else {
return false;
};
let last = callee.rsplit(['.', ':']).next().unwrap_or(callee);
// Method names that always terminate when called on any receiver
// that's a testing handle (`*testing.T`, `*testing.B`, `*testing.F`)
// or a logger. Receiver type is unknown to this rule; the names
// are sufficiently distinctive that arbitrary user-defined methods
// sharing the name are vanishingly rare.
if matches!(
last,
// Go testing
"Fatal" | "Fatalf" | "Fatalln" | "FailNow" |
// Go log/slog terminating handlers
"Panic" | "Panicf" | "Panicln" |
// Rust process / never-return std fns
"abort" | "unreachable_unchecked"
) {
return true;
}
// Bare callees (no receiver) that are language builtins or
// unambiguous std-library terminators.
match callee {
// Go builtin
"panic" => return true,
// Go std
"os.Exit" | "syscall.Exit" | "runtime.Goexit" | "log.Fatal" | "log.Fatalf"
| "log.Fatalln" | "log.Panic" | "log.Panicf" | "log.Panicln" | "slog.Fatal"
| "klog.Fatal" | "klog.Fatalf" | "klog.Exit" | "klog.Exitf" => return true,
// Rust std
"process::exit" | "process::abort" | "std::process::exit" | "std::process::abort" => {
return true;
}
// Python std
"sys.exit" | "os._exit" | "os.abort" => return true,
_ => {}
}
false
}
/// Check if all paths from `node` reach a Return/Break/Continue (or a
/// known never-returning call) before exiting scope.
fn terminates_on_all_paths(
cfg: &crate::cfg::Cfg,
node: NodeIndex,
@ -142,10 +230,15 @@ fn terminates_on_all_paths(
}
_ => {}
}
if call_never_returns(info) {
// Documented never-returning call (`t.Fatalf`, `os.Exit`,
// `panic`, `runtime.Goexit`, …), this path terminates.
continue;
}
let successors: Vec<_> = cfg.neighbors(current).collect();
if successors.is_empty() {
// Reached a dead end without terminating — path does not terminate
// Reached a dead end without terminating, path does not terminate
return false;
}
@ -181,7 +274,7 @@ fn find_post_if_sinks(cfg: &crate::cfg::Cfg, if_node: NodeIndex) -> Vec<NodeInde
// Seed from the False edge only. If the if has no explicit False
// edge (some CFG shapes omit it for one-branch ifs), fall back to
// Seq edges from the if node but never follow True edges, which
// Seq edges from the if node, but never follow True edges, which
// lead into the body.
let mut stack: Vec<NodeIndex> = cfg
.edges(if_node)
@ -225,9 +318,9 @@ impl CfgAnalysis for IncompleteErrorHandling {
// Look for If nodes whose CONDITION involves "err" or "error".
// `info.taint.uses` for an If node contains identifiers from the
// whole if statement (condition + body) see
// whole if statement (condition + body), see
// `cfg::literals::extract_defs_uses_extra_defs` Kind::If branch
// so checking it would misfire on `if (!res.ok) { ... const
//, so checking it would misfire on `if (!res.ok) { ... const
// err = await … ; return … }` shapes whose body happens to
// mention `err` even though the condition doesn't. Use
// `info.condition_vars`, which is populated strictly from the
@ -244,7 +337,7 @@ impl CfgAnalysis for IncompleteErrorHandling {
// Polarity gate: only fire when the condition POSITIVELY
// checks for an error. `if (!data.error && other)` is a
// happy-path check the TRUE branch is the success branch
// happy-path check, the TRUE branch is the success branch
// and is not expected to terminate. Detect by scanning the
// condition text for any `!` (logical-not, distinct from
// `!=`) preceding an identifier whose name contains "err".
@ -354,7 +447,7 @@ mod err_ident_tests {
fn rejects_camelcase_method_names() {
// Spring `logger.isErrorEnabled()` lifts `isErrorEnabled` into
// `condition_vars`; under the old `lower.contains("err")` check
// this fired the rule. The new strict check rejects it the
// this fired the rule. The new strict check rejects it, the
// condition is asking "is logging enabled", not "is there an
// error".
assert!(!is_error_var_ident("isErrorEnabled"));
@ -371,3 +464,103 @@ mod err_ident_tests {
assert!(!is_error_var_ident("perform"));
}
}
#[cfg(test)]
mod terminator_call_tests {
use super::call_never_returns;
use crate::cfg::{CallMeta, NodeInfo, StmtKind};
fn call_node(callee: &str) -> NodeInfo {
NodeInfo {
kind: StmtKind::Call,
call: CallMeta {
callee: Some(callee.to_string()),
..Default::default()
},
..Default::default()
}
}
#[test]
fn recognises_go_testing_fatal_methods() {
// Bare method name on any receiver, the canonical minio test
// shape `c.Fatalf("bucket creat error: %v", err)`.
assert!(call_never_returns(&call_node("c.Fatalf")));
assert!(call_never_returns(&call_node("t.Fatal")));
assert!(call_never_returns(&call_node("t.Fatalf")));
assert!(call_never_returns(&call_node("t.Fatalln")));
assert!(call_never_returns(&call_node("b.Fatal")));
assert!(call_never_returns(&call_node("t.FailNow")));
// Logger panics (handler-style fatal).
assert!(call_never_returns(&call_node("logger.Panic")));
assert!(call_never_returns(&call_node("logger.Panicf")));
}
#[test]
fn recognises_go_std_terminators() {
assert!(call_never_returns(&call_node("os.Exit")));
assert!(call_never_returns(&call_node("syscall.Exit")));
assert!(call_never_returns(&call_node("runtime.Goexit")));
assert!(call_never_returns(&call_node("log.Fatal")));
assert!(call_never_returns(&call_node("log.Fatalf")));
assert!(call_never_returns(&call_node("log.Fatalln")));
assert!(call_never_returns(&call_node("log.Panic")));
assert!(call_never_returns(&call_node("klog.Exit")));
// Bare builtin
assert!(call_never_returns(&call_node("panic")));
}
#[test]
fn recognises_rust_and_python_std_terminators() {
assert!(call_never_returns(&call_node("std::process::exit")));
assert!(call_never_returns(&call_node("std::process::abort")));
assert!(call_never_returns(&call_node("process::exit")));
assert!(call_never_returns(&call_node("sys.exit")));
assert!(call_never_returns(&call_node("os._exit")));
}
#[test]
fn does_not_claim_user_defined_lookalikes() {
// Bare `Exit` on a custom receiver is a normal method, not the
// process-level terminator. The bare callee path only matches
// exact std-library forms.
assert!(!call_never_returns(&call_node("server.Exit")));
assert!(!call_never_returns(&call_node("Exit")));
assert!(!call_never_returns(&call_node("session.exit")));
// Bare `panic` is a Go builtin; method `panic` is not.
// The recogniser keys off the full callee path so
// `widget.panic` does not match.
assert!(!call_never_returns(&call_node("widget.panic")));
// Common helpers that *don't* terminate.
assert!(!call_never_returns(&call_node("log.Print")));
assert!(!call_never_returns(&call_node("log.Println")));
assert!(!call_never_returns(&call_node("t.Errorf")));
assert!(!call_never_returns(&call_node("t.Logf")));
assert!(!call_never_returns(&call_node("c.Skip")));
}
#[test]
fn requires_call_kind() {
// Only StmtKind::Call nodes are inspected; an If or Seq node
// carrying the same callee text wouldn't ever come through
// this path. Defensive: confirm the kind gate.
let mut node = call_node("t.Fatal");
node.kind = StmtKind::Seq;
assert!(!call_never_returns(&node));
node.kind = StmtKind::If;
assert!(!call_never_returns(&node));
}
#[test]
fn missing_callee_does_not_panic() {
let node = NodeInfo {
kind: StmtKind::Call,
call: CallMeta {
callee: None,
..Default::default()
},
..Default::default()
};
assert!(!call_never_returns(&node));
}
}

View file

@ -29,7 +29,7 @@ pub struct UnguardedSink;
/// receiver recorded as a compound identifier rather than a named binding).
fn is_all_args_constant(ctx: &AnalysisContext, sink: NodeIndex) -> bool {
// Fast path: syntactic literal detection from CFG construction.
// Strictly weaker than the one-hop trace below serves as an
// Strictly weaker than the one-hop trace below, serves as an
// optimization for the common case of inline literal arguments.
if ctx.cfg[sink].all_args_literal {
return true;
@ -127,17 +127,17 @@ fn ssa_all_sink_operands_constant(
/// SSA-backed reassign-aware safety probe: every operand of the sink
/// resolves to a constant, callee fragment, OR a function parameter that
/// is not itself a Source. Used at the cfg-unguarded-sink site under
/// `!has_taint` the taint engine has already proved no source-tainted
/// `!has_taint`, the taint engine has already proved no source-tainted
/// data reaches the sink, so a non-source Param at operand position is
/// inert payload-wise (e.g. HTTP writer in `Fprintf(w, "<h1>", "Guest")`).
///
/// Gated on the function body actually exhibiting the reassign-to-constant
/// signature at least one named SSA def whose RHS is a literal Const
/// signature, at least one named SSA def whose RHS is a literal Const
/// (`name = "Guest"`). In a thin wrapper without a same-block named
/// const assignment (`fn wrap(p) { sink(p) }`, or C `popen(buf, "r")` where
/// `buf` is filled in-place by `sprintf` with no Const Assign on `buf`),
/// the bare Param at operand position IS the payload and the suppression's
/// rationale does not apply `cfg-unguarded-sink` must still fire.
/// rationale does not apply, `cfg-unguarded-sink` must still fire.
fn ssa_all_sink_operands_const_or_param(ctx: &AnalysisContext, sink: NodeIndex) -> bool {
let Some(facts) = ctx.body_const_facts else {
return false;
@ -165,13 +165,13 @@ fn ssa_all_sink_operands_const_or_param(ctx: &AnalysisContext, sink: NodeIndex)
}
/// Return true if the SSA body contains a *named* variable whose definition
/// is a constant the SSA signature of an explicit `name = "literal"`
/// is a constant, the SSA signature of an explicit `name = "literal"`
/// reassignment. Used as the gate for the broader operand-Param suppression:
/// the suppression's purpose is the reassign-to-constant idiom, which by
/// definition has at least one named const assignment. In a thin wrapper
/// (`fn wrap(p) { sink(p) }` or `popen(buf, "r")` where `buf` is filled by
/// `sprintf`), no such named const assignment exists and the suppression's
/// rationale doesn't apply so the bare-Param structural finding fires.
/// rationale doesn't apply, so the bare-Param structural finding fires.
fn func_body_has_named_const_assign(facts: &BodyConstFacts) -> bool {
for block in &facts.ssa.blocks {
for inst in &block.body {
@ -228,7 +228,7 @@ fn ssa_operand_const_or_param(
// CFG-node-level Source label: when an SSA `Call` corresponds to a
// Source-labeled CFG node (e.g. `env::var(...)` whose callee
// matches a `LabelRule` Source matcher), the call's result is
// tainted user input refuse, regardless of how the SSA
// tainted user input, refuse, regardless of how the SSA
// happened to lower. Catches the `SsaOp::Call` lowering of
// labeled Source functions, which the `SsaOp::Source` arm only
// sees for callee-less pure sources like PHP `$_GET`.
@ -266,7 +266,7 @@ fn ssa_operand_const_or_param(
}
SsaOp::Source => return false,
SsaOp::Nop | SsaOp::Undef => {}
// FieldProj: walk the receiver `obj.f` is constant iff `obj`
// FieldProj: walk the receiver, `obj.f` is constant iff `obj`
// is constant under the same definition. The field name itself
// is structural and adds no runtime value.
SsaOp::FieldProj { receiver, .. } => stack.push(*receiver),
@ -321,7 +321,7 @@ fn ssa_operand_constant(
}
SsaOp::Param { .. } | SsaOp::SelfParam | SsaOp::CatchParam | SsaOp::Source => {
// Only acceptable when the param's `var_name` is a callee
// fragment i.e. an identifier that only appears because
// fragment, i.e. an identifier that only appears because
// the CFG recorded name components of the dotted/chained
// callee as uses. Real parameters and sources are dynamic.
let name = inst.var_name.as_deref().unwrap_or("");
@ -333,7 +333,7 @@ fn ssa_operand_constant(
}
}
SsaOp::Nop => {}
// Undef is a non-user, non-dynamic sentinel treat like Const
// Undef is a non-user, non-dynamic sentinel, treat like Const
// (no additional operands to trace).
SsaOp::Undef => {}
// FieldProj: structural field read; constness reduces to the
@ -440,7 +440,7 @@ fn sink_args_typed_safe(ctx: &AnalysisContext, sink: NodeIndex, sink_caps: Cap)
!is_callee_fragment(name, callee_desc, &callee_parts, &outer_parts)
}
// Constant string literals used as inline args (e.g. `"listener"`,
// `"-c"`) are not user-controlled treat as non-real for the
// `"-c"`) are not user-controlled, treat as non-real for the
// "all int-typed" test so they don't block suppression.
SsaOp::Const(_) => false,
_ => true,
@ -477,7 +477,7 @@ fn type_facts_suppress(values: &[SsaValue], sink_caps: Cap, type_facts: &TypeFac
/// lookup idiom (e.g. `map.get(x).unwrap_or("safe")` over literal inserts)
/// should clear a command-injection sink.
///
/// Only fires for `Cap::SHELL_ESCAPE` SQL / path suppression from this
/// Only fires for `Cap::SHELL_ESCAPE`, SQL / path suppression from this
/// domain would require stronger reasoning (literal keys can still carry
/// SQL tokens if the inserts themselves contain them).
fn sink_args_static_map_safe(ctx: &AnalysisContext, sink: NodeIndex, sink_caps: Cap) -> bool {
@ -595,6 +595,71 @@ fn match_config_sanitizer(callee: &str, extra: &[RuntimeLabelRule]) -> Option<Ca
None
}
/// Resolve the `if (X)` / `if (!X)` indirect-validator pattern: the
/// condition has exactly one bare-identifier variable whose defining
/// CFG node is a [`StmtKind::Call`] whose `defines` is the same name
/// and whose `callee` is recognised by
/// [`crate::ssa::type_facts::classify_input_validator_callee`].
///
/// Returns the validator callee name when the pattern matches, `None`
/// otherwise. Conservative: bails when the condition has zero or more
/// than one variable, when no defining call is found, or when the
/// callee doesn't match a validator pattern. Mirrors the SSA
/// branch-narrowing layer
/// ([`crate::taint::ssa_transfer::apply_input_validator_branch_narrowing`])
/// so the structural `cfg-unguarded-sink` suppression matches the
/// taint engine's validator recognition.
///
/// Driven off CFG `TaintMeta.defines` rather than the per-body SSA
/// value-defs because nested arrow-function bodies are sometimes
/// lowered with empty SSA in the cfg-analysis context, but the CFG
/// nodes themselves carry `defines` in every body.
fn cond_indirect_validator_callee(
info: &crate::cfg::NodeInfo,
ctx: &AnalysisContext,
) -> Option<String> {
if info.condition_vars.len() != 1 {
return None;
}
let var_name = info.condition_vars[0].as_str();
let cond_func = info.ast.enclosing_func.as_deref();
let cond_span_start = info.ast.span.0;
// Walk the CFG for any node that DEFINES `var_name` via a Call
// expression. Same-function only, and only consider definitions
// textually before the condition: a reassignment after the `if`
// cannot be the def reaching it. Among the eligible defs, take
// the textually-last one (highest span start), a conservative
// latest-def proxy without paying for full dominator analysis.
let mut best: Option<(usize, &str)> = None;
for nidx in ctx.cfg.node_indices() {
let n = &ctx.cfg[nidx];
if n.kind != crate::cfg::StmtKind::Call {
continue;
}
if n.taint.defines.as_deref() != Some(var_name) {
continue;
}
if n.ast.enclosing_func.as_deref() != cond_func {
continue;
}
let span_start = n.ast.span.0;
if span_start >= cond_span_start {
continue;
}
let Some(callee) = n.call.callee.as_deref() else {
continue;
};
match best {
Some((s, _)) if s >= span_start => {}
_ => best = Some((span_start, callee)),
}
}
let (_, callee) = best?;
crate::ssa::type_facts::classify_input_validator_callee(callee).map(|_| callee.to_string())
}
/// Find all nodes in the CFG that are calls to guard functions.
fn find_guard_nodes(ctx: &AnalysisContext) -> Vec<(NodeIndex, Cap)> {
let guard_rules = rules::guard_rules(ctx.lang);
@ -620,6 +685,24 @@ fn find_guard_nodes(ctx: &AnalysisContext) -> Vec<(NodeIndex, Cap)> {
| PredicateKind::ValidationCall
) {
result.push((idx, Cap::all()));
} else if cond_indirect_validator_callee(info, ctx).is_some() {
// Indirect-validator pattern:
// const err = validate(x); if (err) throw …;
// const ok = isValid(x); if (!ok) throw …;
// The classifier returns Unknown / NullCheck / ErrorCheck
// because the if-condition is a bare result variable, not
// a direct call expression. `cond_indirect_validator_callee`
// handles that by scanning the CFG for nodes whose
// `TaintMeta.defines` matches the condition variable and
// checking whether any defining Call has an
// `is_input_validator_callee`-recognised callee. This keeps
// cfg-unguarded-sink suppression aligned with the same
// structural validator recognition the SSA branch-narrowing
// layer uses, without requiring the condition itself to be
// a direct call expression.
//
// Motivated by Novu CVE GHSA-4x48-cgf9-q33f.
result.push((idx, Cap::all()));
} else if matches!(
kind,
PredicateKind::ShellMetaValidated | PredicateKind::BoundedLength
@ -733,7 +816,7 @@ fn sink_arg_is_parameter_only(ctx: &AnalysisContext, sink: NodeIndex) -> bool {
let sink_uses = &sink_info.taint.uses;
if sink_uses.is_empty() {
// No identifiable arguments could be a constant call like Command::new("ls")
// No identifiable arguments, could be a constant call like Command::new("ls")
return true; // treat as non-dangerous (constant arg)
}
@ -787,7 +870,7 @@ pub(crate) fn has_redirect_path_prefix(source_bytes: &[u8], span: (usize, usize)
false
}
/// Check if this sink is an internal redirect a `res.redirect` (SSRF sink)
/// Check if this sink is an internal redirect, a `res.redirect` (SSRF sink)
/// whose argument is a template literal or string starting with `/`, indicating
/// a server-relative path rather than an attacker-controlled URL.
fn is_internal_redirect(ctx: &AnalysisContext, sink: NodeIndex, sink_caps: Cap) -> bool {
@ -896,7 +979,7 @@ impl CfgAnalysis for UnguardedSink {
let source_derived = sink_arg_is_source_derived(ctx, *sink);
// If sink args are all constants (including one-hop constant bindings)
// and taint didn't confirm, this is a false positive skip it.
// and taint didn't confirm, this is a false positive, skip it.
if is_all_args_constant(ctx, *sink) && !has_taint {
continue;
}
@ -904,7 +987,7 @@ impl CfgAnalysis for UnguardedSink {
// SSA latest-def suppression: when the taint engine has already
// proved no source-tainted data reaches this sink (`!has_taint`)
// and every SSA operand resolves to a constant, callee-fragment
// pseudo-name, OR a function parameter that is not a Source
// pseudo-name, OR a function parameter that is not a Source ,
// the sink's actual arguments cannot carry an injection payload.
// Catches the reassign-to-constant idiom (`name := req.x; name =
// "Guest"; sink(name)`) where the latest SSA def is a literal
@ -919,7 +1002,7 @@ impl CfgAnalysis for UnguardedSink {
// Type-aware suppression: when all SSA operand values of the sink
// are proven to carry non-injectable types (e.g. integers parsed
// from a raw source), the arguments cannot form a payload for
// SHELL/SQL/FILE sinks. Skip the structural finding the taint
// SHELL/SQL/FILE sinks. Skip the structural finding, the taint
// engine already covers the source→sink flow via type-aware
// suppression. Unknown-typed or mixed operands fall through.
if !has_taint && sink_args_typed_safe(ctx, *sink, sink_caps) {
@ -936,13 +1019,13 @@ impl CfgAnalysis for UnguardedSink {
// Parameterized SQL queries: arg 0 is a string literal with
// placeholders ($1, ?, %s, :name) and a params argument exists.
// These are safe by construction the driver handles escaping.
// These are safe by construction, the driver handles escaping.
if sink_info.parameterized_query {
continue;
}
// Internal redirects: res.redirect(`/path/...`) with a path-prefix
// argument are server-relative not attacker-controlled URLs.
// argument are server-relative, not attacker-controlled URLs.
if is_internal_redirect(ctx, *sink, sink_caps) {
continue;
}
@ -953,10 +1036,10 @@ impl CfgAnalysis for UnguardedSink {
let (severity, confidence) = if has_taint || source_derived {
(Severity::High, Confidence::High)
} else if param_only && !in_entrypoint {
// Wrapper function with param-only args zero signal. Suppress.
// Wrapper function with param-only args, zero signal. Suppress.
continue;
} else if !ctx.taint_active {
// AST-only / cfg-only mode preserve as LOW (unchanged)
// AST-only / cfg-only mode, preserve as LOW (unchanged)
(Severity::Low, Confidence::Low)
} else {
// taint_active=true but found nothing.
@ -970,7 +1053,7 @@ impl CfgAnalysis for UnguardedSink {
// If the function containing the sink has no Source-labeled
// nodes AND no parameters (through which taint could flow
// from callers), taint ran and found nothing because there
// is nothing to find. Suppress the structural finding
// is nothing to find. Suppress, the structural finding
// is noise.
let sink_func = sink_info.ast.enclosing_func.as_deref();
let has_sources = ctx.cfg.node_indices().any(|n| {

View file

@ -1,3 +1,5 @@
#![doc = include_str!(concat!(env!("OUT_DIR"), "/cfg_analysis.md"))]
pub mod auth;
pub mod dominators;
pub mod error_handling;
@ -30,17 +32,15 @@ pub struct BodyConstFacts {
pub type_facts: TypeFactResult,
/// Field-sensitive Steensgaard points-to facts.
///
/// Computed only when [`crate::pointer::is_enabled()`] (i.e. the
/// `NYX_POINTER_ANALYSIS=1` env var is set). Phase 2 of the
/// pointer-analysis rollout consumes this in `state::transfer.rs`
/// to suppress proxy-acquire mis-attribution on field-aliased
/// locals like `m := c.mu`. When `None`, every consumer must fall
/// back to its existing pointer-unaware behaviour.
/// Computed only when [`crate::pointer::is_enabled()`].
/// `state::transfer.rs` consumes this to suppress proxy-acquire
/// mis-attribution on field-aliased locals like `m := c.mu`. When
/// `None`, consumers fall back to pointer-unaware behaviour.
pub pointer_facts: Option<crate::pointer::PointsToFacts>,
}
/// Lower a body to SSA and run constant propagation. Returns `None` when
/// lowering fails (empty CFG, invalid entry) callers treat absence as
/// lowering fails (empty CFG, invalid entry), callers treat absence as
/// "no SSA facts available" and fall back to the syntactic path.
pub fn build_body_const_facts(body: &crate::cfg::BodyCfg, lang: Lang) -> Option<BodyConstFacts> {
let mut ssa = crate::ssa::lower_to_ssa_with_params(
@ -116,13 +116,13 @@ pub struct AnalysisContext<'a> {
/// Structural analyses use it to suppress findings when a sink's argument
/// SSA values are proven to carry non-injectable types (e.g. integers
/// parsed from a raw source can't form SHELL/SQL/path payloads). Sourced
/// from `body_const_facts` when present keep both pointers coherent.
/// from `body_const_facts` when present, keep both pointers coherent.
pub type_facts: Option<&'a TypeFactResult>,
/// Decorators / annotations / attributes attached to the body's
/// declaration (e.g. Python `@login_required`, Java `@PreAuthorize`,
/// Symfony `#[IsGranted(...)]`). Consumed by the AuthGap analysis to
/// suppress `cfg-auth-gap` when the framework already enforces auth at
/// the function-declaration level the gap only matters when the
/// the function-declaration level, the gap only matters when the
/// auth call has to live inside the body.
pub auth_decorators: &'a [String],
}

View file

@ -25,7 +25,7 @@ fn find_acquire_nodes(
}
if let Some(callee) = &info.call.callee {
let callee_lower = callee.to_ascii_lowercase();
// Check exclusions first if the callee matches an exclude
// Check exclusions first, if the callee matches an exclude
// pattern, it is NOT an acquire even if it also matches an
// acquire pattern (e.g. `freopen` ends with `fopen`).
let excluded = exclude_patterns.iter().any(|p| {
@ -74,7 +74,7 @@ fn find_release_nodes(ctx: &AnalysisContext, release_patterns: &[&str]) -> Vec<N
/// `if (acquire_var)` (or `if (!acquire_var)`) and the edge represents
/// "acquire_var is null", the resource was never actually produced on that
/// path, so a release is unnecessary. This closes the canonical
/// `FILE *f = fopen(...); if (f) fclose(f);` idiom without this rule the
/// `FILE *f = fopen(...); if (f) fclose(f);` idiom, without this rule the
/// false edge of the null check provides a path acquire→exit that misses
/// the release, producing a may-leak FP.
fn release_on_all_exit_paths(
@ -103,8 +103,8 @@ fn release_on_all_exit_paths(
/// the resource handle is null/falsy and therefore not actually acquired.
///
/// Recognises:
/// * `if (var)` false edge means `var` is null
/// * `if (!var)` true edge means `var` is null
/// * `if (var)`, false edge means `var` is null
/// * `if (!var)`, true edge means `var` is null
///
/// Rejects comparisons (`if (var != NULL)`), method calls
/// (`if (var.is_valid())`), and composite conditions (`if (var && cond)`).
@ -198,7 +198,7 @@ fn all_paths_pass_through(
/// - `obj.field = var` (C dot / generic field store)
/// - `list->next = ...` (linked-list insertion)
///
/// If the variable is transferred, there is no leak the receiving struct is
/// If the variable is transferred, there is no leak, the receiving struct is
/// responsible for the lifetime.
fn is_ownership_transferred(ctx: &AnalysisContext, acquire: NodeIndex) -> bool {
let acquired_var = match &ctx.cfg[acquire].taint.defines {
@ -258,7 +258,7 @@ fn is_ownership_transferred(ctx: &AnalysisContext, acquire: NodeIndex) -> bool {
false
};
if !is_field_write {
continue; // genuine redefinition stop this path
continue; // genuine redefinition, stop this path
}
}
@ -343,7 +343,7 @@ fn is_consumed_by_owner(ctx: &AnalysisContext, acquire: NodeIndex) -> bool {
}
}
// Also check the span text for consuming calls handles cases where
// Also check the span text for consuming calls, handles cases where
// the call is embedded in a return statement (e.g. `return FileResponse(f)`)
if info.taint.uses.iter().any(|u| u == &acquired_var) {
let (start, end) = info.ast.span;

View file

@ -141,7 +141,7 @@ static JAVA_AUTH: &[AuthRule] = &[AuthRule {
"hasPermission",
"requireRole",
// Spring Security / JAX-RS annotation names (used by decorator
// detection see `extract_auth_decorators` in src/cfg.rs).
// detection, see `extract_auth_decorators` in src/cfg.rs).
"PreAuthorize",
"PostAuthorize",
"Secured",
@ -174,7 +174,7 @@ static JS_AUTH: &[AuthRule] = &[AuthRule {
"jwt.verify",
// NestJS-style decorators and guard class names (seeded by decorator
// arg extraction in `extract_auth_decorators`). `UseGuards` alone is
// too generic we still match on guard *argument* identifiers here.
// too generic, we still match on guard *argument* identifiers here.
"Authenticated",
"AuthGuard",
"JwtAuthGuard",
@ -268,7 +268,7 @@ static CPP_AUTH: &[AuthRule] = &[AuthRule {
"check_auth",
"verify_token",
"validate_token",
// Custom C++ attributes framework-defined, bare-name match.
// Custom C++ attributes, framework-defined, bare-name match.
"authenticated",
"require_auth",
"admin_only",
@ -287,7 +287,7 @@ static RUST_AUTH: &[AuthRule] = &[AuthRule {
"check_auth",
"verify_token",
"validate_token",
// Custom proc-macro attributes framework-defined, bare-name match.
// Custom proc-macro attributes, framework-defined, bare-name match.
"authenticated",
"require_auth",
"admin_only",

View file

@ -127,7 +127,7 @@ fn unreachable_code_detection_runs_without_panic() {
#[test]
fn all_branches_reachable_no_findings() {
// All branches reachable no unreachable-code findings
// All branches reachable, no unreachable-code findings
let src = br#"
use std::process::Command;
fn main() {
@ -282,7 +282,7 @@ fn ssa_const_prop_preserves_sink_on_dynamic_source_arg() {
#[test]
fn unguarded_sink_detected() {
// Sink with no validation should be flagged
// Sink with no validation, should be flagged
let src = br#"
use std::process::Command;
fn main() {
@ -335,6 +335,90 @@ fn guarded_sink_with_sanitizer_not_flagged() {
);
}
/// Regression: `cond_indirect_validator_callee` used to pick the
/// textually-last call defining the condition variable across the
/// whole function, including reassignments that occur **after** the
/// `if`. When that later call wasn't a recognised validator, the
/// validator pattern was missed and the downstream sink was
/// (incorrectly) flagged as `cfg-unguarded-sink`.
///
/// Pattern:
/// let err = validateInput(cmd); // real validator, before the if
/// if (err) throw …; // sink-guarding branch
/// eval(cmd); // sink dominated by the guard
/// err = recordMetric(); // later reassignment, NOT a validator
///
/// The defining call reaching the `if` is `validateInput`; the
/// `recordMetric` reassignment is downstream of the use and must not
/// shadow it.
#[test]
fn indirect_validator_ignores_reassignment_after_if() {
let src = br#"
async function handler(req) {
const cmd = req.query.cmd;
let err = await validateInput(cmd);
if (err) {
throw new Error('blocked');
}
eval(cmd);
err = recordMetric();
}
"#;
let findings = parse_and_analyse(
&guards::UnguardedSink,
src,
"javascript",
Language::from(tree_sitter_javascript::LANGUAGE),
);
let guard_findings: Vec<_> = findings
.iter()
.filter(|f| f.rule_id == "cfg-unguarded-sink")
.collect();
assert!(
guard_findings.is_empty(),
"later non-validator reassignment must not shadow the real validator def reaching the if; got {:?}",
guard_findings
);
}
/// Companion sanity check for `indirect_validator_ignores_reassignment_after_if`:
/// without the trailing reassignment the same pattern is already
/// suppressed by `cond_indirect_validator_callee`. Pinned so a future
/// change to the indirect-validator recognition can't silently regress
/// this baseline alongside the regression case above.
#[test]
fn indirect_validator_baseline_suppresses_dominated_sink() {
let src = br#"
async function handler(req) {
const cmd = req.query.cmd;
const err = await validateInput(cmd);
if (err) {
throw new Error('blocked');
}
eval(cmd);
}
"#;
let findings = parse_and_analyse(
&guards::UnguardedSink,
src,
"javascript",
Language::from(tree_sitter_javascript::LANGUAGE),
);
let guard_findings: Vec<_> = findings
.iter()
.filter(|f| f.rule_id == "cfg-unguarded-sink")
.collect();
assert!(
guard_findings.is_empty(),
"indirect-validator pattern (no reassignment) must suppress dominated sink; got {:?}",
guard_findings
);
}
// ─── Auth gap tests ────────────────────────────────────────────────────
#[test]
@ -397,7 +481,7 @@ fn auth_check_before_sink_no_finding() {
#[test]
fn error_fallthrough_analysis_runs_on_go() {
// Go pattern: err check without return, followed by dangerous call.
// This is a heuristic analysis we verify it runs without panicking.
// This is a heuristic analysis, we verify it runs without panicking.
let src = br#"
package main
import "os/exec"
@ -422,7 +506,7 @@ fn error_fallthrough_analysis_runs_on_go() {
#[test]
fn proper_error_return_no_finding_go() {
// Go pattern: err check with return should not flag error fallthrough.
// Go pattern: err check with return, should not flag error fallthrough.
let src = br#"
package main
import "os/exec"
@ -820,6 +904,7 @@ fn taint_and_unguarded_sink_deduped() {
path_hash: 0,
finding_id: String::new(),
alternative_finding_ids: smallvec::SmallVec::new(),
effective_sink_caps: crate::labels::Cap::empty(),
}];
let findings = parse_and_run_all_with_taint(
@ -949,7 +1034,7 @@ function readFile() {
#[test]
fn js_throw_terminates_block() {
// throw should act as a terminator code directly after throw in the same
// throw should act as a terminator, code directly after throw in the same
// block should be unreachable.
let src = br#"
function fail() {
@ -1031,7 +1116,7 @@ fn configured_terminator_stops_flow() {
"eval should be unreachable after process.exit terminator"
);
}
// If eval_nodes is empty it means the node wasn't created (also acceptable
// If eval_nodes is empty it means the node wasn't created (also acceptable ,
// it's after a terminator so the CFG may not even emit it)
}
@ -1480,7 +1565,7 @@ void process() {
let reachable = dominators::reachable_set(cfg, entry);
// All nodes should be reachable the preproc recovery should prevent
// All nodes should be reachable, the preproc recovery should prevent
// the dangling-else from orphaning downstream code.
let unreachable_count = cfg.node_count() - reachable.len();
assert!(
@ -1515,7 +1600,7 @@ void process() {
let reachable = dominators::reachable_set(cfg, entry);
// All nodes should be reachable break exits the loop and post-loop
// All nodes should be reachable, break exits the loop and post-loop
// code (free(x)) should be connected.
let unreachable_count = cfg.node_count() - reachable.len();
assert!(
@ -1878,7 +1963,7 @@ def run():
#[test]
fn python_one_hop_constant_still_suppressed() {
// cmd = "ls"; os.system(cmd) `all_args_literal` is false (identifier arg),
// cmd = "ls"; os.system(cmd), `all_args_literal` is false (identifier arg),
// but should still be suppressed via existing one-hop constant trace in cfg_analysis.
let src = br#"
import os
@ -1959,7 +2044,7 @@ def run():
#[test]
fn python_constant_receiver_tainted_arg_produces_finding() {
// safe_obj.system(user_input) constant receiver is irrelevant, tainted arg must report
// safe_obj.system(user_input), constant receiver is irrelevant, tainted arg must report
let src = br#"
import os
import sys

View file

@ -26,7 +26,7 @@ fn event_handler_callbacks(ctx: &AnalysisContext) -> HashSet<String> {
.iter()
.any(|h| callee_lower.ends_with(&h.to_ascii_lowercase()));
if is_handler {
// The callback function is typically used within the call any function
// The callback function is typically used within the call, any function
// that appears as `uses` of this call node is a potential callback.
for u in &info.taint.uses {
callbacks.insert(u.clone());
@ -113,7 +113,7 @@ impl CfgAnalysis for UnreachableCode {
Severity::Medium,
)
} else {
// Plain unreachable code low severity
// Plain unreachable code, low severity
continue;
}
};