From 6cbb72406980bb0127b378a770195066bacaf02e Mon Sep 17 00:00:00 2001 From: Jeremy Lin Date: Fri, 29 Oct 2021 11:45:43 -0700 Subject: [PATCH] Fix conflict resolution logic for `read_only` and `hide_passwords` flags For one of these flags to be in effect for a cipher, upstream requires all of (rather than any of) the collections the cipher is in to have that flag set. Also, some of the logic for loading access restrictions was wrong. I think that only malicious clients that also had knowledge of the UUIDs of ciphers they didn't have access to would have been able to take advantage of that. --- src/db/models/cipher.rs | 45 ++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index 437934c7..39aaf580 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -343,36 +343,39 @@ impl Cipher { db_run! {conn: { // Check whether this cipher is in any collections accessible to the // user. If so, retrieve the access flags for each collection. - let query = ciphers::table + let rows = ciphers::table .filter(ciphers::uuid.eq(&self.uuid)) .inner_join(ciphers_collections::table.on( ciphers::uuid.eq(ciphers_collections::cipher_uuid))) .inner_join(users_collections::table.on( ciphers_collections::collection_uuid.eq(users_collections::collection_uuid) .and(users_collections::user_uuid.eq(user_uuid)))) - .select((users_collections::read_only, users_collections::hide_passwords)); + .select((users_collections::read_only, users_collections::hide_passwords)) + .load::<(bool, bool)>(conn) + .expect("Error getting access restrictions"); - // There's an edge case where a cipher can be in multiple collections - // with inconsistent access flags. For example, a cipher could be in - // one collection where the user has read-only access, but also in - // another collection where the user has read/write access. To handle - // this, we do a boolean OR of all values in each of the `read_only` - // and `hide_passwords` columns. This could ideally be done as part - // of the query, but Diesel doesn't support a max() or bool_or() - // function on booleans and this behavior isn't portable anyway. - if let Ok(vec) = query.load::<(bool, bool)>(conn) { - let mut read_only = false; - let mut hide_passwords = false; - for (ro, hp) in vec.iter() { - read_only |= ro; - hide_passwords |= hp; - } - - Some((read_only, hide_passwords)) - } else { + if rows.is_empty() { // This cipher isn't in any collections accessible to the user. - None + return None; } + + // A cipher can be in multiple collections with inconsistent access flags. + // For example, a cipher could be in one collection where the user has + // read-only access, but also in another collection where the user has + // read/write access. For a flag to be in effect for a cipher, upstream + // requires all collections the cipher is in to have that flag set. + // Therefore, we do a boolean AND of all values in each of the `read_only` + // and `hide_passwords` columns. This could ideally be done as part of the + // query, but Diesel doesn't support a min() or bool_and() function on + // booleans and this behavior isn't portable anyway. + let mut read_only = true; + let mut hide_passwords = true; + for (ro, hp) in rows.iter() { + read_only &= ro; + hide_passwords &= hp; + } + + Some((read_only, hide_passwords)) }} }