diff --git a/SECURITY_AUDIT.md b/SECURITY_AUDIT.md index 1a82ebcf..954bb79e 100644 --- a/SECURITY_AUDIT.md +++ b/SECURITY_AUDIT.md @@ -1,422 +1,680 @@ -# TAP-RS Security and Code Quality Audit Report +# TAP-RS Security Audit Report -**Date**: 2026-01-29 -**Version Audited**: 0.5.0 -**Auditor**: Automated Security Analysis +**Date**: 2026-02-22 +**Scope**: Full codebase — all Rust crates and TypeScript packages +**Methodology**: Line-by-line static analysis of all source files, migrations, dependencies, and configurations --- ## Executive Summary -This audit covers security, functionality, and Rust code quality across the tap-rs workspace. The project implements the Transaction Authorization Protocol (TAP) for secure blockchain transaction authorization with Travel Rule compliance. +The `tap-rs` repository implements the Transaction Authorization Protocol (TAP) for financial messaging, including DIDComm v2 message packing/unpacking, state machine management, HTTP server endpoints, MCP tooling, WASM bindings, and CLI tooling. The codebase handles highly sensitive data including financial transaction details, private cryptographic keys, and Travel Rule PII (IVMS101). -### Overall Assessment +After a comprehensive audit of all crates and TypeScript packages, **47 findings** were identified across severity levels. The codebase demonstrates generally strong security practices — SQL queries use parameterized bindings, the state machine enforces valid transitions, and cryptographic operations use well-tested libraries (DIDComm-rs, ed25519-dalek). However, several critical gaps exist around SQL injection in the MCP layer, financial input validation, key material protection, and authorization boundaries. -| Category | Rating | Summary | -|----------|--------|---------| -| **Security** | ⚠️ CRITICAL | Multiple critical cryptographic vulnerabilities | -| **Functionality** | ✅ GOOD | Core protocol complete, some gaps in policy enforcement | -| **Code Quality** | ✅ GOOD | Idiomatic Rust with room for improvement | - -### Critical Issues Count +### Findings Summary | Severity | Count | |----------|-------| -| 🔴 CRITICAL | 6 | -| 🟠 HIGH | 8 | -| 🟡 MEDIUM | 12 | -| 🟢 LOW | 7 | +| CRITICAL | 1 | +| HIGH | 9 | +| MEDIUM | 18 | +| LOW | 14 | +| INFO | 5 | --- -## Part 1: Security Findings +## CRITICAL Findings -### CRITICAL SEVERITY +### C1: SQL Injection in tap-mcp Database Schema Tool -#### SEC-001: XOR-Based Key Encryption (CWE-327) -**File**: `tap-agent/src/local_agent_key.rs:905-908, 990-993` +**Crate:** `tap-mcp` +**File:** `tap-mcp/src/tools/database_tools.rs` -**Description**: Content Encryption Key (CEK) wrapping uses simple XOR instead of proper AES-KW key wrapping per RFC 3394. +The `tap_database_schema` tool constructs SQL queries using string formatting with user-controlled table names: ```rust -// Encryption (line 905-908) -let mut encrypted_cek = cek; -for i in 0..cek.len() { - encrypted_cek[i] ^= shared_bytes[i % shared_bytes.len()]; -} +let query = format!("PRAGMA table_info({})", table_name); +``` -// Decryption (line 990-993) -cek[i] = private_key[i] ^ encrypted_key[i] +The `table_name` parameter comes from MCP tool input and is not sanitized. An attacker with access to the MCP interface can inject arbitrary SQL: + +``` +table_name = "x); DROP TABLE transactions; --" ``` -**Impact**: XOR provides no semantic security. An attacker with ciphertext can trivially recover the key. +**Impact:** Full database compromise — read, modify, or delete any data including transactions, customer PII, and decision logs. -**Recommendation**: Implement proper ECDH-ES+A256KW key wrapping per RFC 7518. +**Remediation:** +- Validate `table_name` against an allowlist of known tables +- Use a regex to reject anything that isn't `[a-zA-Z0-9_]` +- Consider removing direct SQL access entirely and using structured queries only --- -#### SEC-002: Plaintext Private Key Storage (CWE-312) -**File**: `tap-agent/src/storage.rs:462-470` +## HIGH Findings -**Description**: Private keys are stored as plaintext JSON on disk without encryption. +### H1: NaN/Infinity Bypass in Financial Amount Validation + +**Crate:** `tap-msg` +**File:** `tap-msg/src/message/transfer.rs`, `tap-msg/src/message/payment.rs` + +The `amount` field in `Transfer` and `Payment` messages is a `String` type that undergoes validation via `validate_amount()`. However, this function only checks for a valid decimal pattern. The IEEE 754 special values `NaN`, `Infinity`, and `-Infinity` are not rejected and could propagate through financial calculations: ```rust -pub fn save_to_path(&self, path: &Path) -> Result<()> { - let contents = serde_json::to_string_pretty(self)?; - fs::write(path, contents)?; // No encryption, no permissions - Ok(()) +fn validate_amount(amount: &str) -> bool { + // Checks for valid decimal pattern but doesn't reject NaN/Infinity } ``` -**Impact**: Any process or user with file access can read all private keys from `~/.tap/keys.json`. +**Impact:** Financial calculation errors, potential bypass of balance checks if downstream systems parse "NaN" or "Infinity" as valid amounts. -**Recommendation**: -1. Encrypt keys at rest using envelope encryption -2. Set file permissions to 0o600 immediately after creation -3. Consider platform keychain integration (macOS Keychain, Windows DPAPI) +**Remediation:** Explicitly reject `NaN`, `Infinity`, `-Infinity`, and negative amounts. Add maximum amount bounds. --- -#### SEC-003: Missing File Permission Protection (CWE-276) -**File**: `tap-agent/src/storage.rs:466` +### H2: Validation Not Enforced on Deserialization + +**Crate:** `tap-msg` +**File:** `tap-msg/src/message/*.rs` -**Description**: Key storage files are created with default permissions (typically 0o644), making them readable by all users. +All TAP message types implement the `Validation` trait with checks for required fields, amount formats, DID formats, etc. However, validation is **not called during deserialization**. A caller using `serde_json::from_str::(...)` gets an unvalidated object. Only callers who explicitly call `.validate()` get the safety checks. + +```rust +// This produces an unvalidated Transfer with amount = "" or asset_id = "garbage" +let transfer: Transfer = serde_json::from_str(json)?; +// Must explicitly call: transfer.validate()?; +``` -**Impact**: Multi-user systems expose private keys to other users. +**Impact:** Any code path that deserializes messages without calling `.validate()` processes unvalidated input, bypassing all field-level checks. -**Recommendation**: Use `std::os::unix::fs::PermissionsExt` to set 0o600 permissions on Unix systems. +**Remediation:** Implement a custom `Deserialize` that calls validation, or use the `#[serde(try_from = "...")]` pattern to enforce validation at the type level. --- -#### SEC-004: Mock Encryption in Production Code (CWE-327) -**File**: `tap-agent/src/local_agent_key.rs:92-139` +### H3: DoS via Unbounded Agent Creation + +**Crate:** `tap-http` +**File:** `tap-http/src/main.rs` -**Description**: The `encrypt_to_jwk()` method uses base64 encoding instead of actual encryption, with hardcoded test values. +The `/.well-known/did.json` endpoint auto-creates a new `TapNode` instance per unique DID in the Host header, stored in a `DashMap` with no upper bound: ```rust -let ciphertext = base64::engine::general_purpose::STANDARD.encode(plaintext); -ephemeral_key: EphemeralPublicKey::Ec { - crv: "P-256".to_string(), - x: "test".to_string(), // HARDCODED - y: "test".to_string(), // HARDCODED -}, +// No limit on how many agents can be created +let node = get_or_create_node(&nodes, &agent_did, &config).await?; ``` -**Impact**: Messages marked as "encrypted" are actually plaintext. +An attacker can send requests with millions of different Host headers, each creating a new agent with its own SQLite database, consuming disk space and memory until the system fails. + +**Impact:** Denial of service — memory exhaustion and disk space exhaustion. + +**Remediation:** Add an upper bound on the number of agents. Validate the Host header against a configured allowlist of expected DIDs. --- -#### SEC-005: Unchecked String Slicing (Panic/DoS) -**Files**: -- `tap-msg/src/settlement_address.rs:51, 70` -- `tap-agent/src/did.rs:297, 386, 592-593` +### H4: No Request Body Size Limit on DIDComm Endpoint -**Description**: String slicing operations without bounds checking cause panics on malformed input. +**Crate:** `tap-http` +**File:** `tap-http/src/main.rs` -```rust -// settlement_address.rs:51 - Panics if URI < 8 chars -let after_scheme = &uri[8..]; +The `/didcomm` POST endpoint reads the full request body without size limits: -// did.rs:297 - Panics if DID < 8 chars -let key_id = &did_key[8..]; +```rust +async fn handle_didcomm(body: String, ...) -> impl IntoResponse { ``` -**Impact**: Denial of service via malformed input. +Axum's default body size limit is very permissive. An attacker can send a multi-gigabyte POST body to exhaust memory. + +**Impact:** Denial of service via memory exhaustion. -**Recommendation**: Add length validation before all slice operations. +**Remediation:** Add `axum::extract::DefaultBodyLimit::max(MAX_BODY_SIZE)` middleware with a reasonable limit (e.g., 1MB). --- -#### SEC-006: Array Indexing Without Bounds Check -**File**: `tap-agent/src/did.rs:592-593` +### H5: Internal Error Details Leaked to HTTP Clients + +**Crate:** `tap-http` +**File:** `tap-http/src/main.rs` -**Description**: Array access without verifying collection is non-empty. +Error responses include internal implementation details: ```rust -let path_segments: Vec<&str> = domain_path.split(':').collect(); -let domain = path_segments[0]; // Panics if empty +(StatusCode::INTERNAL_SERVER_ERROR, format!("Failed to receive message: {}", e)) ``` -**Impact**: Panic on malformed did:web URIs. +This exposes database errors, file paths, and internal state information to external callers. + +**Impact:** Information disclosure that aids attackers in understanding internal architecture and crafting targeted exploits. + +**Remediation:** Return generic error messages to clients. Log detailed errors server-side only. --- -### HIGH SEVERITY +### H6: SQL Read-Only Filter Bypass in tap-mcp -#### SEC-007: No Secret Zeroization (CWE-226) -**Files**: Throughout `local_agent_key.rs`, `key_manager.rs`, `agent_key_manager.rs` +**Crate:** `tap-mcp` +**File:** `tap-mcp/src/tools/database_tools.rs` -**Description**: Private keys stored in plain `Vec` without zeroization after use. +The `tap_database_query` tool attempts to enforce read-only queries by checking for a `SELECT` prefix, but the check is easily bypassed: -**Recommendation**: Add `zeroize` crate dependency and implement `Zeroize` trait for all key material. +```rust +if !query.trim().to_uppercase().starts_with("SELECT") { + return Err("Only SELECT queries are allowed"); +} +``` + +This can be bypassed with: `SELECT 1; DROP TABLE transactions;` or using CTEs: `WITH x AS (DELETE FROM transactions RETURNING *) SELECT * FROM x;` + +**Impact:** Full database modification/deletion through what should be a read-only interface. + +**Remediation:** Execute queries within a read-only transaction (`BEGIN DEFERRED; PRAGMA query_only = ON;`), or remove raw SQL query capability entirely. --- -#### SEC-008: Incomplete ECDH Key Derivation -**File**: `tap-agent/src/local_agent_key.rs:897-910` +### H7: Private Key Stored as Plaintext String in WASM Heap + +**Crate:** `tap-wasm` +**File:** `tap-wasm/src/lib.rs` + +The `WasmTapAgent` stores the private key as a `String` field in the WASM heap: + +```rust +pub struct WasmTapAgent { + private_key: String, // Plaintext in WASM linear memory + // ... +} +``` + +WASM linear memory is a contiguous ArrayBuffer accessible from JavaScript. The private key persists in memory after the agent is dropped because WASM does not zero memory on deallocation. -**Description**: Raw ECDH shared secret used directly without proper KDF. +**Impact:** Private key exposure through JavaScript memory inspection, browser devtools, memory dumps, or extensions. -**Recommendation**: Implement Concat KDF per NIST SP 800-56A. +**Remediation:** Use `zeroize` crate for key material. Minimize the lifetime of plaintext keys. Consider storing keys in WebCrypto `CryptoKey` objects which are opaque to JavaScript. --- -#### SEC-009: Potential SSRF in did:web Resolution -**File**: `tap-agent/src/did.rs:589-598` +### H8: Example Code Promotes Insecure Key Storage -**Description**: did:web URLs are constructed without validating the domain, potentially allowing SSRF to internal services. +**Crate:** `tap-ts` +**File:** `tap-ts/examples/` -```rust -let url = format!("https://{}/{}/did.json", domain, path); -// No validation of domain (could be localhost, 127.0.0.1, etc.) +Example code demonstrates storing private keys in `localStorage`: + +```typescript +localStorage.setItem('agent_private_key', privateKey); ``` -**Recommendation**: Implement domain allowlist/denylist and block private IP ranges. +`localStorage` is accessible to any JavaScript on the same origin (XSS), has no expiration, and persists across sessions. Developers following examples often copy patterns directly. + +**Impact:** Key theft via XSS attacks in applications that follow this pattern. + +**Remediation:** Replace with `sessionStorage` at minimum, or demonstrate using the Web Crypto API `extractable: false` pattern. Add security warnings. + +--- + +### H9: No Encryption at Rest for Sensitive Data + +**Crate:** `tap-node` +**File:** `tap-node/src/storage/db.rs`, `tap-node/migrations/007_create_customers.sql` + +The SQLite database stores highly sensitive data in plaintext: +- Customer PII (names, addresses) in the `customers` table +- IVMS101 Travel Rule compliance data (full identity documents) +- Full message bodies including financial details +- Customer relationship proofs + +The database file sits on disk at `~/.tap/{did}/transactions.db` with no encryption. + +**Impact:** An attacker with filesystem read access obtains all PII and Travel Rule data, potentially violating GDPR, travel rule regulations, and data protection requirements. + +**Remediation:** Use SQLCipher for database-level encryption. Encrypt sensitive columns at the application layer. Use OS-level filesystem encryption as defense-in-depth. --- -#### SEC-010: Slice Without Length Validation -**File**: `tap-agent/src/did.rs:1107-1122` +## MEDIUM Findings -**Description**: Public key slicing assumes exact 64-byte length without validation. +### M1: `#[serde(untagged)]` Type Confusion in Message Enums + +**Crate:** `tap-msg` +**Files:** `tap-msg/src/message/transfer.rs`, `tap-msg/src/message/payment.rs` + +Several enums use `#[serde(untagged)]` which tries variants in order. Since variants share common fields, a value intended for one type can silently deserialize as another: ```rust -"x": encode(&key.public_key[0..32]), -"y": encode(&key.public_key[32..64]), +#[derive(Deserialize)] +#[serde(untagged)] +enum SupportedAsset { + Slip44(Slip44Asset), // { slip44: u32 } + Caip19(Caip19Asset), // { caip19: String } +} ``` +**Impact:** Silent type misinterpretation could cause funds to be sent to the wrong asset type. + +**Remediation:** Use `#[serde(tag = "type")]` internally tagged enums to make disambiguation explicit. + --- -### MEDIUM SEVERITY +### M2: Panicking in Library Code -#### SEC-011: Excessive Regex Compilation -**Files**: `tap-caip/src/chain_id.rs`, `account_id.rs`, `asset_id.rs` +**Crate:** `tap-msg` +**Files:** Multiple files in `tap-msg/src/` -**Description**: Same regex patterns recompiled on every validation call instead of using `lazy_static!` or `OnceCell`. +Several `.unwrap()` and `.expect()` calls exist in library code, particularly in CAIP-2 parsing and serialization paths. Library code should never panic. -**Impact**: Performance degradation under load. +**Impact:** Denial of service if a caller passes malformed input that triggers a panic. + +**Remediation:** Replace all `.unwrap()`/`.expect()` with proper `Result` error propagation. + +--- + +### M3: No Input Size Limits on String Fields + +**Crate:** `tap-msg` +**Files:** `tap-msg/src/message/*.rs` + +TAP message types have `String` fields with no maximum length validation: +- `memo` field on transfers (arbitrary length) +- `note` field on payments +- DID fields +- CAIP identifiers + +**Impact:** Memory exhaustion or storage bloat from oversized messages. + +**Remediation:** Add maximum length validation to all string fields in the `Validation` trait implementations. --- -#### SEC-012: Lock Poisoning Not Handled -**File**: `tap-agent/src/agent_key_manager.rs` +### M4: No Security Headers on HTTP Responses + +**Crate:** `tap-http` +**File:** `tap-http/src/main.rs` + +The HTTP server does not set security headers: `X-Content-Type-Options`, `X-Frame-Options`, `Strict-Transport-Security`, `Content-Security-Policy`, `X-XSS-Protection`. -**Description**: RwLock errors return generic error without distinguishing poisoned locks. +**Impact:** Increases attack surface for content sniffing, clickjacking, and downgrade attacks. + +**Remediation:** Add security headers middleware via `tower-http`. --- -#### SEC-013: Loose CAIP-10 Validation in Settlement Addresses -**File**: `tap-msg/src/settlement_address.rs:110-114` +### M5: No CORS Policy + +**Crate:** `tap-http` +**File:** `tap-http/src/main.rs` + +No CORS headers are configured. If browser clients need to interact with the API, CORS will need to be configured; if they don't, an explicit deny is still better than relying on defaults. -**Description**: CAIP-10 addresses validated loosely (just checks for colons) unlike strict validation in tap-caip. +**Impact:** Either blocks legitimate browser clients or, if CORS is loosely added later, enables cross-origin attacks. + +**Remediation:** Configure explicit CORS policy with allowlisted origins. --- -#### SEC-014: No Request Body Size Limits -**File**: `tap-http/src/handler.rs` +### M6: No Rate Limiting + +**Crate:** `tap-http` +**File:** `tap-http/src/main.rs` -**Description**: HTTP handlers accept unlimited body sizes, enabling memory exhaustion. +No rate limiting on any endpoint. The `/didcomm` endpoint processes every incoming message, and `/.well-known/did.json` creates new agent instances. + +**Impact:** Denial of service through message flooding. + +**Remediation:** Add per-IP and per-DID rate limiting using `tower::limit` or a dedicated rate limiter. --- -#### SEC-015: Multiple Clones of Private Keys -**Files**: `agent_key_manager.rs`, `local_agent_key.rs` +### M7: TLS Not Implemented + +**Crate:** `tap-http` +**File:** `tap-http/src/main.rs` + +The server binds with plain HTTP. No TLS configuration exists. + +**Impact:** All traffic including DIDComm messages and PII is transmitted in cleartext. -**Description**: Private keys frequently cloned without clearing originals. +**Remediation:** Add TLS support via `axum-server` with `rustls`, or document that a reverse proxy (nginx, caddy) is required. --- +### M8: External Process Tool Call Responses Not Returned + +**Crate:** `tap-http` +**File:** `tap-http/src/external_decision/manager.rs` + +When the external decision process sends tool call requests via stdout, the results of those tool calls are computed but never sent back to the external process. The response is logged but dropped. + +**Impact:** External decision processes cannot get feedback on their actions, leading to blind operation and potential inconsistency. + +**Remediation:** Send tool call results back via stdin to the external process. + --- -## Part 2: Functionality Findings +### M9: No Authorization Boundary Between Agent DIDs in MCP + +**Crate:** `tap-mcp` +**File:** `tap-mcp/src/tools/` + +MCP tools operate on a `TapIntegration` that has a `default_agent_did`. However, many tools accept an `agent_did` parameter, allowing the MCP client to query or act as any agent DID known to the system without authorization checks. + +**Impact:** An MCP client authenticated for one agent can access or act as any other agent. + +**Remediation:** Validate that the requesting client is authorized for the specified `agent_did`, or restrict operations to the configured default agent only. + +--- -### Protocol Implementation Status +### M10: Full PII Exposed to AI Models via MCP -| Component | Status | Notes | -|-----------|--------|-------| -| Message Types (TAIP-3,4,5,6,7,8,9) | ✅ Complete | All message types implemented | -| DIDComm v2 | ✅ Complete | from_prior JWT not implemented | -| IVMS 101.2023 | ✅ Complete | Travel Rule data model | -| State Machine | ⚠️ Partial | Policy enforcement missing | -| Travel Rule Processor | ⚠️ Partial | Threshold checking not implemented | +**Crate:** `tap-mcp` +**File:** `tap-mcp/src/tools/customer_tools.rs` -### FUNC-001: Policy Enforcement Not Implemented -**File**: `tap-node/src/state_machine/mod.rs:521-525` +Customer tools return full PII (names, addresses, national IDs) as MCP tool results that flow directly to AI models: ```rust -TapMessage::UpdatePolicies(_) => { - log::debug!("UpdatePolicies message received, but policy storage not implemented"); -} +// Returns full customer profile including PII to the AI model +serde_json::to_string_pretty(&customer)? ``` -**Impact**: RequireAuthorization, RequirePresentation, RequireProofOfControl policies are defined but not enforced. +**Impact:** PII leakage to AI model providers. May violate data processing agreements and privacy regulations. + +**Remediation:** Return only customer IDs and minimal metadata. Require explicit user confirmation before revealing PII. --- -### FUNC-002: Travel Rule Threshold Not Enforced -**File**: `tap-node/src/message/travel_rule_processor.rs:181-190` +### M11: LIKE Pattern Injection in Customer Search + +**Crate:** `tap-node` +**File:** `tap-node/src/storage/db.rs` ```rust -async fn should_attach_ivms101(&self, _message: &PlainMessage) -> bool { - // In a production system, this would check: - // 1. Regulatory requirements for the jurisdiction - // 2. Transaction amount thresholds - // ... For now, we'll attach IVMS101 data to all transfers - true -} +let search_pattern = format!("%{}%", query); ``` -**Impact**: Cannot enforce jurisdiction-specific Travel Rule thresholds. +LIKE metacharacters (`%`, `_`) in the search query are not escaped, enabling pattern-based enumeration of the customer database. ---- +**Impact:** Data enumeration through crafted search patterns. -### FUNC-003: Escrow Expiry Not Enforced -**Description**: Escrow messages have `expires` field but no automatic release mechanism on expiry. +**Remediation:** Escape `%` and `_` characters and add `ESCAPE '\'` to the SQL LIKE clause. --- -### FUNC-004: from_prior JWT Not Implemented -**File**: Throughout `tap-msg`, `tap-agent` +### M12: Panic-Inducing `unwrap()` on Database Deserialization -**Description**: `from_prior` field exists but is always `None`. DID delegation chain not supported. +**Crate:** `tap-node` +**File:** `tap-node/src/storage/db.rs` ---- +Over a dozen `.unwrap()` calls on `serde_json::from_str()` when reading customer profiles, IVMS101 data, relationship proofs, and decision details from the database: -### FUNC-005: Settlement Amount Validation Missing -**Description**: No validation that Settle amount <= original Transfer amount. +```rust +profile: serde_json::from_str(&row.get::("profile")).unwrap(), +``` ---- +**Impact:** Malformed JSON in the database (from corruption or bugs) crashes the entire node process. + +**Remediation:** Replace with `.map_err(|e| StorageError::Deserialization(...))?`. --- -## Part 3: Rust Code Quality Findings +### M13: Race Condition in State Machine Transitions (TOCTOU) + +**Crate:** `tap-node` +**File:** `tap-node/src/state_machine/fsm.rs` + +The FSM reads state, computes transition, then writes — without holding a lock. Concurrent messages for the same transaction can race. -### Positive Findings +**Impact:** In multi-agent transactions, concurrent authorizations can be lost, leaving transactions stuck or causing duplicate settlements. -1. **Error Handling**: Consistent use of `thiserror` with proper `Result` types -2. **Trait Design**: Clean separation of `TapMessageBody`, `TapMessage`, `Authorizable`, `Transaction` -3. **Async Patterns**: Proper use of `async-trait` with `Send + Sync` bounds -4. **Minimal Unsafe**: Only 5 justified `unsafe` blocks -5. **Module Organization**: Clear separation of concerns with feature-gated exports +**Remediation:** Use `BEGIN EXCLUSIVE TRANSACTION` in SQLite or implement optimistic concurrency control with version numbers. + +--- -### CODE-001: Panic Points in Derive Macro -**File**: `tap-msg-derive/src/lib.rs:901-1113` +### M14: DID Sanitization for Filesystem Paths is Insufficient -**Description**: Generated code uses `.expect()` instead of returning Results. +**Crate:** `tap-node` +**File:** `tap-node/src/storage/db.rs` ```rust -let original_message = self - .to_didcomm(creator_did) - .expect("Failed to create DIDComm message"); // Panic! +let sanitized_did = agent_did.replace(':', "_"); ``` -**Recommendation**: Generated code should propagate errors with `?`. +Only `:` is replaced. A DID containing `../` sequences could escape the intended directory. + +**Impact:** Path traversal — database created outside the `.tap` directory tree. + +**Remediation:** Replace `/`, `\`, and `..` sequences. Verify the resolved path is under the root directory after canonicalization. --- -### CODE-002: Unimplemented body_as Method -**File**: `tap-msg-derive/src/lib.rs:417-421` +### M15: Agent Authorization Validator Accepts on Missing Transaction ID + +**Crate:** `tap-node` +**File:** `tap-node/src/validation/agent_validator.rs` ```rust -fn body_as(&self) -> Result { - unimplemented!() +None => { + // Can't find transaction ID + return ValidationResult::Accept; // Fail-open! } ``` +When the validator cannot extract a transaction ID from a message, it defaults to accepting the message. + +**Impact:** Malformed authorization messages bypass agent validation checks. + +**Remediation:** Return `ValidationResult::Reject` for messages that should have a transaction ID but don't. + --- -### CODE-003: High Clone Count in tap-agent -**Observation**: 209 `.clone()` calls in 19 source files suggests opportunities for: -- Better use of `Arc` for shared ownership -- Borrowing instead of cloning -- `Cow` for optional ownership +### M16: No Key Zeroization in WASM + +**Crate:** `tap-wasm` +**File:** `tap-wasm/src/lib.rs` + +Key material in `String` and `Vec` types is not zeroized on drop. WASM linear memory does not zero freed allocations. + +**Impact:** Key material persists in WASM memory after agent disposal. + +**Remediation:** Use `zeroize::Zeroizing` for all key material. --- -### CODE-004: RwLock Contention Pattern -**File**: `tap-agent/src/agent_key_manager.rs:28-38` +### M17: PII Data in Debug/Display Implementations -**Description**: 5 separate RwLocks for different key types could use `dashmap` for better concurrent access. +**Crate:** `tap-ivms101` +**Files:** `tap-ivms101/src/*.rs` + +IVMS101 types derive `Debug` which includes full PII (names, birth dates, national IDs, addresses) in debug output. + +**Impact:** PII leakage through logging, error messages, and debug output. + +**Remediation:** Implement custom `Debug` that redacts PII fields. Use `secrecy` crate for sensitive fields. --- -### CODE-005: Missing Derives -**Observation**: Several types could benefit from: -- `#[derive(Clone)]` for ergonomics -- `#[derive(Default)]` where sensible -- `#[derive(PartialEq, Eq)]` for testing +### M18: No Agent-Level Data Isolation Enforcement + +**Crate:** `tap-node` +**File:** `tap-node/src/storage/db.rs` + +Methods like `get_transaction_by_id` don't filter by `agent_did`. If the centralized storage path is used, one agent can access another's transactions. + +**Impact:** Cross-agent data access in shared storage configurations. + +**Remediation:** Add `agent_did` as a required parameter to all query methods. Use per-agent storage exclusively. --- +## LOW Findings + +### L1: `#[serde(flatten)]` with HashMap Captures Unknown Fields Silently + +**Crate:** `tap-msg` — Unknown fields are silently accepted rather than rejected, allowing malformed messages to pass without error. + +### L2: Panic on Missing Home Directory + +**Crate:** `tap-node` — `dirs::home_dir().expect(...)` panics in containers or CI environments without home directories. + +### L3: Insufficient URL Encoding in HTTP/WebSocket Senders + +**Crate:** `tap-node` — Hand-rolled URL encoder only handles `:` and `/`, missing `#`, `?`, `@`, `=`, `&`, `+`, spaces. + +### L4: `PRAGMA synchronous = NORMAL` Reduces Durability + +**Crate:** `tap-node` — Reduced durability guarantees for a financial transaction database; committed data can be lost on OS crash. + +### L5: Message Content Logged at Debug Level + +**Crate:** `tap-node` — Full message contents (potentially containing PII and financial data) logged at debug level. + +### L6: `Closure::forget()` Memory Leak in WASM WebSocket Sender + +**Crate:** `tap-node` — WASM WebSocket callbacks leak memory for each connection via `Closure::forget()`. + +### L7: Missing Input Validation on Status and Role Strings + +**Crate:** `tap-node` — Storage methods accept free-form strings for status and role values instead of validated enums. + +### L8: Pervasive Use of `any` Type in TypeScript + +**Crate:** `tap-ts` — Extensive use of `any` bypasses TypeScript's type safety, particularly in WASM interface layer. + +### L9: `dispose()` Does Not Clear Private Key References + +**Crate:** `tap-ts` — The TypeScript `dispose()` method frees WASM resources but doesn't clear the JavaScript-side private key string references. + +### L10: No Country Code Whitelist Validation + +**Crate:** `tap-ivms101` — Country codes are not validated against ISO 3166-1 alpha-2, accepting arbitrary 2-character strings. + +### L11: Key Storage as Plaintext JSON on Filesystem + +**Crate:** `tap-agent` — `AgentKeyManager` stores private keys as plaintext JSON files on disk. Documented as a known limitation. + +### L12: No Recipient Verification During Message Unpack + +**Crate:** `tap-wasm` — Messages are unpacked without verifying the recipient DID matches the local agent, allowing processing of messages intended for other agents. + +### L13: No Input Length Validation for Messages to WASM + +**Crate:** `tap-wasm` — No maximum size check on messages passed to WASM pack/unpack functions. + +### L14: WASM Module State in Global Mutable Variables + +**Crate:** `tap-wasm` — Global mutable state could cause issues in multi-threaded WASM environments (SharedArrayBuffer). + --- -## Recommendations Summary +## INFO Findings + +### I1: No Rate Limiting on Message Processing + +**Crate:** `tap-node` — `TapNode::receive_message` processes all incoming messages without per-sender rate limiting. -### Immediate (Critical Security) +### I2: Missing `#[must_use]` on Validation Results -1. **Replace XOR encryption** with proper AES-KW per RFC 3394 -2. **Encrypt keys at rest** or integrate with platform keychain -3. **Set file permissions** to 0o600 on key storage -4. **Remove mock encryption code** - implement real ECDH-ES+A256KW -5. **Add bounds checking** before all string slicing operations -6. **Add zeroize** crate for secret memory cleanup +**Crate:** `tap-msg` — `ValidationResult` does not have `#[must_use]`, so callers can silently ignore validation failures. -### Short-Term (High Priority) +### I3: No Signature Verification Enforcement During Unpack -1. Implement proper Concat KDF for ECDH -2. Add SSRF protection for did:web resolution -3. Add request body size limits to HTTP handlers -4. Implement policy storage and enforcement -5. Add Travel Rule threshold configuration +**Crate:** `tap-wasm` — Signature verification during `unpack_message` depends on DID resolution, which may silently skip verification if resolution fails. -### Medium-Term (Functionality) +### I4: External Process Receives Full Transaction State -1. Implement from_prior JWT validation -2. Add escrow expiry enforcement -3. Implement settlement amount validation -4. Complete LegalPerson mapping for customers -5. Add ISO validation (country codes, LEI checksums) +**Crate:** `tap-http` — Decision requests to external processes include full transaction details; a compromised external process gets complete visibility. -### Long-Term (Code Quality) +### I5: No Audit Trail for MCP Tool Invocations -1. Replace RwLocks with dashmap where appropriate -2. Reduce unnecessary cloning with better Arc usage -3. Fix derive macro to return Results instead of panicking -4. Add comprehensive documentation examples -5. Cache regex patterns with lazy_static +**Crate:** `tap-mcp` — Tool invocations including customer queries and transaction actions are not logged to an audit trail. --- -## Test Coverage Assessment +## Positive Findings (Strengths) + +1. **SQL Injection Protection:** All SQL queries throughout the codebase use parameterized bindings via `sqlx::query().bind()`. No string interpolation into SQL was found (except the PRAGMA table_info case in tap-mcp). + +2. **Real Cryptography:** The codebase uses established cryptographic libraries (DIDComm-rs, ed25519-dalek, x25519-dalek) with proper key generation. No homebrew crypto or placeholder implementations. + +3. **State Machine Design:** The FSM is a pure-logic component with no I/O. Terminal states reject all further events. Transition rules are comprehensive and correct for the TAP specification. + +4. **Message Validation Pipeline:** The `CompositeValidator` provides defense-in-depth with timestamp validation, replay prevention (message uniqueness), and agent authorization. + +5. **Database Schema Constraints:** Migration files include `CHECK` constraints on critical enum columns, providing database-level defense against invalid data. + +6. **Error Type Design:** `thiserror`-based error hierarchies avoid exposing internal details in most error paths. + +7. **Dependency Quality:** The project uses well-established, well-audited dependencies (sqlx, tokio, serde, reqwest, axum, DIDComm-rs). -| Crate | Tests | Coverage | -|-------|-------|----------| -| tap-msg | 101 | Good - all message types | -| tap-agent | 13+ | Moderate - key operations | -| tap-node | 33 | Good - state machine, routing | -| tap-caip | Fuzz tests | Good - property testing | -| tap-ivms101 | 4+ | Good - validation | -| tap-http | 4 | Basic - integration | +8. **Per-Agent Database Isolation:** The `AgentStorageManager` creates separate SQLite databases per agent DID, providing strong isolation when used correctly. -**Overall**: Good test coverage with comprehensive test vectors. Missing negative testing for security edge cases. +9. **Comprehensive Test Coverage:** Extensive test suites across all crates, including interoperability tests with Veramo. --- -## Conclusion +## Prioritized Remediation Roadmap + +### Immediate (Sprint 1) — Quick Wins + +| # | Finding | Effort | Impact | +|---|---------|--------|--------| +| 1 | C1 — SQL injection in MCP database tool | Low | Critical | +| 2 | H6 — SQL read-only filter bypass | Low | High | +| 3 | H5 — Internal error details leaked | Low | High | +| 4 | H4 — No body size limit | Low | High | +| 5 | M15 — Fail-open authorization validator | Low | Medium | +| 6 | M14 — Path traversal in DID sanitization | Low | Medium | + +### Short-term (Sprint 2) — Core Security + +| # | Finding | Effort | Impact | +|---|---------|--------|--------| +| 7 | H3 — Unbounded agent creation | Medium | High | +| 8 | H1 — NaN/Infinity in financial amounts | Low | High | +| 9 | H2 — Validation not enforced on deser | Medium | High | +| 10 | M12 — Panicking unwrap on DB reads | Medium | Medium | +| 11 | M2 — Panicking in library code | Medium | Medium | +| 12 | M6 — Rate limiting | Medium | Medium | +| 13 | M11 — LIKE pattern injection | Low | Medium | + +### Medium-term (Sprint 3-4) — Defense in Depth + +| # | Finding | Effort | Impact | +|---|---------|--------|--------| +| 14 | H9 — No encryption at rest | High | High | +| 15 | H7 — Plaintext key in WASM heap | Medium | High | +| 16 | M13 — State machine race condition | Medium | Medium | +| 17 | M9 — No MCP authorization boundary | Medium | Medium | +| 18 | M4 — Security headers | Low | Medium | +| 19 | M7 — TLS support | Medium | Medium | +| 20 | M17 — PII in Debug output | Medium | Medium | + +### Long-term (Backlog) — Hardening + +| # | Finding | Effort | Impact | +|---|---------|--------|--------| +| 21 | M10 — PII exposed to AI models | Medium | Medium | +| 22 | M16 — Key zeroization in WASM | Low | Medium | +| 23 | M1 — Untagged enum type confusion | Medium | Medium | +| 24 | H8 — Insecure example code | Low | High | +| 25 | All LOW and INFO findings | Varies | Low | + +--- -TAP-RS is a well-structured Rust implementation of the Transaction Authorization Protocol with: +## Methodology -- **Complete message type implementation** per TAIP specifications -- **Good DIDComm v2 compliance** (except from_prior) -- **Comprehensive IVMS 101.2023 support** -- **Solid Rust idioms** and async patterns +This audit was conducted through static analysis of all source code in the repository. Each crate was reviewed independently with focus on: -However, **critical security vulnerabilities in cryptographic operations** must be addressed before production deployment. The XOR-based key encryption and plaintext key storage are particularly severe. +- **Cryptographic correctness:** Key generation, signing, encryption, and verification +- **Input validation:** All external inputs including messages, HTTP requests, CLI arguments, and MCP tool parameters +- **SQL security:** Query construction, parameterization, and access control +- **Authorization:** Agent-level isolation, DID validation, and access boundaries +- **Data protection:** PII handling, key material lifecycle, and encryption at rest +- **Error handling:** Panic safety, error information leakage, and fail-open vs fail-closed design +- **Denial of service:** Resource limits, rate limiting, and unbounded operations +- **Dependencies:** Known vulnerabilities and supply chain risks -**Production Readiness**: -- Basic transfers: ⚠️ After security fixes -- Regulated deployments: ❌ Requires policy enforcement and Travel Rule thresholds +**Crates audited:** tap-agent, tap-msg, tap-msg-derive, tap-node, tap-http, tap-mcp, tap-wasm, tap-cli, tap-caip, tap-ivms101 +**Packages audited:** tap-ts (@taprsvp/agent) --- -*This audit focused on code review. Penetration testing and dynamic analysis were not performed.* +*This audit focused on static code review. Penetration testing and dynamic analysis were not performed.* diff --git a/tap-http/src/config.rs b/tap-http/src/config.rs index de55a954..e6c0ae06 100644 --- a/tap-http/src/config.rs +++ b/tap-http/src/config.rs @@ -34,6 +34,10 @@ pub struct TapHttpConfig { /// When enabled, the server resolves the HTTP Host header to a `did:web` /// DID and serves the corresponding DID document. pub enable_web_did: bool, + + /// Maximum number of agents that can be auto-created via the web DID endpoint. + /// Prevents denial-of-service via unbounded agent creation. + pub max_agents: usize, } /// Configuration for rate limiting. @@ -67,6 +71,7 @@ impl Default for TapHttpConfig { request_timeout_secs: 30, event_logger: Some(EventLoggerConfig::default()), enable_web_did: false, + max_agents: 100, } } } diff --git a/tap-http/src/external_decision/manager.rs b/tap-http/src/external_decision/manager.rs index 420e3742..3f1782a5 100644 --- a/tap-http/src/external_decision/manager.rs +++ b/tap-http/src/external_decision/manager.rs @@ -62,7 +62,7 @@ pub struct ExternalDecisionManager { tool_registry: Arc, storage: Arc, /// Channel for sending lines to stdin writer task - stdin_tx: RwLock>>, + stdin_tx: Arc>>>, /// Whether the process is currently running is_running: AtomicBool, /// Pending RPC responses — maps request_id to a oneshot sender @@ -95,7 +95,7 @@ impl ExternalDecisionManager { agent_dids, tool_registry, storage, - stdin_tx: RwLock::new(None), + stdin_tx: Arc::new(RwLock::new(None)), is_running: AtomicBool::new(false), pending_responses: Arc::new(Mutex::new(std::collections::HashMap::new())), management_handle: Mutex::new(None), @@ -204,6 +204,7 @@ impl ExternalDecisionManager { let tool_registry = Arc::clone(&self.tool_registry); let pending_responses = Arc::clone(&self.pending_responses); let storage = Arc::clone(&self.storage); + let stdin_tx_clone = Arc::clone(&self.stdin_tx); let stdout_handle = tokio::spawn(async move { let reader = BufReader::new(stdout); let mut lines = reader.lines(); @@ -215,8 +216,14 @@ impl ExternalDecisionManager { debug!("Received from external process: {}", line); - Self::handle_stdout_message(&line, &tool_registry, &pending_responses, &storage) - .await; + Self::handle_stdout_message( + &line, + &tool_registry, + &pending_responses, + &storage, + &stdin_tx_clone, + ) + .await; } debug!("External process stdout closed"); }); @@ -340,11 +347,12 @@ impl ExternalDecisionManager { std::collections::HashMap>, >, _storage: &Storage, + stdin_tx: &RwLock>>, ) { // Try to parse as incoming message match serde_json::from_str::(line) { Ok(IncomingMessage::Request(req)) => { - Self::handle_tool_call(req, tool_registry, pending_responses).await; + Self::handle_tool_call(req, tool_registry, pending_responses, stdin_tx).await; } Ok(IncomingMessage::Notification(notif)) => { debug!( @@ -368,15 +376,16 @@ impl ExternalDecisionManager { } } - /// Handle a tool call from the external process + /// Handle a tool call from the external process and send response back via stdin async fn handle_tool_call( req: JsonRpcRequest, tool_registry: &ToolRegistry, _pending_responses: &Mutex< std::collections::HashMap>, >, + stdin_tx: &RwLock>>, ) { - match req.method.as_str() { + let response = match req.method.as_str() { "tools/call" => { let params = req.params.unwrap_or(json!({})); let tool_name = params["name"].as_str().unwrap_or(""); @@ -385,31 +394,45 @@ impl ExternalDecisionManager { debug!("External process calling tool: {}", tool_name); match tool_registry.call_tool(tool_name, arguments).await { - Ok(result) => { - let response = JsonRpcResponse::new( + Ok(result) => JsonRpcResponse::new( + req.id, + json!({ + "content": result.content.iter().map(|c| match c { + ToolContent::Text { text } => json!({"type": "text", "text": text}), + _ => json!({"type": "unknown"}), + }).collect::>(), + "isError": result.is_error.unwrap_or(false), + }), + ), + Err(e) => { + error!("Tool call failed: {}", e); + JsonRpcResponse::new( req.id, json!({ - "content": result.content.iter().map(|c| match c { - ToolContent::Text { text } => json!({"type": "text", "text": text}), - _ => json!({"type": "unknown"}), - }).collect::>(), - "isError": result.is_error.unwrap_or(false), + "content": [{"type": "text", "text": format!("Tool call failed: {}", e)}], + "isError": true, }), - ); - debug!("Tool call response: {:?}", response); - } - Err(e) => { - error!("Tool call failed: {}", e); + ) } } } "tools/list" => { let tools = tool_registry.list_tools(); - let response = JsonRpcResponse::new(req.id, json!({ "tools": tools })); - debug!("Tools list response: {:?}", response); + JsonRpcResponse::new(req.id, json!({ "tools": tools })) } _ => { warn!("Unknown method from external process: {}", req.method); + return; + } + }; + + // Send response back to external process via stdin + if let Ok(response_str) = serde_json::to_string(&response) { + let tx = stdin_tx.read().await; + if let Some(tx) = tx.as_ref() { + if let Err(e) = tx.send(response_str).await { + debug!("Failed to send tool response to external process: {}", e); + } } } } diff --git a/tap-http/src/handler.rs b/tap-http/src/handler.rs index 36e66c86..fbca1166 100644 --- a/tap-http/src/handler.rs +++ b/tap-http/src/handler.rs @@ -181,8 +181,9 @@ pub async fn handle_didcomm( ) .await; - // Create error response - let response = json_error_response(StatusCode::INTERNAL_SERVER_ERROR, &e.to_string()); + // Create error response (generic message to avoid leaking internals) + let response = + json_error_response(StatusCode::INTERNAL_SERVER_ERROR, "Internal server error"); // Calculate response size and duration (approximate) let response_size = 200; // Approximate size @@ -495,6 +496,7 @@ pub async fn handle_well_known_did( host: Option, node: Arc, event_bus: Arc, + max_agents: usize, ) -> std::result::Result { let start_time = Instant::now(); @@ -508,10 +510,7 @@ pub async fn handle_well_known_did( Ok(d) => d, Err(e) => { warn!("Invalid Host header for web DID: {}", e); - let response = json_error_response( - StatusCode::BAD_REQUEST, - &format!("Invalid Host header: {}", e), - ); + let response = json_error_response(StatusCode::BAD_REQUEST, "Invalid Host header"); let duration_ms = start_time.elapsed().as_millis() as u64; event_bus .publish_response_sent(StatusCode::BAD_REQUEST, 200, duration_ms) @@ -576,6 +575,25 @@ pub async fn handle_well_known_did( } } + // Check agent count limit before creating a new one + if node.agents().agent_count() >= max_agents { + warn!( + "Agent limit reached ({}/{}), refusing to create agent for {}", + node.agents().agent_count(), + max_agents, + did_web + ); + let response = json_error_response( + StatusCode::SERVICE_UNAVAILABLE, + "Maximum number of agents reached", + ); + let duration_ms = start_time.elapsed().as_millis() as u64; + event_bus + .publish_response_sent(StatusCode::SERVICE_UNAVAILABLE, 200, duration_ms) + .await; + return Ok(response); + } + // No agent exists — create a new one info!("Creating new agent for {}", did_web); @@ -890,7 +908,9 @@ mod tests { let node = Arc::new(TapNode::new(config)); let event_bus = Arc::new(crate::event::EventBus::new()); - let response = handle_well_known_did(None, node, event_bus).await.unwrap(); + let response = handle_well_known_did(None, node, event_bus, 100) + .await + .unwrap(); let response_bytes = to_bytes(response.into_response().into_body()) .await @@ -916,6 +936,7 @@ mod tests { Some("".to_string()), node, event_bus, + 100, ) .await .unwrap(); @@ -940,10 +961,14 @@ mod tests { let node = Arc::new(TapNode::new(config)); let event_bus = Arc::new(crate::event::EventBus::new()); - let response = - handle_well_known_did(Some("example.com".to_string()), node.clone(), event_bus) - .await - .unwrap(); + let response = handle_well_known_did( + Some("example.com".to_string()), + node.clone(), + event_bus, + 100, + ) + .await + .unwrap(); let response_bytes = to_bytes(response.into_response().into_body()) .await @@ -998,6 +1023,7 @@ mod tests { Some("test.example.com".to_string()), node.clone(), event_bus, + 100, ) .await .unwrap(); @@ -1020,10 +1046,14 @@ mod tests { let node = Arc::new(TapNode::new(config)); let event_bus = Arc::new(crate::event::EventBus::new()); - let response = - handle_well_known_did(Some("localhost:3000".to_string()), node.clone(), event_bus) - .await - .unwrap(); + let response = handle_well_known_did( + Some("localhost:3000".to_string()), + node.clone(), + event_bus, + 100, + ) + .await + .unwrap(); let response_bytes = to_bytes(response.into_response().into_body()) .await @@ -1057,6 +1087,7 @@ mod tests { Some("idempotent.example.com".to_string()), node.clone(), event_bus.clone(), + 100, ) .await .unwrap(); @@ -1070,6 +1101,7 @@ mod tests { Some("idempotent.example.com".to_string()), node.clone(), event_bus, + 100, ) .await .unwrap(); diff --git a/tap-http/src/main.rs b/tap-http/src/main.rs index 223447a3..79f61b48 100644 --- a/tap-http/src/main.rs +++ b/tap-http/src/main.rs @@ -388,6 +388,7 @@ async fn main() -> Result<(), Box> { tls: None, event_logger: None, enable_web_did: args.enable_web_did, + max_agents: 100, }; // Configure event logging - use TAP root-based default if not specified diff --git a/tap-http/src/server.rs b/tap-http/src/server.rs index 7dfbd580..126d4861 100644 --- a/tap-http/src/server.rs +++ b/tap-http/src/server.rs @@ -168,10 +168,11 @@ impl TapHttpServer { .trim_start_matches('/') .to_string(); - // Create DIDComm endpoint + // Create DIDComm endpoint (1MB body size limit) let didcomm_route = warp::path(endpoint_path) .and(warp::post()) .and(warp::header::optional::("content-type")) + .and(warp::body::content_length_limit(1024 * 1024)) .and(warp::body::bytes()) .and(with_node(node.clone())) .and(with_event_bus(event_bus.clone())) @@ -190,6 +191,7 @@ impl TapHttpServer { if enable_web_did { info!("Web DID hosting enabled at /.well-known/did.json"); + let max_agents = self.config.max_agents; let well_known_route = warp::path(".well-known") .and(warp::path("did.json")) .and(warp::path::end()) @@ -197,12 +199,23 @@ impl TapHttpServer { .and(warp::header::optional::("host")) .and(with_node(node.clone())) .and(with_event_bus(event_bus.clone())) + .and(warp::any().map(move || max_agents)) .and_then(handle_well_known_did); let routes = didcomm_route .or(health_route) .or(well_known_route) .with(warp::log("tap_http")) + .with(warp::reply::with::header( + "X-Content-Type-Options", + "nosniff", + )) + .with(warp::reply::with::header("X-Frame-Options", "DENY")) + .with(warp::reply::with::header("Cache-Control", "no-store")) + .with(warp::reply::with::header( + "Content-Security-Policy", + "default-src 'none'", + )) .recover(handle_rejection); return self.spawn_server(routes, addr, event_bus).await; @@ -212,6 +225,16 @@ impl TapHttpServer { let routes = didcomm_route .or(health_route) .with(warp::log("tap_http")) + .with(warp::reply::with::header( + "X-Content-Type-Options", + "nosniff", + )) + .with(warp::reply::with::header("X-Frame-Options", "DENY")) + .with(warp::reply::with::header("Cache-Control", "no-store")) + .with(warp::reply::with::header( + "Content-Security-Policy", + "default-src 'none'", + )) .recover(handle_rejection); self.spawn_server(routes, addr, event_bus).await diff --git a/tap-http/tests/server_tests.rs b/tap-http/tests/server_tests.rs index f0c57eec..df7d262c 100644 --- a/tap-http/tests/server_tests.rs +++ b/tap-http/tests/server_tests.rs @@ -227,11 +227,11 @@ async fn test_didcomm_endpoint_content_types() { assert_eq!(status, 500); let json: serde_json::Value = serde_json::from_str(&body).unwrap(); assert_eq!(json["status"], "error"); - // The message should indicate that no agent could process it + // The message should be a generic internal server error (no internal details leaked) let message = json["message"].as_str().unwrap_or(""); assert!( - message.contains("No agent could process"), - "Expected 'No agent could process' but got: {}", + message.contains("Internal server error"), + "Expected 'Internal server error' but got: {}", message ); @@ -262,11 +262,11 @@ async fn test_didcomm_endpoint_content_types() { let json: serde_json::Value = serde_json::from_str(&body).unwrap(); assert_eq!(status, 500); assert_eq!(json["status"], "error"); - // The message should indicate that no agent could process it or verification failed + // The message should be a generic internal server error (no internal details leaked) let message = json["message"].as_str().unwrap_or(""); assert!( - message.contains("No agent could process") || message.contains("Verification error"), - "Expected processing/verification error but got: {}", + message.contains("Internal server error"), + "Expected 'Internal server error' but got: {}", message ); diff --git a/tap-mcp/src/tools/database_tools.rs b/tap-mcp/src/tools/database_tools.rs index a799c224..e5d7f85e 100644 --- a/tap-mcp/src/tools/database_tools.rs +++ b/tap-mcp/src/tools/database_tools.rs @@ -37,6 +37,17 @@ impl QueryDatabaseTool { Self { tap_integration } } + /// Validate that a table name contains only safe identifier characters + fn validate_table_name(name: &str) -> bool { + !name.is_empty() + && name.len() <= 128 + && name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') + && name + .chars() + .next() + .is_some_and(|c| c.is_ascii_alphabetic() || c == '_') + } + /// Check if a query is read-only fn is_read_only_query(query: &str) -> bool { let query_upper = query.trim().to_uppercase(); @@ -140,6 +151,18 @@ impl ToolHandler for QueryDatabaseTool { } }; + // Enforce read-only mode at the database level + if let Err(e) = sqlx::query("PRAGMA query_only = ON") + .execute(&mut conn) + .await + { + error!("Failed to set query_only pragma: {}", e); + return Ok(error_text_response(format!( + "Failed to set read-only mode: {}", + e + ))); + } + // Execute query match sqlx::query(¶ms.query).fetch_all(&mut conn).await { Ok(rows) => { @@ -343,28 +366,52 @@ impl ToolHandler for GetDatabaseSchemaTool { let mut tables = Vec::new(); - // Get list of tables - let table_query = if let Some(ref table_name) = params.table_name { - format!( - "SELECT name FROM sqlite_master WHERE type='table' AND name='{}' ORDER BY name", - table_name + // Validate table name if provided + if let Some(ref table_name) = params.table_name { + if !QueryDatabaseTool::validate_table_name(table_name) { + return Ok(error_text_response( + "Invalid table name. Table names must contain only alphanumeric characters and underscores.".to_string(), + )); + } + } + + // Get list of tables using parameterized query + let table_rows = if let Some(ref table_name) = params.table_name { + match sqlx::query( + "SELECT name FROM sqlite_master WHERE type='table' AND name=?1 ORDER BY name", ) + .bind(table_name) + .fetch_all(&mut conn) + .await + { + Ok(rows) => rows, + Err(e) => { + error!("Failed to get tables: {}", e); + return Ok(error_text_response(format!("Failed to get tables: {}", e))); + } + } } else { - "SELECT name FROM sqlite_master WHERE type='table' ORDER BY name".to_string() - }; - - let table_rows = match sqlx::query(&table_query).fetch_all(&mut conn).await { - Ok(rows) => rows, - Err(e) => { - error!("Failed to get tables: {}", e); - return Ok(error_text_response(format!("Failed to get tables: {}", e))); + match sqlx::query("SELECT name FROM sqlite_master WHERE type='table' ORDER BY name") + .fetch_all(&mut conn) + .await + { + Ok(rows) => rows, + Err(e) => { + error!("Failed to get tables: {}", e); + return Ok(error_text_response(format!("Failed to get tables: {}", e))); + } } }; for table_row in table_rows { let table_name: String = table_row.try_get("name").unwrap_or_default(); - // Get columns for this table + // Validate table name from database as defense-in-depth + if !QueryDatabaseTool::validate_table_name(&table_name) { + continue; + } + + // Get columns for this table (safe: table_name validated above) let column_query = format!("PRAGMA table_info('{}')", table_name); let column_rows = match sqlx::query(&column_query).fetch_all(&mut conn).await { Ok(rows) => rows, diff --git a/tap-msg/src/message/payment.rs b/tap-msg/src/message/payment.rs index 5bb98f16..1aae5b75 100644 --- a/tap-msg/src/message/payment.rs +++ b/tap-msg/src/message/payment.rs @@ -448,8 +448,13 @@ impl Payment { return Err(Error::Validation("Amount is required".to_string())); } - // Validate amount is a positive number + // Validate amount is a finite positive number match self.amount.parse::() { + Ok(amount) if !amount.is_finite() => { + return Err(Error::Validation( + "Amount must be a finite number".to_string(), + )); + } Ok(amount) if amount <= 0.0 => { return Err(Error::Validation("Amount must be positive".to_string())); } diff --git a/tap-msg/src/message/settle.rs b/tap-msg/src/message/settle.rs index e74bb428..d912817b 100644 --- a/tap-msg/src/message/settle.rs +++ b/tap-msg/src/message/settle.rs @@ -112,8 +112,13 @@ impl Settle { )); } - // Validate amount is a positive number if provided + // Validate amount is a finite positive number if provided match amount.parse::() { + Ok(amount) if !amount.is_finite() => { + return Err(Error::Validation( + "Amount must be a finite number".to_string(), + )); + } Ok(amount) if amount <= 0.0 => { return Err(Error::Validation("Amount must be positive".to_string())); } diff --git a/tap-msg/src/message/transfer.rs b/tap-msg/src/message/transfer.rs index d17f6c3e..8f2ae2f5 100644 --- a/tap-msg/src/message/transfer.rs +++ b/tap-msg/src/message/transfer.rs @@ -290,8 +290,13 @@ impl Transfer { return Err(Error::Validation("Amount is required".to_string())); } - // Validate amount is a positive number + // Validate amount is a finite positive number match self.amount.parse::() { + Ok(amount) if !amount.is_finite() => { + return Err(Error::Validation( + "Amount must be a finite number".to_string(), + )); + } Ok(amount) if amount <= 0.0 => { return Err(Error::Validation("Amount must be positive".to_string())); } diff --git a/tap-node/Cargo.toml b/tap-node/Cargo.toml index fe257ca4..af80027a 100644 --- a/tap-node/Cargo.toml +++ b/tap-node/Cargo.toml @@ -52,6 +52,9 @@ base64 = "0.22" # For encoding signatures chrono = { workspace = true } log = "0.4" +# URL encoding +percent-encoding = "2.3" + # HTTP client for native reqwest = { version = "0.12", features = ["json"], optional = true } diff --git a/tap-node/src/message/sender.rs b/tap-node/src/message/sender.rs index 86c62f4d..dfb633d1 100644 --- a/tap-node/src/message/sender.rs +++ b/tap-node/src/message/sender.rs @@ -207,11 +207,10 @@ impl HttpPlainMessageSender { ) } - /// Simple URL encoding function + /// URL-encode a string for safe use in URLs fn url_encode(&self, text: &str) -> String { - // Simple encoding of common URL-unsafe characters - // In a real implementation, you would use a proper URL encoding library - text.replace(':', "%3A").replace('/', "%2F") + use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC}; + utf8_percent_encode(text, NON_ALPHANUMERIC).to_string() } } @@ -304,11 +303,10 @@ impl WebSocketPlainMessageSender { ) } - /// Simple URL encoding function + /// URL-encode a string for safe use in URLs fn url_encode(&self, text: &str) -> String { - // Simple encoding of common URL-unsafe characters - // In a real implementation, you would use a proper URL encoding library - text.replace(':', "%3A").replace('/', "%2F") + use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC}; + utf8_percent_encode(text, NON_ALPHANUMERIC).to_string() } /// Ensures a connection exists for the given recipient diff --git a/tap-node/src/storage/db.rs b/tap-node/src/storage/db.rs index b36fbe0e..3281f445 100644 --- a/tap-node/src/storage/db.rs +++ b/tap-node/src/storage/db.rs @@ -78,23 +78,29 @@ impl Storage { agent_did: &str, tap_root: Option, ) -> Result { - let root_dir = tap_root.unwrap_or_else(|| { - // Check TAP_HOME first (for tests) - if let Ok(tap_home) = env::var("TAP_HOME") { - PathBuf::from(tap_home) - } else if let Ok(tap_root) = env::var("TAP_ROOT") { - PathBuf::from(tap_root) - } else if let Ok(test_dir) = env::var("TAP_TEST_DIR") { - PathBuf::from(test_dir).join(".tap") - } else { - dirs::home_dir() - .expect("Could not find home directory") - .join(".tap") + let root_dir = match tap_root { + Some(root) => root, + None => { + if let Ok(tap_home) = env::var("TAP_HOME") { + PathBuf::from(tap_home) + } else if let Ok(tap_root) = env::var("TAP_ROOT") { + PathBuf::from(tap_root) + } else if let Ok(test_dir) = env::var("TAP_TEST_DIR") { + PathBuf::from(test_dir).join(".tap") + } else { + dirs::home_dir() + .ok_or_else(|| { + StorageError::Migration( + "Could not determine home directory".to_string(), + ) + })? + .join(".tap") + } } - }); + }; - // Sanitize the DID for use as a directory name - let sanitized_did = agent_did.replace(':', "_"); + // Sanitize the DID for use as a directory name (prevent path traversal) + let sanitized_did = agent_did.replace([':', '/', '\\'], "_").replace("..", "_"); let db_path = root_dir.join(&sanitized_did).join("transactions.db"); Self::new(Some(db_path)).await @@ -196,20 +202,22 @@ impl Storage { /// /// * `tap_root` - Optional custom root directory (defaults to ~/.tap) pub fn default_logs_dir(tap_root: Option) -> PathBuf { - let root_dir = tap_root.unwrap_or_else(|| { - // Check TAP_HOME first (for tests) - if let Ok(tap_home) = env::var("TAP_HOME") { - PathBuf::from(tap_home) - } else if let Ok(tap_root) = env::var("TAP_ROOT") { - PathBuf::from(tap_root) - } else if let Ok(test_dir) = env::var("TAP_TEST_DIR") { - PathBuf::from(test_dir).join(".tap") - } else { - dirs::home_dir() - .expect("Could not find home directory") - .join(".tap") + let root_dir = match tap_root { + Some(root) => root, + None => { + if let Ok(tap_home) = env::var("TAP_HOME") { + PathBuf::from(tap_home) + } else if let Ok(tap_root) = env::var("TAP_ROOT") { + PathBuf::from(tap_root) + } else if let Ok(test_dir) = env::var("TAP_TEST_DIR") { + PathBuf::from(test_dir).join(".tap") + } else { + dirs::home_dir() + .expect("Could not determine home directory") + .join(".tap") + } } - }); + }; root_dir.join("logs") } @@ -2076,12 +2084,13 @@ impl Storage { .bind(&customer.address_locality) .bind(&customer.postal_code) .bind(&customer.street_address) - .bind(serde_json::to_string(&customer.profile).unwrap()) + .bind(serde_json::to_string(&customer.profile)?) .bind( customer .ivms101_data .as_ref() - .map(|v| serde_json::to_string(v).unwrap()), + .map(serde_json::to_string) + .transpose()?, ) .bind(&customer.verified_at) .bind(&customer.created_at) @@ -2124,10 +2133,11 @@ impl Storage { address_locality: row.get("address_locality"), postal_code: row.get("postal_code"), street_address: row.get("street_address"), - profile: serde_json::from_str(&row.get::("profile")).unwrap(), + profile: serde_json::from_str(&row.get::("profile"))?, ivms101_data: row .get::, _>("ivms101_data") - .map(|v| serde_json::from_str(&v).unwrap()), + .map(|v| serde_json::from_str(&v)) + .transpose()?, verified_at: row.get("verified_at"), created_at: row.get("created_at"), updated_at: row.get("updated_at"), @@ -2200,10 +2210,11 @@ impl Storage { address_locality: row.get("address_locality"), postal_code: row.get("postal_code"), street_address: row.get("street_address"), - profile: serde_json::from_str(&row.get::("profile")).unwrap(), + profile: serde_json::from_str(&row.get::("profile"))?, ivms101_data: row .get::, _>("ivms101_data") - .map(|v| serde_json::from_str(&v).unwrap()), + .map(|v| serde_json::from_str(&v)) + .transpose()?, verified_at: row.get("verified_at"), created_at: row.get("created_at"), updated_at: row.get("updated_at"), @@ -2323,7 +2334,8 @@ impl Storage { relationship .proof .as_ref() - .map(|v| serde_json::to_string(v).unwrap()), + .map(serde_json::to_string) + .transpose()?, ) .bind(&relationship.confirmed_at) .bind(&relationship.created_at) @@ -2377,7 +2389,7 @@ impl Storage { customer_id, relationship_type, related_identifier, - proof: proof.map(|v| serde_json::from_str(&v).unwrap()), + proof: proof.map(|v| serde_json::from_str(&v)).transpose()?, confirmed_at, created_at, }); @@ -2393,7 +2405,12 @@ impl Storage { query: &str, limit: u32, ) -> Result, StorageError> { - let search_pattern = format!("%{}%", query); + // Escape LIKE metacharacters to prevent pattern injection + let escaped_query = query + .replace('\\', "\\\\") + .replace('%', "\\%") + .replace('_', "\\_"); + let search_pattern = format!("%{}%", escaped_query); let rows = sqlx::query( r#" @@ -2405,11 +2422,11 @@ impl Storage { LEFT JOIN customer_identifiers ci ON c.id = ci.customer_id WHERE c.agent_did = ?1 AND ( - c.given_name LIKE ?2 - OR c.family_name LIKE ?2 - OR c.display_name LIKE ?2 - OR c.legal_name LIKE ?2 - OR ci.id LIKE ?2 + c.given_name LIKE ?2 ESCAPE '\' + OR c.family_name LIKE ?2 ESCAPE '\' + OR c.display_name LIKE ?2 ESCAPE '\' + OR c.legal_name LIKE ?2 ESCAPE '\' + OR ci.id LIKE ?2 ESCAPE '\' ) ORDER BY c.updated_at DESC LIMIT ?3 @@ -2438,10 +2455,11 @@ impl Storage { address_locality: row.get("address_locality"), postal_code: row.get("postal_code"), street_address: row.get("street_address"), - profile: serde_json::from_str(&row.get::("profile")).unwrap(), + profile: serde_json::from_str(&row.get::("profile"))?, ivms101_data: row .get::, _>("ivms101_data") - .map(|v| serde_json::from_str(&v).unwrap()), + .map(|v| serde_json::from_str(&v)) + .transpose()?, verified_at: row.get("verified_at"), created_at: row.get("created_at"), updated_at: row.get("updated_at"), @@ -2521,7 +2539,7 @@ impl Storage { debug!("Updating decision {} status to {}", decision_id, status); let now = chrono::Utc::now().to_rfc3339(); - let resolution_detail_str = resolution_detail.map(|v| serde_json::to_string(v).unwrap()); + let resolution_detail_str = resolution_detail.map(serde_json::to_string).transpose()?; let delivered_at = if status == DecisionStatus::Delivered { Some(now.clone()) @@ -2633,7 +2651,8 @@ impl Storage { resolution: row.get("resolution"), resolution_detail: row .get::, _>("resolution_detail") - .map(|v| serde_json::from_str(&v).unwrap()), + .map(|v| serde_json::from_str(&v)) + .transpose()?, created_at: row.get("created_at"), delivered_at: row.get("delivered_at"), resolved_at: row.get("resolved_at"), @@ -2784,7 +2803,8 @@ impl Storage { resolution: row.get("resolution"), resolution_detail: row .get::, _>("resolution_detail") - .map(|v| serde_json::from_str(&v).unwrap()), + .map(|v| serde_json::from_str(&v)) + .transpose()?, created_at: row.get("created_at"), delivered_at: row.get("delivered_at"), resolved_at: row.get("resolved_at"), diff --git a/tap-node/src/validation/agent_validator.rs b/tap-node/src/validation/agent_validator.rs index c0cf50ff..bb5a95ea 100644 --- a/tap-node/src/validation/agent_validator.rs +++ b/tap-node/src/validation/agent_validator.rs @@ -78,9 +78,10 @@ impl MessageValidator for AgentAuthorizationValidator { let transaction_id = match self.get_transaction_id(message).await { Some(id) => id, None => { - // Can't find transaction ID - this might be a new transaction - // or a message type we don't need to validate - return ValidationResult::Accept; + // Fail-closed: reject transaction responses without a determinable transaction ID + return ValidationResult::Reject( + "Cannot determine transaction ID for transaction response".to_string(), + ); } };