EncryptedMaps: a ReadWriteManage grantee can make the owner's map appear twice in get_all_accessible_encrypted_maps

[marc0olo]

Summary

A user with ReadWriteManage on a map can cause the map owner to see their own map twice in get_all_accessible_encrypted_maps. Two independent gaps combine: an ACL mutation targeting the owner is accepted from a non-owner caller, and the accessible-map listing concatenates two sources without de-duplicating.

Both the Motoko and Rust implementations are affected, at the same places.

This is not a privilege-escalation issue. Owner rights are derived from identity (ensure_user_can_* short-circuits on caller == key_id.0), so the owner cannot be locked out — I verified this. The impact is that a collaborator can corrupt the owner's view of their own vault list.

Reproduction

Against a canister using the EncryptedMaps mixin/macro:

1. owner inserts a value into map (owner, "Team")
2. owner calls set_user_rights(owner, "Team", mgr, ReadWriteManage)
3. mgr calls set_user_rights(owner, "Team", owner, Read) — or remove_user(owner, "Team", owner)
4. owner calls get_all_accessible_encrypted_maps()

Observed, on a local replica:

1. mgr.removeUser(owner)                   SUCCEEDED
2. mgr.setUserRights(owner, Read)          SUCCEEDED
3. owner reads / writes / manages          all still SUCCEEDED   <- correctly not locked out
4. owner's listing: 2 entries -> [ 'Team@mjmmq', 'Team@mjmmq' ]  <- same map twice

Expected: one entry. A client keyed on (map_owner, map_name) now renders two identical rows with the same key.

Cause 1 — the owner guard only fires when the owner targets themselves

Both implementations use &&, so the check passes for any other caller targeting the owner:

backend/rs/ic_vetkeys/src/key_manager/mod.rs:249

if caller == key_id.0 && caller == user {
    return Err("cannot change key owner's user rights".to_string());
}

and :267 in remove_user. The Motoko equivalents are KeyManager.mo:211 and :264.

Since owner rights are identity-derived, an ACL entry for the owner conveys nothing — it is dead state that should be rejected rather than stored.

A second consequence: the ACL API contradicts itself about the owner

The same && lets remove_user accept a mutation targeting the owner, and there the result is an ambiguous response rather than dead state. Measured on a local replica, caller holding ReadWriteManage:

remove_user(owner)           -> Ok(None)                   // nothing was removed
remove_user(never-granted)   -> Ok(None)                   // identical answer
get_user_rights(owner)       -> Ok(Some(ReadWriteManage))  // derived from identity

get_user_rights derives the owner's rights from identity, while remove_user operates on the raw ACL, where the owner has no entry. The two endpoints therefore disagree about whether the owner is a member, and a caller cannot distinguish "the owner is protected" from "that principal had nothing to remove".

Security is unaffected — the owner retains write and manage throughout, verified — but it makes suggested fix 1 worth doing independently of fix 2: rejecting the mutation replaces an ambiguous Ok(None) with an explicit refusal, which is the only answer that separates the two cases.

Cause 2 — the accessible-map listing does not de-duplicate

backend/rs/ic_vetkeys/src/encrypted_maps/mod.rs:243

let accessible_map_ids = self.get_accessible_shared_map_names(caller).into_iter();
let owned_map_ids = std::iter::repeat(caller).zip(self.get_owned_non_empty_map_names(caller));
accessible_map_ids.chain(owned_map_ids)

Motoko, EncryptedMaps.mo:333, is the same concat. Once the owner appears in their own map's ACL, the map is yielded by both halves.

Suggested fix

1. In set_user_rights and remove_user, reject any mutation where user == key_id.0, regardless of caller. That is a strict tightening: such entries have no authorization effect today.
2. De-duplicate in get_accessible_map_ids_iter — worthwhile independently, since it makes the listing robust to any other route to the same state.

Happy to open a PR for either if useful.

[marc0olo]

Added a second observable consequence of Cause 1 to the description: because the same && lets remove_user accept a mutation targeting the owner, remove_user(owner) answers Ok(None) — indistinguishable from removing a principal who was never granted anything — while get_user_rights(owner) reports Some(ReadWriteManage). The two endpoints disagree about whether the owner is a member.

Security is unaffected (the owner keeps write and manage, verified on a replica), but it means suggested fix 1 stands on its own merits, separately from the de-duplication in fix 2. Still happy to open a PR.