fix(security): safety layer bypass via output truncation [HIGH]

[lycheepuppy]

Summary

• Fix safety layer bypass where sanitize_tool_output() returned immediately after truncating oversized tool output, skipping all three safety checks (leak detection, policy enforcement, injection scanning)
• Restructured truncation path so truncated content flows through the full safety pipeline before being returned
• Added regression tests proving truncated output is now scanned for injection patterns

Security Finding: Safety Layer Bypass via Output Truncation

Severity: HIGH
Reported by: FailSafe Security Researcher
Component: crates/ironclaw_safety/src/lib.rs — sanitize_tool_output()

Description

The sanitize_tool_output() method truncated oversized tool output and returned it immediately — before leak detection, policy enforcement, or injection scanning ever ran. This meant any tool output exceeding max_output_length was delivered to the LLM with zero safety checks applied.

Order-of-operations problem (before fix):

1. Length check → if oversized, truncate to UTF-8 boundary
2. Early return → truncated content returned with only a low-severity output_too_large warning
3. Leak detection → SKIPPED for truncated output
4. Policy enforcement → SKIPPED for truncated output
5. Injection scanning → SKIPPED for truncated output

Attack Vector

A malicious tool (compromised MCP server, poisoned web page via HTTP tool, sandbox job processing attacker-controlled input) could craft output where:

• The first N bytes (within the truncation boundary) contain prompt injection payloads, credential exfiltration instructions, or policy-violating content
• The total output exceeds max_output_length, triggering the early-return path
• The adversarial content reaches the LLM without any safety scanning

The attacker does not need to know the exact truncation threshold — they only need to ensure the total output is large enough while placing the payload near the beginning.

Impact

• Any tool output exceeding max_output_length bypassed all three safety layers
• Undermines the entire safety architecture — the safety layer is the single choke point for tool output
• The bypass is trivially triggered by any tool that returns large output (web fetch, file read, shell command, MCP tool)

Fix

Removed the early return after truncation. Truncation now produces (content, was_modified, extra_warnings) and falls through to the same leak detection → policy enforcement → injection scanning pipeline as non-truncated content. Truncation warnings are preserved and merged into the final output.

Test plan

• cargo test -p ironclaw_safety — existing adversarial truncation tests still pass
• New test truncated_output_still_scanned_for_injection — verifies injection patterns are detected in oversized output
• New test truncated_output_preserves_truncation_warning — verifies truncation warning survives the full pipeline
• cargo clippy -p ironclaw_safety -- -D warnings — zero warnings

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request refactors the sanitize_tool_output function to ensure that truncated output is still processed by subsequent safety layers, such as leak detection and injection scanning, instead of returning early. It also adds regression tests to confirm that security checks are applied to truncated content. Feedback was provided to correct an invalid range in the truncation warning that could cause out-of-bounds errors and to append low-severity warnings to the end of the list to maintain proper sorting.

[gemini-code-assist]

The location range 0..output.len() is incorrect because it refers to the original, untruncated string. Since sanitize_tool_output returns a modified string (truncated prefix plus notice), any consumer using this range will encounter an out-of-bounds panic. Additionally, when truncating UTF-8 strings, ensure the split occurs at a valid character boundary using is_char_boundary to prevent panics on multi-byte characters.

Suggested change

	location: 0..output.len(),
	location: cut..(cut + notice.len()),

References

1. When truncating a UTF-8 string at a byte boundary, walk backwards from the desired length until a valid character boundary is found using is_char_boundary to prevent panics.
2. Always truncate tool output for previews or status updates to a reasonable maximum length to prevent excessive memory/bandwidth usage.

[gemini-code-assist]

Merging extra_warnings at the beginning of the list breaks the severity-based sort order (Critical/High first) established by the Sanitizer. Since the truncation warning is Severity::Low, it should be appended to the end of the sanitized.warnings list. This ensures that more urgent security warnings remain at the top of the list for the caller. Additionally, using extend is more idiomatic than the current append and reassignment pattern.

