Updated instructions for AI agents reporting vulnerabilities

Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>

contrib/ai/personas/ocserv-contributor.md

@@ -51,12 +51,41 @@ authentication flow.
 2. Create a **confidential** issue at:
    https://gitlab.com/openconnect/ocserv/-/issues/new?type=ISSUE&initialCreationContext=list-route
    On the issue form, check "This issue is confidential."
-3. Describe the potential impact and how to reproduce it. Do not include a public patch.
+3. Fill in every field of the structured template below. Do not include a public patch.
 4. Wait for maintainer response before proceeding.
 
 This applies to suspicions as well as confirmed bugs. If you are not sure whether
 something is a vulnerability, use the confidential path and let the maintainers decide.
 
+**Required fields for every confidential security report:**
+
+```
+Impact: <one sentence — what an attacker concretely achieves.
+        Examples: "unauthenticated access to the VPN tunnel",
+                  "prevents affected users from completing MFA login",
+                  "leaks session token to a co-tenant on the same server".
+        Do NOT write: "could affect authentication" or "may be exploited".
+        If the outcome is auth failure rather than auth bypass, say so explicitly.>
+
+Affected versions: <e.g. 1.4.2, or "all versions since commit X">
+
+Reproduction: <minimal numbered steps to trigger the behavior>
+
+Severity self-assessment: Critical | High | Medium | Low
+Justification: <one sentence — why you chose that severity, by reference to the
+               Impact line. If you are unsure, write "unsure — see Impact.">
+
+What I ruled out: <one sentence — the worse interpretation you checked and why it
+                  does not apply. E.g. "I verified this cannot allow login without
+                  credentials because the RADIUS server rejects the truncated State
+                  with an Access-Reject response.">
+```
+
+If you are performing a code-level analysis before reporting, load
+`contrib/ai/protocols/security-vulnerability.md` for the full analysis protocol
+and finding format. The ocserv-specific trust boundary model and vulnerability
+taxonomy are in the extension section of that file.
+
 ---
 
 ## Guardrails — Hard Stops

contrib/ai/personas/ocserv-core-dev.md

@@ -86,91 +86,29 @@ Rules:
 
 ## Protocol: Memory Safety
 
-ocserv is C99. There is no garbage collection and no ownership abstraction beyond
+Load and follow `contrib/ai/protocols/memory-safety-c.md` for the full analysis
-what the programmer enforces.
+protocol. The ocserv-specific allocator rules (talloc vs. gnutls_malloc),
+cross-process pointer lifetime constraints, and `goto cleanup` discipline are in
+the extension section of that file.
 
-Rules:
+seccomp isolation in workers prevents `mmap`/`mprotect` — this limits certain
-- **talloc is the project-wide allocator.** Use `talloc_zero`, `talloc_strdup`,
+exploit primitives, but memory corruption still causes crashes and client denial
-  `talloc_array`, etc. for all allocations. Before introducing a new allocation,
+of service. Treat all memory bugs as high-severity.
-  check how surrounding code allocates similar data.
-- **Exception:** When passing memory to a GnuTLS API that will take ownership and
-  free it (e.g., `gnutls_datum_t` fields consumed by GnuTLS internals), use
-  `gnutls_malloc()` / `gnutls_free()`. Never pass talloc-allocated memory to a
-  GnuTLS API that will call `gnutls_free()` on it, and vice versa.
-- Check every allocation return value before use. Null-pointer dereferences in a
-  VPN server are denial-of-service vulnerabilities.
-- **Lifetime awareness:** `sec-mod` and `main` manage long-lived allocations that
-  persist across client connections. Worker allocations are per-connection and are
-  freed when the worker exits. Do not assume that a pointer valid in one process
-  is accessible or valid in another.
-- **Error paths:** Use `goto cleanup` with a single label that frees all resources
-  allocated in the function. Avoid multiple return paths that each partially free state.
-- seccomp isolation in workers prevents `mmap`/`mprotect` — this limits certain
-  exploit primitives, but memory corruption still causes crashes and client denial
-  of service. Treat all memory bugs as high-severity.
 
 ---
 
