Validate tuning inputs; remove dead pass block (audit fixes #989)
- _prompt() clamps non-positive tuning values to default with operator warning (HIGH from AUDIT_SEC_989: prevents garbage display when vars file has masscan_rate: -100 or nuclei_concurrency: 0) - Remove empty 'if scan_mode in (...): pass' placeholder left from refactor (P3 from AUDIT_CODE_989) - Add AUDIT_CODE/SEC/ENV_989.md and FIXES_989.md with skip justifications (path traversal not applicable to single-operator threat model; Tor wording already adequate; estimate disclosure interactive-only)
This commit is contained in:
n0mad1k
@@ -0,0 +1,88 @@
|
||||
---
|
||||
agent: code-auditor
|
||||
status: COMPLETE
|
||||
timestamp: 2026-05-02T00:00:00Z
|
||||
duration_seconds: 180
|
||||
files_scanned: 2
|
||||
findings_count: 5
|
||||
errors: []
|
||||
skipped_checks: []
|
||||
---
|
||||
|
||||
# Code Audit — DevTrack #989: Webrunner Estimator Rewrite
|
||||
|
||||
## Findings
|
||||
|
||||
### P2 (Medium Priority)
|
||||
|
||||
- **`modules/webrunner/deploy_webrunner.py:306`** — `gather_webrunner_parameters()` is 227 lines. Complexity is high due to sequential parameter gathering, but each section is self-contained (providers, countries, scan mode, tuning, estimate display, preset selection, CIDR distribution). Refactor would require breaking into multiple functions that each prompt and return data — tradeoff: current structure is arguably clearer for a linear setup wizard. **Recommendation**: Document the phases with section headers (already done with comments) and consider splitting only if interactivity requirements change.
|
||||
|
||||
- **`utils/provider_rates.py:87-145`** — `estimate_scan_hours()` has cyclomatic complexity of 11 (threshold: 10). The complexity is necessary: 4 scan modes × nested `use_tor` conditions = 5 paths per phase × 4 phases + rate check. Each branch is distinct (different time calculations). Logic is correct but at the boundary. **Recommendation**: acceptable due to inherent problem structure, but monitor for future growth.
|
||||
|
||||
- **`modules/webrunner/deploy_webrunner.py:200,206`** — Tuning params `nmap_timeout` and `nuclei_timeout` are prompted from user (lines 200, 206) and stored in config (lines 559, 562 show them printed), but **NOT passed to `estimate_scan_hours()`** in provider_rates.py. These parameters **DO NOT affect the time estimate** — they are only used at scan execution time (node_scanner.py lines 376-381, 396). This is **correct behavior** but the params are dead-code for estimation purposes. Current design: `estimate_scan_hours()` uses fixed constants (`NMAP_TIME_BY_TIMING`, `NUCLEI_TIME_PER_TARGET_SEC`) which are conservative defaults. The tuning params would require significant refactor to propagate through the estimation function signature (nmap_timeout not indexed by timing template, nuclei_timeout not indexed by template complexity). **Assessment**: No bug, by design. Timeouts are runtime constraints, not estimation inputs.
|
||||
|
||||
### P3 (Low Priority)
|
||||
|
||||
- **`modules/webrunner/deploy_webrunner.py:196`** — Dead `pass` statement. Line 195-196:
|
||||
```python
|
||||
if scan_mode in ('masscan+nmap', 'geo-scout', 'masscan+nuclei'):
|
||||
pass # masscan_rate already handled
|
||||
```
|
||||
This is a placeholder comment; the condition is never needed since `masscan_rate` is always prompted on line 193. **Recommendation**: Remove the conditional block entirely.
|
||||
|
||||
- **`modules/webrunner/deploy_webrunner.py:265,279,281`** — Magic numbers (70, 56, 10, 25) in format strings are visual constants for table borders and tuning display. All acceptable.
|
||||
|
||||
### P4 (Informational)
|
||||
|
||||
- No `print()` statements in production code detected — all output uses Rich-style ANSI color codes via `COLORS` dict (good practice for TUI).
|
||||
- No TODO/FIXME/HACK comments detected.
|
||||
- No unused imports detected.
|
||||
- All function signatures updated consistently (`_show_estimate_table` receives tuning dict, passes to `build_estimate_table`).
|
||||
- Nuclei phase throughput calculation at `utils/provider_rates.py:140-142` is mathematically correct:
|
||||
```
|
||||
parallel_throughput = nuclei_concurrency / per_target
|
||||
rate_throughput = nuclei_rate
|
||||
effective_rps = min(rate_throughput, parallel_throughput)
|
||||
```
|
||||
With defaults (25 concurrency, 5s per target, 150 req/s rate): min(150, 5) = 5 req/s bottleneck (parallelism-limited). Edge cases handled correctly.
|
||||
|
||||
## Stats
|
||||
|
||||
- **Files >500 lines**: 1 (`deploy_webrunner.py`, 617 lines)
|
||||
- **Functions >50 lines**: 1 (`gather_webrunner_parameters`, 227 lines)
|
||||
- **Cyclomatic complexity >10**: 1 (`estimate_scan_hours`, CC=11)
|
||||
- **Nesting depth >3**: 1 instance (L60, necessary for loop logic)
|
||||
- **print() calls**: 0 (ANSI output via COLORS dict only)
|
||||
- **TODO/FIXME/HACK**: 0
|
||||
- **Dead code blocks**: 1 placeholder conditional (lines 195-196)
|
||||
- **Unused imports**: 0
|
||||
- **Signature changes**: 1 (`_show_estimate_table` + tuning param) — validated across 2 call sites
|
||||
|
||||
## Verification Notes
|
||||
|
||||
### Nuclei Throughput Logic ✓
|
||||
The calculation `effective_rps = min(rate_throughput, parallel_throughput)` correctly identifies which constraint is tighter:
|
||||
- If concurrency is small relative to per_target time (e.g., 25 / 5 = 5 req/s), parallelism is the bottleneck.
|
||||
- If rate limit is small (e.g., 2 req/s), rate limit is the bottleneck.
|
||||
- The `max(min(...), 1.0)` ensures effective_rps ≥ 1.0 (prevents division by very small numbers).
|
||||
|
||||
### Phase Decomposition Correctness ✓
|
||||
Each phase (masscan, nmap, probe, nuclei) applies Tor multiplier only to TCP-based operations (nmap, nuclei, probe), never to masscan (raw sockets). This is correct per OPSEC comment on line 77-79.
|
||||
|
||||
### Tuning Param Integration ✓
|
||||
All 6 tuning params extracted in `build_estimate_table()` are correctly passed to `estimate_scan_hours()`:
|
||||
- `nmap_timing`, `nmap_workers`, `nuclei_rate`, `nuclei_concurrency`, `hit_rate`, `masscan_rate`
|
||||
- No params are lost; no params are unused.
|
||||
|
||||
---
|
||||
|
||||
## No Code Quality Issues Detected
|
||||
|
||||
All critical paths reviewed:
|
||||
- Module imports: complete and used
|
||||
- Function signatures: refactored correctly
|
||||
- Logic bugs: none detected
|
||||
- Dead code: 1 placeholder conditional (recommended removal)
|
||||
- Complexity: at acceptable threshold, necessary for problem domain
|
||||
|
||||
**Refactor Status**: Ready for deployment. Consider future simplification of `gather_webrunner_parameters()` if user interaction patterns change.
|
||||
@@ -0,0 +1,61 @@
|
||||
---
|
||||
agent: env-validator
|
||||
status: PASS
|
||||
timestamp: 2026-05-02T00:00:00Z
|
||||
findings_count: 0
|
||||
errors: []
|
||||
---
|
||||
|
||||
# Env Validation — DevTrack #989
|
||||
|
||||
## Summary
|
||||
Comprehensive secrets hygiene audit for webrunner estimator rewrite (provider_rates.py and deploy_webrunner.py). **All checks passed.**
|
||||
|
||||
## PASS
|
||||
|
||||
### Secrets in source
|
||||
- **provider_rates.py**: Clean. Contains only tuning constants (rate tables, timing defaults, cost multipliers). No API keys, tokens, passwords, or credential patterns detected.
|
||||
- **deploy_webrunner.py**: Clean. Contains no hardcoded secrets. All credential handling is delegated to provider-specific utilities (linode_utils, aws_utils, flokinet_utils) which retrieve secrets from Infisical at runtime.
|
||||
|
||||
### API key/token patterns
|
||||
Searched all files for:
|
||||
- `ghp_[A-Za-z0-9_]+` (GitHub tokens) — NOT FOUND
|
||||
- `sk_live_[A-Za-z0-9]+` or `sk_test_[A-Za-z0-9]+` (Stripe) — NOT FOUND
|
||||
- `AKIA[A-Z0-9]{16}` (AWS access keys) — NOT FOUND
|
||||
- `Bearer [A-Za-z0-9._-]{20,}` (bearer tokens) — NOT FOUND
|
||||
- `password\s*=\s*["'][^"']{8,}` (hardcoded passwords) — NOT FOUND
|
||||
- `api[_-]?key\s*=\s*["'][^"']{10,}` (API keys) — NOT FOUND
|
||||
|
||||
### .env file content
|
||||
- No new .env files created in either module
|
||||
- No .env file writes detected in code
|
||||
- Tuning parameters stored in safe yaml configs:
|
||||
- `inputs/targets.yaml` — target definitions, probe patterns, vulnerability tags only
|
||||
- `inputs/countries.yaml` — country codes, priority levels, exclude lists only
|
||||
- User-supplied vars files (optional) — only tuning parameters (masscan_rate, nmap_timing, hit_rate, etc.)
|
||||
|
||||
### Credential handling verification
|
||||
All provider credentials properly retrieved from Infisical:
|
||||
- **Linode**: `creds get LINODE_TOKEN homelab` (linode_utils.py:16-19)
|
||||
- **AWS**: `creds get AWS_ACCESS_KEY_ID homelab` and `creds get AWS_SECRET_ACCESS_KEY homelab` (aws_utils.py:17-24)
|
||||
- Fallback to vars file templates only (marked as placeholders like 'YOUR_AWS')
|
||||
- No secrets persisted to disk; only set in `os.environ` in memory (deployment_engine.py:set_provider_environment)
|
||||
|
||||
### Git history check
|
||||
- No secrets in recent commits to these files
|
||||
- 16 commits to webrunner module examined (latest: "Phase-accurate webrunner cost/time estimator...")
|
||||
- Notable commit: "Fix P1 WEBRUNNER deployment failures and secret leakage" (18039b2) — prior leak was addressed and fixed
|
||||
- No secret patterns detected in any commit
|
||||
|
||||
### Code review
|
||||
- `provider_rates.py` (209 lines): Pure computation — estimate_scan_hours(), build_estimate_table(), formatting helpers. No file I/O, no env var access.
|
||||
- `deploy_webrunner.py` (617 lines): Parameter gathering and deployment orchestration. All secrets retrieved via subprocess calls to `creds` binary (lines 16-24 in provider utils). No new credential handling code introduced in this rewrite.
|
||||
|
||||
## Findings
|
||||
**0 issues.**
|
||||
|
||||
No hardcoded secrets, no .env writes, no credential handling added. Secrets flow:
|
||||
```
|
||||
User input → provider_utils → creds binary (Infisical) → os.environ (memory only)
|
||||
```
|
||||
This is correct per secrets management rules.
|
||||
@@ -0,0 +1,135 @@
|
||||
---
|
||||
agent: security-auditor
|
||||
status: COMPLETE
|
||||
timestamp: 2026-05-02T00:00:00Z
|
||||
duration_seconds: 15
|
||||
files_scanned: 3
|
||||
findings_count: 4
|
||||
critical_count: 0
|
||||
high_count: 1
|
||||
errors: []
|
||||
skipped_checks: []
|
||||
---
|
||||
|
||||
# Security Audit — DevTrack #989 (Webrunner Estimator Rewrite)
|
||||
|
||||
## Scope
|
||||
Files audited:
|
||||
- `/home/n0mad1k/tools/c2itall/utils/provider_rates.py` (209 lines)
|
||||
- `/home/n0mad1k/tools/c2itall/modules/webrunner/deploy_webrunner.py` (617 lines)
|
||||
- `/home/n0mad1k/tools/c2itall/modules/webrunner/tasks/merge_results.py` (118 lines)
|
||||
|
||||
Checks performed: input injection, hardcoded secrets, auth, OPSEC, malformed input handling, path safety, estimate disclosure.
|
||||
|
||||
---
|
||||
|
||||
## Findings
|
||||
|
||||
### HIGH (CVSS 7.0–8.9)
|
||||
|
||||
#### 1. Unvalidated Negative/Zero Tuning Parameters Allow Cost Calculation DoS
|
||||
**Location**: `deploy_webrunner.py:181-191`, `provider_rates.py:177-143`
|
||||
|
||||
**Description**: Tuning parameters from vars file or CLI input are cast to `int` without validation. Negative or zero values for `masscan_rate`, `nmap_workers`, `nuclei_concurrency` propagate directly into mathematical operations (division, multiplication) in the estimate functions, resulting in nonsensical estimates or potential floating-point exceptions.
|
||||
|
||||
**Attack vector**: Operator provides malformed vars file with `masscan_rate: -100` or `nuclei_concurrency: 0`, causing:
|
||||
- `estimate_scan_hours()` to compute incorrect hours (negative division result at line 141)
|
||||
- Cost estimates to be silently corrupted
|
||||
- Decision-making based on invalid data
|
||||
|
||||
**Remediation**:
|
||||
- In `_prompt()` (line 181), add validation after cast: `if value <= 0: value = default`
|
||||
- In `build_estimate_table()` (line 177), enforce minimum values: `masscan_rate = max(1, int(...))`
|
||||
- Validate `nmap_workers` and `nuclei_concurrency` before use in math (already partially done at line 104, but only for nmap_workers; nuclei_concurrency lacks guard).
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM (CVSS 4.0–6.9)
|
||||
|
||||
#### 2. Tor Warning Phrasing Could Be Misinterpreted — Default Behavior Correct But Language Ambiguous
|
||||
**Location**: `deploy_webrunner.py:211-227`
|
||||
|
||||
**Description**: The warning at line 217 states "CRITICAL OPSEC WARNING — MASSCAN BYPASSES TOR" and explains that masscan uses raw sockets and cannot be proxied. However, the framing "THIS CLOUD NODE'S IP is exposed to targets" is accurate but could mislead an operator into thinking *all* scanning traffic is exposed.
|
||||
|
||||
The code correctly defaults to `[y/N]` (line 401), requiring explicit `y` or `yes` to proceed — this is secure. However, the message could clarify that:
|
||||
1. Only masscan SYN packets leak the node IP.
|
||||
2. nmap, nuclei, and probes ARE protected by Tor (correctly stated at line 223–224).
|
||||
3. The operator is consenting to a *known, documented limitation*, not an accidental leak.
|
||||
|
||||
**Current text**: "Every SYN packet sent during the masscan phase reveals THIS CLOUD NODE'S IP to the targets and any monitoring along the path."
|
||||
|
||||
**Better text**: "Every SYN packet sent during the masscan phase reveals THIS CLOUD NODE'S IP address to targets (raw sockets bypass proxychains entirely). Use 'nmap-only' mode for full Tor protection."
|
||||
|
||||
**Remediation**: Update warning wording to disambiguate masscan-only exposure vs. the rest of the stack.
|
||||
|
||||
---
|
||||
|
||||
#### 3. Path Traversal Risk in Template and Targets File Loading
|
||||
**Location**: `deploy_webrunner.py:162-173` (nuclei template), `deploy_webrunner.py:133-149` (targets.yaml)
|
||||
|
||||
**Description**: User-supplied paths to nuclei templates and targets.yaml files are opened without sanitization:
|
||||
- Line 169: `local_path = input(" Path to nuclei template (.yaml): ").strip()` → directly to `Path(local_path).exists()` → `open(path)` at line 141.
|
||||
- Line 136: `raw = input(f"Path to targets.yaml [{default}]: ").strip()` → `path = raw or default` → `open(path)` at line 141.
|
||||
|
||||
An operator (or vars file) could supply `../../../etc/passwd` or symlinks, allowing:
|
||||
1. Reading arbitrary files on the deployment controller.
|
||||
2. Leaking environment variables or private keys if yaml.safe_load() processes them.
|
||||
|
||||
**Attack vector**: Vars file with `nuclei_template: /etc/hostname` or `targets_file: ~/.ssh/id_rsa` — files are read and displayed.
|
||||
|
||||
**Remediation**:
|
||||
- Add path validation: `p = Path(local_path).resolve(); if not p.is_relative_to(Path.cwd()): reject`.
|
||||
- Alternatively, restrict to a specific directory: `p = (WEBRUNNER_INPUTS / local_path).resolve(); if not p.is_relative_to(WEBRUNNER_INPUTS): reject`.
|
||||
- Use pathlib consistently to prevent directory traversal.
|
||||
|
||||
---
|
||||
|
||||
#### 4. YAML Safe-Load Correctly Used — No Code Execution Risk
|
||||
**Location**: `deploy_webrunner.py:115-130`, `deploy_webrunner.py:139-149`
|
||||
|
||||
**Status**: PASS — `yaml.safe_load()` is used (not `yaml.load()`), preventing arbitrary Python object instantiation. No code execution risk from malformed YAML.
|
||||
|
||||
---
|
||||
|
||||
### LOW (CVSS <4.0)
|
||||
|
||||
#### 5. Estimate Display Discloses Target Scope and Timing to Console
|
||||
**Location**: `deploy_webrunner.py:256-301`, output at lines 272–296
|
||||
|
||||
**Description**: The estimate table displays:
|
||||
- Total IPs being scanned (`fmt_ip_count(total_ips)`)
|
||||
- Country codes (implicit in CIDR resolution)
|
||||
- Scan mode (masscan, nmap, nuclei)
|
||||
- Time estimates per node
|
||||
- Total cost
|
||||
|
||||
This information is printed to stdout and may be captured in shell history, logs, or screenshots. For offensive ops, this data could reveal:
|
||||
- Scope scale (e.g., "2.5M IPs" suggests multi-country / large infrastructure)
|
||||
- Duration (e.g., "12h per node" reveals patience/persistence)
|
||||
- Provider selection (e.g., "Linode + AWS" may hint at region strategy)
|
||||
|
||||
**Mitigated by**:
|
||||
- Information is shown only during interactive setup (not in batch mode).
|
||||
- Operator sees warning about provisioning time.
|
||||
- Cost estimates are precise only for the estimator, not for actual billing.
|
||||
|
||||
**Recommendation**:
|
||||
- Consider adding a `--quiet` flag to suppress estimate display in sensitive environments.
|
||||
- Ensure logs are cleaned post-deployment (already handled by log rotation).
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
**Secure patterns**:
|
||||
- YAML loading uses `safe_load()` ✓
|
||||
- No subprocess injection or eval/exec ✓
|
||||
- No hardcoded secrets or credentials ✓
|
||||
- Tor warning defaults to `[y/N]` (opt-in, secure) ✓
|
||||
- CIDR country map is read-only JSON, not executed ✓
|
||||
|
||||
**Action items**:
|
||||
1. **HIGH**: Add validation to tuning parameters to prevent negative/zero values.
|
||||
2. **MEDIUM**: Clarify Tor warning wording and add path traversal guards.
|
||||
3. **LOW**: Consider audit log suppression for estimate table in future versions.
|
||||
|
||||
@@ -0,0 +1,33 @@
|
||||
---
|
||||
devtrack: 989
|
||||
timestamp: 2026-05-03T00:00:00Z
|
||||
findings_addressed: 2
|
||||
findings_skipped: 3
|
||||
---
|
||||
|
||||
# Fix Summary — DevTrack #989
|
||||
|
||||
## Applied
|
||||
|
||||
### HIGH — Tuning input validation (AUDIT_SEC_989#1)
|
||||
**File:** `modules/webrunner/deploy_webrunner.py:181-195`
|
||||
**Fix:** `_prompt()` now clamps non-positive values to the default and warns the operator. Applies to all tuning keys (masscan_rate, nmap_timing, nmap_timeout, nmap_workers, nuclei_rate, nuclei_concurrency, nuclei_timeout) regardless of whether the value came from interactive input or a vars file.
|
||||
**Verification:** Engine math also has floors (`max(nmap_workers, 1)`, `max(min(...), 1.0)` in nuclei throughput, `if rate <= 0: return PROVISION_OVERHEAD_HOURS`), so even pre-fix garbage in produced sane numbers — but operator's displayed tuning would have been wrong. Now the displayed and computed values match.
|
||||
|
||||
### P3 — Dead code removal (AUDIT_CODE_989#3)
|
||||
**File:** `modules/webrunner/deploy_webrunner.py:195-196`
|
||||
**Fix:** Removed the empty `if scan_mode in (...): pass` placeholder that was a refactor leftover. masscan_rate is already prompted unconditionally above.
|
||||
|
||||
## Skipped (with justification)
|
||||
|
||||
### MEDIUM — Tor warning phrasing (AUDIT_SEC_989#2)
|
||||
Auditor suggested rewording the masscan-Tor warning. Existing text already states "raw sockets bypass proxychains entirely" and "use 'nmap-only' mode for full Tor coverage" — auditor's suggestion is semantically equivalent. No material improvement.
|
||||
|
||||
### MEDIUM — Path traversal in template/yaml file loading (AUDIT_SEC_989#3)
|
||||
False positive for this threat model. Webrunner is a single-operator attack tool, not multi-tenant. The "attack vector" of "operator supplies malicious vars file pointing at /etc/passwd" doesn't apply — the operator IS the trusted party and runs the tool on their own machine. They can already read any file they want. Adding path restriction would just frustrate legitimate workflows (e.g., operator's nuclei templates are typically in `~/templates/` outside the WEBRUNNER_INPUTS dir). YAML uses `safe_load` so no code execution. The file content goes only to the operator's stdout, not to a remote server.
|
||||
|
||||
### LOW — Estimate display discloses scope (AUDIT_SEC_989#5)
|
||||
Interactive console output to the operator. Not logged externally. Operator chose to run the tool. Suppression flag is overkill for a tool with one user.
|
||||
|
||||
## Gate Review Status
|
||||
Ready for gate-reviewer. No P1 or unaddressed HIGH findings remain.
|
||||
Reference in New Issue
Block a user