Add audit reports and fix plan for DevTrack #1259

audits/AUDIT_CODE_1259.md

@@ -0,0 +1,62 @@
+---
+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
+

audits/AUDIT_env_1259.md

@@ -0,0 +1,53 @@
+---
+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 (`/`, `&`, `%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
+

audits/FIXES_1259.md

@@ -0,0 +1,46 @@
+---
+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.
+