Fix grants get lost on key upgrade
The upgrade routine used to call KeyStore->del which purges the given
key blob from the keystore including all existing grants.
With this patch, upgrade only calls Keymaster::delete on the keyblobs
without purging it from the keystore. Also it only calls
Keymaster::delete once the upgrade key was successfully written to disk.
This patch also calls fsync on the directory containing keyblobs to
narrow the window in which keyblob may be lost due to power loss.
Bug: 110450771
Test: Upgrade path tested by manually creating a key, bumping the
patchlevel, using the key subsequently and inspecting the logs.
Change-Id: I89241b5d4033b270733ff61838ab9244fce28c60
diff --git a/keystore/blob.cpp b/keystore/blob.cpp
index d1629cb..0987139 100644
--- a/keystore/blob.cpp
+++ b/keystore/blob.cpp
@@ -36,6 +36,7 @@
#include <string>
#include <android-base/logging.h>
+#include <android-base/unique_fd.h>
namespace {
@@ -341,22 +342,35 @@
size_t fileLength = offsetof(blobv3, value) + dataLength + rawBlob->info;
- int out =
- TEMP_FAILURE_RETRY(open(filename.c_str(), O_WRONLY | O_TRUNC | O_CREAT, S_IRUSR | S_IWUSR));
- if (out < 0) {
- ALOGW("could not open file: %s: %s", filename.c_str(), strerror(errno));
+ char tmpFileName[] = ".tmpXXXXXX";
+ {
+ android::base::unique_fd out(TEMP_FAILURE_RETRY(mkstemp(tmpFileName)));
+ if (out < 0) {
+ LOG(ERROR) << "could not open temp file: " << tmpFileName
+ << " for writing blob file: " << filename.c_str()
+ << " because: " << strerror(errno);
+ return ResponseCode::SYSTEM_ERROR;
+ }
+
+ const size_t writtenBytes =
+ writeFully(out, reinterpret_cast<uint8_t*>(rawBlob), fileLength);
+
+ if (writtenBytes != fileLength) {
+ LOG(ERROR) << "blob not fully written " << writtenBytes << " != " << fileLength;
+ unlink(tmpFileName);
+ return ResponseCode::SYSTEM_ERROR;
+ }
+ }
+
+ if (rename(tmpFileName, filename.c_str()) == -1) {
+ LOG(ERROR) << "could not rename blob file to " << filename
+ << " because: " << strerror(errno);
+ unlink(tmpFileName);
return ResponseCode::SYSTEM_ERROR;
}
- const size_t writtenBytes = writeFully(out, reinterpret_cast<uint8_t*>(rawBlob), fileLength);
- if (close(out) != 0) {
- return ResponseCode::SYSTEM_ERROR;
- }
- if (writtenBytes != fileLength) {
- ALOGW("blob not fully written %zu != %zu", writtenBytes, fileLength);
- unlink(filename.c_str());
- return ResponseCode::SYSTEM_ERROR;
- }
+ fsyncDirectory(getContainingDirectory(filename));
+
return ResponseCode::NO_ERROR;
}
diff --git a/keystore/keymaster_worker.cpp b/keystore/keymaster_worker.cpp
index 922ef0a..23a0023 100644
--- a/keystore/keymaster_worker.cpp
+++ b/keystore/keymaster_worker.cpp
@@ -22,6 +22,10 @@
#include <android-base/logging.h>
+#include <log/log_event_list.h>
+
+#include <private/android_logger.h>
+
#include "KeyStore.h"
#include "keymaster_enforcement.h"
@@ -74,6 +78,26 @@
keymasterDevice_->logIfKeymasterVendorError(ec);
}
+void KeymasterWorker::deleteOldKeyOnUpgrade(const LockedKeyBlobEntry& blobfile, Blob keyBlob) {
+ // if we got the blob successfully, we try and delete it from the keymaster device
+ auto& dev = keymasterDevice_;
+ uid_t uid = blobfile->uid();
+ const auto& alias = blobfile->alias();
+
+ if (keyBlob.getType() == ::TYPE_KEYMASTER_10) {
+ auto ret = KS_HANDLE_HIDL_ERROR(dev, dev->deleteKey(blob2hidlVec(keyBlob)));
+ // A device doesn't have to implement delete_key.
+ bool success = ret == ErrorCode::OK || ret == ErrorCode::UNIMPLEMENTED;
+ if (__android_log_security()) {
+ android_log_event_list(SEC_TAG_KEY_DESTROYED)
+ << int32_t(success) << alias << int32_t(uid) << LOG_ID_SECURITY;
+ }
+ if (!success) {
+ LOG(ERROR) << "Keymaster delete for key " << alias << " of uid " << uid << " failed";
+ }
+ }
+}
+
std::tuple<KeyStoreServiceReturnCode, Blob>
KeymasterWorker::upgradeKeyBlob(const LockedKeyBlobEntry& lockedEntry,
const AuthorizationSet& params) {
@@ -111,12 +135,6 @@
return;
}
- error = keyStore_->del(lockedEntry);
- if (!error.isOk()) {
- ALOGI("upgradeKeyBlob keystore->del failed %d", error.getErrorCode());
- return;
- }
-
Blob newBlob(&upgradedKeyBlob[0], upgradedKeyBlob.size(), nullptr /* info */,
0 /* infoLength */, ::TYPE_KEYMASTER_10);
newBlob.setSecurityLevel(blob.getSecurityLevel());
@@ -129,6 +147,8 @@
ALOGI("upgradeKeyBlob keystore->put failed %d", error.getErrorCode());
return;
}
+
+ deleteOldKeyOnUpgrade(lockedEntry, std::move(blob));
blob = std::move(newBlob);
};
diff --git a/keystore/keymaster_worker.h b/keystore/keymaster_worker.h
index e1a1c02..2c72c80 100644
--- a/keystore/keymaster_worker.h
+++ b/keystore/keymaster_worker.h
@@ -175,6 +175,8 @@
unwrap_tuple(kmfn, std::move(cb), tuple, std::index_sequence_for<Args...>{});
});
}
+
+ void deleteOldKeyOnUpgrade(const LockedKeyBlobEntry& blobfile, Blob keyBlob);
std::tuple<KeyStoreServiceReturnCode, Blob>
upgradeKeyBlob(const LockedKeyBlobEntry& lockedEntry, const AuthorizationSet& params);
std::tuple<KeyStoreServiceReturnCode, KeyCharacteristics, Blob, Blob>
diff --git a/keystore/keystore_utils.cpp b/keystore/keystore_utils.cpp
index 78056d6..f0f6098 100644
--- a/keystore/keystore_utils.cpp
+++ b/keystore/keystore_utils.cpp
@@ -31,6 +31,9 @@
#include <keystore/keymaster_types.h>
#include <keystore/keystore_client.h>
+#include <android-base/logging.h>
+#include <android-base/unique_fd.h>
+
#include "blob.h"
size_t readFully(int fd, uint8_t* data, size_t size) {
@@ -64,6 +67,44 @@
return size;
}
+std::string getContainingDirectory(const std::string& filename) {
+ std::string containing_dir;
+ size_t last_pos;
+ size_t pos = std::string::npos;
+
+ __builtin_add_overflow(filename.size(), -1, &last_pos);
+
+ // strip all trailing '/'
+ while ((pos = filename.find_last_of('/', last_pos)) == last_pos && pos != 0) {
+ --last_pos;
+ }
+
+ if (pos == 0) {
+ containing_dir = "/";
+ } else if (pos == std::string::npos) {
+ containing_dir = ".";
+ } else {
+ containing_dir = filename.substr(0, pos);
+ }
+
+ return containing_dir;
+}
+
+void fsyncDirectory(const std::string& path) {
+ android::base::unique_fd dir_fd(TEMP_FAILURE_RETRY(open(path.c_str(), O_DIRECTORY | O_RDONLY)));
+
+ if (dir_fd < 0) {
+ LOG(WARNING) << "Could not open dir: " << path << " error: " << strerror(errno);
+ return;
+ }
+
+ if (TEMP_FAILURE_RETRY(fsync(dir_fd)) == -1) {
+ LOG(WARNING) << "Failed to fsync the directory " << path << " error: " << strerror(errno);
+ }
+
+ return;
+}
+
void add_legacy_key_authorizations(int keyType, keystore::AuthorizationSet* params) {
using namespace keystore;
params->push_back(TAG_PURPOSE, KeyPurpose::SIGN);
diff --git a/keystore/keystore_utils.h b/keystore/keystore_utils.h
index 3bc9c01..380eb4e 100644
--- a/keystore/keystore_utils.h
+++ b/keystore/keystore_utils.h
@@ -18,6 +18,7 @@
#define KEYSTORE_KEYSTORE_UTILS_H_
#include <cstdint>
+#include <string>
#include <vector>
#include <openssl/evp.h>
@@ -29,6 +30,8 @@
size_t readFully(int fd, uint8_t* data, size_t size);
size_t writeFully(int fd, uint8_t* data, size_t size);
+std::string getContainingDirectory(const std::string& filename);
+void fsyncDirectory(const std::string& path);
void add_legacy_key_authorizations(int keyType, keystore::AuthorizationSet* params);
