From 2bd2c18ddb7991a402ae310c60993188bfeb02d7 Mon Sep 17 00:00:00 2001 From: Akatenvictor <“akatenvictor@gmail.com”> Date: Sat, 27 Jun 2026 22:37:43 +0100 Subject: [PATCH] sanctifier harden imput validation --- DOCUMENTATION_INDEX.md | 7 + docs/rules/s001-auth-gap.md | 117 +++++++++++++++++ tooling/sanctifier-core/src/lib.rs | 44 +++---- tooling/sanctifier-core/src/rules/auth_gap.rs | 122 +++++++++++++++++- .../sanctifier-core/src/rules/crlf_tests.rs | 31 ++++- 5 files changed, 294 insertions(+), 27 deletions(-) create mode 100644 docs/rules/s001-auth-gap.md diff --git a/DOCUMENTATION_INDEX.md b/DOCUMENTATION_INDEX.md index 57493d4c..b7b3938b 100644 --- a/DOCUMENTATION_INDEX.md +++ b/DOCUMENTATION_INDEX.md @@ -160,6 +160,13 @@ ### Finding Code Documentation +**[docs/rules/s001-auth-gap.md](docs/rules/s001-auth-gap.md)** - S001: Missing Authorization Guard (`auth_gap`) + +- What constitutes a privileged operation (storage mutation, external call) +- Vulnerable vs fixed code examples +- Input validation behaviour (null bytes, oversized source) +- Auto-fix notes and suppression guidance + **[docs/rules/s012-sep41-interface.md](docs/rules/s012-sep41-interface.md)** - S012: SEP-41 Token Interface Compliance - Complete SEP-41 standard interface verification diff --git a/docs/rules/s001-auth-gap.md b/docs/rules/s001-auth-gap.md new file mode 100644 index 00000000..3ba0e51b --- /dev/null +++ b/docs/rules/s001-auth-gap.md @@ -0,0 +1,117 @@ +# S001 — Missing Authorization Guard (`auth_gap`) + +**Category:** authentication +**Severity:** Warning (Critical in `FindingCode` catalogue) +**Rule name:** `auth_gap` + +--- + +## What it detects + +Any `pub` function inside an `impl` block that performs a **privileged operation** without +first calling `require_auth()` or `require_auth_for_args()`. Privileged operations are: + +| Operation class | Detected patterns | +|---|---| +| Storage mutation | `.set()`, `.update()`, `.remove()`, `.extend_ttl()` on `storage()`, `persistent()`, `temporary()`, or `instance()` | +| External contract call | `.invoke_contract()`, or a method call on a receiver whose type name ends in `Client` / `_client` | + +Reserved Soroban entry-points (`__constructor`, `__check_auth`) are excluded automatically. +Private (`fn`) and non-`impl` functions are not checked. + +--- + +## Why it matters + +A public function that writes state or calls external contracts without checking the caller's +identity can be invoked by **anyone** — draining balances, replacing the admin, or triggering +arbitrary cross-contract logic. This is one of the most common critical findings in smart-contract +audits on Stellar. + +--- + +## Examples + +### Vulnerable — flagged by S001 + +```rust +impl Token { + pub fn set_admin(env: Env, new_admin: Address) { + // Missing require_auth — anyone can replace the admin. + env.storage().instance().set(&DataKey::Admin, &new_admin); + } +} +``` + +### Fixed + +```rust +impl Token { + pub fn set_admin(env: Env, new_admin: Address) { + new_admin.require_auth(); // ← guard added + env.storage().instance().set(&DataKey::Admin, &new_admin); + } +} +``` + +### Also fixed — using `require_auth_for_args` + +```rust +impl Token { + pub fn transfer(env: Env, from: Address, to: Address, amount: i128) { + from.require_auth_for_args((&from, &to, amount).into_val(&env)); + // ... storage mutations ... + } +} +``` + +--- + +## Remediation + +1. Call `addr.require_auth()` (or `addr.require_auth_for_args(...)` — see [S030](require-auth-for-args.md)) + as the **first statement** in every state-mutating public function. +2. Consider whether read-only functions that expose sensitive data also need auth. +3. After adding the guard, run `sanctifier fix --rule auth_gap` to apply the automatic patch. + +--- + +## Input validation + +The rule validates its input before parsing: + +| Input condition | Behaviour | +|---|---| +| Empty source | Returns no violations (no contract to analyse) | +| Source > 10 MB | Returns one `Error`-severity violation with code `SOURCE_TOO_LARGE` | +| Source contains null bytes | Returns one `Error`-severity violation with code `NULL_BYTE_DETECTED` | +| Source does not parse as valid Rust | Returns no violations (parse errors are surfaced separately by the CLI) | + +--- + +## Auto-fix + +`sanctifier fix` inserts `env.require_auth();` as the first statement in each flagged function. +Review the patch before committing — the correct auth target (caller vs. a specific `Address` +argument) depends on the function's semantics. + +--- + +## Suppression + +```rust +// sanctifier:ignore auth_gap +pub fn set_admin(env: Env, new_admin: Address) { ... } +``` + +Only suppress when you have a documented reason (e.g. the function is already called through an +authenticated wrapper contract). + +--- + +## References + +- Stellar SEP-41 — required auth patterns for token interfaces +- [S030 — Missing `require_auth_for_args`](require-auth-for-args.md) +- [Soroban auth model](https://soroban.stellar.org/docs/fundamentals-and-concepts/authorization) +- [docs/rule-authoring-guide.md](../rule-authoring-guide.md) diff --git a/tooling/sanctifier-core/src/lib.rs b/tooling/sanctifier-core/src/lib.rs index a19baf5b..eac63e34 100644 --- a/tooling/sanctifier-core/src/lib.rs +++ b/tooling/sanctifier-core/src/lib.rs @@ -54,6 +54,7 @@ pub mod complexity; pub mod finding_codes; pub mod gas_estimator; pub mod gas_report; +pub mod input_validation; pub mod patcher; pub mod reentrancy; pub mod rules; @@ -419,30 +420,25 @@ impl Analyzer { } fn scan_auth_gaps_impl(&self, source: &str) -> Vec { - let file = match parse_str::(source) { - Ok(f) => f, - Err(_) => return vec![], - }; - - let mut gaps = Vec::new(); - for item in &file.items { - if let Item::Impl(i) = item { - for impl_item in &i.items { - if let syn::ImplItem::Fn(f) = impl_item { - if let syn::Visibility::Public(_) = f.vis { - let fn_name = f.sig.ident.to_string(); - let mut has_mutation = false; - let mut has_auth = false; - self.check_fn_body(&f.block, &mut has_mutation, &mut has_auth); - if has_mutation && !has_auth { - gaps.push(fn_name); - } - } - } - } - } - } - gaps + rules::auth_gap::AuthGapRule::new() + .check(source) + .into_iter() + .filter_map(|v| { + // Validation-error violations use a "" pseudo-location; + // skip them — they are not real auth-gap findings. + if v.location.starts_with('<') { + return None; + } + // Location format is "fn_name:line"; extract the function name. + let name = v + .location + .split(':') + .next() + .unwrap_or(&v.location) + .to_string(); + Some(name) + }) + .collect() } pub fn scan_panics(&self, source: &str) -> Vec { diff --git a/tooling/sanctifier-core/src/rules/auth_gap.rs b/tooling/sanctifier-core/src/rules/auth_gap.rs index dc21f170..c8f0bf6e 100644 --- a/tooling/sanctifier-core/src/rules/auth_gap.rs +++ b/tooling/sanctifier-core/src/rules/auth_gap.rs @@ -1,3 +1,4 @@ +use crate::input_validation::{validate_no_null_bytes, validate_source_size}; use crate::rules::{Patch, Rule, RuleViolation, Severity}; use syn::spanned::Spanned; use syn::{parse_str, File, Item}; @@ -45,6 +46,37 @@ impl Rule for AuthGapRule { } fn check(&self, source: &str) -> Vec { + // Guard: empty source has no findings (fast-path, no parse needed). + if let Err(e) = validate_source_size(source) { + if e.code == "EMPTY_SOURCE" { + return vec![]; + } + // Source too large — emit a structured diagnostic so CI can surface it. + return vec![RuleViolation::new( + self.name(), + Severity::Error, + format!("Input rejected by auth_gap rule: {}", e.message), + "".to_string(), + ) + .with_suggestion( + "Split the contract into smaller files and analyse each separately.".to_string(), + )]; + } + + // Guard: null bytes are never valid in Rust source and indicate binary + // data or a potential injection attempt. + if let Err(e) = validate_no_null_bytes(source) { + return vec![RuleViolation::new( + self.name(), + Severity::Error, + format!("Input rejected by auth_gap rule: {}", e.message), + "".to_string(), + ) + .with_suggestion( + "Ensure the file is saved as UTF-8 text and contains no binary data.".to_string(), + )]; + } + let file = match parse_str::(source) { Ok(f) => f, Err(_) => return vec![], @@ -60,6 +92,7 @@ impl Rule for AuthGapRule { if is_reserved_soroban_entrypoint(&fn_name) { continue; } + let fn_line = f.sig.ident.span().start().line; let mut summary = FunctionSecuritySummary::default(); check_fn_body(&f.block, &mut summary); if summary.has_sensitive_action() && !summary.has_auth { @@ -67,7 +100,7 @@ impl Rule for AuthGapRule { self.name(), Severity::Warning, format!("Function '{}' performs a privileged operation without authentication", fn_name), - fn_name.clone(), + format!("{}:{}", fn_name, fn_line), ).with_suggestion("Add require_auth() or require_auth_for_args() before storage operations or external contract calls".to_string())); } } @@ -368,4 +401,91 @@ mod tests { "parse error must return empty, not panic" ); } + + // ── Input validation guards ─────────────────────────────────────────────── + + #[test] + fn null_byte_source_produces_error_violation() { + let rule = AuthGapRule::new(); + let source = "fn foo() { \0 }"; + let violations = rule.check(source); + assert_eq!(violations.len(), 1, "null-byte input must emit exactly one violation"); + assert_eq!(violations[0].severity, super::Severity::Error); + assert!( + violations[0].message.contains("null bytes"), + "message must mention null bytes; got: {}", + violations[0].message + ); + assert_eq!(violations[0].location, ""); + } + + #[test] + fn oversized_source_produces_error_violation() { + use crate::input_validation::MAX_SOURCE_BYTES; + let rule = AuthGapRule::new(); + let over = "x".repeat(MAX_SOURCE_BYTES + 1); + let violations = rule.check(&over); + assert_eq!(violations.len(), 1, "oversized input must emit exactly one violation"); + assert_eq!(violations[0].severity, super::Severity::Error); + assert!( + violations[0].message.contains("too large") || violations[0].message.contains("maximum"), + "message must mention size limit; got: {}", + violations[0].message + ); + assert_eq!(violations[0].location, ""); + } + + #[test] + fn violation_location_includes_line_number() { + let rule = AuthGapRule::new(); + let source = r#" + impl MyContract { + pub fn set_value(env: Env, v: u32) { + env.storage().persistent().set(&symbol_short!("V"), &v); + } + } + "#; + let violations = rule.check(source); + assert!(!violations.is_empty(), "must detect auth gap"); + let loc = &violations[0].location; + assert!( + loc.contains(':'), + "location must be 'fn_name:line', got: {loc}" + ); + let parts: Vec<&str> = loc.splitn(2, ':').collect(); + assert_eq!(parts[0], "set_value"); + let line: usize = parts[1].parse().expect("line part must be a number"); + assert!(line > 0, "line number must be positive"); + } + + #[test] + fn whitespace_only_source_produces_no_findings() { + let rule = AuthGapRule::new(); + // Whitespace is non-empty (passes size guard) but parses to an empty AST. + let violations = rule.check(" \n\t "); + assert!( + violations.is_empty(), + "whitespace-only source must produce no findings" + ); + } + + #[test] + fn crlf_source_produces_same_findings_as_lf() { + let rule = AuthGapRule::new(); + let lf = r#" +impl MyContract { + pub fn set_admin(env: Env, admin: Address) { + env.storage().persistent().set(&symbol_short!("A"), &admin); + } +} +"#; + let crlf = lf.replace('\n', "\r\n"); + let lf_violations = rule.check(lf); + let crlf_violations = rule.check(&crlf); + assert_eq!( + lf_violations.len(), + crlf_violations.len(), + "violation count must be identical for LF and CRLF input" + ); + } } diff --git a/tooling/sanctifier-core/src/rules/crlf_tests.rs b/tooling/sanctifier-core/src/rules/crlf_tests.rs index 4d0d4852..b4360408 100644 --- a/tooling/sanctifier-core/src/rules/crlf_tests.rs +++ b/tooling/sanctifier-core/src/rules/crlf_tests.rs @@ -7,8 +7,9 @@ #[allow(clippy::module_inception)] mod crlf_tests { use crate::rules::{ - instance_storage_misuse::InstanceStorageMisuseRule, panic_detection::PanicDetectionRule, - reentrancy::ReentrancyRule, unhandled_result::UnhandledResultRule, Rule, + auth_gap::AuthGapRule, instance_storage_misuse::InstanceStorageMisuseRule, + panic_detection::PanicDetectionRule, reentrancy::ReentrancyRule, + unhandled_result::UnhandledResultRule, Rule, }; /// Convert LF source to CRLF by replacing every `\n` with `\r\n`. @@ -30,6 +31,32 @@ mod crlf_tests { }}; } + // ── AuthGapRule ─────────────────────────────────────────────────────────── + + const AUTH_GAP_SOURCE: &str = r#" +impl Contract { + pub fn set_admin(env: Env, admin: Address) { + env.storage().persistent().set(&symbol_short!("A"), &admin); + } +} +"#; + + #[test] + fn auth_gap_lf_crlf_parity() { + assert_crlf_parity!(AuthGapRule::new(), AUTH_GAP_SOURCE, "AuthGapRule"); + } + + #[test] + fn auth_gap_crlf_finds_violations() { + let rule = AuthGapRule::new(); + let crlf = to_crlf(AUTH_GAP_SOURCE); + let v = rule.check(&crlf); + assert!( + !v.is_empty(), + "AuthGapRule must flag missing auth in CRLF input" + ); + } + // ── PanicDetectionRule ──────────────────────────────────────────────────── const PANIC_SOURCE: &str = r#"