From fca8b8c040bbee37f5d0a68284650e183c0e3e39 Mon Sep 17 00:00:00 2001 From: n0mad1k Date: Thu, 25 Jun 2026 10:26:46 -0400 Subject: [PATCH] Add audit reports and fix plan for DevTrack #1259 --- audits/AUDIT_CODE_1259.md | 62 +++++++++++++++++++++++++++++++++++++++ audits/AUDIT_env_1259.md | 53 +++++++++++++++++++++++++++++++++ audits/FIXES_1259.md | 46 +++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+) create mode 100644 audits/AUDIT_CODE_1259.md create mode 100644 audits/AUDIT_env_1259.md create mode 100644 audits/FIXES_1259.md diff --git a/audits/AUDIT_CODE_1259.md b/audits/AUDIT_CODE_1259.md new file mode 100644 index 0000000..f1ad390 --- /dev/null +++ b/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 + diff --git a/audits/AUDIT_env_1259.md b/audits/AUDIT_env_1259.md new file mode 100644 index 0000000..9e29d6f --- /dev/null +++ b/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: `` + +### 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 + diff --git a/audits/FIXES_1259.md b/audits/FIXES_1259.md new file mode 100644 index 0000000..f3fd976 --- /dev/null +++ b/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 %}{% 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. +