Merge changes I7cf1b0d5,Ib9943513,I98e0d91a,I7cb60da1 am: 74cdafdfd1
Original change: https://android-review.googlesource.com/c/platform/system/security/+/1418713
Change-Id: I0b2c007e59ce8c5ffa98c37dcd93c32cb33e87cc
diff --git a/keystore2/Android.bp b/keystore2/Android.bp
index 338f37e..b5728a3 100644
--- a/keystore2/Android.bp
+++ b/keystore2/Android.bp
@@ -22,7 +22,6 @@
"libandroid_security_keystore2",
"libanyhow",
"libbinder_rs",
- "libkeystore_aidl_generated",
"libkeystore2_selinux",
"liblazy_static",
"liblibsqlite3_sys",
@@ -45,7 +44,6 @@
"libandroid_security_keystore2",
"libanyhow",
"libbinder_rs",
- "libkeystore_aidl_generated",
"libkeystore2_selinux",
"liblazy_static",
"liblibsqlite3_sys",
@@ -58,15 +56,6 @@
// This is a placeholder for the libraries that will be generated from the AIDL specs
// eventually.
rust_library {
- name: "libkeystore_aidl_generated",
- crate_name: "keystore_aidl_generated",
-
- srcs: ["src/aidl_generated.rs"],
-}
-
-// This is a placeholder for the libraries that will be generated from the AIDL specs
-// eventually.
-rust_library {
name: "libandroid_hardware_keymint",
crate_name: "android_hardware_keymint",
diff --git a/keystore2/selinux/src/lib.rs b/keystore2/selinux/src/lib.rs
index 08d84b2..8bc3bc4 100644
--- a/keystore2/selinux/src/lib.rs
+++ b/keystore2/selinux/src/lib.rs
@@ -267,7 +267,7 @@
/// * Err(anyhow!(Error::perm()))) if the permission was denied.
/// * Err(anyhow!(ioError::last_os_error())) if any other error occurred while performing
/// the access check.
-pub fn check_access(source: &Context, target: &Context, tclass: &str, perm: &str) -> Result<()> {
+pub fn check_access(source: &CStr, target: &CStr, tclass: &str, perm: &str) -> Result<()> {
init_logger_once();
let c_tclass = CString::new(tclass).with_context(|| {
format!("check_access: Failed to convert tclass \"{}\" to CString.", tclass)
@@ -295,7 +295,7 @@
.with_context(|| {
format!(
concat!(
- "check_access: Failed with sctx: {} tctx: {}",
+ "check_access: Failed with sctx: {:?} tctx: {:?}",
" with target class: \"{}\" perm: \"{}\""
),
source, target, tclass, perm
diff --git a/keystore2/src/aidl_generated.rs b/keystore2/src/aidl_generated.rs
deleted file mode 100644
index 4b6a844..0000000
--- a/keystore2/src/aidl_generated.rs
+++ /dev/null
@@ -1,99 +0,0 @@
-// Copyright 2020, The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-#![allow(missing_docs)]
-
-//! This crate holds types that we are depending on and will be generated by the AIDL
-//! compiler.
-
-use std::cmp::PartialEq;
-use std::fmt;
-
-#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
-pub struct Result {
- pub rc: ResponseCode,
- pub km_error_code: i32,
-}
-
-impl fmt::Display for Result {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
- write!(f, "{:?}", self)
- }
-}
-
-#[repr(i32)]
-#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
-pub enum ResponseCode {
- Ok = 0,
- // 1 Reserved - formerly NO_ERROR
- Locked = 2,
- Uninitialized = 3,
- SystemError = 4,
- // 5 Reserved - formerly "protocol error" was never used
- PermissionDenied = 6,
- KeyNotFound = 7,
- ValueCorrupted = 8,
- // 9 Reserved - formerly "undefined action" was never used
- WrongPassword = 10,
- // 11 - 13 Reserved - formerly password retry count indicators: obsolete
- //
- // 14 Reserved - formerly SIGNATURE_INVALID: Keystore does not perform public key
- // operations any more.
-
- // Indicates to the caller that user authorization is required before the operation may
- // commence.
- OpAuthNeeded = 15,
- // 16 Reserved
- KeyPermanentlyInvalidated = 17,
- NoSuchSecurityLevel = 18,
- KeymintErrorCode = 19,
- BackendBusy = 20,
-}
-
-pub type ErrorCode = i32;
-
-#[repr(i32)]
-#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
-pub enum KeyPermission {
- None = 0,
- Delete = 1,
- GenUniqueId = 2,
- GetInfo = 4,
- Grant = 8,
- List = 0x10,
- ManageBlob = 0x20,
- Rebind = 0x40,
- ReqForcedOp = 0x80,
- Update = 0x100,
- Use = 0x200,
- UseDevId = 0x400,
-}
-
-#[repr(i32)]
-#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
-pub enum Domain {
- App = 0,
- Grant = 1,
- SELinux = 2,
- Blob = 3,
- KeyId = 4,
-}
-
-#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
-pub struct KeyDescriptor {
- pub domain: Domain,
- pub namespace_: i64,
- pub alias: Option<String>,
- pub blob: Option<Vec<u8>>,
-}
diff --git a/keystore2/src/database.rs b/keystore2/src/database.rs
index 2ff5cf0..b1cde6e 100644
--- a/keystore2/src/database.rs
+++ b/keystore2/src/database.rs
@@ -17,7 +17,11 @@
use crate::error::Error as KsError;
use anyhow::{Context, Result};
-use keystore_aidl_generated as aidl;
+
+use android_security_keystore2::aidl::android::security::keystore2::{
+ Domain, Domain::Domain as DomainType,
+};
+
#[cfg(not(test))]
use rand::prelude::random;
use rusqlite::{params, Connection, TransactionBehavior, NO_PARAMS};
@@ -76,9 +80,9 @@
Ok(conn)
}
- pub fn create_key_entry(&self, domain: aidl::Domain, namespace: i64) -> Result<i64> {
+ pub fn create_key_entry(&self, domain: DomainType, namespace: i64) -> Result<i64> {
match domain {
- aidl::Domain::App | aidl::Domain::SELinux => {}
+ Domain::App | Domain::SELinux => {}
_ => {
return Err(KsError::sys())
.context(format!("Domain {:?} must be either App or SELinux.", domain));
@@ -110,11 +114,11 @@
&mut self,
newid: u32,
alias: &str,
- domain: aidl::Domain,
+ domain: DomainType,
namespace: i64,
) -> Result<()> {
match domain {
- aidl::Domain::App | aidl::Domain::SELinux => {}
+ Domain::App | Domain::SELinux => {}
_ => {
return Err(KsError::sys())
.context(format!("Domain {:?} must be either App or SELinux.", domain));
@@ -209,7 +213,7 @@
fn test_no_persistence_for_tests() -> Result<()> {
let db = new_test_db()?;
- db.create_key_entry(aidl::Domain::App, 100)?;
+ db.create_key_entry(Domain::App, 100)?;
let entries = get_keyentry(&db)?;
assert_eq!(entries.len(), 1);
let db = new_test_db()?;
@@ -225,7 +229,7 @@
let _file_guard_perboot = TempFile { filename: PERBOOT_TEST_SQL };
let db = new_test_db_with_persistent_file()?;
- db.create_key_entry(aidl::Domain::App, 100)?;
+ db.create_key_entry(Domain::App, 100)?;
let entries = get_keyentry(&db)?;
assert_eq!(entries.len(), 1);
let db = new_test_db_with_persistent_file()?;
@@ -237,9 +241,7 @@
#[test]
fn test_create_key_entry() -> Result<()> {
- use aidl::Domain;
-
- fn extractor(ke: &KeyEntryRow) -> (Domain, i64, Option<&str>) {
+ fn extractor(ke: &KeyEntryRow) -> (DomainType, i64, Option<&str>) {
(ke.domain.unwrap(), ke.namespace.unwrap(), ke.alias.as_deref())
}
@@ -256,15 +258,15 @@
// Test that we must pass in a valid Domain.
check_result_is_error_containing_string(
db.create_key_entry(Domain::Grant, 102),
- "Domain Grant must be either App or SELinux.",
+ "Domain 1 must be either App or SELinux.",
);
check_result_is_error_containing_string(
db.create_key_entry(Domain::Blob, 103),
- "Domain Blob must be either App or SELinux.",
+ "Domain 3 must be either App or SELinux.",
);
check_result_is_error_containing_string(
db.create_key_entry(Domain::KeyId, 104),
- "Domain KeyId must be either App or SELinux.",
+ "Domain 4 must be either App or SELinux.",
);
Ok(())
@@ -272,9 +274,7 @@
#[test]
fn test_rebind_alias() -> Result<()> {
- use aidl::Domain;
-
- fn extractor(ke: &KeyEntryRow) -> (Option<Domain>, Option<i64>, Option<&str>) {
+ fn extractor(ke: &KeyEntryRow) -> (Option<DomainType>, Option<i64>, Option<&str>) {
(ke.domain, ke.namespace, ke.alias.as_deref())
}
@@ -303,15 +303,15 @@
// Test that we must pass in a valid Domain.
check_result_is_error_containing_string(
db.rebind_alias(0, "foo", Domain::Grant, 42),
- "Domain Grant must be either App or SELinux.",
+ "Domain 1 must be either App or SELinux.",
);
check_result_is_error_containing_string(
db.rebind_alias(0, "foo", Domain::Blob, 42),
- "Domain Blob must be either App or SELinux.",
+ "Domain 3 must be either App or SELinux.",
);
check_result_is_error_containing_string(
db.rebind_alias(0, "foo", Domain::KeyId, 42),
- "Domain KeyId must be either App or SELinux.",
+ "Domain 4 must be either App or SELinux.",
);
// Test that we correctly handle setting an alias for something that does not exist.
@@ -349,7 +349,7 @@
struct KeyEntryRow {
id: u32,
creation_date: String,
- domain: Option<aidl::Domain>,
+ domain: Option<DomainType>,
namespace: Option<i64>,
alias: Option<String>,
}
@@ -358,11 +358,10 @@
db.conn
.prepare("SELECT * FROM persistent.keyentry;")?
.query_map(NO_PARAMS, |row| {
- let domain: Option<i32> = row.get(2)?;
Ok(KeyEntryRow {
id: row.get(0)?,
creation_date: row.get(1)?,
- domain: domain.map(domain_from_integer),
+ domain: row.get(2)?,
namespace: row.get(3)?,
alias: row.get(4)?,
})
@@ -371,19 +370,6 @@
.collect::<Result<Vec<_>>>()
}
- // TODO: Replace this with num_derive.
- fn domain_from_integer(value: i32) -> aidl::Domain {
- use aidl::Domain;
- match value {
- x if Domain::App as i32 == x => Domain::App,
- x if Domain::Grant as i32 == x => Domain::Grant,
- x if Domain::SELinux as i32 == x => Domain::SELinux,
- x if Domain::Blob as i32 == x => Domain::Blob,
- x if Domain::KeyId as i32 == x => Domain::KeyId,
- _ => panic!("Unexpected domain: {}", value),
- }
- }
-
// A class that deletes a file when it is dropped.
// TODO: If we ever add a crate that does this, we can use it instead.
struct TempFile {
diff --git a/keystore2/src/error.rs b/keystore2/src/error.rs
index f4da749..0326610 100644
--- a/keystore2/src/error.rs
+++ b/keystore2/src/error.rs
@@ -40,7 +40,9 @@
use keystore2_selinux as selinux;
-use android_security_keystore2::binder::{Result as BinderResult, Status as BinderStatus};
+use android_security_keystore2::binder::{
+ ExceptionCode, Result as BinderResult, Status as BinderStatus,
+};
/// This is the main Keystore error type. It wraps the Keystore `ResponseCode` generated
/// from AIDL in the `Rc` variant and Keymint `ErrorCode` in the Km variant.
@@ -52,6 +54,9 @@
/// Wraps a Keymint `ErrorCode` as defined by the Keymint AIDL interface specification.
#[error("Error::Km({0:?})")]
Km(ErrorCode),
+ /// Wraps a Binder exception code other than a service specific exception.
+ #[error("Binder exception code {0:?}, {1:?}")]
+ Binder(ExceptionCode, i32),
}
impl Error {
@@ -66,6 +71,36 @@
}
}
+/// Helper function to map the binder status we get from calls into KeyMint
+/// to a Keystore Error. We don't create an anyhow error here to make
+/// it easier to evaluate KeyMint errors, which we must do in some cases, e.g.,
+/// when diagnosing authentication requirements, update requirements, and running
+/// out of operation slots.
+pub fn map_km_error<T>(r: BinderResult<T>) -> Result<T, Error> {
+ r.map_err(|s| {
+ match s.exception_code() {
+ ExceptionCode::SERVICE_SPECIFIC => {
+ let se = s.service_specific_error();
+ if se < 0 {
+ // Negative service specific errors are KM error codes.
+ Error::Km(s.service_specific_error())
+ } else {
+ // Non negative error codes cannot be KM error codes.
+ // So we create an `Error::Binder` variant to preserve
+ // the service specific error code for logging.
+ // `map_or_log_err` will map this on a system error,
+ // but not before logging the details to logcat.
+ Error::Binder(ExceptionCode::SERVICE_SPECIFIC, se)
+ }
+ }
+ // We create `Error::Binder` to preserve the exception code
+ // for logging.
+ // `map_or_log_err` will map this on a system error.
+ e_code => Error::Binder(e_code, 0),
+ }
+ })
+}
+
/// This function should be used by Keystore service calls to translate error conditions
/// into `android.security.keystore2.Result` which is imported here as `aidl::Result`
/// and newtyped as AidlResult.
@@ -107,6 +142,10 @@
let rc = match root_cause.downcast_ref::<Error>() {
Some(Error::Rc(rcode)) => *rcode,
Some(Error::Km(ec)) => *ec,
+ // If an Error::Binder reaches this stage we report a system error.
+ // The exception code and possible service specific error will be
+ // printed in the error log above.
+ Some(Error::Binder(_, _)) => Rc::SystemError,
None => match root_cause.downcast_ref::<selinux::Error>() {
Some(selinux::Error::PermissionDenied) => Rc::PermissionDenied,
_ => Rc::SystemError,
@@ -122,6 +161,9 @@
pub mod tests {
use super::*;
+ use android_security_keystore2::binder::{
+ ExceptionCode, Result as BinderResult, Status as BinderStatus,
+ };
use anyhow::{anyhow, Context};
fn nested_nested_rc(rc: ResponseCode) -> anyhow::Result<()> {
@@ -170,6 +212,14 @@
nested_nested_other_error().context("nested other error")
}
+ fn binder_sse_error(sse: i32) -> BinderResult<()> {
+ Err(BinderStatus::new_service_specific_error(sse, None))
+ }
+
+ fn binder_exception(ex: ExceptionCode) -> BinderResult<()> {
+ Err(BinderStatus::new_exception(ex, None))
+ }
+
#[test]
fn keystore_error_test() -> anyhow::Result<(), String> {
android_logger::init_once(
@@ -187,7 +237,7 @@
);
}
- // All KeystoreKerror::Km(x) get mapped on a service
+ // All Keystore Error::Km(x) get mapped on a service
// specific error of x.
for ec in Ec::UNKNOWN_ERROR..Ec::ROOT_OF_TRUST_ALREADY_SET {
assert_eq!(
@@ -197,6 +247,46 @@
);
}
+ // All Keymint errors x received through a Binder Result get mapped on
+ // a service specific error of x.
+ for ec in Ec::UNKNOWN_ERROR..Ec::ROOT_OF_TRUST_ALREADY_SET {
+ assert_eq!(
+ Result::<(), i32>::Err(ec),
+ map_or_log_err(
+ map_km_error(binder_sse_error(ec))
+ .with_context(|| format!("Km error code: {}.", ec)),
+ |_| Err(BinderStatus::ok())
+ )
+ .map_err(|s| s.service_specific_error())
+ );
+ }
+
+ // map_km_error creates an Error::Binder variant storing
+ // ExceptionCode::SERVICE_SPECIFIC and the given
+ // service specific error.
+ let sse = map_km_error(binder_sse_error(1));
+ assert_eq!(Err(Error::Binder(ExceptionCode::SERVICE_SPECIFIC, 1)), sse);
+ // map_or_log_err then maps it on a service specific error of Rc::SystemError.
+ assert_eq!(
+ Result::<(), i32>::Err(Rc::SystemError),
+ map_or_log_err(sse.context("Non negative service specific error."), |_| Err(
+ BinderStatus::ok()
+ ))
+ .map_err(|s| s.service_specific_error())
+ );
+
+ // map_km_error creates a Error::Binder variant storing the given exception code.
+ let binder_exception = map_km_error(binder_exception(ExceptionCode::TRANSACTION_FAILED));
+ assert_eq!(Err(Error::Binder(ExceptionCode::TRANSACTION_FAILED, 0)), binder_exception);
+ // map_or_log_err then maps it on a service specific error of Rc::SystemError.
+ assert_eq!(
+ Result::<(), i32>::Err(Rc::SystemError),
+ map_or_log_err(binder_exception.context("Binder Exception."), |_| Err(
+ BinderStatus::ok()
+ ))
+ .map_err(|s| s.service_specific_error())
+ );
+
// selinux::Error::Perm() needs to be mapped to Rc::PermissionDenied
assert_eq!(
Result::<(), i32>::Err(Rc::PermissionDenied),
diff --git a/keystore2/src/permission.rs b/keystore2/src/permission.rs
index e5939c8..df59484 100644
--- a/keystore2/src/permission.rs
+++ b/keystore2/src/permission.rs
@@ -24,6 +24,7 @@
use std::cmp::PartialEq;
use std::convert::From;
+use std::ffi::CStr;
use crate::error::Error as KsError;
use keystore2_selinux as selinux;
@@ -412,10 +413,7 @@
/// Uses `selinux::check_access` to check if the given caller context `caller_cxt` may access
/// the given permision `perm` of the `keystore2` security class.
-pub fn check_keystore_permission(
- caller_ctx: &selinux::Context,
- perm: KeystorePerm,
-) -> anyhow::Result<()> {
+pub fn check_keystore_permission(caller_ctx: &CStr, perm: KeystorePerm) -> anyhow::Result<()> {
let target_context = getcon().context("check_keystore_permission: getcon failed.")?;
selinux::check_access(caller_ctx, &target_context, "keystore2", perm.to_selinux())
}
@@ -434,7 +432,7 @@
/// SELinux keystore key backend, and the result is used
/// as target context.
pub fn check_grant_permission(
- caller_ctx: &selinux::Context,
+ caller_ctx: &CStr,
access_vec: KeyPermSet,
key: &KeyDescriptor,
) -> anyhow::Result<()> {
@@ -484,7 +482,7 @@
/// was supplied. It is also produced if `Domain::KeyId` was selected, and
/// on various unexpected backend failures.
pub fn check_key_permission(
- caller_ctx: &selinux::Context,
+ caller_ctx: &CStr,
perm: KeyPerm,
key: &KeyDescriptor,
access_vector: &Option<KeyPermSet>,