From 45760026eadf09d36df73c488b8a7d3e2034e1b3 Mon Sep 17 00:00:00 2001 From: Janis Danisevskis Date: Tue, 19 Jan 2021 16:34:10 -0800 Subject: [PATCH] Keystore 2.0: Allow by key id usage of granted keys. When keys are loaded by grant they may be used by key id subsequently. This patch adds a check of the grant database when loading the access tuple. If one is found the access vector is populated allowing the permission callback to perform access control based on the grant. Test: keystore2_test Change-Id: If70dfbc035aed5aa3842663d475b489df3e3dd4e --- keystore2/src/database.rs | 136 +++++++++++++++++++++++++--- keystore2/src/permission.rs | 173 ++++++++++++++++++++++++++---------- keystore2/src/utils.rs | 1 + 3 files changed, 250 insertions(+), 60 deletions(-) diff --git a/keystore2/src/database.rs b/keystore2/src/database.rs index ffec931d..db30e072 100644 --- a/keystore2/src/database.rs +++ b/keystore2/src/database.rs @@ -1229,18 +1229,18 @@ impl KeystoreDB { // Domain::KEY_ID. In this case we load the domain and namespace from the // keyentry database because we need them for access control. Domain::KEY_ID => { - let mut stmt = tx - .prepare( - "SELECT domain, namespace FROM persistent.keyentry - WHERE - id = ? - AND state = ?;", - ) - .context("Domain::KEY_ID: prepare statement failed")?; - let mut rows = stmt - .query(params![key.nspace, KeyLifeCycle::Live]) - .context("Domain::KEY_ID: query failed.")?; - let (domain, namespace): (Domain, i64) = + let (domain, namespace): (Domain, i64) = { + let mut stmt = tx + .prepare( + "SELECT domain, namespace FROM persistent.keyentry + WHERE + id = ? + AND state = ?;", + ) + .context("Domain::KEY_ID: prepare statement failed")?; + let mut rows = stmt + .query(params![key.nspace, KeyLifeCycle::Live]) + .context("Domain::KEY_ID: query failed.")?; db_utils::with_rows_extract_one(&mut rows, |row| { let r = row.map_or_else(|| Err(KsError::Rc(ResponseCode::KEY_NOT_FOUND)), Ok)?; @@ -1249,13 +1249,37 @@ impl KeystoreDB { r.get(1).context("Failed to unpack namespace.")?, )) }) - .context("Domain::KEY_ID.")?; + .context("Domain::KEY_ID.")? + }; + + // We may use a key by id after loading it by grant. + // In this case we have to check if the caller has a grant for this particular + // key. We can skip this if we already know that the caller is the owner. + // But we cannot know this if domain is anything but App. E.g. in the case + // of Domain::SELINUX we have to speculatively check for grants because we have to + // consult the SEPolicy before we know if the caller is the owner. + let access_vector: Option = + if domain != Domain::APP || namespace != caller_uid as i64 { + let access_vector: Option = tx + .query_row( + "SELECT access_vector FROM persistent.grant + WHERE grantee = ? AND keyentryid = ?;", + params![caller_uid as i64, key.nspace], + |row| row.get(0), + ) + .optional() + .context("Domain::KEY_ID: query grant failed.")?; + access_vector.map(|p| p.into()) + } else { + None + }; + let key_id = key.nspace; let mut access_key = key; access_key.domain = domain; access_key.nspace = namespace; - Ok((key_id, access_key, None)) + Ok((key_id, access_key, access_vector)) } _ => Err(anyhow!(KsError::sys())), } @@ -2436,6 +2460,90 @@ mod tests { Ok(()) } + // This test attempts to load a key by key id while the caller is not the owner + // but a grant exists for the given key and the caller. + #[test] + fn test_insert_and_load_full_keyentry_from_grant_by_key_id() -> Result<()> { + let mut db = new_test_db()?; + const OWNER_UID: u32 = 1u32; + const GRANTEE_UID: u32 = 2u32; + const SOMEONE_ELSE_UID: u32 = 3u32; + let key_id = make_test_key_entry(&mut db, Domain::APP, OWNER_UID as i64, TEST_ALIAS, None) + .context("test_insert_and_load_full_keyentry_from_grant_by_key_id")? + .0; + + db.grant( + KeyDescriptor { + domain: Domain::APP, + nspace: 0, + alias: Some(TEST_ALIAS.to_string()), + blob: None, + }, + OWNER_UID, + GRANTEE_UID, + key_perm_set![KeyPerm::use_()], + |_k, _av| Ok(()), + ) + .unwrap(); + + debug_dump_grant_table(&mut db)?; + + let id_descriptor = + KeyDescriptor { domain: Domain::KEY_ID, nspace: key_id, ..Default::default() }; + + let (_, key_entry) = db + .load_key_entry( + id_descriptor.clone(), + KeyType::Client, + KeyEntryLoadBits::BOTH, + GRANTEE_UID, + |k, av| { + assert_eq!(Domain::APP, k.domain); + assert_eq!(OWNER_UID as i64, k.nspace); + assert!(av.unwrap().includes(KeyPerm::use_())); + Ok(()) + }, + ) + .unwrap(); + + assert_eq!(key_entry, make_test_key_entry_test_vector(key_id, None)); + + let (_, key_entry) = db + .load_key_entry( + id_descriptor.clone(), + KeyType::Client, + KeyEntryLoadBits::BOTH, + SOMEONE_ELSE_UID, + |k, av| { + assert_eq!(Domain::APP, k.domain); + assert_eq!(OWNER_UID as i64, k.nspace); + assert!(av.is_none()); + Ok(()) + }, + ) + .unwrap(); + + assert_eq!(key_entry, make_test_key_entry_test_vector(key_id, None)); + + db.unbind_key(id_descriptor.clone(), KeyType::Client, OWNER_UID, |_, _| Ok(())).unwrap(); + + assert_eq!( + Some(&KsError::Rc(ResponseCode::KEY_NOT_FOUND)), + db.load_key_entry( + id_descriptor, + KeyType::Client, + KeyEntryLoadBits::NONE, + GRANTEE_UID, + |_k, _av| Ok(()), + ) + .unwrap_err() + .root_cause() + .downcast_ref::() + ); + + Ok(()) + } + static KEY_LOCK_TEST_ALIAS: &str = "my super duper locked key"; #[test] diff --git a/keystore2/src/permission.rs b/keystore2/src/permission.rs index 09172567..a81954fe 100644 --- a/keystore2/src/permission.rs +++ b/keystore2/src/permission.rs @@ -482,25 +482,41 @@ pub fn check_grant_permission( /// was supplied. It is also produced if `Domain::KEY_ID` was selected, and /// on various unexpected backend failures. pub fn check_key_permission( + caller_uid: u32, caller_ctx: &CStr, perm: KeyPerm, key: &KeyDescriptor, access_vector: &Option, ) -> anyhow::Result<()> { + // If an access vector was supplied, the key is either accessed by GRANT or by KEY_ID. + // In the former case, key.domain was set to GRANT and we check the failure cases + // further below. If the access is requested by KEY_ID, key.domain would have been + // resolved to APP or SELINUX depending on where the key actually resides. + // Either way we can return here immediately if the access vector covers the requested + // permission. If it does not, we can still check if the caller has access by means of + // ownership. + if let Some(access_vector) = access_vector { + if access_vector.includes(perm) { + return Ok(()); + } + } + let target_context = match key.domain { // apps get the default keystore context - Domain::APP => getcon().context("check_key_permission: getcon failed.")?, + Domain::APP => { + if caller_uid as i64 != key.nspace { + return Err(selinux::Error::perm()) + .context("Trying to access key without ownership."); + } + getcon().context("check_key_permission: getcon failed.")? + } Domain::SELINUX => lookup_keystore2_key_context(key.nspace) .context("check_key_permission: Domain::SELINUX: Failed to lookup namespace.")?, Domain::GRANT => { match access_vector { - Some(pv) => { - if pv.includes(perm) { - return Ok(()); - } else { - return Err(selinux::Error::perm()) - .context(format!("\"{}\" not granted", perm.to_selinux())); - } + Some(_) => { + return Err(selinux::Error::perm()) + .context(format!("\"{}\" not granted", perm.to_selinux())); } None => { // If DOMAIN_GRANT was selected an access vector must be supplied. @@ -685,6 +701,7 @@ mod tests { let key = KeyDescriptor { domain: Domain::GRANT, nspace: 0, alias: None, blob: None }; assert_perm_failed!(check_key_permission( + 0, &selinux::Context::new("ignored").unwrap(), KeyPerm::grant(), &key, @@ -692,6 +709,7 @@ mod tests { )); check_key_permission( + 0, &selinux::Context::new("ignored").unwrap(), KeyPerm::use_(), &key, @@ -707,38 +725,82 @@ mod tests { let key = KeyDescriptor { domain: Domain::APP, nspace: 0, alias: None, blob: None }; - assert!(check_key_permission(&system_server_ctx, KeyPerm::use_(), &key, &None).is_ok()); - assert!(check_key_permission(&system_server_ctx, KeyPerm::delete(), &key, &None).is_ok()); - assert!(check_key_permission(&system_server_ctx, KeyPerm::get_info(), &key, &None).is_ok()); - assert!(check_key_permission(&system_server_ctx, KeyPerm::rebind(), &key, &None).is_ok()); - assert!(check_key_permission(&system_server_ctx, KeyPerm::update(), &key, &None).is_ok()); - assert!(check_key_permission(&system_server_ctx, KeyPerm::grant(), &key, &None).is_ok()); + assert!(check_key_permission(0, &system_server_ctx, KeyPerm::use_(), &key, &None).is_ok()); + assert!(check_key_permission(0, &system_server_ctx, KeyPerm::delete(), &key, &None).is_ok()); assert!( - check_key_permission(&system_server_ctx, KeyPerm::use_dev_id(), &key, &None).is_ok() + check_key_permission(0, &system_server_ctx, KeyPerm::get_info(), &key, &None).is_ok() + ); + assert!(check_key_permission(0, &system_server_ctx, KeyPerm::rebind(), &key, &None).is_ok()); + assert!(check_key_permission(0, &system_server_ctx, KeyPerm::update(), &key, &None).is_ok()); + assert!(check_key_permission(0, &system_server_ctx, KeyPerm::grant(), &key, &None).is_ok()); + assert!( + check_key_permission(0, &system_server_ctx, KeyPerm::use_dev_id(), &key, &None).is_ok() + ); + assert!( + check_key_permission(0, &gmscore_app, KeyPerm::gen_unique_id(), &key, &None).is_ok() ); - assert!(check_key_permission(&gmscore_app, KeyPerm::gen_unique_id(), &key, &None).is_ok()); - assert!(check_key_permission(&shell_ctx, KeyPerm::use_(), &key, &None).is_ok()); - assert!(check_key_permission(&shell_ctx, KeyPerm::delete(), &key, &None).is_ok()); - assert!(check_key_permission(&shell_ctx, KeyPerm::get_info(), &key, &None).is_ok()); - assert!(check_key_permission(&shell_ctx, KeyPerm::rebind(), &key, &None).is_ok()); - assert!(check_key_permission(&shell_ctx, KeyPerm::update(), &key, &None).is_ok()); - assert_perm_failed!(check_key_permission(&shell_ctx, KeyPerm::grant(), &key, &None)); + assert!(check_key_permission(0, &shell_ctx, KeyPerm::use_(), &key, &None).is_ok()); + assert!(check_key_permission(0, &shell_ctx, KeyPerm::delete(), &key, &None).is_ok()); + assert!(check_key_permission(0, &shell_ctx, KeyPerm::get_info(), &key, &None).is_ok()); + assert!(check_key_permission(0, &shell_ctx, KeyPerm::rebind(), &key, &None).is_ok()); + assert!(check_key_permission(0, &shell_ctx, KeyPerm::update(), &key, &None).is_ok()); + assert_perm_failed!(check_key_permission(0, &shell_ctx, KeyPerm::grant(), &key, &None)); assert_perm_failed!(check_key_permission( + 0, &shell_ctx, KeyPerm::req_forced_op(), &key, &None )); - assert_perm_failed!(check_key_permission(&shell_ctx, KeyPerm::manage_blob(), &key, &None)); - assert_perm_failed!(check_key_permission(&shell_ctx, KeyPerm::use_dev_id(), &key, &None)); assert_perm_failed!(check_key_permission( + 0, + &shell_ctx, + KeyPerm::manage_blob(), + &key, + &None + )); + assert_perm_failed!(check_key_permission( + 0, + &shell_ctx, + KeyPerm::use_dev_id(), + &key, + &None + )); + assert_perm_failed!(check_key_permission( + 0, &shell_ctx, KeyPerm::gen_unique_id(), &key, &None )); + // Also make sure that the permission fails if the caller is not the owner. + assert_perm_failed!(check_key_permission( + 1, // the owner is 0 + &system_server_ctx, + KeyPerm::use_(), + &key, + &None + )); + // Unless there was a grant. + assert!(check_key_permission( + 1, + &system_server_ctx, + KeyPerm::use_(), + &key, + &Some(key_perm_set![KeyPerm::use_()]) + ) + .is_ok()); + // But fail if the grant did not cover the requested permission. + assert_perm_failed!(check_key_permission( + 1, + &system_server_ctx, + KeyPerm::use_(), + &key, + &Some(key_perm_set![KeyPerm::get_info()]) + )); + Ok(()) } @@ -753,27 +815,45 @@ mod tests { }; if is_su { - assert!(check_key_permission(&sctx, KeyPerm::use_(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::delete(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::get_info(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::rebind(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::update(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::grant(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::manage_blob(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::use_dev_id(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::gen_unique_id(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::req_forced_op(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::use_(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::delete(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::get_info(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::rebind(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::update(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::grant(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::manage_blob(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::use_dev_id(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::gen_unique_id(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::req_forced_op(), &key, &None).is_ok()); } else { - assert!(check_key_permission(&sctx, KeyPerm::use_(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::delete(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::get_info(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::rebind(), &key, &None).is_ok()); - assert!(check_key_permission(&sctx, KeyPerm::update(), &key, &None).is_ok()); - assert_perm_failed!(check_key_permission(&sctx, KeyPerm::grant(), &key, &None)); - assert_perm_failed!(check_key_permission(&sctx, KeyPerm::req_forced_op(), &key, &None)); - assert_perm_failed!(check_key_permission(&sctx, KeyPerm::manage_blob(), &key, &None)); - assert_perm_failed!(check_key_permission(&sctx, KeyPerm::use_dev_id(), &key, &None)); - assert_perm_failed!(check_key_permission(&sctx, KeyPerm::gen_unique_id(), &key, &None)); + assert!(check_key_permission(0, &sctx, KeyPerm::use_(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::delete(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::get_info(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::rebind(), &key, &None).is_ok()); + assert!(check_key_permission(0, &sctx, KeyPerm::update(), &key, &None).is_ok()); + assert_perm_failed!(check_key_permission(0, &sctx, KeyPerm::grant(), &key, &None)); + assert_perm_failed!(check_key_permission( + 0, + &sctx, + KeyPerm::req_forced_op(), + &key, + &None + )); + assert_perm_failed!(check_key_permission( + 0, + &sctx, + KeyPerm::manage_blob(), + &key, + &None + )); + assert_perm_failed!(check_key_permission(0, &sctx, KeyPerm::use_dev_id(), &key, &None)); + assert_perm_failed!(check_key_permission( + 0, + &sctx, + KeyPerm::gen_unique_id(), + &key, + &None + )); } Ok(()) } @@ -789,9 +869,9 @@ mod tests { }; if is_su { - check_key_permission(&sctx, KeyPerm::use_(), &key, &None) + check_key_permission(0, &sctx, KeyPerm::use_(), &key, &None) } else { - assert_perm_failed!(check_key_permission(&sctx, KeyPerm::use_(), &key, &None)); + assert_perm_failed!(check_key_permission(0, &sctx, KeyPerm::use_(), &key, &None)); Ok(()) } } @@ -803,6 +883,7 @@ mod tests { assert_eq!( Some(&KsError::sys()), check_key_permission( + 0, &selinux::Context::new("ignored").unwrap(), KeyPerm::use_(), &key, diff --git a/keystore2/src/utils.rs b/keystore2/src/utils.rs index 080348ca..870b7fc5 100644 --- a/keystore2/src/utils.rs +++ b/keystore2/src/utils.rs @@ -77,6 +77,7 @@ pub fn check_key_permission( ) -> anyhow::Result<()> { ThreadState::with_calling_sid(|calling_sid| { permission::check_key_permission( + ThreadState::get_calling_uid(), &calling_sid .ok_or_else(Error::sys) .context("In check_key_permission: Cannot check permission without calling_sid.")?,