Merge "Revert "Remove fsverity_init"" into main
diff --git a/OWNERS b/OWNERS
index 03e5769..6fdb550 100644
--- a/OWNERS
+++ b/OWNERS
@@ -1,5 +1,4 @@
alanstokes@google.com
-cbrubaker@google.com
drysdale@google.com
eranm@google.com
hasinitg@google.com
@@ -8,4 +7,5 @@
kroot@google.com
sethmo@google.com
swillden@google.com
+trong@google.com
zeuthen@google.com
diff --git a/keystore/keystore_attestation_id.cpp b/keystore/keystore_attestation_id.cpp
index ccd3808..75c62dd 100644
--- a/keystore/keystore_attestation_id.cpp
+++ b/keystore/keystore_attestation_id.cpp
@@ -74,8 +74,8 @@
}
KeyAttestationApplicationIdProvider::KeyAttestationApplicationIdProvider()
- : BpKeyAttestationApplicationIdProvider(
- android::defaultServiceManager()->getService(String16("sec_key_att_app_id_provider"))) {}
+ : BpKeyAttestationApplicationIdProvider(android::defaultServiceManager()->waitForService(
+ String16("sec_key_att_app_id_provider"))) {}
DECLARE_STACK_OF(ASN1_OCTET_STRING);
diff --git a/keystore2/src/enforcements.rs b/keystore2/src/enforcements.rs
index 8d5e985..3bf582f 100644
--- a/keystore2/src/enforcements.rs
+++ b/keystore2/src/enforcements.rs
@@ -51,9 +51,9 @@
/// An outstanding per operation authorization request.
OpAuth,
/// An outstanding request for per operation authorization and secure timestamp.
- TimeStampedOpAuth(Receiver<Result<TimeStampToken, Error>>),
+ TimeStampedOpAuth(Mutex<Receiver<Result<TimeStampToken, Error>>>),
/// An outstanding request for a timestamp token.
- TimeStamp(Receiver<Result<TimeStampToken, Error>>),
+ TimeStamp(Mutex<Receiver<Result<TimeStampToken, Error>>>),
}
#[derive(Debug)]
@@ -64,8 +64,6 @@
hat: Mutex<Option<HardwareAuthToken>>,
}
-unsafe impl Sync for AuthRequest {}
-
impl AuthRequest {
fn op_auth() -> Arc<Self> {
Arc::new(Self { state: AuthRequestState::OpAuth, hat: Mutex::new(None) })
@@ -73,7 +71,7 @@
fn timestamped_op_auth(receiver: Receiver<Result<TimeStampToken, Error>>) -> Arc<Self> {
Arc::new(Self {
- state: AuthRequestState::TimeStampedOpAuth(receiver),
+ state: AuthRequestState::TimeStampedOpAuth(Mutex::new(receiver)),
hat: Mutex::new(None),
})
}
@@ -82,7 +80,10 @@
hat: HardwareAuthToken,
receiver: Receiver<Result<TimeStampToken, Error>>,
) -> Arc<Self> {
- Arc::new(Self { state: AuthRequestState::TimeStamp(receiver), hat: Mutex::new(Some(hat)) })
+ Arc::new(Self {
+ state: AuthRequestState::TimeStamp(Mutex::new(receiver)),
+ hat: Mutex::new(Some(hat)),
+ })
}
fn add_auth_token(&self, hat: HardwareAuthToken) {
@@ -100,7 +101,11 @@
let tst = match &self.state {
AuthRequestState::TimeStampedOpAuth(recv) | AuthRequestState::TimeStamp(recv) => {
- let result = recv.recv().context("In get_auth_tokens: Sender disconnected.")?;
+ let result = recv
+ .lock()
+ .unwrap()
+ .recv()
+ .context("In get_auth_tokens: Sender disconnected.")?;
Some(result.context(ks_err!(
"Worker responded with error \
from generating timestamp token.",
diff --git a/keystore2/src/id_rotation.rs b/keystore2/src/id_rotation.rs
index 460caa7..5904047 100644
--- a/keystore2/src/id_rotation.rs
+++ b/keystore2/src/id_rotation.rs
@@ -26,7 +26,7 @@
use std::fs;
use std::io::ErrorKind;
use std::path::{Path, PathBuf};
-use std::time::Duration;
+use std::time::{Duration, SystemTime};
const ID_ROTATION_PERIOD: Duration = Duration::from_secs(30 * 24 * 60 * 60); // Thirty days.
static TIMESTAMP_FILE_NAME: &str = "timestamp";
@@ -36,6 +36,8 @@
/// and passed down to the users of the feature which can then query the timestamp on demand.
#[derive(Debug, Clone)]
pub struct IdRotationState {
+ /// We consider the time of last factory reset to be the point in time when this timestamp file
+ /// is created.
timestamp_path: PathBuf,
}
@@ -47,25 +49,41 @@
Self { timestamp_path }
}
- /// Reads the metadata of or creates the timestamp file. It returns true if the timestamp
- /// file is younger than `ID_ROTATION_PERIOD`, i.e., 30 days.
- pub fn had_factory_reset_since_id_rotation(&self) -> Result<bool> {
+ /// Returns true iff a factory reset has occurred since the last ID rotation.
+ pub fn had_factory_reset_since_id_rotation(
+ &self,
+ creation_datetime: &SystemTime,
+ ) -> Result<bool> {
match fs::metadata(&self.timestamp_path) {
Ok(metadata) => {
- let duration_since_factory_reset = metadata
- .modified()
- .context("File creation time not supported.")?
- .elapsed()
- .context("Failed to compute time elapsed since factory reset.")?;
- Ok(duration_since_factory_reset < ID_ROTATION_PERIOD)
+ // For Tag::UNIQUE_ID, temporal counter value is defined as Tag::CREATION_DATETIME
+ // divided by 2592000000, dropping any remainder. Temporal counter value is
+ // effectively the index of the ID rotation period that we are currently in, with
+ // each ID rotation period being 30 days.
+ let temporal_counter_value = creation_datetime
+ .duration_since(SystemTime::UNIX_EPOCH)
+ .context(ks_err!("Failed to get epoch time"))?
+ .as_millis()
+ / ID_ROTATION_PERIOD.as_millis();
+
+ // Calculate the beginning of the current ID rotation period, which is also the
+ // last time ID was rotated.
+ let id_rotation_time: SystemTime = SystemTime::UNIX_EPOCH
+ .checked_add(ID_ROTATION_PERIOD * temporal_counter_value.try_into()?)
+ .context(ks_err!("Failed to get ID rotation time."))?;
+
+ let factory_reset_time =
+ metadata.modified().context(ks_err!("File creation time not supported."))?;
+
+ Ok(id_rotation_time <= factory_reset_time)
}
Err(e) => match e.kind() {
ErrorKind::NotFound => {
fs::File::create(&self.timestamp_path)
- .context("Failed to create timestamp file.")?;
+ .context(ks_err!("Failed to create timestamp file."))?;
Ok(true)
}
- _ => Err(e).context("Failed to open timestamp file."),
+ _ => Err(e).context(ks_err!("Failed to open timestamp file.")),
},
}
.context(ks_err!())
@@ -78,47 +96,75 @@
use keystore2_test_utils::TempDir;
use nix::sys::stat::utimes;
use nix::sys::time::{TimeVal, TimeValLike};
- use std::convert::TryInto;
- use std::time::UNIX_EPOCH;
+ use std::thread::sleep;
- #[test]
- fn test_had_factory_reset_since_id_rotation() -> Result<()> {
- let temp_dir = TempDir::new("test_had_factory_reset_since_id_rotation_")
- .expect("Failed to create temp dir.");
+ static TEMP_DIR_NAME: &str = "test_had_factory_reset_since_id_rotation_";
+
+ fn set_up() -> (TempDir, PathBuf, IdRotationState) {
+ let temp_dir = TempDir::new(TEMP_DIR_NAME).expect("Failed to create temp dir.");
+ let mut timestamp_file_path = temp_dir.path().to_owned();
+ timestamp_file_path.push(TIMESTAMP_FILE_NAME);
let id_rotation_state = IdRotationState::new(temp_dir.path());
- let mut temp_file_path = temp_dir.path().to_owned();
- temp_file_path.push(TIMESTAMP_FILE_NAME);
+ (temp_dir, timestamp_file_path, id_rotation_state)
+ }
+
+ #[test]
+ fn test_timestamp_creation() {
+ let (_temp_dir, timestamp_file_path, id_rotation_state) = set_up();
+ let creation_datetime = SystemTime::now();
// The timestamp file should not exist.
- assert!(!temp_file_path.exists());
+ assert!(!timestamp_file_path.exists());
- // This should return true.
- assert!(id_rotation_state.had_factory_reset_since_id_rotation()?);
+ // Trigger timestamp file creation one second later.
+ sleep(Duration::new(1, 0));
+ assert!(id_rotation_state.had_factory_reset_since_id_rotation(&creation_datetime).unwrap());
// Now the timestamp file should exist.
- assert!(temp_file_path.exists());
+ assert!(timestamp_file_path.exists());
- // We should still return true because the timestamp file is young.
- assert!(id_rotation_state.had_factory_reset_since_id_rotation()?);
+ let metadata = fs::metadata(×tamp_file_path).unwrap();
+ assert!(metadata.modified().unwrap() > creation_datetime);
+ }
- // Now let's age the timestamp file by backdating the modification time.
- let metadata = fs::metadata(&temp_file_path)?;
- let mtime = metadata.modified()?;
- let mtime = mtime.duration_since(UNIX_EPOCH)?;
- let mtime =
- mtime.checked_sub(ID_ROTATION_PERIOD).expect("Failed to subtract id rotation period");
- let mtime = TimeVal::seconds(mtime.as_secs().try_into().unwrap());
+ #[test]
+ fn test_existing_timestamp() {
+ let (_temp_dir, timestamp_file_path, id_rotation_state) = set_up();
- let atime = metadata.accessed()?;
- let atime = atime.duration_since(UNIX_EPOCH)?;
- let atime = TimeVal::seconds(atime.as_secs().try_into().unwrap());
+ // Let's start with at a known point in time, so that it's easier to control which ID
+ // rotation period we're in.
+ let mut creation_datetime = SystemTime::UNIX_EPOCH;
- utimes(&temp_file_path, &atime, &mtime)?;
+ // Create timestamp file and backdate it back to Unix epoch.
+ fs::File::create(×tamp_file_path).unwrap();
+ let mtime = TimeVal::seconds(0);
+ let atime = TimeVal::seconds(0);
+ utimes(×tamp_file_path, &atime, &mtime).unwrap();
- // Now that the file has aged we should see false.
- assert!(!id_rotation_state.had_factory_reset_since_id_rotation()?);
+ // Timestamp file was backdated to the very beginning of the current ID rotation period.
+ // So, this should return true.
+ assert!(id_rotation_state.had_factory_reset_since_id_rotation(&creation_datetime).unwrap());
- Ok(())
+ // Move time forward, but stay in the same ID rotation period.
+ creation_datetime += Duration::from_millis(1);
+
+ // We should still return true because we're in the same ID rotation period.
+ assert!(id_rotation_state.had_factory_reset_since_id_rotation(&creation_datetime).unwrap());
+
+ // Move time to the next ID rotation period.
+ creation_datetime += ID_ROTATION_PERIOD;
+
+ // Now we should see false.
+ assert!(!id_rotation_state
+ .had_factory_reset_since_id_rotation(&creation_datetime)
+ .unwrap());
+
+ // Move timestamp to the future. This shouldn't ever happen, but even in this edge case ID
+ // must be rotated.
+ let mtime = TimeVal::seconds((ID_ROTATION_PERIOD.as_secs() * 10).try_into().unwrap());
+ let atime = TimeVal::seconds((ID_ROTATION_PERIOD.as_secs() * 10).try_into().unwrap());
+ utimes(×tamp_file_path, &atime, &mtime).unwrap();
+ assert!(id_rotation_state.had_factory_reset_since_id_rotation(&creation_datetime).unwrap());
}
}
diff --git a/keystore2/src/security_level.rs b/keystore2/src/security_level.rs
index 5eed37c..db44d4b 100644
--- a/keystore2/src/security_level.rs
+++ b/keystore2/src/security_level.rs
@@ -405,8 +405,7 @@
) -> Result<Vec<KeyParameter>> {
let mut result = params.to_vec();
- // Unconditionally add the CREATION_DATETIME tag and prevent callers from
- // specifying it.
+ // Prevent callers from specifying the CREATION_DATETIME tag.
if params.iter().any(|kp| kp.tag == Tag::CREATION_DATETIME) {
return Err(Error::Rc(ResponseCode::INVALID_ARGUMENT)).context(ks_err!(
"KeystoreSecurityLevel::add_required_parameters: \
@@ -414,12 +413,16 @@
));
}
+ // Use this variable to refer to notion of "now". This eliminates discrepancies from
+ // quering the clock multiple times.
+ let creation_datetime = SystemTime::now();
+
// Add CREATION_DATETIME only if the backend version Keymint V1 (100) or newer.
if self.hw_info.versionNumber >= 100 {
result.push(KeyParameter {
tag: Tag::CREATION_DATETIME,
value: KeyParameterValue::DateTime(
- SystemTime::now()
+ creation_datetime
.duration_since(SystemTime::UNIX_EPOCH)
.context(ks_err!(
"KeystoreSecurityLevel::add_required_parameters: \
@@ -462,7 +465,7 @@
}
if self
.id_rotation_state
- .had_factory_reset_since_id_rotation()
+ .had_factory_reset_since_id_rotation(&creation_datetime)
.context(ks_err!("Call to had_factory_reset_since_id_rotation failed."))?
{
result.push(KeyParameter {
diff --git a/prng_seeder/src/cutils_socket.rs b/prng_seeder/src/cutils_socket.rs
index ab2c869..b408be6 100644
--- a/prng_seeder/src/cutils_socket.rs
+++ b/prng_seeder/src/cutils_socket.rs
@@ -19,7 +19,11 @@
pub fn android_get_control_socket(name: &str) -> Result<UnixListener> {
let name = CString::new(name)?;
+ // SAFETY: name is a valid C string, and android_get_control_socket doesn't retain it after it
+ // returns.
let fd = unsafe { cutils_socket_bindgen::android_get_control_socket(name.as_ptr()) };
ensure!(fd >= 0, "android_get_control_socket failed");
+ // SAFETY: android_get_control_socket either returns a valid and open FD or -1, and we checked
+ // that it's not -1.
Ok(unsafe { UnixListener::from_raw_fd(fd) })
}
diff --git a/prng_seeder/src/drbg.rs b/prng_seeder/src/drbg.rs
index 89c5a88..808ea18 100644
--- a/prng_seeder/src/drbg.rs
+++ b/prng_seeder/src/drbg.rs
@@ -23,6 +23,9 @@
impl Drbg {
pub fn new(entropy: &Entropy) -> Result<Drbg> {
+ // SAFETY: entropy must be a valid pointer because it comes from a reference, and a null
+ // pointer is allowed for personalization. CTR_DRBG_new doesn't retain the entropy pointer
+ // for use after it returns.
let p = unsafe { bssl_sys::CTR_DRBG_new(entropy.as_ptr(), std::ptr::null(), 0) };
ensure!(!p.is_null(), "CTR_DRBG_new failed");
Ok(Drbg(p))
@@ -30,6 +33,9 @@
pub fn reseed(&mut self, entropy: &Entropy) -> Result<()> {
ensure!(
+ // SAFETY: We know that self.0 is valid because it was initialised from CTR_DRBG_new in
+ // Drbg::new above. The entropy pointer must be valid because it comes from a reference,
+ // and CTR_DRBG_reseed doesn't retain it after it returns.
unsafe { bssl_sys::CTR_DRBG_reseed(self.0, entropy.as_ptr(), std::ptr::null(), 0) }
== 1,
"CTR_DRBG_reseed failed"
@@ -39,6 +45,10 @@
pub fn generate(&mut self, buf: &mut [u8]) -> Result<()> {
ensure!(
+ // SAFETY: We know that self.0 is valid because it was initialised from CTR_DRBG_new in
+ // Drbg::new above. The out pointer and length must be valid and unaliased because they
+ // come from a mutable slice reference, and CTR_DRBG_generate doesn't retain them after
+ // it returns.
unsafe {
bssl_sys::CTR_DRBG_generate(
self.0,
@@ -56,10 +66,13 @@
impl Drop for Drbg {
fn drop(&mut self) {
+ // SAFETY: We know that self.0 is valid because it was initialised from CTR_DRBG_new in
+ // Drbg::new above, and this is the only place that frees it.
unsafe {
bssl_sys::CTR_DRBG_free(self.0);
}
}
}
+// SAFETY: CTR_DRBG functions can be called from any thread.
unsafe impl Send for Drbg {}
diff --git a/prng_seeder/src/main.rs b/prng_seeder/src/main.rs
index 924481a..f8b0c63 100644
--- a/prng_seeder/src/main.rs
+++ b/prng_seeder/src/main.rs
@@ -70,6 +70,7 @@
fn setup() -> Result<(ConditionerBuilder, UnixListener)> {
configure_logging()?;
let cli = Cli::try_parse()?;
+ // SAFETY: Nothing else sets the signal handler, so either it was set here or it is the default.
unsafe { signal::signal(signal::Signal::SIGPIPE, signal::SigHandler::SigIgn) }
.context("In setup, setting SIGPIPE to SIG_IGN")?;