-## Protocol: Security Vulnerability Taxonomy
+## Protocol: Security Vulnerability Analysis
 
-When reviewing or investigating code for security issues, reason against this
+Load and follow `contrib/ai/protocols/security-vulnerability.md` for the full
-ocserv-specific taxonomy before concluding that code is safe.
+analysis protocol. The ocserv-specific trust boundary model, vulnerability
+taxonomy (IPC violations, TLS downgrade, seccomp escape, auth bypass,
+configuration injection, accounting manipulation), adversarial falsification
+discipline, and the enhanced 9-field output format (including the required
+**Impact** and **Why not a false positive** fields) are in the extension section
+of that file.
 
-**IPC trust boundary violations**
+If you identify a potential issue, **do not open a public issue.** Follow the
-Worker processes are unprivileged and potentially compromised. Data arriving at
+security disclosure procedure in `AGENTS.md`.
-main or sec-mod from a worker via IPC must be treated as untrusted. Check:
-- Are protobuf fields from a worker used without length or range validation?
-- Are string fields from a worker used in a format string, file path, or exec call?
-- Can a worker send an IPC message that causes main or sec-mod to act on behalf
-  of a different client (SID confusion, cookie substitution)?
-
-**TLS/DTLS downgrade paths**
-- Does a change allow a client to negotiate a weaker cipher, an older protocol
-  version, or skip certificate verification?
-- Does a change affect resumption logic in a way that skips re-authentication?
-- Are DTLS and TLS sessions kept properly synchronized (a DTLS session must
-  correspond to an authenticated TLS session)?
-
-**seccomp escape vectors**
-- Does a new code path in the worker call a syscall not in the existing allowlist?
-- If yes, this requires an explicit seccomp filter update reviewed by a maintainer.
-  Do not add syscalls silently.
-
-**Authentication bypass**
-- Is `SEC_AUTH_INIT` always called for new sessions before any auth data is processed?
-- Can a client reuse a SID from a different session?
-- Can a client present a cookie for a session that has been invalidated or timed out?
-- Are multi-factor steps enforced in the correct order?
-
-**Configuration injection**
-- Does untrusted input (from a client or an unauthenticated IPC message) reach
-  `config.c` or `subconfig.c` parsers?
-- Are bracketed option strings (`radius[config=...]`) validated before parsing?
-
-**Accounting manipulation**
-- Can a worker supply falsified session statistics (bytes transferred, duration) to
-  RADIUS or PAM accounting?
-- Is the accounting data sourced from the worker (untrusted) or from main/sec-mod
-  (trusted)?
-
-If you identify a potential issue in any of these categories, **do not open a public
-issue.** Follow the security disclosure procedure in `AGENTS.md`.
-
-**Output format for every security finding:**
-```
-[SEVERITY: Critical | High | Medium | Low | Informational]
-CWE: <CWE-ID if applicable, e.g. CWE-416 Use After Free>
-Location: <file>:<line> or <function>
-Issue: <one-sentence description>
-Attack scenario: <concrete exploit path — who sends what, what executes, what is the impact>
-Remediation: <specific fix, not "validate input">
-Confidence: Confirmed | High | Needs-domain-check
-Why not a false positive: <the disproof attempt that failed — see Adversarial Falsification>
-```
-
-Do not file a finding without filling every field. "Possible" or "could" in the
-attack scenario means the finding is not yet Confirmed — downgrade to High or
-Needs-domain-check and state what additional evidence is required.
 
 ---
 

contrib/ai/protocols/memory-safety-c.md

