diff --git a/.github/scripts/sort-trivy-results.py b/.github/scripts/sort-trivy-results.py new file mode 100755 index 000000000..0fa56064d --- /dev/null +++ b/.github/scripts/sort-trivy-results.py @@ -0,0 +1,193 @@ +#!/usr/bin/env python3 +"""Create a severity-sorted Markdown security audit report from Trivy JSON.""" + +from __future__ import annotations + +import argparse +import json +import sys +from collections import Counter +from pathlib import Path +from typing import Any + +SEVERITY_ORDER = { + "CRITICAL": 0, + "HIGH": 1, + "MEDIUM": 2, + "LOW": 3, + "UNKNOWN": 4, +} +DISPLAY_SEVERITIES = ("CRITICAL", "HIGH", "MEDIUM", "LOW", "UNKNOWN") + + +def parse_args() -> argparse.Namespace: + parser = argparse.ArgumentParser( + description=( + "Read Trivy JSON output and write a Markdown report sorted by " + "vulnerability severity, with CRITICAL findings first." + ) + ) + parser.add_argument("trivy_json", type=Path, help="Path to Trivy JSON output") + parser.add_argument( + "--output", + type=Path, + default=Path("security-audit-report.md"), + help="Markdown report path (default: security-audit-report.md)", + ) + parser.add_argument( + "--max-rows", + type=int, + default=200, + help="Maximum vulnerability rows to include in the table (default: 200)", + ) + parser.add_argument( + "--fail-on-severity", + choices=(*DISPLAY_SEVERITIES, "NONE"), + default="CRITICAL", + help=( + "Exit with status 1 when vulnerabilities at or above this severity " + "are found. Use NONE to disable. (default: CRITICAL)" + ), + ) + return parser.parse_args() + + +def normalized_severity(value: str | None) -> str: + severity = (value or "UNKNOWN").upper() + return severity if severity in SEVERITY_ORDER else "UNKNOWN" + + +def markdown_escape(value: Any) -> str: + text = "" if value is None else str(value) + return text.replace("|", "\\|").replace("\n", " ").replace("\r", " ") + + +def cvss_v3_score(vulnerability: dict[str, Any]) -> float: + cvss = vulnerability.get("CVSS") + if not isinstance(cvss, dict): + return 0.0 + + for source in ("nvd", "redhat", "ghsa"): + score = cvss.get(source, {}).get("V3Score", 0) + if score: + return float(score) + + return 0.0 + + +def vulnerability_sort_key(vulnerability: dict[str, Any]) -> tuple[Any, ...]: + severity = normalized_severity(vulnerability.get("Severity")) + + return ( + SEVERITY_ORDER[severity], + -cvss_v3_score(vulnerability), + str(vulnerability.get("VulnerabilityID", "")), + str(vulnerability.get("PkgName", "")), + str(vulnerability.get("Target", "")), + ) + + +def collect_vulnerabilities(report: dict[str, Any]) -> list[dict[str, Any]]: + findings: list[dict[str, Any]] = [] + + for result in report.get("Results", []): + target = result.get("Target", "") + result_type = result.get("Type", "") + + for vulnerability in result.get("Vulnerabilities") or []: + item = dict(vulnerability) + item["Target"] = target + item["ResultType"] = result_type + item["Severity"] = normalized_severity(item.get("Severity")) + findings.append(item) + + findings.sort(key=vulnerability_sort_key) + return findings + + +def format_summary(counts: Counter[str]) -> str: + return " | ".join(f"{severity}: {counts.get(severity, 0)}" for severity in DISPLAY_SEVERITIES) + + +def render_markdown(findings: list[dict[str, Any]], max_rows: int) -> str: + counts = Counter(finding["Severity"] for finding in findings) + lines = [ + "# Security Audit", + "", + "Vulnerability findings are sorted by severity, with critical findings first.", + "", + "## Summary", + "", + f"Total vulnerabilities: **{len(findings)}**", + "", + f"{format_summary(counts)}", + "", + ] + + if not findings: + lines.extend(["## Vulnerabilities", "", "No vulnerabilities were reported by Trivy.", ""]) + return "\n".join(lines) + + displayed_findings = findings[:max_rows] + lines.extend( + [ + "## Vulnerabilities", + "", + "| Severity | Vulnerability | Package | Installed | Fixed | Target | Title |", + "| --- | --- | --- | --- | --- | --- | --- |", + ] + ) + + for finding in displayed_findings: + lines.append( + "| {severity} | {vuln_id} | {package} | {installed} | {fixed} | {target} | {title} |".format( + severity=markdown_escape(finding.get("Severity")), + vuln_id=markdown_escape(finding.get("VulnerabilityID")), + package=markdown_escape(finding.get("PkgName")), + installed=markdown_escape(finding.get("InstalledVersion")), + fixed=markdown_escape(finding.get("FixedVersion") or "not fixed"), + target=markdown_escape(finding.get("Target")), + title=markdown_escape(finding.get("Title")), + ) + ) + + omitted = len(findings) - len(displayed_findings) + if omitted > 0: + lines.extend(["", f"_{omitted} additional vulnerabilities omitted from this table._"]) + + lines.append("") + return "\n".join(lines) + + +def should_fail(findings: list[dict[str, Any]], fail_on_severity: str) -> bool: + if fail_on_severity == "NONE": + return False + + threshold = SEVERITY_ORDER[fail_on_severity] + return any(SEVERITY_ORDER[finding["Severity"]] <= threshold for finding in findings) + + +def main() -> int: + args = parse_args() + + with args.trivy_json.open(encoding="utf-8") as fh: + report = json.load(fh) + + findings = collect_vulnerabilities(report) + markdown = render_markdown(findings, args.max_rows) + args.output.parent.mkdir(parents=True, exist_ok=True) + args.output.write_text(markdown, encoding="utf-8") + print(markdown) + + if should_fail(findings, args.fail_on_severity): + print( + f"Security audit failed: found vulnerabilities at or above {args.fail_on_severity} severity.", + file=sys.stderr, + ) + return 1 + + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/workflows/security-audit.yml b/.github/workflows/security-audit.yml new file mode 100644 index 000000000..3f3035388 --- /dev/null +++ b/.github/workflows/security-audit.yml @@ -0,0 +1,104 @@ +name: Security Audit + +on: + pull_request: + paths: + - '**/Cargo.lock' + - '**/go.mod' + - '**/go.sum' + - '**/package-lock.json' + - '**/package.json' + - '**/requirements*.txt' + - '**/Dockerfile*' + - '.github/scripts/sort-trivy-results.py' + - '.github/workflows/security-audit.yml' + push: + branches: master + paths: + - '**/Cargo.lock' + - '**/go.mod' + - '**/go.sum' + - '**/package-lock.json' + - '**/package.json' + - '**/requirements*.txt' + - '**/Dockerfile*' + - '.github/scripts/sort-trivy-results.py' + - '.github/workflows/security-audit.yml' + schedule: + - cron: '24 3 * * 1' + workflow_dispatch: + inputs: + fail_on_severity: + description: 'Fail the audit when vulnerabilities at or above this severity are found.' + type: choice + default: CRITICAL + options: + - CRITICAL + - HIGH + - MEDIUM + - LOW + - UNKNOWN + - NONE + +permissions: + contents: read + security-events: write + +jobs: + trivy: + name: Trivy filesystem audit + runs-on: ubuntu-latest + + steps: + - name: Checkout + uses: actions/checkout@v5 + + - name: Prepare report directory + run: mkdir -p security-audit + + - name: Run Trivy vulnerability audit + uses: aquasecurity/trivy-action@v0.36.0 + with: + scan-type: fs + scan-ref: . + scanners: vuln + format: json + output: security-audit/trivy-results.json + severity: UNKNOWN,LOW,MEDIUM,HIGH,CRITICAL + exit-code: 0 + + - name: Run Trivy SARIF audit + uses: aquasecurity/trivy-action@v0.36.0 + with: + scan-type: fs + scan-ref: . + scanners: vuln + format: sarif + output: security-audit/trivy-results.sarif + severity: UNKNOWN,LOW,MEDIUM,HIGH,CRITICAL + exit-code: 0 + + - name: Upload SARIF results + uses: github/codeql-action/upload-sarif@v4 + with: + sarif_file: security-audit/trivy-results.sarif + + - name: Sort audit by critical severity + env: + FAIL_ON_SEVERITY: ${{ github.event.inputs.fail_on_severity || 'CRITICAL' }} + run: | + set +e + .github/scripts/sort-trivy-results.py \ + security-audit/trivy-results.json \ + --output security-audit/security-audit-report.md \ + --fail-on-severity "${FAIL_ON_SEVERITY}" + status=$? + cat security-audit/security-audit-report.md >> "$GITHUB_STEP_SUMMARY" + exit "$status" + + - name: Upload security audit report + if: always() + uses: actions/upload-artifact@v4 + with: + name: security-audit + path: security-audit/ diff --git a/docs/security-audit-findings.md b/docs/security-audit-findings.md new file mode 100644 index 000000000..7216edd25 --- /dev/null +++ b/docs/security-audit-findings.md @@ -0,0 +1,43 @@ +# Security Audit Findings + +This document captures follow-up findings from a targeted manual security review of +recent security-audit changes and adjacent Unit code paths. These items are +**potential vulnerabilities** until each has a reproducer, impact analysis, and +fix or explicit dismissal. + +## Scope + +Reviewed areas: + +- Configuration validation and process isolation paths. +- Static file serving controls related to chroot, symlinks, and mount traversal. +- GitHub Actions and Docker release-image supply-chain paths. +- New Trivy audit workflow and report-generation script. + +## Candidate vulnerabilities + +| ID | Severity | Confidence | Area | Summary | Primary evidence | Suggested next step | +| --- | --- | --- | --- | --- | --- | --- | +| SA-001 | High | Medium | Process isolation | `rootfs` accepts length-tracked config strings but later stores and uses them as NUL-terminated C strings. Embedded NUL bytes could truncate the effective root path passed to `chroot()`/`pivot_root()` and related mount setup. | `rootfs` is declared as a plain string without a NUL-byte validator in `src/nxt_conf_validation.c:1352-1356`; `nxt_isolation_set_rootfs()` copies the configured bytes and appends `\0` in `src/nxt_isolation.c:501-525`; the path is later used as a C string in `src/nxt_isolation.c:817-825`. | Add a rootfs validator that rejects empty, non-absolute, `/`, embedded-NUL, and path-normalization edge cases; add regression tests mirroring the cgroup embedded-NUL test. | +| SA-002 | Medium | Medium | Configuration validation | Several application path-like fields are validated only as strings even though downstream code commonly treats them as filesystem paths, module names, or C strings. This may create truncation, path confusion, or audit-bypass behavior for embedded NUL input. | Common fields such as `working_directory`, `stdout`, and `stderr` are plain strings in `src/nxt_conf_validation.c:1274-1291`; language-specific path/module fields are also plain strings, e.g. Ruby `script` and `hooks` in `src/nxt_conf_validation.c:1134-1148`, Java `webapp` and `unit_jars` in `src/nxt_conf_validation.c:1152-1170`, and Wasm module/component names in `src/nxt_conf_validation.c:1184-1239`. | Introduce reusable validators for C-string-backed paths and module identifiers, then apply them consistently to all fields consumed by C APIs. | +| SA-003 | Medium | Low | User namespace isolation | `uidmap`/`gidmap` entries require integer fields but do not appear to enforce non-negative values, kernel UID/GID-map bounds, overlap checks, or a maximum number of entries during configuration validation. Invalid or pathological maps could produce confusing authorization decisions or resource-exhaustion behavior before kernel rejection. | Required integer fields are defined in `src/nxt_conf_validation.c:1470-1485`; arrays are copied with `map->size * sizeof(nxt_clone_map_entry_t)` in `src/nxt_isolation.c:346-362`; map output is sized around 32-bit decimal entries in `src/nxt_clone.c:128-147`. | Add validation for non-negative 32-bit IDs, positive sizes, no arithmetic overflow, no overlaps, and a practical entry-count limit. Add tests for negative, oversized, overlapping, and excessive maps. | +| SA-004 | Medium | Medium | Release supply chain | Docker images clone source by mutable branch/tag name inside the Docker build rather than using the already-checked-out commit or verifying a commit SHA. A moved tag or manual workflow input could build and publish an image from code different from the workflow revision under review. | `pkg/docker/template.Dockerfile:38` runs `git clone --depth 1 -b @@VERSION@@ https://github.com/freeunitorg/freeunit unit`; `.github/workflows/docker.yml:131-147` derives a version string and rewrites Dockerfiles before publishing. | Build from the checked-out workspace, or pass and verify an immutable commit SHA; record the source revision in image labels and provenance. | +| SA-005 | Medium | Medium | CI/CD supply chain | Security-sensitive workflows use mutable action tags while granting permissions such as `security-events: write`. A compromised or retagged third-party action could affect audit integrity or code-scanning uploads. | The new audit workflow grants `security-events: write` in `.github/workflows/security-audit.yml:43-45` and uses version tags for actions in `.github/workflows/security-audit.yml:53-104`; the Docker publisher similarly uses mutable action tags in `.github/workflows/docker.yml:127-160`. | Pin third-party actions to full commit SHAs, enable Dependabot updates for GitHub Actions SHAs, and keep write permissions scoped to the minimal job/step that needs them. | +| SA-006 | Low | Medium | HTTP compatibility/security hardening | `discard_unsafe_fields` can be disabled, allowing non-token header names such as underscores and punctuation to reach applications. This is intentional configurability, but can reintroduce header-smuggling or auth-bypass risk behind intermediaries with different header-name normalization. | The parser skips unsafe fields only when `discard_unsafe_fields` is enabled in `src/nxt_http_parse.c:539-550`; tests confirm unsafe names are passed through when disabled in `test/test_http_header.py:484-490`. | Document this as a dangerous compatibility mode, consider warning on disable, and add proxy/intermediary threat-model notes. | + +## Recently addressed during this audit thread + +| ID | Status | Summary | Evidence | +| --- | --- | --- | --- | +| SA-FIXED-001 | Fixed in `b928b85` | Cgroup isolation path validation now rejects embedded NUL bytes before constructing a slash-wrapped validation path. | `src/nxt_conf_validation.c:3367-3375` and `test/test_python_isolation.py:225-228`. | + +## Recommended prioritization + +1. Fix SA-001 first because it is adjacent to privileged isolation setup and is + structurally similar to the cgroup NUL issue already fixed. +2. Triage SA-002 by enumerating every config string that is later passed to a C + API expecting NUL termination. +3. Harden CI/CD supply-chain items SA-004 and SA-005 before relying on the new + audit workflow as a release gate. +4. Treat SA-003 and SA-006 as defense-in-depth unless a concrete reproducer + shows direct privilege escalation or request smuggling. diff --git a/src/nxt_conf_validation.c b/src/nxt_conf_validation.c index 8324a4419..19d752768 100644 --- a/src/nxt_conf_validation.c +++ b/src/nxt_conf_validation.c @@ -3364,9 +3364,17 @@ nxt_conf_vldt_cgroup_path(nxt_conf_validation_t *vldt, nxt_conf_value_t *value, &cgpath); } - sprintf(path, "/%*s/", (int) cgpath.length, cgpath.start); + if (cgpath.length == 0 + || memchr(cgpath.start, '\0', cgpath.length) != NULL) + { + return nxt_conf_vldt_error(vldt, + "The cgroup path \"%V\" is invalid.", + &cgpath); + } + + snprintf(path, sizeof(path), "/%.*s/", (int) cgpath.length, cgpath.start); - if (cgpath.length == 0 || strstr(path, "/../") != NULL) { + if (strstr(path, "/../") != NULL) { return nxt_conf_vldt_error(vldt, "The cgroup path \"%V\" is invalid.", &cgpath); diff --git a/test/test_python_isolation.py b/test/test_python_isolation.py index d4f205346..c6234569e 100644 --- a/test/test_python_isolation.py +++ b/test/test_python_isolation.py @@ -225,3 +225,4 @@ def check_invalid(path): check_invalid('') check_invalid('../scope') check_invalid('scope/../python') + check_invalid('scope\0python')