Suggested change

	extra_warnings.append(&mut sanitized.warnings);
	sanitized.warnings = extra_warnings;
	sanitized.warnings.extend(extra_warnings);

[zmanian]

Review -- APPROVE

Legitimate HIGH severity security fix. The vulnerability is real: sanitize_tool_output() has an early return after truncation that skips leak detection, policy enforcement, and injection scanning. An attacker only needs to pad output past max_output_length while placing malicious content in the first N bytes.

The fix is correct -- truncation now produces a tuple that falls through to the full safety pipeline. Truncated content is scanned (which is right, since it's what the LLM sees). Warning merging preserves both truncation and downstream warnings.

No bypass vectors in the fix. Low regression risk -- non-truncated output path is unchanged.

Suggestions (non-blocking)

• The policy Block path discards all warnings including truncation warning -- consider preserving for logging (pre-existing issue)
• Consider adding a test where truncated output contains a leaked secret pattern (currently only injection scanning is tested)

[zmanian]

Re-review -- APPROVE (post-approval commit check)

The new commit (4e2ff0b) is a merge of staging into the feature branch. It brings in unrelated staging changes (channels, config, e2e tests, github tool, etc.) but does not touch crates/ironclaw_safety/ at all -- zero files in the safety crate were modified by the merge.

The security fix from the original commit (a3fa6df) is intact and unchanged:

• Truncation no longer early-returns; content flows through the full safety pipeline
• Warning merging preserves truncation warnings alongside downstream findings
• Regression tests cover injection scanning on truncated output

Gemini's comments (not addressed, both non-blocking)

1. location: 0..output.len() range -- refers to original string length, not truncated content. Pre-existing pattern, not introduced by this PR. Low risk since location is metadata on the warning struct, not used for content slicing.

2. Warning ordering -- truncation warnings (Low severity) end up before injection warnings (higher severity) due to the append + reassign pattern. Cosmetic; callers should sort/filter by severity if ordering matters.

Neither weakens the security fix. Safe to merge.

[claude]

Code review

Found 2 issues:

1. [HIGH:95] Truncation warnings may be silently lost due to append order at lines 127-128

https://github.com/anthropics/ironclaw/blob/e8b1989ae24bb6917f1e14478dc60ad6cee7df55/crates/ironclaw_safety/src/lib.rs#L124-L129

         if self.config.injection_check_enabled || force_sanitize {
             let mut sanitized = self.sanitizer.sanitize(&content);
             sanitized.was_modified = sanitized.was_modified || was_modified;
+            extra_warnings.append(&mut sanitized.warnings);
+            sanitized.warnings = extra_warnings;
             sanitized

The code appends sanitized.warnings into extra_warnings, then replaces sanitized.warnings with the merged vector. This loses truncation warnings when no injection patterns are detected in the branch below. The append should be reversed: sanitized.warnings.append(&mut extra_warnings) to prepend truncation warnings to injection warnings in the correct order.

2. [MEDIUM:70] Missing documentation of was_modified contract in bypass path

https://github.com/anthropics/ironclaw/blob/e8b1989ae24bb6917f1e14478dc60ad6cee7df55/crates/ironclaw_safety/src/lib.rs#L130-L140

         } else {
             SanitizedOutput {
                 content,
-                warnings: vec![],
+                warnings: extra_warnings,
                 was_modified,
             }
         }

When injection_check_enabled=false, the was_modified flag reflects only truncation and policy actions, not injection pattern modifications. This coupling is fragile for future changes. Consider documenting: "was_modified reflects truncation and policy actions only, not injection pattern modification when injection_check is disabled."

---

Security & Safety: ✅ No issues — fix correctly prevents truncation from bypassing all downstream checks (leak detection, policy, injection scanning).

Bug Scan: ✅ No obvious logic errors — refactoring preserves safety check execution order.

Performance: Append order at lines 127-128 could be optimized (see issue #1 above).