security - low - fault in migrations (#3182)

A fault existed in the server's internal migration code, where attributes
that were multivalued would be merged rather than replaced in certain
contexts. This migration path is used for access controls, meaning that
on upgrades, attributes that were meant to be removed from access
controls or changes to access control target groups were not reflected
during the upgrade process.

This has a potentially low security impact as it may have allowed
users to change their name/displayname even if the administrator
had disable the name_self_write access control.

server/lib/src/entry.rs

@@ -2944,10 +2944,19 @@ impl<VALID, STATE> Entry<VALID, STATE> {
             match schema.is_multivalue(k) {
                 Ok(r) => {
                     // As this is single value, purge then present to maintain this
-                    // invariant. The other situation we purge is within schema with
-                    // the system types where we need to be able to express REMOVAL
-                    // of attributes, thus we need the purge.
-                    if !r || *k == Attribute::SystemMust || *k == Attribute::SystemMay {
+                    // invariant.
+                    if !r ||
+                        // we need to be able to express REMOVAL of attributes, so we
+                        // purge here for migrations of certain system attributes.
+                        *k == Attribute::AcpReceiverGroup ||
+                        *k == Attribute::AcpCreateAttr ||
+                        *k == Attribute::AcpCreateClass ||
+                        *k == Attribute::AcpModifyPresentAttr ||
+                        *k == Attribute::AcpModifyRemovedAttr ||
+                        *k == Attribute::AcpModifyClass ||
+                        *k == Attribute::SystemMust ||
+                        *k == Attribute::SystemMay
+                    {
                         mods.push_mod(Modify::Purged(k.clone()));
                     }
                 }

server/lib/src/server/access/mod.rs

@@ -137,7 +137,10 @@ fn resolve_access_conditions(
         AccessControlReceiver::Group(groups) => {
             let group_check = ident_memberof
                 // Have at least one group allowed.
-                .map(|imo| imo.intersection(groups).next().is_some())
+                .map(|imo| {
+                    trace!(?imo, ?groups);
+                    imo.intersection(groups).next().is_some()
+                })
                 .unwrap_or_default();
 
             if group_check {
@@ -401,6 +404,7 @@ pub trait AccessControlsTransaction<'a> {
         let related_acp: Vec<_> = modify_state
             .iter()
             .filter_map(|acs| {
+                trace!(acs_name = ?acs.acp.name);
                 let (receiver_condition, target_condition) = resolve_access_conditions(
                     ident,
                     ident_memberof,

server/lib/src/server/modify.rs

@@ -863,4 +863,91 @@ mod tests {
         // do a pw check.
         assert!(cred_ref.verify_password("test_password").unwrap());
     }
+
+    #[qs_test]
+    async fn test_modify_name_self_write(server: &QueryServer) {
+        let user_uuid = uuid!("cc8e95b4-c24f-4d68-ba54-8bed76f63930");
+        let e1 = entry_init!(
+            (Attribute::Class, EntryClass::Object.to_value()),
+            (Attribute::Class, EntryClass::Person.to_value()),
+            (Attribute::Class, EntryClass::Account.to_value()),
+            (Attribute::Name, Value::new_iname("testperson1")),
+            (Attribute::Uuid, Value::Uuid(user_uuid)),
+            (Attribute::Description, Value::new_utf8s("testperson1")),
+            (Attribute::DisplayName, Value::new_utf8s("testperson1"))
+        );
+        let mut server_txn = server.write(duration_from_epoch_now()).await.unwrap();
+
+        assert!(server_txn.internal_create(vec![e1]).is_ok());
+
+        // Impersonate the user.
+
+        let testperson_entry = server_txn.internal_search_uuid(user_uuid).unwrap();
+
+        let user_ident = Identity::from_impersonate_entry_readwrite(testperson_entry);
+
+        // Can we change ourself?
+        let me_inv_m = ModifyEvent::new_impersonate_identity(
+            user_ident,
+            filter!(f_eq(Attribute::Uuid, PartialValue::Uuid(user_uuid),)),
+            ModifyList::new_list(vec![
+                Modify::Purged(Attribute::Name),
+                Modify::Present(Attribute::Name, Value::new_iname("test_person_renamed")),
+                Modify::Purged(Attribute::DisplayName),
+                Modify::Present(
+                    Attribute::DisplayName,
+                    Value::Utf8("test_person_renamed".into()),
+                ),
+                Modify::Purged(Attribute::LegalName),
+                Modify::Present(
+                    Attribute::LegalName,
+                    Value::Utf8("test_person_renamed".into()),
+                ),
+            ]),
+        );
+
+        // Modify success.
+        assert!(server_txn.modify(&me_inv_m).is_ok());
+
+        // Alter the deal.
+        let modify_remove_person = ModifyEvent::new_internal_invalid(
+            filter!(f_eq(
+                Attribute::Uuid,
+                PartialValue::Uuid(UUID_IDM_PEOPLE_SELF_NAME_WRITE),
+            )),
+            ModifyList::new_list(vec![Modify::Purged(Attribute::Member)]),
+        );
+
+        assert!(server_txn.modify(&modify_remove_person).is_ok());
+
+        // Reload the users identity which will cause the memberships to be reflected now.
+        let testperson_entry = server_txn.internal_search_uuid(user_uuid).unwrap();
+
+        let user_ident = Identity::from_impersonate_entry_readwrite(testperson_entry);
+
+        let me_inv_m = ModifyEvent::new_impersonate_identity(
+            user_ident,
+            filter!(f_eq(Attribute::Uuid, PartialValue::Uuid(user_uuid),)),
+            ModifyList::new_list(vec![
+                Modify::Purged(Attribute::Name),
+                Modify::Present(Attribute::Name, Value::new_iname("test_person_renamed")),
+                Modify::Purged(Attribute::DisplayName),
+                Modify::Present(
+                    Attribute::DisplayName,
+                    Value::Utf8("test_person_renamed".into()),
+                ),
+                Modify::Purged(Attribute::LegalName),
+                Modify::Present(
+                    Attribute::LegalName,
+                    Value::Utf8("test_person_renamed".into()),
+                ),
+            ]),
+        );
+
+        // The modification must now fail.
+        assert_eq!(
+            server_txn.modify(&me_inv_m),
+            Err(OperationError::AccessDenied)
+        );
+    }
 }