@@ -0,0 +1,148 @@
+<!-- SPDX-License-Identifier: MIT -->
+<!-- Copyright (c) PromptKit Contributors -->
+
+---
+name: memory-safety-c
+type: analysis
+description: >
+  Systematic protocol for analyzing memory safety issues in C codebases.
+  Covers allocation/deallocation pairing, pointer lifecycle, buffer boundaries,
+  and undefined behavior.
+language: C
+applicable_to:
+  - investigate-bug
+  - review-code
+  - investigate-security
+---
+
+# Protocol: Memory Safety Analysis (C)
+
+Apply this protocol when analyzing C code for memory safety defects. Execute
+each phase in order. Do not skip phases — apparent simplicity often hides
+subtle bugs.
+
+## Phase 1: Allocation / Deallocation Pairing
+
+For every allocation site (`malloc`, `calloc`, `realloc`, `strdup`, custom allocators):
+
+1. Trace **all** code paths from allocation to deallocation.
+2. Identify paths where deallocation is **missing** (leak) or **unreachable**
+   (early return, exception-like longjmp, error branch).
+3. Check for **double free**: paths where the same pointer is freed more than once.
+4. Check for **mismatched APIs**: `malloc`/`free` vs `new`/`delete` vs custom
+   allocator pairs.
+
+## Phase 2: Pointer Lifecycle Analysis
+
+For every pointer variable:
+
+1. Determine its **ownership semantics**: who is responsible for freeing it?
+   Is ownership transferred? Is it documented?
+2. Check for **use-after-free**: any access to a pointer after its referent
+   has been freed. Pay special attention to:
+   - Pointers stored in structs or global state that outlive the allocation.
+   - Pointers passed to callbacks or stored in event loops.
+   - Conditional free followed by unconditional use.
+3. Check for **dangling pointers**: pointers to stack variables that escape
+   their scope (returned from function, stored in heap struct).
+4. Verify **NULL checks** after allocation and after any operation that may
+   invalidate a pointer (e.g., `realloc`).
+
+## Phase 3: Buffer Boundary Analysis
+
+For every buffer (stack arrays, heap allocations, string buffers):
+
+1. Identify all **read and write accesses** to the buffer.
+2. Verify that every access is **bounds-checked** or provably within bounds.
+3. Check for **off-by-one errors** in loop conditions and index calculations.
+4. Check `strncpy`, `snprintf`, `memcpy` calls for correct size arguments.
+5. Identify any **user-controlled index or size** values that flow into
+   buffer accesses without validation.
+
+## Phase 4: Undefined Behavior Audit
+
+Check for common sources of undefined behavior:
+
+1. **Signed integer overflow** in size calculations.
+2. **Null pointer dereference** on error paths.
+3. **Uninitialized memory reads** — especially stack variables and struct
+   fields after partial initialization.
+4. **Type punning** violations (strict aliasing).
+5. **Sequence point violations** in complex expressions.
+
+## Output Format
+
+For each finding, report:
+
+```
+[SEVERITY: Critical|High|Medium|Low]
+Location: <file>:<line> or <function name>
+Issue: <concise description>
+Evidence: <code path or snippet demonstrating the issue>
+Remediation: <specific fix recommendation>
+Confidence: <High|Medium|Low — with justification if not High>
+```
+
+<!-- END PromptKit base -->
+
+---
+
+<!-- BEGIN ocserv extensions -->
+
+## ocserv-Specific Extensions
+
+The sections below extend the generic protocol with ocserv's allocator rules,
+cross-process lifetime constraints, and error-path discipline.
+
+### Allocator Rules (extends Phase 1)
+
+ocserv uses **talloc** as its project-wide allocator. The generic phase 1
+checks apply, but substitute these rules for allocator pairing:
+
+- **talloc is the default.** Use `talloc_zero`, `talloc_strdup`,
+  `talloc_memdup`, `talloc_array`, etc. for all allocations. Before
+  introducing a new allocation, check how surrounding code allocates
+  similar data.
+- **`gnutls_malloc` / `gnutls_free` are the exception.** Use them only
+  for memory whose lifetime GnuTLS owns — i.e., memory passed to a GnuTLS
+  API that will call `gnutls_free()` on it internally (e.g., `gnutls_datum_t`
+  fields consumed by GnuTLS internals). Never use `gnutls_free()` on a
+  talloc allocation, and never pass a `gnutls_malloc` allocation to
+  `talloc_free()`.
+- **Mismatch check (ocserv-specific, not in the base protocol):** For every
+  allocation, confirm the free call uses the matching API. A
+  `talloc_strdup` freed with `gnutls_free()`, or vice versa, is a
+  heap-corruption bug. Flag any site where the allocator is ambiguous.
+- Check every allocation return value before use. A NULL return in a VPN
+  server is a denial-of-service vulnerability.
+
+### Cross-Process Pointer Lifetime (extends Phase 2)
+
+ocserv's three processes (main, sec-mod, worker) have independent address
+spaces. Extend the pointer lifecycle analysis with:
+
+- **Allocations do not cross process boundaries.** A pointer allocated in
+  worker memory is not accessible in main or sec-mod, and vice versa. Flag
+  any struct field or IPC message that appears to transfer a raw pointer
+  rather than serialized data.
+- **Lifetime by process role:**
+  - `sec-mod` and `main`: long-lived allocations that persist across client
+    connections. These require explicit cleanup on session teardown.
+  - `worker`: per-connection allocations freed when the worker exits. Do not
+    store worker-process pointers in IPC messages intended for main or sec-mod.
+
+### Error-Path Discipline (extends Phase 1 and Phase 2)
+
+ocserv follows a `goto cleanup` pattern for resource management. When
+reviewing allocation and free pairing:
+
+- Every function that allocates resources must have a single `cleanup` label
+  that frees all resources allocated so far.
+- Multiple `return` paths that each partially free state are a defect —
+  they produce leaks or double-frees on uncommon error paths.
+- Verify that every early `goto cleanup` path leaves the cleanup label able
+  to safely free whatever was allocated before the jump (i.e., pointers not
+  yet allocated are NULL, and the cleanup code checks for NULL before
+  freeing).
+
+<!-- END ocserv extensions -->

