Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).

### Added

- feat(security): MCP-to-ACP confused-deputy boundary enforcement — when `mcp_to_acp_boundary = true` (default) and agent is in an ACP session, MCP tool results are unconditionally quarantined before entering the ACP response stream; cross-boundary flows emit `CrossBoundaryMcpToAcp` security events and `cross_boundary_mcp_to_acp: true` audit entries (#2417)
- feat(security): env var sanitization for MCP stdio child processes — `LD_PRELOAD`, `LD_LIBRARY_PATH`, `DYLD_INSERT_LIBRARIES`, `DYLD_LIBRARY_PATH`, `DYLD_FRAMEWORK_PATH`, `DYLD_FALLBACK_LIBRARY_PATH` are stripped from ACP-provided env vars (#2417)

### Changed

- **BREAKING**: `tool_allowlist` type changed from `Vec<String>` to `Option<Vec<String>>` in `ServerEntry` and `McpServerConfig` — `None` means no override (all tools, with untrusted warning), `Some(vec![])` means explicit deny-all (fail-closed), `Some(vec![...])` filters to named tools (#2417)

### Added (continued)

- feat(acp): implement `session/close` handler — `ZephAcpAgent::close_session` removes the in-memory session entry, fires `cancel_signal` to stop any running turn, and returns idempotent `Ok` for unknown session IDs; advertise `session.close` capability in `initialize()`; gated behind `unstable-session-close` feature included in `default` and `acp-unstable` (closes #2421)
- feat(acp): bump `agent-client-protocol` 0.10.2→0.10.3, `agent-client-protocol-schema` 0.11.2→0.11.3; add `unstable-logout` feature with no-op logout handler and `auth.logout` capability advertisement; add `unstable-elicitation` feature gate (exposes schema types; SDK methods not yet available upstream); fix discovery endpoint `protocol_version` to use `ProtocolVersion::LATEST`; fix double-feature-activation antipattern in `zeph-acp` feature flags (#2411)
- feat(skills): add `category` field to SKILL.md frontmatter — optional grouping for skill library organisation; all 26 bundled skills annotated with categories (`web`, `data`, `dev`, `system`) (#2268)
Expand Down
73 changes: 68 additions & 5 deletions crates/zeph-acp/src/mcp_bridge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,10 @@ pub fn acp_mcp_servers_to_entries(servers: &[acp::McpServer]) -> Vec<ServerEntry
.iter()
.filter_map(|s| match s {
acp::McpServer::Stdio(stdio) => {
// IDE is the trusted client in the stdio transport model; env vars are passed
// as-is to the MCP server child process without further sanitization.
let env: HashMap<String, String> = stdio
.env
.iter()
.filter(|e| !is_dangerous_env_var(&e.name))
.map(|e| (e.name.clone(), e.value.clone()))
.collect();
Some(ServerEntry {
Expand All @@ -35,7 +34,7 @@ pub fn acp_mcp_servers_to_entries(servers: &[acp::McpServer]) -> Vec<ServerEntry
},
timeout: Duration::from_secs(DEFAULT_MCP_TIMEOUT_SECS),
trust_level: McpTrustLevel::Untrusted,
tool_allowlist: Vec::new(),
tool_allowlist: None,
expected_tools: Vec::new(),
})
}
Expand All @@ -47,7 +46,7 @@ pub fn acp_mcp_servers_to_entries(servers: &[acp::McpServer]) -> Vec<ServerEntry
},
timeout: Duration::from_secs(DEFAULT_MCP_TIMEOUT_SECS),
trust_level: McpTrustLevel::Untrusted,
tool_allowlist: Vec::new(),
tool_allowlist: None,
expected_tools: Vec::new(),
}),
acp::McpServer::Sse(sse) => {
Expand All @@ -61,7 +60,7 @@ pub fn acp_mcp_servers_to_entries(servers: &[acp::McpServer]) -> Vec<ServerEntry
},
timeout: Duration::from_secs(DEFAULT_MCP_TIMEOUT_SECS),
trust_level: McpTrustLevel::Untrusted,
tool_allowlist: Vec::new(),
tool_allowlist: None,
expected_tools: Vec::new(),
})
}
Expand All @@ -73,6 +72,21 @@ pub fn acp_mcp_servers_to_entries(servers: &[acp::McpServer]) -> Vec<ServerEntry
.collect()
}

