Merge "Keystore 2.0: add entropy feeder on idle"
diff --git a/keystore2/aidl/android/security/remoteprovisioning/IRemoteProvisioning.aidl b/keystore2/aidl/android/security/remoteprovisioning/IRemoteProvisioning.aidl
index 0d4c30f..5c2d0b1 100644
--- a/keystore2/aidl/android/security/remoteprovisioning/IRemoteProvisioning.aidl
+++ b/keystore2/aidl/android/security/remoteprovisioning/IRemoteProvisioning.aidl
@@ -133,4 +133,13 @@
* @return The array of security levels.
*/
SecurityLevel[] getSecurityLevels();
+
+ /**
+ * This method deletes all remotely provisioned attestation keys in the database, regardless
+ * of what state in their life cycle they are in. This is primarily useful to facilitate
+ * testing.
+ *
+ * @return Number of keys deleted
+ */
+ long deleteAllKeys();
}
diff --git a/keystore2/selinux/src/lib.rs b/keystore2/selinux/src/lib.rs
index 2b5091d..cc707e7 100644
--- a/keystore2/selinux/src/lib.rs
+++ b/keystore2/selinux/src/lib.rs
@@ -455,7 +455,6 @@
check_keystore_perm!(add_auth);
check_keystore_perm!(clear_ns);
- check_keystore_perm!(get_state);
check_keystore_perm!(lock);
check_keystore_perm!(reset);
check_keystore_perm!(unlock);
diff --git a/keystore2/src/database.rs b/keystore2/src/database.rs
index c3b90e7..22ab02e 100644
--- a/keystore2/src/database.rs
+++ b/keystore2/src/database.rs
@@ -1794,6 +1794,33 @@
.context("In delete_expired_attestation_keys: ")
}
+ /// Deletes all remotely provisioned attestation keys in the system, regardless of the state
+ /// they are in. This is useful primarily as a testing mechanism.
+ pub fn delete_all_attestation_keys(&mut self) -> Result<i64> {
+ self.with_transaction(TransactionBehavior::Immediate, |tx| {
+ let mut stmt = tx
+ .prepare(
+ "SELECT id FROM persistent.keyentry
+ WHERE key_type IS ?;",
+ )
+ .context("Failed to prepare statement")?;
+ let keys_to_delete = stmt
+ .query_map(params![KeyType::Attestation], |row| Ok(row.get(0)?))?
+ .collect::<rusqlite::Result<Vec<i64>>>()
+ .context("Failed to execute statement")?;
+ let num_deleted = keys_to_delete
+ .iter()
+ .map(|id| Self::mark_unreferenced(&tx, *id))
+ .collect::<Result<Vec<bool>>>()
+ .context("Failed to execute mark_unreferenced on a keyid")?
+ .into_iter()
+ .filter(|result| *result)
+ .count() as i64;
+ Ok(num_deleted).do_gc(num_deleted != 0)
+ })
+ .context("In delete_all_attestation_keys: ")
+ }
+
/// Counts the number of keys that will expire by the provided epoch date and the number of
/// keys not currently assigned to a domain.
pub fn get_attestation_pool_status(
@@ -3356,6 +3383,23 @@
}
#[test]
+ fn test_delete_all_attestation_keys() -> Result<()> {
+ let mut db = new_test_db()?;
+ load_attestation_key_pool(&mut db, 45 /* expiration */, 1 /* namespace */, 0x02)?;
+ load_attestation_key_pool(&mut db, 80 /* expiration */, 2 /* namespace */, 0x03)?;
+ db.create_key_entry(&Domain::APP, &42, &KEYSTORE_UUID)?;
+ let result = db.delete_all_attestation_keys()?;
+
+ // Give the garbage collector half a second to catch up.
+ std::thread::sleep(Duration::from_millis(500));
+
+ // Attestation keys should be deleted, and the regular key should remain.
+ assert_eq!(result, 2);
+
+ Ok(())
+ }
+
+ #[test]
fn test_rebind_alias() -> Result<()> {
fn extractor(
ke: &KeyEntryRow,
diff --git a/keystore2/src/km_compat/km_compat.cpp b/keystore2/src/km_compat/km_compat.cpp
index 3439d2f..773b4b9 100644
--- a/keystore2/src/km_compat/km_compat.cpp
+++ b/keystore2/src/km_compat/km_compat.cpp
@@ -216,6 +216,8 @@
*/
bool isNewAndKeystoreEnforceable(const KMV1::KeyParameter& param) {
switch (param.tag) {
+ case KMV1::Tag::MAX_BOOT_LEVEL:
+ return true;
case KMV1::Tag::USAGE_COUNT_LIMIT:
return true;
default:
diff --git a/keystore2/src/permission.rs b/keystore2/src/permission.rs
index 905b460..7f63834 100644
--- a/keystore2/src/permission.rs
+++ b/keystore2/src/permission.rs
@@ -675,7 +675,7 @@
let shell_ctx = Context::new("u:r:shell:s0")?;
assert_perm_failed!(check_keystore_permission(&shell_ctx, KeystorePerm::add_auth()));
assert_perm_failed!(check_keystore_permission(&shell_ctx, KeystorePerm::clear_ns()));
- assert_perm_failed!(check_keystore_permission(&shell_ctx, KeystorePerm::get_state()));
+ assert!(check_keystore_permission(&shell_ctx, KeystorePerm::get_state()).is_ok());
assert_perm_failed!(check_keystore_permission(&shell_ctx, KeystorePerm::list()));
assert_perm_failed!(check_keystore_permission(&shell_ctx, KeystorePerm::lock()));
assert_perm_failed!(check_keystore_permission(&shell_ctx, KeystorePerm::reset()));
diff --git a/keystore2/src/remote_provisioning.rs b/keystore2/src/remote_provisioning.rs
index d6cc680..cc97573 100644
--- a/keystore2/src/remote_provisioning.rs
+++ b/keystore2/src/remote_provisioning.rs
@@ -367,6 +367,15 @@
pub fn get_security_levels(&self) -> Result<Vec<SecurityLevel>> {
Ok(self.device_by_sec_level.keys().cloned().collect())
}
+
+ /// Deletes all attestation keys generated by the IRemotelyProvisionedComponent from the device,
+ /// regardless of what state of the attestation key lifecycle they were in.
+ pub fn delete_all_keys(&self) -> Result<i64> {
+ DB.with::<_, Result<i64>>(|db| {
+ let mut db = db.borrow_mut();
+ Ok(db.delete_all_attestation_keys()?)
+ })
+ }
}
impl binder::Interface for RemoteProvisioningService {}
@@ -422,4 +431,8 @@
fn getSecurityLevels(&self) -> binder::public_api::Result<Vec<SecurityLevel>> {
map_or_log_err(self.get_security_levels(), Ok)
}
+
+ fn deleteAllKeys(&self) -> binder::public_api::Result<i64> {
+ map_or_log_err(self.delete_all_keys(), Ok)
+ }
}
diff --git a/ondevice-signing/CertUtils.cpp b/ondevice-signing/CertUtils.cpp
index cbd1942..b0b75a6 100644
--- a/ondevice-signing/CertUtils.cpp
+++ b/ondevice-signing/CertUtils.cpp
@@ -147,8 +147,10 @@
x509->signature->flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07);
x509->signature->flags |= ASN1_STRING_FLAG_BITS_LEFT;
- auto f = fopen(path.c_str(), "wb");
- // TODO error checking
+ auto f = fopen(path.c_str(), "wbe");
+ if (f == nullptr) {
+ return Error() << "Failed to open " << path;
+ }
i2d_X509_fp(f, x509.get());
fclose(f);
@@ -199,8 +201,12 @@
Result<std::vector<uint8_t>> extractPublicKeyFromX509(const std::string& path) {
X509* cert;
- auto f = fopen(path.c_str(), "r");
+ auto f = fopen(path.c_str(), "re");
+ if (f == nullptr) {
+ return Error() << "Failed to open " << path;
+ }
if (!d2i_X509_fp(f, &cert)) {
+ fclose(f);
return Error() << "Unable to decode x509 cert at " << path;
}
diff --git a/ondevice-signing/VerityUtils.cpp b/ondevice-signing/VerityUtils.cpp
index ff7de7e..cab92e2 100644
--- a/ondevice-signing/VerityUtils.cpp
+++ b/ondevice-signing/VerityUtils.cpp
@@ -74,8 +74,14 @@
Result<std::vector<uint8_t>> createDigest(const std::string& path) {
struct stat filestat;
unique_fd fd(TEMP_FAILURE_RETRY(open(path.c_str(), O_RDONLY | O_CLOEXEC)));
+ if (fd < 0) {
+ return ErrnoError() << "Failed to open " << path;
+ }
- stat(path.c_str(), &filestat);
+ int ret = stat(path.c_str(), &filestat);
+ if (ret < 0) {
+ return ErrnoError() << "Failed to stat " << path;
+ }
struct libfsverity_merkle_tree_params params = {
.version = 1,
.hash_algorithm = FS_VERITY_HASH_ALG_SHA256,
@@ -84,9 +90,13 @@
};
struct libfsverity_digest* digest;
- libfsverity_compute_digest(&fd, &read_callback, ¶ms, &digest);
-
- return std::vector<uint8_t>(&digest->digest[0], &digest->digest[32]);
+ ret = libfsverity_compute_digest(&fd, &read_callback, ¶ms, &digest);
+ if (ret < 0) {
+ return ErrnoError() << "Failed to compute fs-verity digest for " << path;
+ }
+ std::vector<uint8_t> digestVector(&digest->digest[0], &digest->digest[32]);
+ free(digest);
+ return digestVector;
}
namespace {
diff --git a/ondevice-signing/odsign_main.cpp b/ondevice-signing/odsign_main.cpp
index eeef868..58e49ec 100644
--- a/ondevice-signing/odsign_main.cpp
+++ b/ondevice-signing/odsign_main.cpp
@@ -26,6 +26,7 @@
#include <android-base/file.h>
#include <android-base/logging.h>
+#include <android-base/properties.h>
#include <android-base/scopeguard.h>
#include <logwrap/logwrap.h>
@@ -39,6 +40,7 @@
using android::base::ErrnoError;
using android::base::Error;
using android::base::Result;
+using android::base::SetProperty;
using OdsignInfo = ::odsign::proto::OdsignInfo;
@@ -56,6 +58,13 @@
static const bool kForceCompilation = false;
static const bool kUseKeystore = false;
+static const char* kOdsignVerificationDoneProp = "odsign.verification.done";
+static const char* kOdsignKeyDoneProp = "odsign.key.done";
+
+static const char* kOdsignVerificationStatusProp = "odsign.verification.success";
+static const char* kOdsignVerificationStatusValid = "1";
+static const char* kOdsignVerificationStatusError = "0";
+
Result<void> verifyExistingCert(const SigningKey& key) {
if (access(kSigningKeyCert.c_str(), F_OK) < 0) {
return ErrnoError() << "Key certificate not found: " << kSigningKeyCert;
@@ -237,23 +246,62 @@
return {};
}
-int main(int /* argc */, char** /* argv */) {
- auto removeArtifacts = []() -> std::uintmax_t {
- std::error_code ec;
- auto num_removed = std::filesystem::remove_all(kArtArtifactsDir, ec);
- if (ec) {
- // TODO can't remove artifacts, signal Zygote shouldn't use them
- LOG(ERROR) << "Can't remove " << kArtArtifactsDir << ": " << ec.message();
- return 0;
- } else {
- if (num_removed > 0) {
- LOG(INFO) << "Removed " << num_removed << " entries from " << kArtArtifactsDir;
- }
- return num_removed;
+static int removeArtifacts() {
+ std::error_code ec;
+ auto num_removed = std::filesystem::remove_all(kArtArtifactsDir, ec);
+ if (ec) {
+ LOG(ERROR) << "Can't remove " << kArtArtifactsDir << ": " << ec.message();
+ return 0;
+ } else {
+ if (num_removed > 0) {
+ LOG(INFO) << "Removed " << num_removed << " entries from " << kArtArtifactsDir;
}
+ return num_removed;
+ }
+}
+
+static Result<void> verifyArtifacts(const SigningKey& key, bool supportsFsVerity) {
+ auto signInfo = getOdsignInfo(key);
+ // Tell init we're done with the key; this is a boot time optimization
+ // in particular for the no fs-verity case, where we need to do a
+ // costly verification. If the files haven't been tampered with, which
+ // should be the common path, the verification will succeed, and we won't
+ // need the key anymore. If it turns out the artifacts are invalid (eg not
+ // in fs-verity) or the hash doesn't match, we won't be able to generate
+ // new artifacts without the key, so in those cases, remove the artifacts,
+ // and use JIT zygote for the current boot. We should recover automatically
+ // by the next boot.
+ SetProperty(kOdsignKeyDoneProp, "1");
+ if (!signInfo.ok()) {
+ return Error() << signInfo.error().message();
+ }
+ std::map<std::string, std::string> trusted_digests(signInfo->file_hashes().begin(),
+ signInfo->file_hashes().end());
+ Result<void> integrityStatus;
+
+ if (supportsFsVerity) {
+ integrityStatus = verifyIntegrityFsVerity(trusted_digests);
+ } else {
+ integrityStatus = verifyIntegrityNoFsVerity(trusted_digests);
+ }
+ if (!integrityStatus.ok()) {
+ return Error() << integrityStatus.error().message();
+ }
+
+ return {};
+}
+
+int main(int /* argc */, char** /* argv */) {
+ auto errorScopeGuard = []() {
+ // In case we hit any error, remove the artifacts and tell Zygote not to use anything
+ removeArtifacts();
+ // Tell init we don't need to use our key anymore
+ SetProperty(kOdsignKeyDoneProp, "1");
+ // Tell init we're done with verification, and that it was an error
+ SetProperty(kOdsignVerificationDoneProp, "1");
+ SetProperty(kOdsignVerificationStatusProp, kOdsignVerificationStatusError);
};
- // Make sure we delete the artifacts in all early (error) exit paths
- auto scope_guard = android::base::make_scope_guard(removeArtifacts);
+ auto scope_guard = android::base::make_scope_guard(errorScopeGuard);
SigningKey* key;
if (kUseKeystore) {
@@ -301,35 +349,25 @@
}
}
- auto signInfo = getOdsignInfo(*key);
- if (!signInfo.ok()) {
- int num_removed = removeArtifacts();
- // Only a warning if there were artifacts to begin with, which suggests tampering or
- // corruption
- if (num_removed > 0) {
- LOG(WARNING) << signInfo.error().message();
- }
- } else {
- std::map<std::string, std::string> trusted_digests(signInfo->file_hashes().begin(),
- signInfo->file_hashes().end());
- Result<void> integrityStatus;
-
- if (supportsFsVerity) {
- integrityStatus = verifyIntegrityFsVerity(trusted_digests);
- } else {
- integrityStatus = verifyIntegrityNoFsVerity(trusted_digests);
- }
- if (!integrityStatus.ok()) {
- LOG(WARNING) << integrityStatus.error().message() << ", removing " << kArtArtifactsDir;
- removeArtifacts();
- }
- }
-
// Ask ART whether it considers the artifacts valid
LOG(INFO) << "Asking odrefresh to verify artifacts (if present)...";
bool artifactsValid = validateArtifacts();
LOG(INFO) << "odrefresh said they are " << (artifactsValid ? "VALID" : "INVALID");
+ // A post-condition of validating artifacts is that if the ones on /system
+ // are used, kArtArtifactsDir is removed. Conversely, if kArtArtifactsDir
+ // exists, those are artifacts that will be used, and we should verify them.
+ int err = access(kArtArtifactsDir.c_str(), F_OK);
+ // If we receive any error other than ENOENT, be suspicious
+ bool artifactsPresent = (err == 0) || (err < 0 && errno != ENOENT);
+ if (artifactsPresent) {
+ auto verificationResult = verifyArtifacts(*key, supportsFsVerity);
+ if (!verificationResult.ok()) {
+ LOG(ERROR) << verificationResult.error().message();
+ return -1;
+ }
+ }
+
if (!artifactsValid || kForceCompilation) {
LOG(INFO) << "Starting compilation... ";
bool ret = compileArtifacts(kForceCompilation);
@@ -355,10 +393,13 @@
}
}
- // TODO we want to make sure Zygote only picks up the artifacts if we deemed
- // everything was ok here. We could use a sysprop, or some other mechanism?
LOG(INFO) << "On-device signing done.";
scope_guard.Disable();
+ // At this point, we're done with the key for sure
+ SetProperty(kOdsignKeyDoneProp, "1");
+ // And we did a successful verification
+ SetProperty(kOdsignVerificationDoneProp, "1");
+ SetProperty(kOdsignVerificationStatusProp, kOdsignVerificationStatusValid);
return 0;
}