Skip to content

Commit c608c13

Browse files
authored
fix(prover): distinguish sibling endpoints and fail closed on solver errors (#4326)
Signed-off-by: Adrien Langou <alangou@nvidia.com>
1 parent fe743a3 commit c608c13

7 files changed

Lines changed: 467 additions & 61 deletions

File tree

‎crates/openshell-prover/README.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,12 @@ not form part of the authored policy schema.
186186

187187
See `src/model.rs`. Briefly:
188188

189+
- Endpoint facts are indexed by rule, endpoint position, and effective port.
190+
Sibling endpoints retain their own grants even when their destinations overlap.
191+
- Query entrypoints return an error if the base constraints are inconsistent or
192+
the solver cannot reach a conclusion. Callers must propagate that error; it
193+
cannot be accepted as a finding or compared as an empty finding list.
194+
189195
- Bool sorts per `(binary, endpoint)` pair encode policy reachability,
190196
filtered by binary capability flags (`can_exfiltrate`,
191197
`bypasses_l7`).

‎crates/openshell-prover/src/lib.rs‎

Lines changed: 104 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ use report::{render_compact, render_report};
3131
///
3232
/// - `Ok(0)` — pass (no findings, or all accepted)
3333
/// - `Ok(1)` — fail (one or more unaccepted findings present)
34-
/// - `Err(_)` — input or registry loading error
34+
/// - `Err(_)` — input, registry, or solver error
3535
///
3636
/// Binary and API capability registries are embedded at compile time.
3737
/// Pass `registry_dir` to override with a custom filesystem registry.
@@ -59,7 +59,7 @@ pub fn prove(
5959
};
6060

6161
let z3_model = build_model(policy, credential_set, binary_registry);
62-
let mut findings = run_all_queries(&z3_model);
62+
let mut findings = run_all_queries(&z3_model)?;
6363

6464
if let Some(ar_path) = accepted_risks_path {
6565
let accepted = load_accepted_risks(Path::new(ar_path))?;
@@ -223,7 +223,7 @@ filesystem_policy:
223223
let bin_reg = registry::load_embedded_binary_registry().expect("load registry");
224224

225225
let z3_model = build_model(pol, cred_set, bin_reg);
226-
let findings = run_all_queries(&z3_model);
226+
let findings = run_all_queries(&z3_model).expect("run queries");
227227

228228
let categories: std::collections::HashSet<&str> =
229229
findings.iter().map(|f| f.query.as_str()).collect();
@@ -276,7 +276,7 @@ network_policies:
276276
let bin_reg = registry::load_embedded_binary_registry().expect("load registry");
277277

278278
let z3_model = build_model(policy, cred_set, bin_reg);
279-
let findings = run_all_queries(&z3_model);
279+
let findings = run_all_queries(&z3_model).expect("run queries");
280280

281281
let reach = findings
282282
.iter()
@@ -310,7 +310,7 @@ network_policies:
310310
let bin_reg = registry::load_embedded_binary_registry().expect("load registry");
311311

312312
let z3_model = build_model(policy, credentials::CredentialSet::default(), bin_reg);
313-
let findings = run_all_queries(&z3_model);
313+
let findings = run_all_queries(&z3_model).expect("run queries");
314314

315315
let link_local = findings
316316
.iter()
@@ -323,7 +323,104 @@ network_policies:
323323
)));
324324
}
325325

