Merge "Log key integrity violation to NIAP audit log."
diff --git a/keystore/keystore_cli_v2.cpp b/keystore/keystore_cli_v2.cpp
index 6e45ee2..43f72a9 100644
--- a/keystore/keystore_cli_v2.cpp
+++ b/keystore/keystore_cli_v2.cpp
@@ -56,7 +56,7 @@
keymint::AuthorizationSet parameters;
};
-constexpr const char keystore2_service_name[] = "android.system.keystore2";
+constexpr const char keystore2_service_name[] = "android.system.keystore2.IKeystoreService/default";
int unwrapError(const ndk::ScopedAStatus& status) {
if (status.isOk()) return 0;
@@ -769,7 +769,7 @@
sec_level->generateKey(keyDescriptor(name), {} /* attestationKey */, params.vector_data(),
0 /* flags */, {} /* entropy */, &keyMetadata);
- if (rc.isOk()) {
+ if (!rc.isOk()) {
std::cerr << "GenerateKey failed: " << rc.getDescription() << std::endl;
return unwrapError(rc);
}
diff --git a/keystore/tests/Android.bp b/keystore/tests/Android.bp
index 249cb77..39601eb 100644
--- a/keystore/tests/Android.bp
+++ b/keystore/tests/Android.bp
@@ -62,9 +62,9 @@
"libgtest_main",
"libutils",
"liblog",
+ "android.security.apc-ndk_platform",
],
shared_libs: [
- "android.security.apc-ndk_platform",
"libbinder_ndk",
],
sanitize: {
diff --git a/keystore2/Android.bp b/keystore2/Android.bp
index 0ba49ed..32493c0 100644
--- a/keystore2/Android.bp
+++ b/keystore2/Android.bp
@@ -145,4 +145,6 @@
],
vintf_fragments: ["android.system.keystore2-service.xml"],
+
+ required: ["keystore_cli_v2"],
}
diff --git a/keystore2/selinux/Android.bp b/keystore2/selinux/Android.bp
index 748e406..0810855 100644
--- a/keystore2/selinux/Android.bp
+++ b/keystore2/selinux/Android.bp
@@ -63,3 +63,24 @@
"libthiserror",
],
}
+
+rust_test {
+ name: "keystore2_selinux_concurrency_test",
+ srcs: [
+ "src/concurrency_test.rs",
+ ],
+ crate_name: "keystore2_selinux_concurrency_test",
+ test_suites: ["deneral-tests"],
+ auto_gen_config: true,
+
+ rustlibs: [
+ "libandroid_logger",
+ "libanyhow",
+ "libkeystore2_selinux",
+ "liblazy_static",
+ "liblog_rust",
+ "libnix",
+ "libnum_cpus",
+ "libthiserror",
+ ],
+}
diff --git a/keystore2/selinux/src/concurrency_test.rs b/keystore2/selinux/src/concurrency_test.rs
new file mode 100644
index 0000000..a5d2df2
--- /dev/null
+++ b/keystore2/selinux/src/concurrency_test.rs
@@ -0,0 +1,190 @@
+// Copyright 2021, 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.
+
+use keystore2_selinux::{check_access, Context};
+use nix::sched::sched_setaffinity;
+use nix::sched::CpuSet;
+use nix::unistd::getpid;
+use std::thread;
+use std::{
+ sync::{atomic::AtomicU8, atomic::Ordering, Arc},
+ time::{Duration, Instant},
+};
+
+#[derive(Clone, Copy)]
+struct CatCount(u8, u8, u8, u8);
+
+impl CatCount {
+ fn next(&mut self) -> CatCount {
+ let result = *self;
+ if self.3 == 255 {
+ if self.2 == 254 {
+ if self.1 == 253 {
+ if self.0 == 252 {
+ self.0 = 255;
+ }
+ self.0 += 1;
+ self.1 = self.0;
+ }
+ self.1 += 1;
+ self.2 = self.1;
+ }
+ self.2 += 1;
+ self.3 = self.2;
+ }
+ self.3 += 1;
+ result
+ }
+
+ fn make_string(&self) -> String {
+ format!("c{},c{},c{},c{}", self.0, self.1, self.2, self.3)
+ }
+}
+
+impl Default for CatCount {
+ fn default() -> Self {
+ Self(0, 1, 2, 3)
+ }
+}
+
+/// This test calls selinux_check_access concurrently causing access vector cache misses
+/// in libselinux avc. The test then checks if any of the threads fails to report back
+/// after a burst of access checks. The purpose of the test is to draw out a specific
+/// access vector cache corruption that sends a calling thread into an infinite loop.
+/// This was observed when keystore2 used libselinux concurrently in a non thread safe
+/// way. See b/184006658.
+#[test]
+fn test_concurrent_check_access() {
+ android_logger::init_once(
+ android_logger::Config::default()
+ .with_tag("keystore2_selinux_concurrency_test")
+ .with_min_level(log::Level::Debug),
+ );
+
+ let cpus = num_cpus::get();
+ let turnpike = Arc::new(AtomicU8::new(0));
+ let complete_count = Arc::new(AtomicU8::new(0));
+ let mut threads: Vec<thread::JoinHandle<()>> = Vec::new();
+
+ for i in 0..cpus {
+ log::info!("Spawning thread {}", i);
+ let turnpike_clone = turnpike.clone();
+ let complete_count_clone = complete_count.clone();
+ threads.push(thread::spawn(move || {
+ let mut cpu_set = CpuSet::new();
+ cpu_set.set(i).unwrap();
+ sched_setaffinity(getpid(), &cpu_set).unwrap();
+ let mut cat_count: CatCount = Default::default();
+
+ log::info!("Thread 0 reached turnpike");
+ loop {
+ turnpike_clone.fetch_add(1, Ordering::Relaxed);
+ loop {
+ match turnpike_clone.load(Ordering::Relaxed) {
+ 0 => break,
+ 255 => return,
+ _ => {}
+ }
+ }
+
+ for _ in 0..250 {
+ let (tctx, sctx, perm, class) = (
+ Context::new("u:object_r:keystore:s0").unwrap(),
+ Context::new(&format!(
+ "u:r:untrusted_app:s0:{}",
+ cat_count.next().make_string()
+ ))
+ .unwrap(),
+ "use",
+ "keystore2_key",
+ );
+
+ check_access(&sctx, &tctx, class, perm).unwrap();
+ }
+
+ complete_count_clone.fetch_add(1, Ordering::Relaxed);
+ while complete_count_clone.load(Ordering::Relaxed) as usize != cpus {
+ thread::sleep(Duration::from_millis(5));
+ }
+ }
+ }));
+ }
+
+ let mut i = 0;
+ let run_time = Instant::now();
+
+ loop {
+ const TEST_ITERATIONS: u32 = 500;
+ const MAX_SLEEPS: u64 = 500;
+ const SLEEP_MILLISECONDS: u64 = 5;
+ let mut sleep_count: u64 = 0;
+ while turnpike.load(Ordering::Relaxed) as usize != cpus {
+ thread::sleep(Duration::from_millis(SLEEP_MILLISECONDS));
+ sleep_count += 1;
+ assert!(
+ sleep_count < MAX_SLEEPS,
+ "Waited too long to go ready on iteration {}, only {} are ready",
+ i,
+ turnpike.load(Ordering::Relaxed)
+ );
+ }
+
+ if i % 100 == 0 {
+ let elapsed = run_time.elapsed().as_secs();
+ println!("{:02}:{:02}: Iteration {}", elapsed / 60, elapsed % 60, i);
+ }
+
+ // Give the threads some time to reach and spin on the turn pike.
+ assert_eq!(turnpike.load(Ordering::Relaxed) as usize, cpus, "i = {}", i);
+ if i >= TEST_ITERATIONS {
+ turnpike.store(255, Ordering::Relaxed);
+ break;
+ }
+
+ // Now go.
+ complete_count.store(0, Ordering::Relaxed);
+ turnpike.store(0, Ordering::Relaxed);
+ i += 1;
+
+ // Wait for them to all complete.
+ sleep_count = 0;
+ while complete_count.load(Ordering::Relaxed) as usize != cpus {
+ thread::sleep(Duration::from_millis(SLEEP_MILLISECONDS));
+ sleep_count += 1;
+ if sleep_count >= MAX_SLEEPS {
+ // Enable the following block to park the thread to allow attaching a debugger.
+ if false {
+ println!(
+ "Waited {} seconds and we seem stuck. Going to sleep forever.",
+ (MAX_SLEEPS * SLEEP_MILLISECONDS) as f32 / 1000.0
+ );
+ loop {
+ thread::park();
+ }
+ } else {
+ assert!(
+ sleep_count < MAX_SLEEPS,
+ "Waited too long to complete on iteration {}, only {} are complete",
+ i,
+ complete_count.load(Ordering::Relaxed)
+ );
+ }
+ }
+ }
+ }
+
+ for t in threads {
+ t.join().unwrap();
+ }
+}
diff --git a/keystore2/src/database.rs b/keystore2/src/database.rs
index 876f4ce..2930162 100644
--- a/keystore2/src/database.rs
+++ b/keystore2/src/database.rs
@@ -2967,19 +2967,28 @@
/// Returns a list of KeyDescriptors in the selected domain/namespace.
/// The key descriptors will have the domain, nspace, and alias field set.
/// Domain must be APP or SELINUX, the caller must make sure of that.
- pub fn list(&mut self, domain: Domain, namespace: i64) -> Result<Vec<KeyDescriptor>> {
+ pub fn list(
+ &mut self,
+ domain: Domain,
+ namespace: i64,
+ key_type: KeyType,
+ ) -> Result<Vec<KeyDescriptor>> {
let _wp = wd::watch_millis("KeystoreDB::list", 500);
self.with_transaction(TransactionBehavior::Deferred, |tx| {
let mut stmt = tx
.prepare(
"SELECT alias FROM persistent.keyentry
- WHERE domain = ? AND namespace = ? AND alias IS NOT NULL AND state = ?;",
+ WHERE domain = ?
+ AND namespace = ?
+ AND alias IS NOT NULL
+ AND state = ?
+ AND key_type = ?;",
)
.context("In list: Failed to prepare.")?;
let mut rows = stmt
- .query(params![domain.0 as u32, namespace, KeyLifeCycle::Live])
+ .query(params![domain.0 as u32, namespace, KeyLifeCycle::Live, key_type])
.context("In list: Failed to query.")?;
let mut descriptors: Vec<KeyDescriptor> = Vec::new();
@@ -4747,7 +4756,7 @@
})
.collect();
list_o_descriptors.sort();
- let mut list_result = db.list(*domain, *namespace)?;
+ let mut list_result = db.list(*domain, *namespace, KeyType::Client)?;
list_result.sort();
assert_eq!(list_o_descriptors, list_result);
@@ -4777,7 +4786,7 @@
loaded_entries.sort_unstable();
assert_eq!(list_o_ids, loaded_entries);
}
- assert_eq!(Vec::<KeyDescriptor>::new(), db.list(Domain::SELINUX, 101)?);
+ assert_eq!(Vec::<KeyDescriptor>::new(), db.list(Domain::SELINUX, 101, KeyType::Client)?);
Ok(())
}
@@ -5240,11 +5249,11 @@
make_test_key_entry(&mut db, Domain::APP, 110000, TEST_ALIAS, None)?;
db.unbind_keys_for_user(2, false)?;
- assert_eq!(1, db.list(Domain::APP, 110000)?.len());
- assert_eq!(0, db.list(Domain::APP, 210000)?.len());
+ assert_eq!(1, db.list(Domain::APP, 110000, KeyType::Client)?.len());
+ assert_eq!(0, db.list(Domain::APP, 210000, KeyType::Client)?.len());
db.unbind_keys_for_user(1, true)?;
- assert_eq!(0, db.list(Domain::APP, 110000)?.len());
+ assert_eq!(0, db.list(Domain::APP, 110000, KeyType::Client)?.len());
Ok(())
}
diff --git a/keystore2/src/service.rs b/keystore2/src/service.rs
index 3ce0550..1f61729 100644
--- a/keystore2/src/service.rs
+++ b/keystore2/src/service.rs
@@ -291,7 +291,7 @@
&mut DB
.with(|db| {
let mut db = db.borrow_mut();
- db.list(k.domain, k.nspace)
+ db.list(k.domain, k.nspace, KeyType::Client)
})
.context("In list_entries: Trying to list keystore database.")?,
);