contrib/ai/protocols/security-vulnerability.md

@@ -0,0 +1,220 @@
+<!-- SPDX-License-Identifier: MIT -->
+<!-- Copyright (c) PromptKit Contributors -->
+
+---
+name: security-vulnerability
+type: analysis
+description: >
+  Protocol for systematic security vulnerability analysis.
+  Covers input validation, authentication/authorization, injection,
+  cryptographic misuse, and privilege escalation. Language-agnostic.
+applicable_to:
+  - investigate-security
+  - review-code
+  - review-infrastructure
+---
+
+# Protocol: Security Vulnerability Analysis
+
+Apply this protocol when analyzing code for security vulnerabilities.
+Execute all phases systematically. Do not skip phases even if the code
+appears simple — security bugs hide in assumptions.
+
+## Phase 1: Trust Boundary Mapping
+
+1. Identify all **trust boundaries** in the system:
+   - External inputs (network, files, environment variables, CLI arguments)
+   - Inter-process communication
+   - Privilege transitions (user → kernel, unprivileged → privileged)
+   - Cross-tenant or cross-user data access points
+2. For each boundary, determine:
+   - What data crosses the boundary?
+   - Who controls that data?
+   - What validation occurs at the boundary?
+
+## Phase 2: Input Validation Audit
+
+For every external input:
+
+1. Trace the input from its **entry point** to every **use site**.
+2. Verify that validation occurs **before** the input is used in any
+   security-sensitive operation:
+   - SQL queries → check for parameterized queries (not string concatenation)
+   - Shell commands → check for proper escaping or allowlisting
+   - File paths → check for path traversal (`../`, null bytes, symlinks)
+   - HTML/XML output → check for encoding/escaping (XSS prevention)
+   - Deserialization → check for type constraints and allowlisting
+3. Check for **validation bypass**: inputs that are validated but then
+   re-encoded, decoded, or transformed before use.
+4. Check for **integer overflow/underflow** in size or length parameters
+   derived from external input.
+
+## Phase 3: Authentication and Authorization
+
+1. **Authentication**:
+   - How are credentials validated? Are timing-safe comparisons used?
+   - Are sessions or tokens properly generated (sufficient entropy)?
+   - Is session fixation possible?
+   - Are credentials stored securely (hashed with salt, appropriate algorithm)?
+2. **Authorization**:
+   - Is authorization checked on **every** access to protected resources?
+   - Can authorization be bypassed via direct object references (IDOR)?
+   - Are privilege checks performed on the server side, not just client side?
+   - Is the principle of least privilege applied?
+
+## Phase 4: Cryptographic Misuse
+
+1. Check for use of **deprecated or weak algorithms** (MD5, SHA1 for security,
+   DES, RC4, ECB mode).
+2. Check for **hardcoded keys, secrets, or IVs** in source code.
+3. Verify that **random number generation** uses cryptographically secure
+   sources (`/dev/urandom`, `CSPRNG`, not `rand()`).
+4. Check for **IV/nonce reuse** in symmetric encryption.
+5. Verify **certificate validation** is not disabled or weakened.
+
+## Phase 5: Information Disclosure
+
+1. Check for **sensitive data in logs** (passwords, tokens, PII).
+2. Check for **verbose error messages** that reveal internal structure
+   (stack traces, SQL errors, file paths).
+3. Check for **timing side channels** in authentication or authorization logic.
+4. Verify that **debug endpoints or features** are disabled in production.
+
+## Output Format
+
+For each finding, report:
+
+```
+[SEVERITY: Critical|High|Medium|Low|Informational]
+CWE: <CWE ID if applicable>
+Location: <file>:<line> or <component>
+Issue: <concise description>
+Attack scenario: <concrete exploit path or abuse case>
+Remediation: <specific fix recommendation>
+Confidence: <High|Medium|Low — with justification if not High>
+```
+
+<!-- END PromptKit base -->
+
+---
+
+<!-- BEGIN ocserv extensions -->
+
+## ocserv-Specific Extensions
+
+The sections below extend the generic protocol with ocserv's three-process
+architecture, its specific vulnerability categories, and the adversarial
+falsification discipline required for maintainer-level analysis.
+
+### Trust Boundary Model (extends Phase 1)
+
+ocserv has three processes with distinct privilege levels. Map every finding
+to one of these boundaries before reporting:
+
+| Process | Privilege | Responsibility |
+|---------|-----------|---------------|
+| **main** (`main.c`, `main-*.c`) | root | TCP/UDP listeners, TUN devices, IP allocation, process lifecycle |
+| **sec-mod** (`sec-mod.c`, `sec-mod-*.c`) | root | Authentication (except client certificates), private keys, session state, PAM, accounting |
+| **worker** (`worker.c`, `worker-*.c`) | unprivileged + seccomp | TLS/DTLS per client, VPN traffic bridging, client certificate authentication |
+
+Workers communicate with main and sec-mod exclusively over Unix sockets using
+protobuf IPC. A worker is treated as potentially compromised: data it sends to
+main or sec-mod must be validated at the receiving end before use.
+
+### ocserv Vulnerability Taxonomy (extends Phase 3)
+
+When reviewing ocserv code, explicitly check each category below before
+concluding that code is safe.
+
+**IPC trust boundary violations**
+Worker processes are unprivileged and potentially compromised. Data arriving at
+main or sec-mod from a worker via IPC must be treated as untrusted. Check:
+- Are protobuf fields from a worker used without length or range validation?
+- Are string fields from a worker used in a format string, file path, or exec call?
+- Can a worker send an IPC message that causes main or sec-mod to act on behalf
+  of a different client (SID confusion, cookie substitution)?
+
+**TLS/DTLS downgrade paths**
+- Does a change allow a client to negotiate a weaker cipher, an older protocol
+  version, or skip certificate verification?
+- Does a change affect resumption logic in a way that skips re-authentication?
+- Are DTLS and TLS sessions kept properly synchronized (a DTLS session must
+  correspond to an authenticated TLS session)?
+
+**seccomp escape vectors**
+- Does a new code path in the worker call a syscall not in the existing allowlist?
+- If yes, this requires an explicit seccomp filter update reviewed by a maintainer.
+  Do not add syscalls silently.
+
+**Authentication bypass**
+- Is `SEC_AUTH_INIT` always called for new sessions before any auth data is processed?
+- Can a client reuse a SID from a different session?
+- Can a client present a cookie for a session that has been invalidated or timed out?
+- Can a client present a client TLS certificate that is invalid and being reported as valid?
+- Are multi-factor steps enforced in the correct order?
+
+**Configuration injection**
+- Does untrusted input (from a client or an unauthenticated IPC message) reach
+  `config.c` or `subconfig.c` parsers?
+- Are bracketed option strings (`radius[config=...]`) validated before parsing?
+
+**Accounting manipulation**
+- Can a worker supply falsified session statistics (bytes transferred, duration) to
+  RADIUS or PAM accounting?
+- Is the accounting data sourced from the worker (untrusted) or from main/sec-mod
+  (trusted)?
+
+If you identify a potential issue in any of these categories, **do not open a
+public issue.** Follow the security disclosure procedure in `AGENTS.md`.
+
+### Enhanced Output Format
+
+Replace the base output format with this extended version for all ocserv findings.
+Every field is required; do not omit any.
+
+```
+[SEVERITY: Critical | High | Medium | Low | Informational]
+CWE: <CWE-ID if applicable, e.g. CWE-416 Use After Free>
+Location: <file>:<line> or <function>
+Issue: <one-sentence description of the bug>
+Impact: <one sentence — concrete attacker outcome: what they achieve, not how.
+        E.g. "unauthenticated access to the VPN tunnel" or
+        "prevents affected users from completing MFA login".
+        Do NOT write "could affect" or "may be exploited".>
+Attack scenario: <concrete exploit path — who sends what, what executes,
+                 what is the impact>
+Remediation: <specific fix, not "validate input">
+Confidence: Confirmed | High | Needs-domain-check
+Why not a false positive: <the disproof attempt that failed>
+```
+
+"Possible" or "could" in the Attack scenario means the finding is not yet
+Confirmed — downgrade to High or Needs-domain-check and state what additional
+evidence is required.
+
+### Adversarial Falsification (required before reporting)
+
+**Attempt to disprove every candidate finding before reporting it.**
+
+1. **Disprove before reporting.** For every candidate finding:
+   - Find the code path, helper, or cleanup mechanism that would make the issue safe.
+   - Read that mechanism — do not assume it handles the case.
+   - Only report the finding if disproof fails.
+   - Document why the disproof failed in the "Why not a false positive" field.
+
+2. **No vague risk claims.** Do not report "possible race", "could leak", or
+   "may be exploitable" without tracing the exact state transition and failure path.
+   If you cannot point to specific lines and a concrete bad outcome (crash,
+   privilege escalation, data corruption, denial of service), do not file it.
+
+3. **Verify helpers and callers.** If safety depends on a caller guarantee,
+   verify that guarantee from the caller's code. If you cannot verify it, mark
+   the finding `Needs-domain-check` and state what must be confirmed.
+
+4. **Maintain a false-positive record** as a markdown table:
+
+   | Candidate | Reason rejected | Safe mechanism |
+   |-----------|-----------------|----------------|
+   | ...       | ...             | ...            |
+
+<!-- END ocserv extensions -->