[automerger skipped] Fix keystore wifi concurrency issue. am: 54fcc9954b -s ours am: e7f2c8fb9f -s ours
am: 15d53c8525 -s ours
am skip reason: change_id I8c5602d2c2cb1dd9423df713037e99b247cee71f with SHA1 4ea6d7a447 is in history
Change-Id: I7ba2977ffb0a503c0d133469a75b995901e78782
diff --git a/keystore/blob.cpp b/keystore/blob.cpp
index eac8f11..69cbabb 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 {
@@ -142,12 +143,12 @@
EVP_DecryptUpdate(ctx.get(), out_pos, &out_len, in, len);
out_pos += out_len;
if (!EVP_DecryptFinal_ex(ctx.get(), out_pos, &out_len)) {
- ALOGD("Failed to decrypt blob; ciphertext or tag is likely corrupted");
+ ALOGE("Failed to decrypt blob; ciphertext or tag is likely corrupted");
return ResponseCode::VALUE_CORRUPTED;
}
out_pos += out_len;
if (out_pos - out_tmp.get() != static_cast<ssize_t>(len)) {
- ALOGD("Encrypted plaintext is the wrong size, expected %zu, got %zd", len,
+ ALOGE("Encrypted plaintext is the wrong size, expected %zu, got %zd", len,
out_pos - out_tmp.get());
return ResponseCode::VALUE_CORRUPTED;
}
@@ -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;
}
@@ -401,6 +415,7 @@
}
if (fileLength == 0) {
+ LOG(ERROR) << __func__ << " VALUE_CORRUPTED file length == 0";
return ResponseCode::VALUE_CORRUPTED;
}
@@ -412,7 +427,10 @@
if (state == STATE_UNINITIALIZED) return ResponseCode::UNINITIALIZED;
}
- if (fileLength < offsetof(blobv3, value)) return ResponseCode::VALUE_CORRUPTED;
+ if (fileLength < offsetof(blobv3, value)) {
+ LOG(ERROR) << __func__ << " VALUE_CORRUPTED blob file too short: " << fileLength;
+ return ResponseCode::VALUE_CORRUPTED;
+ }
if (rawBlob->version == 3) {
const ssize_t encryptedLength = ntohl(rawBlob->length);
@@ -428,6 +446,8 @@
(rc == ResponseCode::VALUE_CORRUPTED)) {
return ResponseCode::KEY_PERMANENTLY_INVALIDATED;
}
+ LOG(ERROR) << __func__ << " AES_gcm_decrypt returned: " << uint32_t(rc);
+
return rc;
}
}
@@ -435,10 +455,16 @@
blobv2& v2blob = reinterpret_cast<blobv2&>(*rawBlob);
const size_t headerLength = offsetof(blobv2, encrypted);
const ssize_t encryptedLength = fileLength - headerLength - v2blob.info;
- if (encryptedLength < 0) return ResponseCode::VALUE_CORRUPTED;
+ if (encryptedLength < 0) {
+ LOG(ERROR) << __func__ << " VALUE_CORRUPTED v2blob file too short";
+ return ResponseCode::VALUE_CORRUPTED;
+ }
if (rawBlobIsEncrypted(*rawBlob)) {
if (encryptedLength % AES_BLOCK_SIZE != 0) {
+ LOG(ERROR) << __func__
+ << " VALUE_CORRUPTED encrypted length is not a multiple"
+ " of the AES block size";
return ResponseCode::VALUE_CORRUPTED;
}
@@ -452,6 +478,7 @@
ssize_t digestedLength = encryptedLength - MD5_DIGEST_LENGTH;
MD5(v2blob.digested, digestedLength, computedDigest);
if (memcmp(v2blob.digest, computedDigest, MD5_DIGEST_LENGTH) != 0) {
+ LOG(ERROR) << __func__ << " v2blob MD5 digest mismatch";
return ResponseCode::VALUE_CORRUPTED;
}
}
@@ -462,6 +489,7 @@
if (rawBlob->length < 0 || rawBlob->length > maxValueLength ||
rawBlob->length + rawBlob->info + AES_BLOCK_SIZE >
static_cast<ssize_t>(sizeof(rawBlob->value))) {
+ LOG(ERROR) << __func__ << " raw blob length is out of bounds";
return ResponseCode::VALUE_CORRUPTED;
}
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);