[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

@@ -3192,6 +3192,7 @@ dependencies = [
  "hex",
  "idlset",
  "image 0.24.8",
+ "itertools 0.12.1",
  "kanidm_build_profiles",
  "kanidm_lib_crypto",
  "kanidm_proto",

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" }

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 }

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 {
+        let join_str = match self.join {
-            OauthClaimMapJoin::CommaSeparatedValue => ',',
-            OauthClaimMapJoin::SpaceSeparatedValue => ' ',
             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)
     }

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
+        use itertools::Itertools;
-        let max_join_chars: usize = $str_iter.len() - 1;
+        $str_iter.iter().join($join_char)
-        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
     }};
 }

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_error!("requested_rem is not a subset of allowed");
-                        security_access!(
+                        security_error!("requested_rem: {:?} !⊆ allowed: {:?}", requested_rem, rem);
-                            "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_error!("requested_rem is not a subset of allowed");
-                        security_access!(
+                        security_error!("requested_rem: {:?} !⊆ allowed: {:?}", requested_rem, rem);
-                            "requested_rem: {:?} !⊆ allowed: {:?}",
-                            requested_rem,
-                            rem
-                        );
                         false
                     } else if !requested_classes.is_subset(&cls) {
                         security_access!("requested_classes is not a subset of allowed");

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)

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<DbValueOauthClaimMapJoinV1> for OauthClaimMapJoin {
     fn from(value: DbValueOauthClaimMapJoinV1) -> OauthClaimMapJoin {
         match value {

server/lib/src/valueset/mod.rs

@@ -368,7 +368,6 @@ pub trait ValueSetT: std::fmt::Debug + DynClone {
     }
 
     fn as_oauthclaim_map(&self) -> Option<&BTreeMap<String, OauthClaimMapping>> {
-        debug_assert!(false);
         None
     }
 

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
+        self.trim();
-            .values_mut()
-            .for_each(|mapping_mut| mapping_mut.values.retain(|_k, v| !v.is_empty()));
-
-        self.map.retain(|_k, v| !v.values.is_empty());
 
         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<dyn Iterator<Item = String> + '_> {
         Box::new(self.map.iter().flat_map(|(name, mapping)| {
             mapping.values.iter().map(move |(group, claims)| {
-                let join_char = match mapping.join {
+                let join_str = mapping.join.to_str();
-                    OauthClaimMapJoin::CommaSeparatedValue => ',',
-                    OauthClaimMapJoin::SpaceSeparatedValue => ' ',
-                    // Should this be something else?
-                    OauthClaimMapJoin::JsonArray => ';',
-                };
 
-                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 \"\"\"\""
+        );
+    }
+}