Skip to content

Commit 720fa67

Browse files
better idempotency for creating/updating firewall rules during setup. (#8686)
make sure if the Sandbox has to re-initialize with different Sandbox user SID, it still finds/updates the firewall rule instead of creating a new one.
1 parent fabb797 commit 720fa67

File tree

2 files changed

+154
-90
lines changed

2 files changed

+154
-90
lines changed
Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
#![cfg(target_os = "windows")]
2+
3+
use anyhow::Result;
4+
use std::fs::File;
5+
use std::io::Write;
6+
7+
use windows::core::Interface;
8+
use windows::core::BSTR;
9+
use windows::Win32::Foundation::VARIANT_TRUE;
10+
use windows::Win32::NetworkManagement::WindowsFirewall::INetFwPolicy2;
11+
use windows::Win32::NetworkManagement::WindowsFirewall::INetFwRule3;
12+
use windows::Win32::NetworkManagement::WindowsFirewall::NetFwPolicy2;
13+
use windows::Win32::NetworkManagement::WindowsFirewall::NetFwRule;
14+
use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_ACTION_BLOCK;
15+
use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_IP_PROTOCOL_ANY;
16+
use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_PROFILE2_ALL;
17+
use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_RULE_DIR_OUT;
18+
use windows::Win32::System::Com::CoCreateInstance;
19+
use windows::Win32::System::Com::CoInitializeEx;
20+
use windows::Win32::System::Com::CoUninitialize;
21+
use windows::Win32::System::Com::CLSCTX_INPROC_SERVER;
22+
use windows::Win32::System::Com::COINIT_APARTMENTTHREADED;
23+
24+
// This is the stable identifier we use to find/update the rule idempotently.
25+
// It intentionally does not change between installs.
26+
const OFFLINE_BLOCK_RULE_NAME: &str = "codex_sandbox_offline_block_outbound";
27+
28+
// Friendly text shown in the firewall UI.
29+
const OFFLINE_BLOCK_RULE_FRIENDLY: &str = "Codex Sandbox Offline - Block Outbound";
30+
31+
pub fn ensure_offline_outbound_block(offline_sid: &str, log: &mut File) -> Result<()> {
32+
let local_user_spec = format!("O:LSD:(A;;CC;;;{offline_sid})");
33+
34+
let hr = unsafe { CoInitializeEx(None, COINIT_APARTMENTTHREADED) };
35+
if hr.is_err() {
36+
return Err(anyhow::anyhow!("CoInitializeEx failed: {hr:?}"));
37+
}
38+
39+
let result = unsafe {
40+
(|| -> Result<()> {
41+
let policy: INetFwPolicy2 = CoCreateInstance(&NetFwPolicy2, None, CLSCTX_INPROC_SERVER)
42+
.map_err(|e| anyhow::anyhow!("CoCreateInstance NetFwPolicy2: {e:?}"))?;
43+
let rules = policy
44+
.Rules()
45+
.map_err(|e| anyhow::anyhow!("INetFwPolicy2::Rules: {e:?}"))?;
46+
47+
// Block all outbound IP protocols for this user.
48+
ensure_block_rule(
49+
&rules,
50+
OFFLINE_BLOCK_RULE_NAME,
51+
OFFLINE_BLOCK_RULE_FRIENDLY,
52+
NET_FW_IP_PROTOCOL_ANY.0,
53+
&local_user_spec,
54+
offline_sid,
55+
log,
56+
)?;
57+
Ok(())
58+
})()
59+
};
60+
61+
unsafe {
62+
CoUninitialize();
63+
}
64+
result
65+
}
66+
67+
fn ensure_block_rule(
68+
rules: &windows::Win32::NetworkManagement::WindowsFirewall::INetFwRules,
69+
internal_name: &str,
70+
friendly_desc: &str,
71+
protocol: i32,
72+
local_user_spec: &str,
73+
offline_sid: &str,
74+
log: &mut File,
75+
) -> Result<()> {
76+
let name = BSTR::from(internal_name);
77+
let rule: INetFwRule3 = match unsafe { rules.Item(&name) } {
78+
Ok(existing) => existing
79+
.cast()
80+
.map_err(|e| anyhow::anyhow!("cast existing firewall rule to INetFwRule3: {e:?}"))?,
81+
Err(_) => {
82+
let new_rule: INetFwRule3 =
83+
unsafe { CoCreateInstance(&NetFwRule, None, CLSCTX_INPROC_SERVER) }
84+
.map_err(|e| anyhow::anyhow!("CoCreateInstance NetFwRule: {e:?}"))?;
85+
unsafe { new_rule.SetName(&name) }.map_err(|e| anyhow::anyhow!("SetName: {e:?}"))?;
86+
// Set all properties before adding the rule so we don't leave half-configured rules.
87+
configure_rule(
88+
&new_rule,
89+
friendly_desc,
90+
protocol,
91+
local_user_spec,
92+
offline_sid,
93+
)?;
94+
unsafe { rules.Add(&new_rule) }.map_err(|e| anyhow::anyhow!("Rules::Add: {e:?}"))?;
95+
new_rule
96+
}
97+
};
98+
99+
// Always re-apply fields to keep the setup idempotent.
100+
configure_rule(&rule, friendly_desc, protocol, local_user_spec, offline_sid)?;
101+
102+
log_line(
103+
log,
104+
&format!(
105+
"firewall rule configured name={internal_name} protocol={protocol} LocalUserAuthorizedList={local_user_spec}"
106+
),
107+
)?;
108+
Ok(())
109+
}
110+
111+
fn configure_rule(
112+
rule: &INetFwRule3,
113+
friendly_desc: &str,
114+
protocol: i32,
115+
local_user_spec: &str,
116+
offline_sid: &str,
117+
) -> Result<()> {
118+
unsafe {
119+
rule.SetDescription(&BSTR::from(friendly_desc))
120+
.map_err(|e| anyhow::anyhow!("SetDescription: {e:?}"))?;
121+
rule.SetDirection(NET_FW_RULE_DIR_OUT)
122+
.map_err(|e| anyhow::anyhow!("SetDirection: {e:?}"))?;
123+
rule.SetAction(NET_FW_ACTION_BLOCK)
124+
.map_err(|e| anyhow::anyhow!("SetAction: {e:?}"))?;
125+
rule.SetEnabled(VARIANT_TRUE)
126+
.map_err(|e| anyhow::anyhow!("SetEnabled: {e:?}"))?;
127+
rule.SetProfiles(NET_FW_PROFILE2_ALL.0)
128+
.map_err(|e| anyhow::anyhow!("SetProfiles: {e:?}"))?;
129+
rule.SetProtocol(protocol)
130+
.map_err(|e| anyhow::anyhow!("SetProtocol: {e:?}"))?;
131+
rule.SetLocalUserAuthorizedList(&BSTR::from(local_user_spec))
132+
.map_err(|e| anyhow::anyhow!("SetLocalUserAuthorizedList: {e:?}"))?;
133+
}
134+
135+
// Read-back verification: ensure we actually wrote the expected SID scope.
136+
let actual = unsafe { rule.LocalUserAuthorizedList() }
137+
.map_err(|e| anyhow::anyhow!("LocalUserAuthorizedList (read-back): {e:?}"))?;
138+
let actual_str = actual.to_string();
139+
if !actual_str.contains(offline_sid) {
140+
anyhow::bail!(
141+
"offline firewall rule user scope mismatch: expected SID {offline_sid}, got {actual_str}"
142+
);
143+
}
144+
Ok(())
145+
}
146+
147+
fn log_line(log: &mut File, msg: &str) -> Result<()> {
148+
let ts = chrono::Utc::now().to_rfc3339();
149+
writeln!(log, "[{ts}] {msg}")?;
150+
Ok(())
151+
}

codex-rs/windows-sandbox-rs/src/setup_main_win.rs

Lines changed: 3 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#![cfg(target_os = "windows")]
22

3+
mod firewall;
4+
35
use anyhow::Context;
46
use anyhow::Result;
57
use base64::engine::general_purpose::STANDARD as BASE64;
@@ -28,22 +30,6 @@ use std::path::PathBuf;
2830
use std::process::Command;
2931
use std::process::Stdio;
3032
use std::sync::mpsc;
31-
use windows::core::Interface;
32-
use windows::core::BSTR;
33-
use windows::Win32::Foundation::VARIANT_TRUE;
34-
use windows::Win32::NetworkManagement::WindowsFirewall::INetFwPolicy2;
35-
use windows::Win32::NetworkManagement::WindowsFirewall::INetFwRule3;
36-
use windows::Win32::NetworkManagement::WindowsFirewall::NetFwPolicy2;
37-
use windows::Win32::NetworkManagement::WindowsFirewall::NetFwRule;
38-
use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_ACTION_BLOCK;
39-
use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_IP_PROTOCOL_ANY;
40-
use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_PROFILE2_ALL;
41-
use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_RULE_DIR_OUT;
42-
use windows::Win32::System::Com::CoCreateInstance;
43-
use windows::Win32::System::Com::CoInitializeEx;
44-
use windows::Win32::System::Com::CoUninitialize;
45-
use windows::Win32::System::Com::CLSCTX_INPROC_SERVER;
46-
use windows::Win32::System::Com::COINIT_APARTMENTTHREADED;
4733
use windows_sys::Win32::Foundation::GetLastError;
4834
use windows_sys::Win32::Foundation::LocalFree;
4935
use windows_sys::Win32::Foundation::HLOCAL;
@@ -239,79 +225,6 @@ fn read_mask_allows_or_log(
239225
}
240226
}
241227

