From e51d0dee44ecabbf7be9e855753453bb2f61cced Mon Sep 17 00:00:00 2001 From: Firstyear Date: Tue, 26 Mar 2024 11:43:03 +1000 Subject: [PATCH] [SECURITY: LOW] Administrator triggered thread crash in oauth2 claim maps #2686 (#2686) When an admin configured oauth2 custom claims during the creation it was not enforced that at least one value must be present. This led to an incorrect logic flaw in str_concat! which didn't handle the 0 case. This hardens str_concat! to prevent the thread crash by using itertools for the join instead, and it enforces stricter validation on the valueset to deny creation of empty claims. This fix has a low security impact as only an administrator or high level user can trigger this as a possible denial of service. Fixes #2680 Fixes #2681 --- Cargo.lock | 1 + Cargo.toml | 1 + server/lib/Cargo.toml | 1 + server/lib/src/idm/oauth2.rs | 7 ++-- server/lib/src/macros.rs | 18 ++-------- server/lib/src/server/access/mod.rs | 16 +++------ server/lib/src/server/mod.rs | 21 +++++++++++ server/lib/src/value.rs | 11 ++++++ server/lib/src/valueset/mod.rs | 1 - server/lib/src/valueset/oauth.rs | 55 ++++++++++++++++++++++------- 10 files changed, 87 insertions(+), 45 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5b6349079..29d869eff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3192,6 +3192,7 @@ dependencies = [ "hex", "idlset", "image 0.24.8", + "itertools 0.12.1", "kanidm_build_profiles", "kanidm_lib_crypto", "kanidm_proto", diff --git a/Cargo.toml b/Cargo.toml index 5dc577a45..85c1a857e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -144,6 +144,7 @@ image = { version = "0.24.7", default-features = false, features = [ "jpeg", "webp", ] } +itertools = "0.12.1" enum-iterator = "1.4.0" js-sys = "^0.3.65" kanidmd_web_ui_shared = { path = "./server/web_ui/shared" } diff --git a/server/lib/Cargo.toml b/server/lib/Cargo.toml index b4fc6a6ac..1f01a3e39 100644 --- a/server/lib/Cargo.toml +++ b/server/lib/Cargo.toml @@ -36,6 +36,7 @@ fernet = { workspace = true, features = ["fernet_danger_timestamps"] } # futures-util = { workspace = true } hashbrown = { workspace = true } idlset = { workspace = true } +itertools = { workspace = true } kanidm_proto = { workspace = true } kanidm_lib_crypto = { workspace = true } lazy_static = { workspace = true } diff --git a/server/lib/src/idm/oauth2.rs b/server/lib/src/idm/oauth2.rs index dde4a250d..b38776d69 100644 --- a/server/lib/src/idm/oauth2.rs +++ b/server/lib/src/idm/oauth2.rs @@ -254,9 +254,7 @@ impl ClaimValue { } fn to_json_value(&self) -> serde_json::Value { - let join_char = match self.join { - OauthClaimMapJoin::CommaSeparatedValue => ',', - OauthClaimMapJoin::SpaceSeparatedValue => ' ', + let join_str = match self.join { OauthClaimMapJoin::JsonArray => { let arr: Vec<_> = self .values @@ -268,9 +266,10 @@ impl ClaimValue { // This shortcuts out. return serde_json::Value::Array(arr); } + joiner => joiner.to_str(), }; - let joined = str_concat!(&self.values, join_char); + let joined = str_concat!(&self.values, join_str); serde_json::Value::String(joined) } diff --git a/server/lib/src/macros.rs b/server/lib/src/macros.rs index b7222584a..d98236a6e 100644 --- a/server/lib/src/macros.rs +++ b/server/lib/src/macros.rs @@ -655,21 +655,7 @@ macro_rules! limmediate_warning { macro_rules! str_concat { ($str_iter:expr, $join_char:expr) => {{ - // Sub 1 here because we need N minus 1 join chars - let max_join_chars: usize = $str_iter.len() - 1; - let data_len: usize = $str_iter - .iter() - .map(|s| s.len()) - .fold(max_join_chars, |acc, x| acc + x); - - let mut joined = String::with_capacity(data_len); - for (i, value) in $str_iter.iter().enumerate() { - joined.push_str(value); - if i < max_join_chars { - joined.push($join_char); - } - } - - joined + use itertools::Itertools; + $str_iter.iter().join($join_char) }}; } diff --git a/server/lib/src/server/access/mod.rs b/server/lib/src/server/access/mod.rs index ec842ff56..013d96e2a 100644 --- a/server/lib/src/server/access/mod.rs +++ b/server/lib/src/server/access/mod.rs @@ -493,12 +493,8 @@ pub trait AccessControlsTransaction<'a> { ); false } else if !requested_rem.is_subset(&rem) { - security_access!("requested_rem is not a subset of allowed"); - security_access!( - "requested_rem: {:?} !⊆ allowed: {:?}", - requested_rem, - rem - ); + security_error!("requested_rem is not a subset of allowed"); + security_error!("requested_rem: {:?} !⊆ allowed: {:?}", requested_rem, rem); false } else if !requested_classes.is_subset(&cls) { security_access!("requested_classes is not a subset of allowed"); @@ -626,12 +622,8 @@ pub trait AccessControlsTransaction<'a> { ); false } else if !requested_rem.is_subset(&rem) { - security_access!("requested_rem is not a subset of allowed"); - security_access!( - "requested_rem: {:?} !⊆ allowed: {:?}", - requested_rem, - rem - ); + security_error!("requested_rem is not a subset of allowed"); + security_error!("requested_rem: {:?} !⊆ allowed: {:?}", requested_rem, rem); false } else if !requested_classes.is_subset(&cls) { security_access!("requested_classes is not a subset of allowed"); diff --git a/server/lib/src/server/mod.rs b/server/lib/src/server/mod.rs index 692f2972a..dfe84f27b 100644 --- a/server/lib/src/server/mod.rs +++ b/server/lib/src/server/mod.rs @@ -764,6 +764,27 @@ pub trait QueryServerTransaction<'a> { }) .collect(); v + } else if let Some(r_map) = value.as_oauthclaim_map() { + let mut v = Vec::new(); + for (claim_name, mapping) in r_map.iter() { + for (group_ref, claims) in mapping.values() { + let join_char = mapping.join().to_str(); + + let nv = self.uuid_to_spn(*group_ref)?; + let resolved_id = match nv { + Some(v) => v.to_proto_string_clone(), + None => uuid_to_proto_string(*group_ref), + }; + + let joined = str_concat!(claims, ","); + + v.push(format!( + "{}:{}:{}:{:?}", + claim_name, resolved_id, join_char, joined + )) + } + } + Ok(v) } else { let v: Vec<_> = value.to_proto_string_clone_iter().collect(); Ok(v) diff --git a/server/lib/src/value.rs b/server/lib/src/value.rs index 681348b93..8fcf3fa0b 100644 --- a/server/lib/src/value.rs +++ b/server/lib/src/value.rs @@ -1000,6 +1000,17 @@ pub enum OauthClaimMapJoin { JsonArray, } +impl OauthClaimMapJoin { + pub(crate) fn to_str(self) -> &'static str { + match self { + OauthClaimMapJoin::CommaSeparatedValue => ",", + OauthClaimMapJoin::SpaceSeparatedValue => " ", + // Should this be something else? + OauthClaimMapJoin::JsonArray => ";", + } + } +} + impl From for OauthClaimMapJoin { fn from(value: DbValueOauthClaimMapJoinV1) -> OauthClaimMapJoin { match value { diff --git a/server/lib/src/valueset/mod.rs b/server/lib/src/valueset/mod.rs index 440bd204e..c79f791ea 100644 --- a/server/lib/src/valueset/mod.rs +++ b/server/lib/src/valueset/mod.rs @@ -368,7 +368,6 @@ pub trait ValueSetT: std::fmt::Debug + DynClone { } fn as_oauthclaim_map(&self) -> Option<&BTreeMap> { - debug_assert!(false); None } diff --git a/server/lib/src/valueset/oauth.rs b/server/lib/src/valueset/oauth.rs index 00d276a30..892cfc7e1 100644 --- a/server/lib/src/valueset/oauth.rs +++ b/server/lib/src/valueset/oauth.rs @@ -463,6 +463,14 @@ impl ValueSetOauthClaimMap { Some(Box::new(ValueSetOauthClaimMap { map })) } */ + + fn trim(&mut self) { + self.map + .values_mut() + .for_each(|mapping_mut| mapping_mut.values.retain(|_k, v| !v.is_empty())); + + self.map.retain(|_k, v| !v.values.is_empty()); + } } impl ValueSetT for ValueSetOauthClaimMap { @@ -561,11 +569,7 @@ impl ValueSetT for ValueSetOauthClaimMap { }; // Trim anything that is now empty. - self.map - .values_mut() - .for_each(|mapping_mut| mapping_mut.values.retain(|_k, v| !v.is_empty())); - - self.map.retain(|_k, v| !v.values.is_empty()); + self.trim(); res } @@ -622,6 +626,16 @@ impl ValueSetT for ValueSetOauthClaimMap { fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { self.map.keys().all(|s| OAUTHSCOPE_RE.is_match(s)) + && self + .map + .values() + .flat_map(|mapping| { + mapping + .values + .values() + .map(|claim_values| claim_values.is_empty()) + }) + .all(|is_empty| !is_empty) && self .map .values() @@ -637,14 +651,9 @@ impl ValueSetT for ValueSetOauthClaimMap { fn to_proto_string_clone_iter(&self) -> Box + '_> { Box::new(self.map.iter().flat_map(|(name, mapping)| { mapping.values.iter().map(move |(group, claims)| { - let join_char = match mapping.join { - OauthClaimMapJoin::CommaSeparatedValue => ',', - OauthClaimMapJoin::SpaceSeparatedValue => ' ', - // Should this be something else? - OauthClaimMapJoin::JsonArray => ';', - }; + let join_str = mapping.join.to_str(); - let joined = str_concat!(claims, join_char); + let joined = str_concat!(claims, join_str); format!( "{}: {} \"{:?}\"", @@ -730,3 +739,25 @@ impl ValueSetT for ValueSetOauthClaimMap { )) } } + +#[cfg(test)] +mod tests { + use super::ValueSetOauthClaimMap; + use crate::valueset::ValueSetT; + use std::collections::BTreeSet; + + #[test] + fn test_oauth_claim_invalid_str_concat_when_empty() { + let group_uuid = uuid::uuid!("5a6b8783-3f67-4ebb-b6aa-77fd6e66589f"); + let vs = + ValueSetOauthClaimMap::new_value("claim".to_string(), group_uuid, BTreeSet::default()); + + // Invalid handling of an empty claim map would cause a crash. + let proto_value = vs.to_proto_string_clone_iter().next().unwrap(); + + assert_eq!( + &proto_value, + "claim: 5a6b8783-3f67-4ebb-b6aa-77fd6e66589f \"\"\"\"" + ); + } +}