[GH-ISSUE #88] Security Audit: 18 findings (5 Critical, 7 High) - Argus Security Report #30

New issue

Closed

opened 2026-02-27 07:20:04 +03:00 by kerem · 1 comment

kerem commented

2026-02-27 07:20:04 +03:00

Owner

Originally created by @devatsecure on GitHub (Feb 8, 2026).
Original GitHub issue: https://github.com/KeygraphHQ/shannon/issues/88

Argus Security Audit Report

Scan Date: 2026-02-08
Scanner: Argus Security (6-phase AI-powered pipeline)
Model: claude-sonnet-4-5-20250929
Files Analyzed: 30
Threat Model: 16 threats identified (6 deterministic + 10 AI-discovered)

Summary

Severity	Count
Critical	5
High	7
Medium	6
Total	18

Overall Status: REQUIRES FIXES
Risk Level: HIGH

Critical Issues (Must Fix Immediately)

1. Command Injection in Tool Call Filtering

Location: src/ai/message-handlers.ts:44-46
Issue: filterJsonToolCalls is called on AI-generated content before tool invocation. Insufficient filtering could allow malicious AI output to inject commands through tool parameters.
Impact: Remote code execution if AI output contains unescaped shell metacharacters
Fix: Use structured parsing instead of string filtering; validate cleaned output before tool execution

2. Path Traversal in Save Deliverable Tool

Location: mcp-server/src/tools/save-deliverable.ts:95
Issue: saveDeliverableFile(targetDir, filename, content) concatenates paths without validating targetDir for traversal sequences.
Impact: Arbitrary file write if targetDir is user-controlled or from untrusted config
Fix: Add path.normalize(targetDir) and check it doesn't escape expected base directory

3. Insufficient TOTP Secret Validation

Location: mcp-server/src/tools/generate-totp.ts:59-61
Issue: TOTP secret validated with regex but no minimum length check. RFC 4226 recommends ≥160 bits (≥32 base32 chars).
Impact: Weak TOTP secrets (e.g., 8 chars = 40 bits) are cryptographically broken and enable brute force
Fix: Add minimum length validation: secret.length >= 32 in validateTotpSecret

4. Secret Exposure in Error Messages

Location: src/error-handling.ts:54-55
Issue: Error context logged to console and file including error.context. Tool errors with credentials in context leak to plaintext logs.
Impact: Credential leakage through logs when tools fail with auth parameters
Fix: Sanitize sensitive fields (passwords, API keys, tokens) from context before logging

5. Prototype Pollution via YAML Parsing

Location: src/config-parser.ts:72-78
Issue: Uses yaml.load with FAILSAFE_SCHEMA but JSON Schema validation happens AFTER parsing, so malicious YAML could pollute prototypes before validation.
Impact: Prototype pollution could bypass security checks or alter application behavior
Fix: Use yaml.load(..., { json: true }) and validate immediately, or use stricter parsing options

High Priority Issues

6. Insufficient Dangerous Pattern Coverage

Location: src/config-parser.ts:29-36
Issue: DANGEROUS_PATTERNS misses $(), backticks, |, ;, &, %00, CRLF injection
Fix: Expand pattern list or use allowlist validation

7. Unvalidated Git Commit Hash Usage

Location: src/temporal/activities.ts:173-179
Issue: Git commit hash stored in audit logs without format validation
Fix: Validate with /^[0-9a-f]{40}$/ before storage

8. Error Message Information Disclosure

Location: src/error-handling.ts:44-58
Issue: Full error messages including stack traces written to error.log, exposing internal paths, API URLs, config details
Fix: Sanitize stack traces and paths; store logs outside web-accessible directories

9. TOCTOU Race Condition in Queue Validation

Location: src/queue-validation.ts:132-137
Issue: Separate file existence check and file read creates race window
Fix: Combine into single operation: try fs.readFile, catch ENOENT

10. Unbounded Error Message Accumulation

Location: src/temporal/activities.ts:18-26
Issue: ApplicationFailure.details array is unbounded; retry storms could cause memory exhaustion
Fix: Limit details array to last N errors

11. Missing Authentication in Config

Location: src/config-parser.ts:106-110
Issue: Missing auth config proceeds silently, causing false negatives in authenticated pentests
Fix: Make authentication required for authenticated target workflows

12. API Error Detection Too Broad

Location: src/ai/message-handlers.ts:73-76
Issue: Generic patterns like 'api error' trigger error detection but continue execution, masking real failures
Fix: Make detection more specific or throw non-retryable errors

Medium Priority Issues

Inconsistent Error Handling Patterns - src/temporal/activities.ts:181-192
Magic Numbers in Validation - src/config-parser.ts:53, src/temporal/activities.ts:18
Overly Complex Functional Pipeline - src/queue-validation.ts:169-178
Incomplete Type Safety in Message Handling - src/ai/message-handlers.ts:167-171
Missing Null Checks in Optional Fields - src/ai/message-handlers.ts:130-133
Lack of Input Sanitization for Display - src/ai/message-handlers.ts:151-157 (terminal injection via ANSI codes)

Recommended Action Plan

Immediate

Fix path traversal in save-deliverable.ts
Enforce TOTP secret minimum length (32 base32 chars)
Sanitize error context before logging
Fix prototype pollution in config parser
Add security tests for MCP tool inputs

Follow-up

Expand dangerous pattern detection
Fix TOCTOU race in queue validation
Validate git commit hash format
Bound error details accumulation
Add comprehensive error classification tests

🤖 Generated by Argus Security — AI-powered 6-phase security audit pipeline

Originally created by @devatsecure on GitHub (Feb 8, 2026). Original GitHub issue: https://github.com/KeygraphHQ/shannon/issues/88 ## Argus Security Audit Report **Scan Date:** 2026-02-08 **Scanner:** Argus Security (6-phase AI-powered pipeline) **Model:** claude-sonnet-4-5-20250929 **Files Analyzed:** 30 **Threat Model:** 16 threats identified (6 deterministic + 10 AI-discovered) --- ## Summary | Severity | Count | |----------|-------| | Critical | 5 | | High | 7 | | Medium | 6 | | **Total** | **18** | **Overall Status:** REQUIRES FIXES **Risk Level:** HIGH --- ## Critical Issues (Must Fix Immediately) ### 1. Command Injection in Tool Call Filtering - **Location:** `src/ai/message-handlers.ts:44-46` - **Issue:** `filterJsonToolCalls` is called on AI-generated content before tool invocation. Insufficient filtering could allow malicious AI output to inject commands through tool parameters. - **Impact:** Remote code execution if AI output contains unescaped shell metacharacters - **Fix:** Use structured parsing instead of string filtering; validate cleaned output before tool execution ### 2. Path Traversal in Save Deliverable Tool - **Location:** `mcp-server/src/tools/save-deliverable.ts:95` - **Issue:** `saveDeliverableFile(targetDir, filename, content)` concatenates paths without validating `targetDir` for traversal sequences. - **Impact:** Arbitrary file write if `targetDir` is user-controlled or from untrusted config - **Fix:** Add `path.normalize(targetDir)` and check it doesn't escape expected base directory ### 3. Insufficient TOTP Secret Validation - **Location:** `mcp-server/src/tools/generate-totp.ts:59-61` - **Issue:** TOTP secret validated with regex but no minimum length check. RFC 4226 recommends ≥160 bits (≥32 base32 chars). - **Impact:** Weak TOTP secrets (e.g., 8 chars = 40 bits) are cryptographically broken and enable brute force - **Fix:** Add minimum length validation: `secret.length >= 32` in `validateTotpSecret` ### 4. Secret Exposure in Error Messages - **Location:** `src/error-handling.ts:54-55` - **Issue:** Error context logged to console and file including `error.context`. Tool errors with credentials in context leak to plaintext logs. - **Impact:** Credential leakage through logs when tools fail with auth parameters - **Fix:** Sanitize sensitive fields (passwords, API keys, tokens) from context before logging ### 5. Prototype Pollution via YAML Parsing - **Location:** `src/config-parser.ts:72-78` - **Issue:** Uses `yaml.load` with `FAILSAFE_SCHEMA` but JSON Schema validation happens AFTER parsing, so malicious YAML could pollute prototypes before validation. - **Impact:** Prototype pollution could bypass security checks or alter application behavior - **Fix:** Use `yaml.load(..., { json: true })` and validate immediately, or use stricter parsing options --- ## High Priority Issues ### 6. Insufficient Dangerous Pattern Coverage - **Location:** `src/config-parser.ts:29-36` - **Issue:** `DANGEROUS_PATTERNS` misses `$()`, backticks, `|`, `;`, `&`, `%00`, CRLF injection - **Fix:** Expand pattern list or use allowlist validation ### 7. Unvalidated Git Commit Hash Usage - **Location:** `src/temporal/activities.ts:173-179` - **Issue:** Git commit hash stored in audit logs without format validation - **Fix:** Validate with `/^[0-9a-f]{40}$/` before storage ### 8. Error Message Information Disclosure - **Location:** `src/error-handling.ts:44-58` - **Issue:** Full error messages including stack traces written to `error.log`, exposing internal paths, API URLs, config details - **Fix:** Sanitize stack traces and paths; store logs outside web-accessible directories ### 9. TOCTOU Race Condition in Queue Validation - **Location:** `src/queue-validation.ts:132-137` - **Issue:** Separate file existence check and file read creates race window - **Fix:** Combine into single operation: try `fs.readFile`, catch ENOENT ### 10. Unbounded Error Message Accumulation - **Location:** `src/temporal/activities.ts:18-26` - **Issue:** `ApplicationFailure.details` array is unbounded; retry storms could cause memory exhaustion - **Fix:** Limit details array to last N errors ### 11. Missing Authentication in Config - **Location:** `src/config-parser.ts:106-110` - **Issue:** Missing auth config proceeds silently, causing false negatives in authenticated pentests - **Fix:** Make authentication required for authenticated target workflows ### 12. API Error Detection Too Broad - **Location:** `src/ai/message-handlers.ts:73-76` - **Issue:** Generic patterns like `'api error'` trigger error detection but continue execution, masking real failures - **Fix:** Make detection more specific or throw non-retryable errors --- ## Medium Priority Issues 13. **Inconsistent Error Handling Patterns** - `src/temporal/activities.ts:181-192` 14. **Magic Numbers in Validation** - `src/config-parser.ts:53`, `src/temporal/activities.ts:18` 15. **Overly Complex Functional Pipeline** - `src/queue-validation.ts:169-178` 16. **Incomplete Type Safety in Message Handling** - `src/ai/message-handlers.ts:167-171` 17. **Missing Null Checks in Optional Fields** - `src/ai/message-handlers.ts:130-133` 18. **Lack of Input Sanitization for Display** - `src/ai/message-handlers.ts:151-157` (terminal injection via ANSI codes) --- ## Recommended Action Plan ### Immediate 1. Fix path traversal in `save-deliverable.ts` 2. Enforce TOTP secret minimum length (32 base32 chars) 3. Sanitize error context before logging 4. Fix prototype pollution in config parser 5. Add security tests for MCP tool inputs ### Follow-up 1. Expand dangerous pattern detection 2. Fix TOCTOU race in queue validation 3. Validate git commit hash format 4. Bound error details accumulation 5. Add comprehensive error classification tests --- > 🤖 Generated by [Argus Security](https://github.com/devatsecure/Argus-Security) — AI-powered 6-phase security audit pipeline