Add Trivy security-audit workflow and report sorter; validate cgroup paths against embedded NUL

.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())

.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/

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.

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);

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')