/// Env vars that must never be passed from ACP clients to MCP child processes.
/// These enable library injection, path hijacking, and other privilege escalation vectors.
fn is_dangerous_env_var(name: &str) -> bool {
let upper = name.to_ascii_uppercase();
matches!(
upper.as_str(),
"LD_PRELOAD"
| "LD_LIBRARY_PATH"
| "DYLD_INSERT_LIBRARIES"
| "DYLD_LIBRARY_PATH"
| "DYLD_FRAMEWORK_PATH"
| "DYLD_FALLBACK_LIBRARY_PATH"
)
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -176,4 +190,53 @@ mod tests {
assert_eq!(entries[2].id, "stdio-2");
assert_eq!(entries[3].id, "sse-1");
}

#[test]
fn dangerous_env_vars_stripped() {
let stdio = acp::McpServerStdio::new("env-mcp", "/bin/mcp").env(vec![
acp::EnvVariable::new("SAFE_VAR", "ok"),
acp::EnvVariable::new("LD_PRELOAD", "/tmp/evil.so"),
acp::EnvVariable::new("DYLD_INSERT_LIBRARIES", "/tmp/evil.dylib"),
acp::EnvVariable::new("LD_LIBRARY_PATH", "/tmp"),
]);
let entries = acp_mcp_servers_to_entries(&[acp::McpServer::Stdio(stdio)]);
if let McpTransport::Stdio { env, .. } = &entries[0].transport {
assert_eq!(env.get("SAFE_VAR"), Some(&"ok".to_owned()));
assert!(env.get("LD_PRELOAD").is_none());
assert!(env.get("DYLD_INSERT_LIBRARIES").is_none());
assert!(env.get("LD_LIBRARY_PATH").is_none());
} else {
panic!("expected Stdio transport");
}
}

#[test]
fn acp_servers_have_none_allowlist() {
let servers = vec![
acp::McpServer::Stdio(acp::McpServerStdio::new("s", "/bin/s")),
acp::McpServer::Http(acp::McpServerHttp::new("h", "http://localhost")),
acp::McpServer::Sse(acp::McpServerSse::new("e", "http://localhost/sse")),
];
let entries = acp_mcp_servers_to_entries(&servers);
for entry in &entries {
assert!(
entry.tool_allowlist.is_none(),
"ACP-requested server '{}' must have tool_allowlist=None",
entry.id
);
}
}

#[test]
fn is_dangerous_env_var_cases() {
assert!(super::is_dangerous_env_var("LD_PRELOAD"));
assert!(super::is_dangerous_env_var("ld_preload"));
assert!(super::is_dangerous_env_var("DYLD_INSERT_LIBRARIES"));
assert!(super::is_dangerous_env_var("DYLD_LIBRARY_PATH"));
assert!(super::is_dangerous_env_var("DYLD_FRAMEWORK_PATH"));
assert!(super::is_dangerous_env_var("DYLD_FALLBACK_LIBRARY_PATH"));
assert!(!super::is_dangerous_env_var("PATH"));
assert!(!super::is_dangerous_env_var("HOME"));
assert!(!super::is_dangerous_env_var("MY_VAR"));
}
}
9 changes: 4 additions & 5 deletions crates/zeph-config/src/channels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,12 +376,11 @@ pub struct McpServerConfig {
/// Trust level for this server. Default: Untrusted.
#[serde(default)]
pub trust_level: McpTrustLevel,
/// Tool allowlist. Behavior depends on `trust_level`:
/// - Trusted: ignored (all tools exposed)
/// - Untrusted: empty = all tools (with warning), non-empty = only listed tools
/// - Sandboxed: empty = NO tools (fail-closed), non-empty = only listed tools
/// Tool allowlist. `None` means no override (inherit defaults).
/// `Some(vec![])` is an explicit empty list (deny all for Untrusted/Sandboxed).
/// `Some(vec!["a", "b"])` allows only listed tools.
#[serde(default)]
pub tool_allowlist: Vec<String>,
pub tool_allowlist: Option<Vec<String>>,
/// Expected tool names for attestation. Supplements `tool_allowlist`.
///
/// When non-empty: tools not in this list are filtered out (Untrusted/Sandboxed)
Expand Down
30 changes: 30 additions & 0 deletions crates/zeph-config/src/sanitizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ impl Default for EmbeddingGuardConfig {
/// Configuration for the content isolation pipeline, nested under
/// `[security.content_isolation]` in the agent config file.
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize)]
#[allow(clippy::struct_excessive_bools)]
pub struct ContentIsolationConfig {
/// When `false`, the sanitizer is a no-op: content passes through unchanged.
#[serde(default = "default_true")]
Expand All @@ -126,6 +127,13 @@ pub struct ContentIsolationConfig {
/// Embedding anomaly guard configuration.
#[serde(default)]
pub embedding_guard: EmbeddingGuardConfig,

/// When `true`, MCP tool results flowing through ACP-serving sessions receive
/// unconditional quarantine summarization and cross-boundary audit log entries.
/// This prevents confused-deputy attacks where untrusted MCP output influences
/// responses served to ACP clients (e.g. IDE integrations).
#[serde(default = "default_true")]
pub mcp_to_acp_boundary: bool,
}

impl Default for ContentIsolationConfig {
Expand All @@ -137,6 +145,7 @@ impl Default for ContentIsolationConfig {
spotlight_untrusted: true,
quarantine: QuarantineConfig::default(),
embedding_guard: EmbeddingGuardConfig::default(),
mcp_to_acp_boundary: true,
}
}
}
Expand Down Expand Up @@ -474,6 +483,27 @@ impl Default for ResponseVerificationConfig {
mod tests {
use super::*;

#[test]
fn content_isolation_default_mcp_to_acp_boundary_true() {
let cfg = ContentIsolationConfig::default();
assert!(cfg.mcp_to_acp_boundary);
}

#[test]
fn content_isolation_deserialize_mcp_to_acp_boundary_false() {
let toml = r"
mcp_to_acp_boundary = false
";
let cfg: ContentIsolationConfig = toml::from_str(toml).unwrap();
assert!(!cfg.mcp_to_acp_boundary);
}

#[test]
fn content_isolation_deserialize_absent_defaults_true() {
let cfg: ContentIsolationConfig = toml::from_str("").unwrap();
assert!(cfg.mcp_to_acp_boundary);
}

fn de_guard(toml: &str) -> Result<EmbeddingGuardConfig, toml::de::Error> {
toml::from_str(toml)
}
Expand Down
3 changes: 3 additions & 0 deletions crates/zeph-core/config/default.toml
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,9 @@ max_content_size = 65536
flag_injection_patterns = true
# Wrap untrusted content in spotlighting XML delimiters (default: true)
spotlight_untrusted = true
# Enforce strict MCP→ACP trust boundary when agent serves ACP clients (default: true)
# When enabled, MCP tool results in ACP sessions are unconditionally quarantined
mcp_to_acp_boundary = true

[security.content_isolation.quarantine]
# Route high-risk content through an isolated LLM for fact extraction (default: false)
Expand Down
9 changes: 9 additions & 0 deletions crates/zeph-core/src/agent/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,15 @@ impl<C: Channel> Agent<C> {
self
}

/// Mark this agent session as serving an ACP client.
/// When `true` and `mcp_to_acp_boundary` is enabled, MCP tool results
/// receive unconditional quarantine and cross-boundary audit logging.
#[must_use]
pub fn with_acp_session(mut self, is_acp: bool) -> Self {
self.security.is_acp_session = is_acp;
self
}

/// Attach an ML classifier backend to the sanitizer for injection detection.
///
/// When attached, `classify_injection()` is called on each incoming user message when
Expand Down
2 changes: 1 addition & 1 deletion crates/zeph-core/src/agent/mcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ impl<C: Channel> Agent<C> {
transport,
timeout: std::time::Duration::from_secs(30),
trust_level: zeph_mcp::McpTrustLevel::Untrusted,
tool_allowlist: Vec::new(),
tool_allowlist: None,
expected_tools: Vec::new(),
};

Expand Down
1 change: 1 addition & 0 deletions crates/zeph-core/src/agent/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ impl<C: Channel> Agent<C> {
security: SecurityState {
sanitizer: ContentSanitizer::new(&zeph_sanitizer::ContentIsolationConfig::default()),
quarantine_summarizer: None,
is_acp_session: false,
exfiltration_guard: zeph_sanitizer::exfiltration::ExfiltrationGuard::new(
zeph_sanitizer::exfiltration::ExfiltrationGuardConfig::default(),
),
Expand Down
4 changes: 4 additions & 0 deletions crates/zeph-core/src/agent/state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,10 @@ pub(crate) struct FeedbackState {
pub(crate) struct SecurityState {
pub(crate) sanitizer: ContentSanitizer,
pub(crate) quarantine_summarizer: Option<QuarantinedSummarizer>,
/// Whether this agent session is serving an ACP client.
/// When `true` and `mcp_to_acp_boundary` is enabled, MCP tool results
/// receive unconditional quarantine and cross-boundary audit logging.
pub(crate) is_acp_session: bool,
pub(crate) exfiltration_guard: zeph_sanitizer::exfiltration::ExfiltrationGuard,
pub(crate) flagged_urls: HashSet<String>,
/// URLs explicitly provided by the user across all turns in this session.
Expand Down
66 changes: 65 additions & 1 deletion crates/zeph-core/src/agent/tool_execution/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,72 @@ impl<C: Channel> Agent<C> {
}
}

// MCP-to-ACP boundary enforcement: unconditionally quarantine MCP tool results
// in ACP sessions, bypassing `should_quarantine()` source config (#2427, S10).
let is_cross_boundary = self.security.is_acp_session
&& self.runtime.security.content_isolation.mcp_to_acp_boundary
&& kind == ContentSourceKind::McpResponse;
if is_cross_boundary {
tracing::warn!(
tool = %tool_name,
mcp_server_id = tool_name.split(':').next().unwrap_or("unknown"),
"MCP tool result crossing ACP trust boundary"
);
self.push_security_event(
crate::metrics::SecurityEventCategory::CrossBoundaryMcpToAcp,
tool_name,
"MCP result force-quarantined for ACP session",
);
if let Some(ref logger) = self.tool_orchestrator.audit_logger {
let entry = zeph_tools::AuditEntry {
timestamp: zeph_tools::chrono_now(),
tool: tool_name.to_owned(),
command: String::new(),
result: zeph_tools::AuditResult::Success,
duration_ms: 0,
error_category: None,
error_domain: Some("security".to_owned()),
error_phase: None,
claim_source: None,
mcp_server_id: tool_name.split(':').next().map(ToOwned::to_owned),
injection_flagged: has_injection_flags,
embedding_anomalous: false,
cross_boundary_mcp_to_acp: true,
};
let logger = std::sync::Arc::clone(logger);
tokio::spawn(async move { logger.log(&entry).await });
}
if let Some(ref qs) = self.security.quarantine_summarizer {
match qs.extract_facts(&sanitized, &self.security.sanitizer).await {
Ok((facts, flags)) => {
self.update_metrics(|m| m.quarantine_invocations += 1);
let escaped =
zeph_sanitizer::ContentSanitizer::escape_delimiter_tags(&facts);
return (
zeph_sanitizer::ContentSanitizer::apply_spotlight(
&escaped,
&sanitized.source,
&flags,
),
has_injection_flags,
);
}
Err(e) => {
tracing::warn!(
tool = %tool_name,
error = %e,
"cross-boundary quarantine failed, using spotlighted output"
);
self.update_metrics(|m| m.quarantine_failures += 1);
}
}
}
// Quarantine summarizer unavailable or failed — fall through to spotlighted output.
}

// Quarantine step: route high-risk sources through an isolated LLM (defense-in-depth).
if self.security.sanitizer.is_enabled()
if !is_cross_boundary
&& self.security.sanitizer.is_enabled()
&& let Some(ref qs) = self.security.quarantine_summarizer
&& qs.should_quarantine(kind)
{
Expand Down
1 change: 1 addition & 0 deletions crates/zeph-core/src/agent/tool_execution/native.rs
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,7 @@ impl<C: Channel> Agent<C> {
mcp_server_id: None,
injection_flagged: false,
embedding_anomalous: false,
cross_boundary_mcp_to_acp: false,
};
let logger = std::sync::Arc::clone(logger);
tokio::spawn(async move { logger.log(&entry).await });
Expand Down
Loading
Loading