Skip to content

Commit b1c918d

Browse files
zhao-oaibolinfest
andauthored
feat: exec policy integration in shell mcp (#7609)
adding execpolicy support into the `posix` mcp Co-authored-by: Michael Bolin <mbolin@openai.com>
1 parent 4c9762d commit b1c918d

File tree

6 files changed

+171
-41
lines changed

6 files changed

+171
-41
lines changed

codex-rs/Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

codex-rs/core/src/lib.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,10 @@ mod user_shell_command;
9797
pub mod util;
9898

9999
pub use apply_patch::CODEX_APPLY_PATCH_ARG1;
100+
pub use command_safety::is_dangerous_command;
100101
pub use command_safety::is_safe_command;
102+
pub use exec_policy::ExecPolicyError;
103+
pub use exec_policy::load_exec_policy;
101104
pub use safety::get_platform_sandbox;
102105
pub use safety::set_windows_sandbox_enabled;
103106
// Re-export the protocol types from the standalone `codex-protocol` crate so existing

codex-rs/exec-server/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ anyhow = { workspace = true }
2424
async-trait = { workspace = true }
2525
clap = { workspace = true, features = ["derive"] }
2626
codex-core = { workspace = true }
27+
codex-execpolicy = { workspace = true }
2728
libc = { workspace = true }
2829
path-absolutize = { workspace = true }
2930
rmcp = { workspace = true, default-features = false, features = [

codex-rs/exec-server/src/posix.rs

Lines changed: 134 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,17 @@
5757
//!
5858
use std::path::Path;
5959
use std::path::PathBuf;
60+
use std::sync::Arc;
6061

62+
use anyhow::Context as _;
6163
use clap::Parser;
64+
use codex_core::config::find_codex_home;
65+
use codex_core::is_dangerous_command::command_might_be_dangerous;
66+
use codex_execpolicy::Decision;
67+
use codex_execpolicy::Policy;
68+
use codex_execpolicy::RuleMatch;
69+
use rmcp::ErrorData as McpError;
70+
use tokio::sync::RwLock;
6271
use tracing_subscriber::EnvFilter;
6372
use tracing_subscriber::{self};
6473

@@ -87,6 +96,11 @@ struct McpServerCli {
8796
/// Path to Bash that has been patched to support execve() wrapping.
8897
#[arg(long = "bash")]
8998
bash_path: Option<PathBuf>,
99+
100+
/// Preserve program paths when applying execpolicy (e.g., keep /usr/bin/echo instead of echo).
101+
/// Note: this does change the actual program being run.
102+
#[arg(long)]
103+
preserve_program_paths: bool,
90104
}
91105

92106
#[tokio::main]
@@ -113,13 +127,19 @@ pub async fn main_mcp_server() -> anyhow::Result<()> {
113127
Some(path) => path,
114128
None => mcp::get_bash_path()?,
115129
};
130+
let policy = Arc::new(RwLock::new(load_exec_policy().await?));
116131

117132
tracing::info!("Starting MCP server");
118-
let service = mcp::serve(bash_path, execve_wrapper, dummy_exec_policy)
119-
.await
120-
.inspect_err(|e| {
121-
tracing::error!("serving error: {:?}", e);
122-
})?;
133+
let service = mcp::serve(
134+
bash_path,
135+
execve_wrapper,
136+
policy,
137+
cli.preserve_program_paths,
138+
)
139+
.await
140+
.inspect_err(|e| {
141+
tracing::error!("serving error: {:?}", e);
142+
})?;
123143

124144
service.waiting().await?;
125145
Ok(())
@@ -146,26 +166,116 @@ pub async fn main_execve_wrapper() -> anyhow::Result<()> {
146166
std::process::exit(exit_code);
147167
}
148168

149-
// TODO: replace with execpolicy
150-
151-
fn dummy_exec_policy(file: &Path, argv: &[String], _workdir: &Path) -> ExecPolicyOutcome {
152-
if file.ends_with("rm") {
153-
ExecPolicyOutcome::Forbidden
154-
} else if file.ends_with("git") {
155-
ExecPolicyOutcome::Prompt {
156-
run_with_escalated_permissions: false,
157-
}
158-
} else if file == Path::new("/opt/homebrew/bin/gh")
159-
&& let [_, arg1, arg2, ..] = argv
160-
&& arg1 == "issue"
161-
&& arg2 == "list"
162-
{
163-
ExecPolicyOutcome::Allow {
164-
run_with_escalated_permissions: true,
169+
/// Decide how to handle an exec() call for a specific command.
170+
///
171+
/// `file` is the absolute, canonical path to the executable to run, i.e. the first arg to exec.
172+
/// `argv` is the argv, including the program name (`argv[0]`).
173+
pub(crate) fn evaluate_exec_policy(
174+
policy: &Policy,
175+
file: &Path,
176+
argv: &[String],
177+
preserve_program_paths: bool,
178+
) -> Result<ExecPolicyOutcome, McpError> {
179+
let program_name = format_program_name(file, preserve_program_paths).ok_or_else(|| {
180+
McpError::internal_error(
181+
format!("failed to format program name for `{}`", file.display()),
182+
None,
183+
)
184+
})?;
185+
let command: Vec<String> = std::iter::once(program_name)
186+
// Use the normalized program name instead of argv[0].
187+
.chain(argv.iter().skip(1).cloned())
188+
.collect();
189+
let evaluation = policy.check(&command, &|cmd| {
190+
if command_might_be_dangerous(cmd) {
191+
Decision::Prompt
192+
} else {
193+
Decision::Allow
165194
}
195+
});
196+
197+
// decisions driven by policy should run outside sandbox
198+
let decision_driven_by_policy = evaluation.matched_rules.iter().any(|rule_match| {
199+
!matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. })
200+
&& rule_match.decision() == evaluation.decision
201+
});
202+
203+
Ok(match evaluation.decision {
204+
Decision::Forbidden => ExecPolicyOutcome::Forbidden,
205+
Decision::Prompt => ExecPolicyOutcome::Prompt {
206+
run_with_escalated_permissions: decision_driven_by_policy,
207+
},
208+
Decision::Allow => ExecPolicyOutcome::Allow {
209+
run_with_escalated_permissions: decision_driven_by_policy,
210+
},
211+
})
212+
}
213+
214+
fn format_program_name(path: &Path, preserve_program_paths: bool) -> Option<String> {
215+
if preserve_program_paths {
216+
path.to_str().map(str::to_string)
166217
} else {
167-
ExecPolicyOutcome::Allow {
168-
run_with_escalated_permissions: false,
169-
}
218+
path.file_name()?.to_str().map(str::to_string)
219+
}
220+
}
221+
222+
async fn load_exec_policy() -> anyhow::Result<Policy> {
223+
let codex_home = find_codex_home().context("failed to resolve codex_home for execpolicy")?;
224+
codex_core::load_exec_policy(&codex_home)
225+
.await
226+
.map_err(anyhow::Error::from)
227+
}
228+
229+
#[cfg(test)]
230+
mod tests {
231+
use super::*;
232+
use codex_execpolicy::Decision;
233+
use codex_execpolicy::Policy;
234+
use pretty_assertions::assert_eq;
235+
use std::path::Path;
236+
237+
#[test]
238+
fn evaluate_exec_policy_uses_heuristics_for_dangerous_commands() {
239+
let policy = Policy::empty();
240+
let file = Path::new("/bin/rm");
241+
let argv = vec!["rm".to_string(), "-rf".to_string(), "/".to_string()];
242+
243+
let outcome = evaluate_exec_policy(&policy, file, &argv, false).expect("policy evaluation");
244+
245+
assert_eq!(
246+
outcome,
247+
ExecPolicyOutcome::Prompt {
248+
run_with_escalated_permissions: false
249+
}
250+
);
251+
}
252+
253+
#[test]
254+
fn evaluate_exec_policy_respects_preserve_program_paths() {
255+
let mut policy = Policy::empty();
256+
policy
257+
.add_prefix_rule(
258+
&[
259+
"/usr/local/bin/custom-cmd".to_string(),
260+
"--flag".to_string(),
261+
],
262+
Decision::Allow,
263+
)
264+
.expect("policy rule should be added");
265+
let file = Path::new("/usr/local/bin/custom-cmd");
266+
let argv = vec![
267+
"/usr/local/bin/custom-cmd".to_string(),
268+
"--flag".to_string(),
269+
"value".to_string(),
270+
];
271+
272+
let outcome = evaluate_exec_policy(&policy, file, &argv, true).expect("policy evaluation");
273+
274+
assert_eq!(
275+
outcome,
276+
ExecPolicyOutcome::Allow {
277+
run_with_escalated_permissions: true
278+
}
279+
);
170280
}
171281
}

codex-rs/exec-server/src/posix/mcp.rs

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ use codex_core::MCP_SANDBOX_STATE_CAPABILITY;
88
use codex_core::MCP_SANDBOX_STATE_NOTIFICATION;
99
use codex_core::SandboxState;
1010
use codex_core::protocol::SandboxPolicy;
11+
use codex_execpolicy::Policy;
1112
use rmcp::ErrorData as McpError;
1213
use rmcp::RoleServer;
1314
use rmcp::ServerHandler;
@@ -27,7 +28,6 @@ use tracing::debug;
2728

2829
use crate::posix::escalate_server::EscalateServer;
2930
use crate::posix::escalate_server::{self};
30-
use crate::posix::mcp_escalation_policy::ExecPolicy;
3131
use crate::posix::mcp_escalation_policy::McpEscalationPolicy;
3232
use crate::posix::stopwatch::Stopwatch;
3333

@@ -78,18 +78,25 @@ pub struct ExecTool {
7878
tool_router: ToolRouter<ExecTool>,
7979
bash_path: PathBuf,
8080
execve_wrapper: PathBuf,
81-
policy: ExecPolicy,
81+
policy: Arc<RwLock<Policy>>,
82+
preserve_program_paths: bool,
8283
sandbox_state: Arc<RwLock<Option<SandboxState>>>,
8384
}
8485

8586
#[tool_router]
8687
impl ExecTool {
87-
pub fn new(bash_path: PathBuf, execve_wrapper: PathBuf, policy: ExecPolicy) -> Self {
88+
pub fn new(
89+
bash_path: PathBuf,
90+
execve_wrapper: PathBuf,
91+
policy: Arc<RwLock<Policy>>,
92+
preserve_program_paths: bool,
93+
) -> Self {
8894
Self {
8995
tool_router: Self::tool_router(),
9096
bash_path,
9197
execve_wrapper,
9298
policy,
99+
preserve_program_paths,
93100
sandbox_state: Arc::new(RwLock::new(None)),
94101
}
95102
}
@@ -121,7 +128,12 @@ impl ExecTool {
121128
let escalate_server = EscalateServer::new(
122129
self.bash_path.clone(),
123130
self.execve_wrapper.clone(),
124-
McpEscalationPolicy::new(self.policy, context, stopwatch.clone()),
131+
McpEscalationPolicy::new(
132+
self.policy.clone(),
133+
context,
134+
stopwatch.clone(),
135+
self.preserve_program_paths,
136+
),
125137
);
126138

127139
let result = escalate_server
@@ -198,9 +210,10 @@ impl ServerHandler for ExecTool {
198210
pub(crate) async fn serve(
199211
bash_path: PathBuf,
200212
execve_wrapper: PathBuf,
201-
policy: ExecPolicy,
213+
policy: Arc<RwLock<Policy>>,
214+
preserve_program_paths: bool,
202215
) -> Result<RunningService<RoleServer, ExecTool>, rmcp::service::ServerInitializeError> {
203-
let tool = ExecTool::new(bash_path, execve_wrapper, policy);
216+
let tool = ExecTool::new(bash_path, execve_wrapper, policy, preserve_program_paths);
204217
tool.serve(stdio()).await
205218
}
206219

codex-rs/exec-server/src/posix/mcp_escalation_policy.rs

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use std::path::Path;
22

3+
use codex_execpolicy::Policy;
34
use rmcp::ErrorData as McpError;
45
use rmcp::RoleServer;
56
use rmcp::model::CreateElicitationRequestParam;
@@ -11,15 +12,10 @@ use rmcp::service::RequestContext;
1112
use crate::posix::escalate_protocol::EscalateAction;
1213
use crate::posix::escalation_policy::EscalationPolicy;
1314
use crate::posix::stopwatch::Stopwatch;
15+
use std::sync::Arc;
16+
use tokio::sync::RwLock;
1417

15-
/// This is the policy which decides how to handle an exec() call.
16-
///
17-
/// `file` is the absolute, canonical path to the executable to run, i.e. the first arg to exec.
18-
/// `argv` is the argv, including the program name (`argv[0]`).
19-
/// `workdir` is the absolute, canonical path to the working directory in which to execute the
20-
/// command.
21-
pub(crate) type ExecPolicy = fn(file: &Path, argv: &[String], workdir: &Path) -> ExecPolicyOutcome;
22-
18+
#[derive(Debug, PartialEq, Eq)]
2319
pub(crate) enum ExecPolicyOutcome {
2420
Allow {
2521
run_with_escalated_permissions: bool,
@@ -33,21 +29,25 @@ pub(crate) enum ExecPolicyOutcome {
3329
/// ExecPolicy with access to the MCP RequestContext so that it can leverage
3430
/// elicitations.
3531
pub(crate) struct McpEscalationPolicy {
36-
policy: ExecPolicy,
32+
/// In-memory execpolicy rules that drive how to handle an exec() call.
33+
policy: Arc<RwLock<Policy>>,
3734
context: RequestContext<RoleServer>,
3835
stopwatch: Stopwatch,
36+
preserve_program_paths: bool,
3937
}
4038

4139
impl McpEscalationPolicy {
4240
pub(crate) fn new(
43-
policy: ExecPolicy,
41+
policy: Arc<RwLock<Policy>>,
4442
context: RequestContext<RoleServer>,
4543
stopwatch: Stopwatch,
44+
preserve_program_paths: bool,
4645
) -> Self {
4746
Self {
4847
policy,
4948
context,
5049
stopwatch,
50+
preserve_program_paths,
5151
}
5252
}
5353

@@ -103,7 +103,9 @@ impl EscalationPolicy for McpEscalationPolicy {
103103
argv: &[String],
104104
workdir: &Path,
105105
) -> Result<EscalateAction, rmcp::ErrorData> {
106-
let outcome = (self.policy)(file, argv, workdir);
106+
let policy = self.policy.read().await;
107+
let outcome =
108+
crate::posix::evaluate_exec_policy(&policy, file, argv, self.preserve_program_paths)?;
107109
let action = match outcome {
108110
ExecPolicyOutcome::Allow {
109111
run_with_escalated_permissions,

0 commit comments

Comments
 (0)