diff --git a/lola-module/skills/secdevai-review/SKILL.md b/lola-module/skills/secdevai-review/SKILL.md index 1ef3c16..ee360da 100644 --- a/lola-module/skills/secdevai-review/SKILL.md +++ b/lola-module/skills/secdevai-review/SKILL.md @@ -1,6 +1,6 @@ --- name: secdevai-review -description: Perform AI-powered security code review using OWASP Top 10 and WSTG patterns. Use when reviewing source code, specific files, git commits, or entire codebases for security vulnerabilities. Supports multi-language analysis and severity classification. +description: Perform AI-powered security code review using OWASP Top 10, CWE/SANS Top 25, and WSTG patterns. Use when reviewing source code, specific files, git commits, or entire codebases for security vulnerabilities. Supports web and non-web code (C/C++, Go, Rust, etc.), multi-language analysis, severity classification, and automated finding validation via subagent. --- # SecDevAI Review Command @@ -65,6 +65,12 @@ Tip: To review an excluded file, remove its pattern from .secdevaiignore. - **Always read**: `secdevai-review/context/security-review.context` for OWASP Top 10 patterns +- **Auto-detect CWE/SANS Top 25 native code context** (additionally read `secdevai-review/context/cwe-top25-native.context` if ANY condition applies): + - Source code includes C (`.c`, `.h`), C++ (`.cpp`, `.cc`, `.cxx`, `.hpp`), or Rust (`.rs`) files + - `Makefile`, `CMakeLists.txt`, `meson.build`, `Cargo.toml`, or `*.sln`/`*.vcxproj` build files are present + - User explicitly mentions: "CWE", "SANS Top 25", "buffer overflow", "memory safety", "native code", "systems code" + - Assembly (`.s`, `.asm`) files or FFI/JNI bindings are detected + - **Auto-detect WSTG context** (additionally read `secdevai-review/context/wstg-testing.context` if ANY condition applies): - Source code is for a web application, web service, or web site - User explicitly mentions: "WSTG", "Web Security Testing Guide", or category numbers (4.1-4.12) @@ -78,7 +84,7 @@ Tip: To review an excluded file, remove its pattern from .secdevaiignore. - OpenShift deployment configs or templates are present - `docker-compose.yml` or `compose.yaml` exists -- **Note**: WSTG patterns enhance web application security analysis; golang-security.context provides Go-specific vulnerability and weakness patterns; OCI image security references provide container supply chain, configuration, hardening, and EOL detection patterns +- **Note**: CWE/SANS Top 25 patterns cover memory safety, integer overflow, race conditions, and privilege management for native/systems code; WSTG patterns enhance web application security analysis; golang-security.context provides Go-specific vulnerability and weakness patterns; OCI image security references provide container supply chain, configuration, hardening, and EOL detection patterns ### Step 4: Optional Tool Integration @@ -88,69 +94,85 @@ Tip: To review an excluded file, remove its pattern from .secdevaiignore. ### Step 5: Perform Analysis -- Scan code for security patterns from loaded context +- Scan code for security patterns from loaded context (OWASP Top 10 and/or CWE/SANS Top 25 depending on loaded contexts) - Classify findings by severity (Critical/High/Medium/Low/Info) -- Reference OWASP categories +- Reference OWASP categories and/or CWE IDs - Provide context-aware explanations +### Step 5.5: Validate Findings β€” Delegate to `secdevai-validate` + +**Purpose**: Reduce false positives and calibrate severity by dispatching findings to the `secdevai-validate` skill. + +**Delegate to the `secdevai-validate` skill.** Pass all findings from Step 5 to the validation skill, which independently: +1. Reads the actual source code at each reported location +2. Checks exploitability β€” whether a realistic attack path exists +3. Calibrates severity against [Red Hat's classification](https://access.redhat.com/security/updates/classification) (Critical / Important / Moderate / Low) +4. Produces a CVSS v3.1 base score analysis for each finding + +Dispatch via subagent when the platform supports parallel task execution. Pass findings using this prompt template: + +``` +Use the secdevai-validate skill to validate the following security findings. +Read the actual source code for each finding. Check exploitability, calibrate severity per Red Hat classification, and produce CVSS v3.1 analysis. + +Findings: +[paste the structured findings list from Step 5] +``` + +**Processing validation results** (returned by `secdevai-validate`): + +| Verdict | Action | +|---------|--------| +| **CONFIRMED** | Keep finding with original severity. Add CVSS vector and score. | +| **ADJUSTED** | Update severity to the validated level. Add CVSS vector, score, and adjustment reason. | +| **DISPUTED** | Keep finding, add "[Needs Manual Review]" tag and the dispute reasoning. | +| **REJECTED** | Remove from results. Log to skipped findings with rejection reason. | + +For all retained findings, enrich the output with: CVSS vector string, CVSS numeric score, Severity, and exploitability verdict. + +**Report only valid, exploitable findings** in the final output. Non-exploitable findings that are still valid issues should be listed in a separate "Informational / Not Exploitable" section with the explanation from the validation skill. + +**If subagent dispatch is unavailable** (e.g., platform does not support parallel task execution): perform the validation inline by reading the `secdevai-validate` skill and applying its steps directly. Still annotate uncertain findings with "[Needs Manual Review]". + ### Step 6: Present Findings +Present only validated, exploitable findings. Group by Severity: + ``` ## πŸ”’ **Security Review Results** -### πŸ”΄ **Critical Findings** (2) -- [Finding 1 with code reference] -- [Finding 2 with code reference] +### πŸ”΄ **Critical** (2) +- [Finding with code reference, CVSS vector/score, exploitability summary] -### 🟠 **High Severity** (3) -- [Finding details] +### 🟠 **Important** (3) +- [Finding details with CVSS] -### 🟑 **Medium Severity** (5) -- [Finding details] +### 🟑 **Moderate** (5) +- [Finding details with CVSS] -**Total**: 10 findings across [file/codebase] -``` +### πŸ”΅ **Low** (1) +- [Finding details with CVSS] + +### ℹ️ **Informational / Not Exploitable** (2) +- [Valid pattern but not exploitable β€” with explanation] -### Step 7: Save Results - -After presenting findings, collect all findings into structured JSON and export: - -```python -import importlib.util -from pathlib import Path - -# Load the exporter from secdevai-export skill scripts -script_path = Path("secdevai-export/scripts/results_exporter.py") -spec = importlib.util.spec_from_file_location("results_exporter", script_path) -mod = importlib.util.module_from_spec(spec) -spec.loader.exec_module(mod) - -# Collect findings into data structure -data = { - "metadata": { - "tool": "secdevai-ai-analysis", - "version": "1.0.0", - "timestamp": datetime.now().isoformat(), - "target_file": "[file path or 'codebase']", - "analyzer": "AI Security Review", - }, - "summary": { - "total_findings": [count], - "critical": [count], - "high": [count], - "medium": [count], - "low": [count], - "info": [count], - }, - "findings": [list of finding objects], -} - -# Export to markdown and SARIF -markdown_path, sarif_path = mod.export_results(data, command_type="review") +**Total**: 11 validated findings across [file/codebase] (2 false positives rejected) ``` -- The exporter will prompt the user to confirm the result directory (default: `secdevai-results`) -- Results are saved with timestamp: `secdevai-review-YYYYMMDD_HHMMSS.md` and `.sarif` +Each finding should include: +- `Severity`: Critical / Important / Moderate / Low +- `CVSS Vector`: CVSS:3.1/AV:X/AC:X/PR:X/UI:X/S:X/C:X/I:X/A:X +- `CVSS Score`: numeric score +- `Exploitability`: Exploitable / Conditionally exploitable / Not exploitable +- `Validation`: CONFIRMED / ADJUSTED (with reason) / DISPUTED + +### Step 7: Save Results (Optional) + +After presenting findings, ask the user: + +> **Would you like to export these findings to Markdown and SARIF report files?** + +If the user confirms, **delegate to the `secdevai-export` skill**. Collect all findings into the structured data format documented in that skill and invoke export with `command_type="review"`. If the user declines, skip export and proceed. ### Step 8: Offer Remediation (if `fix` also specified) @@ -165,9 +187,12 @@ If `fix` is specified alongside review: ## Security Context Sources - `secdevai-review/context/security-review.context` - OWASP Top 10 patterns (always loaded) +- `secdevai-review/context/cwe-top25-native.context` - CWE/SANS Top 25 patterns for native/systems code (auto-loaded for C/C++/Rust) - `secdevai-review/context/wstg-testing.context` - OWASP WSTG v4.2 web app testing patterns (auto-loaded for web code) - `secdevai-review/context/golang-security.context` - Go-specific vulnerabilities and weaknesses (auto-loaded for Go code) +**CWE/SANS Top 25 Auto-Detection**: The native code context automatically loads when reviewing C, C++, Rust, or other compiled/systems code, or when the user mentions CWE, SANS Top 25, buffer overflows, or memory safety. + **WSTG Auto-Detection**: The WSTG context automatically loads when reviewing web application code or when explicitly requested. **Golang Auto-Detection**: The Golang context automatically loads when reviewing Go source (e.g. `.go` files, `go.mod`) or when the user mentions Go/Golang. @@ -181,6 +206,9 @@ If `fix` is specified alongside review: ### Language Detection and Adaptation 1. **Detect the Language**: Identify the programming language from file extension, syntax, or imports + - C: `.c`, `.h`, `#include `, `malloc`, `free` + - C++: `.cpp`, `.cc`, `.cxx`, `.hpp`, `.hxx`, `#include `, `std::`, `class`, `template` + - Rust: `.rs`, `use`, `fn`, `impl`, `unsafe`, `Cargo.toml` - Python: `.py`, imports like `import flask`, `from django` - JavaScript/TypeScript: `.js`, `.ts`, `.jsx`, `.tsx`, `require()`, `import from` - Java: `.java`, `import`, `class`, `public static void` @@ -188,7 +216,6 @@ If `fix` is specified alongside review: - Ruby: `.rb`, `require`, `def`, `class` - PHP: `.php`, ` SIZE_MAX - b)`, use safe integer libraries +- C++: Use ``, `std::numeric_limits`, or safe integer libraries like SafeInt +- Rust: Debug builds panic on overflow; use `checked_add()`, `saturating_add()` in release + **XSS Prevention**: - Python/Flask: Use Jinja2 auto-escaping, `escape()`, `Markup()` - JavaScript/React: Use `textContent`, DOMPurify, React auto-escaping @@ -268,7 +313,9 @@ If `fix` is specified alongside review: ### Response Format for Non-Python Code -When reviewing non-Python code, structure your findings exactly the same way but with appropriate language examples: +When reviewing non-Python code, structure your findings exactly the same way but with appropriate language examples. + +**Web/managed language example (OWASP)**: ``` ## πŸ”΄ **Critical: SQL Injection** @@ -276,6 +323,7 @@ When reviewing non-Python code, structure your findings exactly the same way but **Location**: `UserController.java:42-45` **Language**: Java **OWASP Category**: A03: Injection +**CWE**: CWE-89 **Vulnerable Code**: ```java @@ -288,7 +336,6 @@ ResultSet rs = stmt.executeQuery(query); **Remediation**: ```java -// Use PreparedStatement with parameterized queries String query = "SELECT * FROM users WHERE username = ?"; PreparedStatement stmt = connection.prepareStatement(query); stmt.setString(1, username); @@ -297,7 +344,42 @@ ResultSet rs = stmt.executeQuery(); **References**: - OWASP: https://owasp.org/www-community/attacks/SQL_Injection -- Java: https://docs.oracle.com/javase/tutorial/jdbc/basics/prepared.html +- CWE-89: https://cwe.mitre.org/data/definitions/89.html +``` + +**Native/systems code example (CWE/SANS Top 25)**: + +``` +## πŸ”΄ **Critical: Buffer Overflow (CWE-787)** + +**Location**: `network.c:87-89` +**Language**: C +**CWE**: CWE-787 (Out-of-bounds Write) +**Severity**: Critical +**CVSS Vector**: CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H +**CVSS Score**: 9.8 +**Exploitability**: Exploitable β€” network-reachable parser with no bounds checking +**Validation**: βœ… CONFIRMED + +**Vulnerable Code**: +```c +char buf[64]; +strcpy(buf, packet->payload); // No bounds checking +``` + +**Risk**: Stack-based buffer overflow allows arbitrary code execution via crafted network packet + +**Remediation**: +```c +char buf[64]; +strncpy(buf, packet->payload, sizeof(buf) - 1); +buf[sizeof(buf) - 1] = '\0'; +``` + +**References**: +- CWE-787: https://cwe.mitre.org/data/definitions/787.html +- SANS Top 25: https://www.sans.org/top25-software-errors +- SEI CERT C: https://wiki.sei.cmu.edu/confluence/display/c/STR31-C ``` ## Verification Requirements diff --git a/lola-module/skills/secdevai-review/context/cwe-top25-native.context b/lola-module/skills/secdevai-review/context/cwe-top25-native.context new file mode 100644 index 0000000..1cc5cc3 --- /dev/null +++ b/lola-module/skills/secdevai-review/context/cwe-top25-native.context @@ -0,0 +1,548 @@ +# πŸ”’ CWE/SANS Top 25 β€” Native & Systems Code Security Context + +**Purpose**: Security analysis guidelines for native and systems-level codeβ€”memory safety, integer safety, concurrency, and privilege patterns for AI-powered code review + +**Scope**: C, C++, Rust (unsafe blocks), and other compiled/systems languages. Use alongside `security-review.context` for OWASP framing. Based on the [CWE/SANS Top 25 Most Dangerous Software Errors](https://www.sans.org/top25-software-errors). + +--- + +## 🎯 Key Differences from Web-Focused Review + +Native code introduces vulnerability classes rarely seen in managed languages: + +| Category | Web/Managed Languages | Native/Systems Languages | +|---|---|---| +| Memory safety | GC handles allocation | Manual allocation β†’ use-after-free, buffer overflows | +| Integer safety | Arbitrary precision or overflow-safe | Fixed-width integers β†’ overflow, truncation | +| Concurrency | Often thread-safe by default | Raw threads, shared memory β†’ data races, TOCTOU | +| Privilege | App-level permissions | System-level: setuid, capabilities, file modes | +| Pointers | References / no pointers | Raw pointers β†’ null deref, dangling pointers, type confusion | + +--- + +## πŸ“‹ CWE/SANS Top 25 β€” Native Code Patterns + +### CWE-787: Out-of-bounds Write (Rank #1) + +**Patterns to detect**: +- `strcpy`, `strcat`, `sprintf`, `gets` without bounds checking +- Array indexing without length validation +- `memcpy`/`memmove` with user-controlled size +- Stack buffer overflows via unbounded input + +**C/C++ Examples**: +```c +// BAD: Unbounded copy β€” classic buffer overflow +char buf[64]; +strcpy(buf, user_input); // CWE-787 + +// GOOD: Bounded copy +char buf[64]; +strncpy(buf, user_input, sizeof(buf) - 1); +buf[sizeof(buf) - 1] = '\0'; + +// BETTER (C11): Use safe alternatives +char buf[64]; +strncpy_s(buf, sizeof(buf), user_input, _TRUNCATE); +``` + +```cpp +// BAD: Unchecked vector access +std::vector v = get_data(); +int val = v[user_index]; // No bounds check + +// GOOD: Use .at() for bounds-checked access +int val = v.at(user_index); // Throws std::out_of_range +``` + +### CWE-416: Use After Free (Rank #4) + +**Patterns to detect**: +- `free()` followed by pointer dereference +- Returning pointers to local/freed memory +- Double-free conditions +- Dangling pointers after container operations (C++ iterators) + +**C/C++ Examples**: +```c +// BAD: Use after free +char *ptr = malloc(128); +process(ptr); +free(ptr); +log_result(ptr); // CWE-416: dangling pointer + +// GOOD: Nullify after free +free(ptr); +ptr = NULL; +``` + +```cpp +// BAD: Dangling iterator after container modification +auto it = vec.begin(); +vec.push_back(42); // May invalidate iterators +*it = 10; // CWE-416 + +// GOOD: Re-acquire iterator after modification +vec.push_back(42); +auto it = vec.begin(); +``` + +### CWE-125: Out-of-bounds Read (Rank #7) + +**Patterns to detect**: +- Reading beyond buffer boundaries (information leak, Heartbleed-style) +- Off-by-one in loop bounds +- `strlen` on unterminated strings +- `memcmp`/`memcpy` with incorrect size + +**C/C++ Examples**: +```c +// BAD: Read past buffer end +char buf[10]; +recv(sock, buf, 10, 0); +for (int i = 0; i <= 10; i++) { // Off-by-one: should be < 10 + putchar(buf[i]); // CWE-125 +} + +// GOOD: Correct bounds +for (int i = 0; i < 10; i++) { + putchar(buf[i]); +} +``` + +### CWE-476: NULL Pointer Dereference (Rank #12) + +**Patterns to detect**: +- Dereferencing return values without null check (`malloc`, `fopen`, `strstr`) +- Null pointer passed to functions that don't handle it +- Missing null guard after type-cast or lookup + +**C/C++ Examples**: +```c +// BAD: No null check after malloc +int *arr = malloc(n * sizeof(int)); +arr[0] = 42; // CWE-476 if malloc returns NULL + +// GOOD: Check return value +int *arr = malloc(n * sizeof(int)); +if (arr == NULL) { + return -ENOMEM; +} +arr[0] = 42; +``` + +```cpp +// BAD: Unchecked dynamic_cast +Derived *d = dynamic_cast(base_ptr); +d->method(); // CWE-476 if cast fails + +// GOOD: Check before use +if (auto *d = dynamic_cast(base_ptr)) { + d->method(); +} +``` + +### CWE-190: Integer Overflow or Wraparound (Rank #14) + +**Patterns to detect**: +- Arithmetic on user-controlled integers without overflow check +- Size calculations: `malloc(n * sizeof(T))` where n is user-controlled +- Signed/unsigned comparison or conversion +- Truncation when assigning larger type to smaller type + +**C/C++ Examples**: +```c +// BAD: Integer overflow in allocation size +size_t count = get_user_count(); // User-controlled +size_t size = count * sizeof(struct record); // CWE-190: wraps if count is large +char *buf = malloc(size); // Allocates tiny buffer, writes past end + +// GOOD: Check for overflow before allocation +if (count > SIZE_MAX / sizeof(struct record)) { + return -EINVAL; +} +size_t size = count * sizeof(struct record); +char *buf = malloc(size); +``` + +```cpp +// BAD: Signed/unsigned mismatch +int len = get_length(); // Could be negative +memcpy(dst, src, len); // Interpreted as huge unsigned value + +// GOOD: Validate before use +int len = get_length(); +if (len < 0 || (size_t)len > dst_capacity) { + return -EINVAL; +} +memcpy(dst, src, (size_t)len); +``` + +### CWE-119: Improper Restriction of Memory Buffer Operations (Rank #17) + +**Patterns to detect**: +- Unbounded `memcpy`, `memmove`, `memset` with user-controlled length +- Stack-allocated buffers used with variable-length input +- Missing bounds checks in custom parsers or protocol handlers +- VLA (variable-length array) with unchecked size + +**C/C++ Examples**: +```c +// BAD: User controls memcpy length +void parse_packet(const char *data, size_t data_len) { + char header[16]; + uint32_t payload_len = *(uint32_t *)data; + memcpy(header, data + 4, payload_len); // CWE-119 +} + +// GOOD: Validate length against buffer size +void parse_packet(const char *data, size_t data_len) { + char header[16]; + if (data_len < 4) return; + uint32_t payload_len = *(uint32_t *)data; + if (payload_len > sizeof(header) || payload_len > data_len - 4) return; + memcpy(header, data + 4, payload_len); +} +``` + +### CWE-362: Race Condition (Rank #21) + +**Patterns to detect**: +- TOCTOU (time-of-check-time-of-use) on filesystem operations +- Unprotected shared state across threads (no mutex/lock) +- Signal handlers accessing shared data +- `stat()` then `open()` without O_NOFOLLOW or atomic operations + +**C/C++ Examples**: +```c +// BAD: TOCTOU race on file +if (access(path, W_OK) == 0) { // Check + fd = open(path, O_WRONLY); // Use β€” file could change between check and open +} + +// GOOD: Open and check permissions atomically +fd = open(path, O_WRONLY | O_NOFOLLOW); +if (fd < 0) { + handle_error(); +} +``` + +```cpp +// BAD: Unprotected shared state +int counter = 0; +void worker() { counter++; } // CWE-362: data race + +// GOOD: Use mutex or atomic +std::atomic counter{0}; +void worker() { counter.fetch_add(1); } +``` + +### CWE-78: OS Command Injection (Rank #5) + +**Patterns to detect**: +- `system()`, `popen()` with user-controlled strings +- Shell metacharacter injection via string concatenation +- `execvp`/`execve` with unsanitized arguments + +**C/C++ Examples**: +```c +// BAD: Command injection via system() +char cmd[256]; +snprintf(cmd, sizeof(cmd), "ls %s", user_input); +system(cmd); // CWE-78: user_input may contain "; rm -rf /" + +// GOOD: Use exec* family with argument array; validate input +char *args[] = {"ls", validated_path, NULL}; +execvp("ls", args); +``` + +### CWE-22: Path Traversal (Rank #8) + +**Patterns to detect**: +- File paths constructed from user input without canonicalization +- `../` sequences not stripped or rejected +- Symlink following without restriction + +**C/C++ Examples**: +```c +// BAD: Unsanitized path +char filepath[PATH_MAX]; +snprintf(filepath, sizeof(filepath), "/data/%s", user_filename); +FILE *f = fopen(filepath, "r"); // CWE-22: user_filename = "../../etc/passwd" + +// GOOD: Resolve and validate path stays under base +char resolved[PATH_MAX]; +char filepath[PATH_MAX]; +snprintf(filepath, sizeof(filepath), "/data/%s", user_filename); +if (realpath(filepath, resolved) == NULL) { return -1; } +if (strncmp(resolved, "/data/", 6) != 0) { return -1; } +FILE *f = fopen(resolved, "r"); +``` + +### CWE-798: Use of Hard-coded Credentials (Rank #18) + +**Patterns to detect**: +- String literals matching password/key/token/secret patterns +- Compiled-in default passwords or API keys +- Hardcoded symmetric keys or private keys + +**C/C++ Examples**: +```c +// BAD: Hardcoded credentials +const char *db_password = "s3cret123"; // CWE-798 + +// GOOD: Read from environment or config at runtime +const char *db_password = getenv("DB_PASSWORD"); +if (db_password == NULL) { + fprintf(stderr, "DB_PASSWORD not set\n"); + exit(1); +} +``` + +### CWE-276: Incorrect Default Permissions (Rank #25) + +**Patterns to detect**: +- Files or directories created with world-writable permissions +- `umask(0)` or `chmod(path, 0777)` +- Sensitive files (keys, configs) created without restrictive permissions + +**C/C++ Examples**: +```c +// BAD: World-readable credentials file +int fd = open("/etc/myapp/secrets.conf", O_CREAT | O_WRONLY, 0644); // CWE-276 + +// GOOD: Owner-only permissions +int fd = open("/etc/myapp/secrets.conf", O_CREAT | O_WRONLY, 0600); +``` + +### CWE-20: Improper Input Validation (Rank #6) + +**Patterns to detect**: +- User input used directly without length, range, or type checks +- Missing validation on protocol fields, command arguments, environment variables +- Accepting negative sizes or zero-length allocations + +**C/C++ Examples**: +```c +// BAD: No validation on user-supplied length +void read_data(int sock) { + uint32_t len; + recv(sock, &len, sizeof(len), 0); + char *buf = malloc(len); // CWE-20: len could be 0 or enormous + recv(sock, buf, len, 0); +} + +// GOOD: Validate bounds +void read_data(int sock) { + uint32_t len; + recv(sock, &len, sizeof(len), 0); + len = ntohl(len); + if (len == 0 || len > MAX_MSG_SIZE) { + return; + } + char *buf = malloc(len); + if (buf == NULL) return; + recv(sock, buf, len, 0); +} +``` + +### CWE-269: Improper Privilege Management (Rank #22) + +**Patterns to detect**: +- Running as root when not necessary +- Failing to drop privileges after binding to privileged ports +- setuid binaries with excessive scope +- Capabilities not minimized + +**C/C++ Examples**: +```c +// BAD: Keep root privileges after startup +int main() { + bind_to_port_80(); + serve_requests(); // Still running as root +} + +// GOOD: Drop privileges after bind +int main() { + bind_to_port_80(); + drop_privileges("nobody"); // setuid/setgid to unprivileged user + serve_requests(); +} +``` + +### CWE-502: Deserialization of Untrusted Data (Rank #15) + +**Patterns to detect**: +- Deserializing binary formats (protobuf, msgpack, custom) from untrusted sources without schema validation +- Type confusion through serialized data +- Buffer overruns in custom deserialization code + +**C/C++ Examples**: +```c +// BAD: Custom deserialization without validation +struct msg { + uint32_t type; + uint32_t len; + char data[]; +}; +struct msg *m = (struct msg *)network_buffer; +process(m->data, m->len); // CWE-502 + CWE-119: len not validated + +// GOOD: Validate all fields before use +if (total_len < sizeof(struct msg)) return -1; +struct msg *m = (struct msg *)network_buffer; +uint32_t len = ntohl(m->len); +if (len > total_len - sizeof(struct msg)) return -1; +if (ntohl(m->type) > MSG_TYPE_MAX) return -1; +process(m->data, len); +``` + +--- + +## πŸ›‘οΈ Additional Native Code Patterns + +### Format String Vulnerabilities + +**Patterns to detect**: +- User-controlled format strings in `printf`, `syslog`, `fprintf` +- Missing format specifier (passing user string as first argument) + +```c +// BAD: Format string attack +printf(user_input); // CWE-134: attacker supplies "%x%x%x%n" + +// GOOD: Always use a format specifier +printf("%s", user_input); +``` + +### Double Free + +**Patterns to detect**: +- `free()` called twice on the same pointer +- Error paths that free memory already freed in the success path + +```c +// BAD: Double free +free(ptr); +if (error) { + free(ptr); // CWE-415 +} + +// GOOD: Nullify after free, check before free +free(ptr); +ptr = NULL; +if (error) { + free(ptr); // Safe: free(NULL) is a no-op +} +``` + +### Uninitialized Memory + +**Patterns to detect**: +- Stack variables used before initialization +- `malloc` without `memset` or initialization before read +- Struct padding leaked to output + +```c +// BAD: Uninitialized stack buffer leaked to network +struct response resp; +resp.status = 200; +// resp.padding not initialized β€” may leak stack data +send(sock, &resp, sizeof(resp), 0); // CWE-908 + +// GOOD: Zero-initialize +struct response resp = {0}; +resp.status = 200; +send(sock, &resp, sizeof(resp), 0); +``` + +### Unsafe String Functions β€” Quick Reference + +| Unsafe | Safe Alternative | Notes | +|--------|-----------------|-------| +| `gets` | `fgets` | `gets` has no bounds check; removed in C11 | +| `strcpy` | `strncpy` / `strlcpy` | Always limit to destination size | +| `strcat` | `strncat` / `strlcat` | Track remaining space | +| `sprintf` | `snprintf` | Always pass buffer size | +| `scanf("%s", ...)` | `scanf("%63s", ...)` | Specify max field width | +| `strlen` on untrusted input | `strnlen` | Prevent unbounded scan | + +--- + +## πŸ” Analysis Workflow for Native Code + +1. **Parse code**: Identify memory allocations, pointer operations, system calls, string handling, and privilege operations. +2. **Pattern match**: Scan for CWE Top 25 patterns listed above (buffer overflows, use-after-free, integer overflow, null deref, race conditions, format strings). +3. **Severity**: + - **Critical**: RCE via buffer overflow, use-after-free, format string; command injection; privilege escalation + - **High**: Out-of-bounds read (info leak), integer overflow leading to heap corruption, race conditions on security-sensitive operations + - **Medium**: NULL pointer dereference (DoS), uninitialized memory read, missing input validation, hardcoded credentials + - **Low**: Incorrect permissions, minor TOCTOU, missing bounds on non-security paths + - **Info**: Deprecated function usage, style/hardening suggestions +4. **Context**: Check for mitigations (ASLR, stack canaries, compiler flags `-fstack-protector`, AddressSanitizer, bounds-checking wrappers). +5. **Remediation**: Suggest concrete C/C++ code changes with CWE references. + +--- + +## πŸ“Š Output Format + +Use the same finding structure as `security-review.context`, adding CWE ID: + +``` +## πŸ”΄ **Critical: Buffer Overflow (CWE-787)** + +**Location**: `parser.c:142-145` +**Language**: C +**CWE**: CWE-787 (Out-of-bounds Write) +**OWASP Category**: A03: Injection (when applicable) + +**Vulnerable Code**: +```c +char buf[64]; +strcpy(buf, user_input); +``` + +**Risk**: Stack-based buffer overflow allows arbitrary code execution + +**Remediation**: +```c +char buf[64]; +strncpy(buf, user_input, sizeof(buf) - 1); +buf[sizeof(buf) - 1] = '\0'; +``` + +**References**: +- CWE-787: https://cwe.mitre.org/data/definitions/787.html +- SANS Top 25: https://www.sans.org/top25-software-errors +``` + +--- + +## πŸ”§ Compiler and Tool Hardening Checks + +When reviewing native projects, also check for missing hardening: + +| Check | What to look for | +|-------|-----------------| +| Stack protection | `-fstack-protector-strong` or `-fstack-protector-all` in build flags | +| ASLR/PIE | `-fPIE -pie` for executables | +| FORTIFY_SOURCE | `-D_FORTIFY_SOURCE=2` to catch buffer overflows at runtime | +| RELRO | `-Wl,-z,relro,-z,now` for GOT protection | +| Warnings | `-Wall -Wextra -Werror` to catch implicit issues | +| Static analysis | Integration with `cppcheck`, `clang-tidy`, `scan-build`, or Coverity | +| Sanitizers | AddressSanitizer, UBSan, ThreadSan available for CI | + +--- + +## πŸ“š References + +- [CWE/SANS Top 25](https://www.sans.org/top25-software-errors) +- [CWE List](https://cwe.mitre.org/top25/) +- [OWASP Top 10](https://owasp.org/www-project-top-ten/) +- [SEI CERT C Coding Standard](https://wiki.sei.cmu.edu/confluence/display/c/) +- [SEI CERT C++ Coding Standard](https://wiki.sei.cmu.edu/confluence/display/cplusplus/) +- [C++ Core Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines) + +--- + +**Note**: This context focuses on native/systems code vulnerabilities from the CWE/SANS Top 25. Many entries (injection, hardcoded credentials, path traversal) overlap with OWASP Top 10 but manifest differently in C/C++. Use with `security-review.context` for general OWASP framing. diff --git a/lola-module/skills/secdevai-review/context/security-review.context b/lola-module/skills/secdevai-review/context/security-review.context index 50171ed..a02ca36 100644 --- a/lola-module/skills/secdevai-review/context/security-review.context +++ b/lola-module/skills/secdevai-review/context/security-review.context @@ -316,6 +316,7 @@ def process_data(user_input): **Location**: `file.py:42` **OWASP Category**: A03: Injection +**CWE**: CWE-89 (when applicable β€” always include CWE ID for CWE/SANS Top 25 entries) **Description**: [Clear explanation] **Vulnerable Code**: @@ -333,6 +334,7 @@ def process_data(user_input): **References**: - OWASP: [link] - CWE: [link] +- SANS Top 25: https://www.sans.org/top25-software-errors (when finding maps to a Top 25 entry) ``` ### Prioritization diff --git a/lola-module/skills/secdevai-validate/SKILL.md b/lola-module/skills/secdevai-validate/SKILL.md new file mode 100644 index 0000000..eb9bff3 --- /dev/null +++ b/lola-module/skills/secdevai-validate/SKILL.md @@ -0,0 +1,222 @@ +--- +name: secdevai-validate +description: Validate security findings for exploitability, severity accuracy, and CVSS scoring. Use when secdevai-review dispatches findings for validation, or when the user invokes /secdevai validate to re-validate prior results. Rejects false positives, calibrates severity to Red Hat classification, and produces per-finding CVSS v3.1 analysis. +--- + +# SecDevAI Validate Command + +## Description +Validate security findings by checking exploitability, calibrating severity against Red Hat's classification, and producing CVSS v3.1 analysis. Invoked automatically by `secdevai-review` as a subagent, or manually via `/secdevai validate` or the `/secdevai-validate` alias. + +## Usage +``` +/secdevai validate # Validate findings from prior review +/secdevai-validate # Alias: same as /secdevai validate +``` + +## Expected Response + +When dispatched as a subagent from `secdevai-review`, you receive a list of findings. For each finding, perform the full validation pipeline below. + +### Step 1: Read the Actual Source Code + +**Never rely solely on the review agent's description.** For each finding: + +1. Read the source file at the reported location +2. Read surrounding context (at least 20 lines above and below) +3. Trace the data flow β€” identify where untrusted input enters and whether it reaches the vulnerable sink +4. Check imports, configurations, and middleware that may apply mitigations + +### Step 2: Exploitability Analysis + +Determine whether the finding is exploitable in a realistic attack scenario. + +**Evaluate each condition:** + +| Check | Question | +|-------|----------| +| **Reachability** | Is the vulnerable code reachable from an attacker-controlled input? Trace from entry point (HTTP handler, CLI arg, file parser, IPC) to the vulnerable sink. | +| **Existing mitigations** | Are mitigations already in place? (input validation, bounds checks, sanitization, safe wrappers, WAF rules, compiler hardening flags like FORTIFY_SOURCE, stack canaries, ASLR) | +| **Preconditions** | What must be true for exploitation? (specific config, authentication state, race condition, memory layout, platform/architecture) | +| **Attack surface** | Is the component exposed to untrusted users, or is it internal-only / admin-only / test-only? | +| **Weaponizability** | Can this be turned into a reliable exploit, or is it a theoretical concern? | + +**For specific vulnerability classes:** + +- **Memory safety** (CWE-787, CWE-416, CWE-125, CWE-476, CWE-119, CWE-190): Verify exact buffer sizes, allocation patterns, control flow, and whether modern mitigations (ASLR, stack canaries, FORTIFY_SOURCE) block exploitation +- **Concurrency** (CWE-362): Verify shared state, synchronization primitives, and whether the race window is practically exploitable +- **Injection** (CWE-89, CWE-79, CWE-78): Verify the complete data flow from source to sink β€” is user input actually interpolated without sanitization? +- **Authentication/Authorization** (CWE-287, CWE-862): Verify whether the bypass works against the deployed auth stack, not just the isolated code path + +**Classify exploitability:** + +| Verdict | Criteria | +|---------|----------| +| **Exploitable** | Clear attack path exists with realistic preconditions. A competent attacker could weaponize this. | +| **Conditionally exploitable** | Valid vulnerability, but exploitation requires non-default config, specific platform, unlikely race condition, or chained attack. Explain the conditions. | +| **Not exploitable** | The vulnerability pattern exists in the code but mitigations prevent exploitation, or the code is unreachable from attacker input. **Still report as a valid finding** with explanation of why it is not exploitable in current context. | + +### Step 3: Severity Calibration β€” Red Hat Classification + +Map each finding to [Red Hat's severity classification](https://access.redhat.com/security/updates/classification): + +| Severity | Criteria | +|-----------------|----------| +| **Critical** | Easily exploited by a remote unauthenticated attacker leading to system compromise (arbitrary code execution) without user interaction. Flaws requiring authentication, local/physical access, or unlikely configuration are NOT Critical. | +| **Important** | Easily compromises confidentiality, integrity, or availability. Includes: local/authenticated privilege escalation, unauthenticated remote access to protected resources, authenticated remote code execution, remote denial of service. | +| **Moderate** | More difficult to exploit but could still compromise CIA under certain circumstances. Includes: vulnerabilities that would be Critical/Important but are harder to exploit or affect unlikely configurations. | +| **Low** | Requires unlikely circumstances to exploit, or successful exploit yields minimal consequences. Includes theoretical vulnerabilities with no proven exploitation vector. | + +**Challenge the review agent's severity:** +- If the finding requires local access but was rated Critical, downgrade to Important or Moderate +- If the finding requires authentication but was rated as unauthenticated, adjust accordingly +- If exploitation requires non-default configuration, consider downgrade +- If mitigations (SELinux, sandboxing, containerization) limit impact, note and potentially downgrade + +### Step 4: CVSS v3.1 Base Score Analysis + +Analyze each CVSS base metric independently, arguing for the most accurate value: + +#### Attack Vector (AV) +- **Network (N)**: Vulnerable component reachable over the network without local access +- **Adjacent (A)**: Requires same network segment +- **Local (L)**: Requires local access or user must open a crafted file. Many parser bugs are Local, not Network +- **Physical (P)**: Requires physical hardware access +- *Challenge*: If the PoC runs locally on a crafted file, argue for Local unless there is clear evidence of a network-reachable path + +#### Attack Complexity (AC) +- **Low (L)**: Exploitation repeatable without special conditions +- **High (H)**: Requires race conditions, non-default configurations, specific memory layouts, or platform constraints +- *Challenge*: If the PoC only works on a specific architecture, requires large input, or depends on non-default settings, argue for High + +#### Privileges Required (PR) +- **None (N)**: Completely unauthenticated, anonymous attacker can trigger this +- **Low (L)**: Requires basic authentication or user-level account +- **High (H)**: Requires admin/root privileges +- *Challenge*: If attack requires writing files to a specific directory or calling a privileged API, argue for Low or High + +#### User Interaction (UI) +- **None (N)**: Zero user action required +- **Required (R)**: User must process a crafted file, click a link, visit a page, or import data +- *Challenge*: Parser vulnerabilities typically require a user to process attacker-supplied input β€” argue for Required unless the component autonomously fetches and parses untrusted data + +#### Scope (S) +- **Unchanged (U)**: Impact stays within the vulnerable component's security authority +- **Changed (C)**: Exploitation impacts beyond the vulnerable component (container escape, sandbox bypass) +- *Challenge*: Most library-level bugs have Unchanged scope. Argue against Changed unless cross-boundary impact is demonstrated + +#### Confidentiality (C) +- **None (N)**: No data leaked. Many DoS/crash bugs have zero confidentiality impact +- **Low (L)**: Limited or non-sensitive data leaked +- **High (H)**: Arbitrary sensitive data readable +- *Challenge*: If the finding is a crash/DoS, argue for None + +#### Integrity (I) +- **None (N)**: No data modification. Crashes and info leaks typically have zero integrity impact +- **Low (L)**: Modification limited to non-critical data +- **High (H)**: Arbitrary writes or control flow hijack +- *Challenge*: Integer overflows leading to undersized allocations need careful analysis β€” does the overflow lead to a write primitive, or just a crash? + +#### Availability (A) +- **None (N)**: No availability impact +- **Low (L)**: Degraded performance, service continues +- **High (H)**: Complete denial of service β€” process crash, hang, or resource exhaustion +- *Challenge*: A crash in a library is High only if the consuming application cannot catch/recover + +### Step 5: Produce Verdict + +For each finding, return a structured validation result: + +``` +## Finding : + +### Review Agent's Assessment +- **Severity**: <original severity> +- **CWE/OWASP**: <ID> +- **Location**: <file:line> + +### Exploitability Analysis +- **Verdict**: Exploitable | Conditionally exploitable | Not exploitable +- **Attack path**: <describe the realistic attack scenario or explain why exploitation fails> +- **Mitigations found**: <list existing mitigations in the code/environment> +- **Preconditions**: <what must be true for exploitation> + +### CVSS v3.1 Analysis +- **AV**: <value> β€” <reasoning> +- **AC**: <value> β€” <reasoning> +- **PR**: <value> β€” <reasoning> +- **UI**: <value> β€” <reasoning> +- **S**: <value> β€” <reasoning> +- **C**: <value> β€” <reasoning> +- **I**: <value> β€” <reasoning> +- **A**: <value> β€” <reasoning> + +### CVSS Vector: <CVSS:3.1/AV:X/AC:X/PR:X/UI:X/S:X/C:X/I:X/A:X> +### CVSS Score: <numeric score> +### Severity: <Critical | Important | Moderate | Low> + +### Validation Verdict: <CONFIRMED | ADJUSTED | DISPUTED | REJECTED> +- **CONFIRMED**: Exploitability and severity match the review agent's assessment +- **ADJUSTED**: Valid finding, but severity or exploitability was recalibrated (explain why) +- **DISPUTED**: Finding exists but exploitability is questionable β€” needs manual review +- **REJECTED**: False positive β€” vulnerable pattern does not exist at the reported location, or mitigations fully prevent exploitation + +### Reasoning +<2-3 sentence explanation of the verdict, citing specific code evidence> +``` + +### Step 6: Return Summary to Review Agent + +After validating all findings, return a summary: + +``` +## Validation Summary + +| # | Finding | Original Severity | Validated Severity | CVSS Score | Exploitability | Verdict | +|---|---------|-------------------|-------------------|------------|----------------|---------| +| 1 | <title> | Critical | Important | 7.5 | Conditionally exploitable | ADJUSTED | +| 2 | <title> | High | High | 8.1 | Exploitable | CONFIRMED | +| ... | | | | | | | + +### Statistics +- **Total findings validated**: N +- **Confirmed**: N (severity matches) +- **Adjusted**: N (severity recalibrated) +- **Disputed**: N (needs manual review) +- **Rejected**: N (false positives removed) + +### Rejected Findings (moved to skipped) +- <Finding X>: <reason for rejection> +``` + +## Processing Rules for the Calling Agent (secdevai-review) + +When `secdevai-review` receives validation results, apply: + +| Verdict | Action | +|---------|--------| +| **CONFIRMED** | Keep finding with original severity. Add CVSS vector and score. | +| **ADJUSTED** | Update severity to the validated level. Add CVSS vector, score, and adjustment reason. | +| **DISPUTED** | Keep finding, add "[Needs Manual Review]" tag and the dispute reasoning. | +| **REJECTED** | Remove from results. Log to skipped findings with rejection reason. | + +For all retained findings, enrich the output with: +- `CVSS Vector`: the computed CVSS:3.1 vector string +- `CVSS Score`: the numeric score +- `Severity`: the calibrated severity per Red Hat classification +- `Exploitability`: the exploitability verdict and attack path summary + +## Analysis Rules + +1. **Read the actual source code** β€” never rely solely on the review agent's description +2. **Be specific in challenges** β€” "AV should be Local because the only consumption path requires opening a crafted file" is useful; "I think this is overstated" is not +3. **Acknowledge when the review agent is right** β€” do not downgrade for the sake of downgrading +4. **Do NOT dismiss valid findings** β€” your job is to calibrate severity, not reject discoveries +5. **Do NOT add new findings** β€” you validate what the review agent found +6. **Document your reasoning** β€” every CVSS component and severity decision must have clear justification +7. **Consider deployment context** β€” how is this code actually used? What are typical consuming applications? +8. **Check for mitigating factors** β€” resource limits, process isolation, input size limits, compile-time hardening + +## Fallback: Inline Validation + +If subagent dispatch is unavailable (e.g., platform does not support parallel task execution): the calling agent performs validation inline by re-reading each flagged location and applying the same verification steps from this skill. Annotate uncertain findings with "[Needs Manual Review]". diff --git a/lola-module/skills/secdevai/SKILL.md b/lola-module/skills/secdevai/SKILL.md index 73d78a1..cba851b 100644 --- a/lola-module/skills/secdevai/SKILL.md +++ b/lola-module/skills/secdevai/SKILL.md @@ -23,6 +23,7 @@ AI-powered secure development assistant that dispatches to specialized sub-skill /secdevai dast # DAST scan web app/API using RapiDAST (auto-detects Dockerfile/OpenAPI) /secdevai dast --url <URL> # DAST scan a specific URL (legal warning applies) /secdevai git-commit # Commit approved fixes (requires git config and approved fixes) +/secdevai validate # Validate findings from prior review (exploitability, CVSS, severity) /secdevai export json # Export report (json, markdown, sarif) /secdevai oci-image-security # Analyze OCI container images for security issues ``` @@ -34,6 +35,7 @@ AI-powered secure development assistant that dispatches to specialized sub-skill /secdevai-review # Review code (alias for /secdevai review) /secdevai-report # Generate security report /secdevai-tool # Use specific tool (alias for /secdevai tool) +/secdevai-validate # Validate findings (alias for /secdevai validate) /secdevai-dast # DAST scan (alias for /secdevai dast) /secdevai-export # Export report (alias for /secdevai export) /secdevai-oci-image-security # Analyze OCI container images for security issues @@ -59,16 +61,19 @@ When user runs `/secdevai`, route to the appropriate sub-skill: 4. **`tool`**: **Delegate to the `secdevai-tool` skill.** The tool skill handles external tool execution (Bandit, Scorecard), output parsing, AI synthesis, and result export. -5. **`export`**: **Delegate to the `secdevai-export` skill.** +5. **`validate`**: **Delegate to the `secdevai-validate` skill.** + The validate skill checks exploitability, calibrates severity against Red Hat's classification, and produces CVSS v3.1 analysis. Typically invoked automatically by `secdevai-review` as a subagent, but can be run manually to re-validate prior results. + +6. **`export`**: **Delegate to the `secdevai-export` skill.** The export skill handles converting findings to Markdown and SARIF formats. -6. **`oci-image-security`**: **Delegate to the `secdevai-oci-image-security` skill.** +7. **`oci-image-security`**: **Delegate to the `secdevai-oci-image-security` skill.** The OCI image security skill analyzes container images for CVE/package vulnerabilities, configuration security issues, supply chain risks, and hardening gaps. Use when reviewing Dockerfiles, Containerfiles, or OCI images from any registry. -7. **`dast`**: **Delegate to the `secdevai-dast` skill.** +8. **`dast`**: **Delegate to the `secdevai-dast` skill.** The DAST skill performs dynamic application security testing using RapiDAST. It auto-detects Dockerfile/Compose for container setup, discovers OpenAPI specs, handles authentication, runs passive then optionally active scanning, and exports SARIF results. Crucially, it also **traces each DAST finding back to the exact source file and line that is the root cause** β€” bridging the gap between a runtime HTTP observation (e.g. "SQL injection detected at `/api/pets/name/<param>`") and the vulnerable code that produced it (e.g. raw f-string SQL construction in `db.py:32`). The correlation report is saved alongside the SARIF as `source-correlation.md`. -8. **`git-commit`**: +9. **`git-commit`**: - Only proceed if there are approved fixes that have been applied - Verify git is configured (check for git repository and user config) - If conditions met: Create a commit with descriptive message about security fixes @@ -95,6 +100,7 @@ This command uses multiple security context files: This command integrates with: - `secdevai-review/context/` directory for security analysis guidelines +- `secdevai-validate` for finding validation, exploitability checks, CVSS scoring, and Red Hat severity calibration - `secdevai-tool` for optional tool integration via containerized runners - `secdevai-dast/scripts/rapidast-scan.sh` for DAST scanning via RapiDAST (produces SARIF + `source-correlation.md`) - `secdevai-export/scripts/results_exporter.py` for result export