326-
// 7. Empty policy produces no findings.
326+
#[test]
327+
fn sibling_endpoints_preserve_findings_and_method_identity() {
328+
use finding::{FindingPath, category};
329+
330+
// Exercise both conflicting write facts and agreeing write facts with
331+
// different methods, in either endpoint order and on overlapping ports.
332+
for (first_method, second_method) in [("GET", "*"), ("POST", "DELETE")] {
333+
for reverse in [false, true] {
334+
let mut policy = policy::parse_policy_str(&format!(
335+
r#"
336+
version: 1
337+
network_policies:
338+
github:
339+
endpoints:
340+
- host: api.github.com
341+
ports: [443, 8443]
342+
protocol: rest
343+
rules:
344+
- allow: {{method: "{first_method}", path: "/**"}}
345+
- host: api.github.com
346+
port: 443
347+
protocol: rest
348+
rules:
349+
- allow: {{method: "{second_method}", path: "/**"}}
350+
binaries:
351+
- path: /usr/bin/curl
352+
metadata:
353+
endpoints:
354+
- host: 169.254.169.254
355+
port: 80
356+
binaries:
357+
- path: /usr/bin/curl
358+
"#
359+
))
360+
.unwrap();
361+
if reverse {
362+
policy
363+
.network_policies
364+
.get_mut("github")
365+
.unwrap()
366+
.endpoints
367+
.reverse();
368+
}
369+
let credentials = credentials::load_credential_set_embedded(
370+
&testdata_dir().join("credentials.yaml"),
371+
)
372+
.unwrap();
373+
let registry = registry::load_embedded_binary_registry().unwrap();
374+
let model = build_model(policy, credentials, registry);
375+
let findings = run_all_queries(&model).unwrap();
376+
assert!(
377+
findings
378+
.iter()
379+
.any(|f| f.query == category::LINK_LOCAL_REACH)
380+
);
381+
assert!(
382+
findings
383+
.iter()
384+
.any(|f| f.query == category::CREDENTIAL_REACH_EXPANSION)
385+
);
386+
let methods = &findings
387+
.iter()
388+
.find(|f| f.query == category::CAPABILITY_EXPANSION)
389+
.unwrap()
390+
.paths;
391+
assert!(methods.iter().any(|p| matches!(p, FindingPath::Exfil(p) if p.method == "DELETE" && p.endpoint_port == 443)));
392+
assert!(!methods.iter().any(|p| matches!(p, FindingPath::Exfil(p) if p.method == "DELETE" && p.endpoint_port == 8443)));
393+
}
394+
}
395+
}
396+
397+
#[test]
398+
fn sibling_l4_and_l7_endpoints_preserve_reach() {
399+
let policy = policy::parse_policy_str(
400+
r"
401+
version: 1
402+
network_policies:
403+
metadata:
404+
endpoints:
405+
- host: 169.254.169.254
406+
port: 80
407+
protocol: rest
408+
access: read-only
409+
- host: 169.254.169.254
410+
port: 80
411+
binaries:
412+
- path: /usr/bin/curl
413+
",
414+
)
415+
.unwrap();
416+
let registry = registry::load_embedded_binary_registry().unwrap();
417+
let model = build_model(policy, credentials::CredentialSet::default(), registry);
418+
let findings = run_all_queries(&model).unwrap();
419+
assert_eq!(findings.len(), 1);
420+
assert_eq!(findings[0].query, finding::category::LINK_LOCAL_REACH);
421+
}
422+
423+
// Empty policy produces no findings.
327424
#[test]
328425
fn test_empty_policy_no_findings() {
329426
let policy_path = testdata_dir().join("empty-policy.yaml");
@@ -334,7 +431,7 @@ network_policies:
334431
let bin_reg = registry::load_embedded_binary_registry().expect("load registry");
335432

336433
let z3_model = build_model(pol, cred_set, bin_reg);
337-
let findings = run_all_queries(&z3_model);
434+
let findings = run_all_queries(&z3_model).expect("run queries");
338435

339436
assert!(
340437
findings.is_empty(),

‎crates/openshell-prover/src/model.rs‎

Lines changed: 77 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,23 @@ use crate::registry::BinaryRegistry;
1616
#[derive(Debug, Clone, Hash, PartialEq, Eq)]
1717
pub struct EndpointId {
1818
pub policy_name: String,
19+
/// Position within the policy rule's endpoint list.
20+
pub endpoint_index: usize,
1921
pub host: String,
2022
pub port: u16,
2123
}
2224

2325
impl EndpointId {
2426
/// Stable string key used for Z3 variable naming.
2527
pub fn key(&self) -> String {
26-
format!("{}:{}:{}", self.policy_name, self.host, self.port)
28+
// Length-prefix the authored name so delimiters cannot alias another rule.
29+
format!(
30+
"{}:{}:{}:{}",
31+
self.policy_name.len(),
32+
self.policy_name,
33+
self.endpoint_index,
34+
self.port
35+
)
2736
}
2837
}
2938

@@ -97,10 +106,11 @@ impl ReachabilityModel {
97106

98107
fn index_endpoints(&mut self) {
99108
for (policy_name, rule) in &self.policy.network_policies {
100-
for ep in &rule.endpoints {
109+
for (endpoint_index, ep) in rule.endpoints.iter().enumerate() {
101110
for port in ep.effective_ports() {
102111
self.endpoints.push(EndpointId {
103112
policy_name: policy_name.clone(),
113+
endpoint_index,
104114
host: ep.host.clone(),
105115
port,
106116
});
@@ -122,15 +132,16 @@ impl ReachabilityModel {
122132

123133
fn encode_policy_allows(&mut self) {
124134
for (policy_name, rule) in &self.policy.network_policies {
125-
for ep in &rule.endpoints {
135+
for (endpoint_index, ep) in rule.endpoints.iter().enumerate() {
126136
for port in ep.effective_ports() {
127137
let eid = EndpointId {
128138
policy_name: policy_name.clone(),
139+
endpoint_index,
129140
host: ep.host.clone(),
130141
port,
131142
};
132143
for b in &rule.binaries {
133-
let key = format!("{}:{}", b.path, eid.key());
144+
let key = format!("{}:{}:{}", b.path.len(), b.path, eid.key());
134145
let var = Bool::new_const(format!("policy_allows_{key}"));
135146
self.solver.assert(&var);
136147
self.policy_allows.insert(key, var);
@@ -142,10 +153,11 @@ impl ReachabilityModel {
142153

143154
fn encode_l7_enforcement(&mut self) {
144155
for (policy_name, rule) in &self.policy.network_policies {
145-
for ep in &rule.endpoints {
156+
for (endpoint_index, ep) in rule.endpoints.iter().enumerate() {
146157
for port in ep.effective_ports() {
147158
let eid = EndpointId {
148159
policy_name: policy_name.clone(),
160+
endpoint_index,
149161
host: ep.host.clone(),
150162
port,
151163
};
@@ -284,7 +296,7 @@ impl ReachabilityModel {
284296
/// Build a Z3 expression for whether a binary can write to an endpoint.
285297
pub fn can_write_to_endpoint(&self, bpath: &str, eid: &EndpointId) -> Bool {
286298
let ek = eid.key();
287-
let access_key = format!("{bpath}:{ek}");
299+
let access_key = format!("{}:{bpath}:{ek}", bpath.len());
288300

289301
let has_access = match self.policy_allows.get(&access_key) {
290302
Some(v) => v.clone(),
@@ -328,7 +340,7 @@ impl ReachabilityModel {
328340
/// Build a Z3 expression for whether data can be exfiltrated via this path.
329341
pub fn can_exfil_via_endpoint(&self, bpath: &str, eid: &EndpointId) -> Bool {
330342
let ek = eid.key();
331-
let access_key = format!("{bpath}:{ek}");
343+
let access_key = format!("{}:{bpath}:{ek}", bpath.len());
332344

333345
let has_access = match self.policy_allows.get(&access_key) {
334346
Some(v) => v.clone(),
@@ -372,6 +384,15 @@ impl ReachabilityModel {
372384
])
373385
}
374386

387+
/// Reject inconsistent or inconclusive base constraints before querying them.
388+
pub fn validate(&self) -> miette::Result<()> {
389+
match self.solver.check() {
390+
SatResult::Sat => Ok(()),
391+
SatResult::Unsat => miette::bail!("inconsistent reachability model"),
392+
SatResult::Unknown => miette::bail!("reachability model validation inconclusive"),
393+
}
394+
}
395+
375396
/// Check satisfiability of an expression against the base constraints.
376397
pub fn check_sat(&self, expr: &Bool) -> SatResult {
377398
self.solver.push();
@@ -392,3 +413,52 @@ pub fn build_model(
392413
let _ctx = Context::thread_local();
393414
ReachabilityModel::new(policy, credentials, binary_registry)
394415
}
416+
417+
#[cfg(test)]
418+
mod tests {
419+
use super::*;
420+
use crate::queries::{check_credential_safety, run_all_queries};
421+
422+
#[test]
423+
fn inconsistent_base_model_is_an_error_even_without_query_paths() {
424+
let model = build_model(
425+
PolicyModel::default(),
426+
CredentialSet::default(),
427+
crate::registry::load_embedded_binary_registry().unwrap(),
428+
);
429+
model.solver.assert(Bool::from_bool(false));
430+
assert!(check_credential_safety(&model).is_err());
431+
assert!(run_all_queries(&model).is_err());
432+
}
433+
434+
#[test]
435+
fn inconclusive_base_model_is_an_error() {
436+
let model = build_model(
437+
PolicyModel::default(),
438+
CredentialSet::default(),
439+
crate::registry::load_embedded_binary_registry().unwrap(),
440+
);
441+
model.solver.assert(Bool::new_const("resource_limited"));
442+
let mut params = z3::Params::new();
443+
params.set_u32("rlimit", 1);
444+
model.solver.set_params(&params);
445+
assert_eq!(model.solver.check(), SatResult::Unknown);
446+
assert!(run_all_queries(&model).is_err());
447+
}
448+
449+
#[test]
450+
fn endpoint_keys_distinguish_rule_names_with_delimiters() {
451+
let left = EndpointId {
452+
policy_name: "rule:2001".to_string(),
453+
endpoint_index: 0,
454+
host: "db8::1".to_string(),
455+
port: 443,
456+
};
457+
let right = EndpointId {
458+
policy_name: "rule".to_string(),
459+
host: "2001:db8::1".to_string(),
460+
..left
461+
};
462+
assert_ne!(left.key(), right.key());
463+
}
464+
}

0 commit comments

Comments
 (0)