242-
fn run_netsh_firewall(sid: &str, log: &mut File) -> Result<()> {
243-
let local_user_spec = format!("O:LSD:(A;;CC;;;{sid})");
244-
let hr = unsafe { CoInitializeEx(None, COINIT_APARTMENTTHREADED) };
245-
if hr.is_err() {
246-
return Err(anyhow::anyhow!("CoInitializeEx failed: {hr:?}"));
247-
}
248-
let result = unsafe {
249-
(|| -> Result<()> {
250-
let policy: INetFwPolicy2 = CoCreateInstance(&NetFwPolicy2, None, CLSCTX_INPROC_SERVER)
251-
.map_err(|e| anyhow::anyhow!("CoCreateInstance NetFwPolicy2: {e:?}"))?;
252-
let rules = policy
253-
.Rules()
254-
.map_err(|e| anyhow::anyhow!("INetFwPolicy2::Rules: {e:?}"))?;
255-
let name = BSTR::from("Codex Sandbox Offline - Block Outbound");
256-
let rule: INetFwRule3 = match rules.Item(&name) {
257-
Ok(existing) => existing.cast().map_err(|e| {
258-
anyhow::anyhow!("cast existing firewall rule to INetFwRule3: {e:?}")
259-
})?,
260-
Err(_) => {
261-
let new_rule: INetFwRule3 =
262-
CoCreateInstance(&NetFwRule, None, CLSCTX_INPROC_SERVER)
263-
.map_err(|e| anyhow::anyhow!("CoCreateInstance NetFwRule: {e:?}"))?;
264-
new_rule
265-
.SetName(&name)
266-
.map_err(|e| anyhow::anyhow!("SetName: {e:?}"))?;
267-
new_rule
268-
.SetDirection(NET_FW_RULE_DIR_OUT)
269-
.map_err(|e| anyhow::anyhow!("SetDirection: {e:?}"))?;
270-
new_rule
271-
.SetAction(NET_FW_ACTION_BLOCK)
272-
.map_err(|e| anyhow::anyhow!("SetAction: {e:?}"))?;
273-
new_rule
274-
.SetEnabled(VARIANT_TRUE)
275-
.map_err(|e| anyhow::anyhow!("SetEnabled: {e:?}"))?;
276-
new_rule
277-
.SetProfiles(NET_FW_PROFILE2_ALL.0)
278-
.map_err(|e| anyhow::anyhow!("SetProfiles: {e:?}"))?;
279-
new_rule
280-
.SetProtocol(NET_FW_IP_PROTOCOL_ANY.0)
281-
.map_err(|e| anyhow::anyhow!("SetProtocol: {e:?}"))?;
282-
rules
283-
.Add(&new_rule)
284-
.map_err(|e| anyhow::anyhow!("Rules::Add: {e:?}"))?;
285-
new_rule
286-
}
287-
};
288-
rule.SetLocalUserAuthorizedList(&BSTR::from(local_user_spec.as_str()))
289-
.map_err(|e| anyhow::anyhow!("SetLocalUserAuthorizedList: {e:?}"))?;
290-
rule.SetEnabled(VARIANT_TRUE)
291-
.map_err(|e| anyhow::anyhow!("SetEnabled: {e:?}"))?;
292-
rule.SetProfiles(NET_FW_PROFILE2_ALL.0)
293-
.map_err(|e| anyhow::anyhow!("SetProfiles: {e:?}"))?;
294-
rule.SetAction(NET_FW_ACTION_BLOCK)
295-
.map_err(|e| anyhow::anyhow!("SetAction: {e:?}"))?;
296-
rule.SetDirection(NET_FW_RULE_DIR_OUT)
297-
.map_err(|e| anyhow::anyhow!("SetDirection: {e:?}"))?;
298-
rule.SetProtocol(NET_FW_IP_PROTOCOL_ANY.0)
299-
.map_err(|e| anyhow::anyhow!("SetProtocol: {e:?}"))?;
300-
log_line(
301-
log,
302-
&format!(
303-
"firewall rule configured via COM with LocalUserAuthorizedList={local_user_spec}"
304-
),
305-
)?;
306-
Ok(())
307-
})()
308-
};
309-
unsafe {
310-
CoUninitialize();
311-
}
312-
result
313-
}
314-
315228
fn lock_sandbox_dir(
316229
dir: &Path,
317230
real_user: &str,
@@ -549,7 +462,7 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<(
549462
};
550463
let mut refresh_errors: Vec<String> = Vec::new();
551464
if !refresh_only {
552-
run_netsh_firewall(&offline_sid_str, log)?;
465+
firewall::ensure_offline_outbound_block(&offline_sid_str, log)?;
553466
}
554467

555468
if payload.read_roots.is_empty() {

0 commit comments

Comments
 (0)