From 2dfb9acec54d41bce1013a9342d9f43602777262 Mon Sep 17 00:00:00 2001 From: Nikos Mavrogiannopoulos Date: Tue, 19 May 2026 16:57:44 +0200 Subject: [PATCH] Updated instructions for AI agents reporting vulnerabilities Signed-off-by: Nikos Mavrogiannopoulos --- contrib/ai/personas/ocserv-contributor.md | 31 ++- contrib/ai/personas/ocserv-core-dev.md | 96 ++------ contrib/ai/protocols/memory-safety-c.md | 148 ++++++++++++ .../ai/protocols/security-vulnerability.md | 220 ++++++++++++++++++ 4 files changed, 415 insertions(+), 80 deletions(-) create mode 100644 contrib/ai/protocols/memory-safety-c.md create mode 100644 contrib/ai/protocols/security-vulnerability.md diff --git a/contrib/ai/personas/ocserv-contributor.md b/contrib/ai/personas/ocserv-contributor.md index ed220d54..b09f1aaf 100644 --- a/contrib/ai/personas/ocserv-contributor.md +++ b/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: + +Affected versions: + +Reproduction: + +Severity self-assessment: Critical | High | Medium | Low +Justification: + +What I ruled out: +``` + +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 diff --git a/contrib/ai/personas/ocserv-core-dev.md b/contrib/ai/personas/ocserv-core-dev.md index 84045c6f..2e253518 100644 --- a/contrib/ai/personas/ocserv-core-dev.md +++ b/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 -what the programmer enforces. +Load and follow `contrib/ai/protocols/memory-safety-c.md` for the full analysis +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: -- **talloc is the project-wide allocator.** Use `talloc_zero`, `talloc_strdup`, - `talloc_array`, etc. for all allocations. Before introducing a new allocation, - 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. +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 -ocserv-specific taxonomy before concluding that code is safe. +Load and follow `contrib/ai/protocols/security-vulnerability.md` for the full +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** -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? -- 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: -Location: : or -Issue: -Attack scenario: -Remediation: -Confidence: Confirmed | High | Needs-domain-check -Why not a false positive: -``` - -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. +If you identify a potential issue, **do not open a public issue.** Follow the +security disclosure procedure in `AGENTS.md`. --- diff --git a/contrib/ai/protocols/memory-safety-c.md b/contrib/ai/protocols/memory-safety-c.md new file mode 100644 index 00000000..d89b5951 --- /dev/null +++ b/contrib/ai/protocols/memory-safety-c.md @@ -0,0 +1,148 @@ + + + +--- +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: : or +Issue: +Evidence: +Remediation: +Confidence: +``` + + + +--- + + + +## 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). + + diff --git a/contrib/ai/protocols/security-vulnerability.md b/contrib/ai/protocols/security-vulnerability.md new file mode 100644 index 00000000..b4be4ec8 --- /dev/null +++ b/contrib/ai/protocols/security-vulnerability.md @@ -0,0 +1,220 @@ + + + +--- +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: +Location: : or +Issue: +Attack scenario: +Remediation: +Confidence: +``` + + + +--- + + + +## 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: +Location: : or +Issue: +Impact: +Attack scenario: +Remediation: +Confidence: Confirmed | High | Needs-domain-check +Why not a false positive: +``` + +"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 | + |-----------|-----------------|----------------| + | ... | ... | ... | + +