[DO NOT MERGE] Handle edge cases where input or hash/data could be null.
Bug: b/117993149
Test: Manual; atest net_test_btif net_test_bluetooth
Change-Id: Ia91ea822ee2147b2a2d14bea250a708e8c10bae4
Merged-In: Ia91ea822ee2147b2a2d14bea250a708e8c10bae4
diff --git a/system/btif/Android.bp b/system/btif/Android.bp
index 257ef7d..a89679d 100644
--- a/system/btif/Android.bp
+++ b/system/btif/Android.bp
@@ -127,7 +127,10 @@
name: "net_test_btif",
defaults: ["fluoride_defaults"],
include_dirs: btifCommonIncludes,
- srcs: ["test/btif_storage_test.cc"],
+ srcs: [
+ "test/btif_storage_test.cc",
+ "test/btif_keystore_test.cc"
+ ],
header_libs: ["libbluetooth_headers"],
shared_libs: [
"libaudioclient",
@@ -144,6 +147,7 @@
"libkeystore_aidl",
"libkeystore_binder",
"libkeystore_parcelables",
+ "libbinder",
],
static_libs: [
"libbt-bta",
@@ -165,6 +169,9 @@
"libbluetooth-for-tests",
],
cflags: ["-DBUILDCFG"],
+ sanitize: {
+ integer_overflow: true,
+ },
}
// btif profile queue unit tests for target
diff --git a/system/btif/include/btif_keystore.h b/system/btif/include/btif_keystore.h
index 1e5c2ea..01ccd66 100644
--- a/system/btif/include/btif_keystore.h
+++ b/system/btif/include/btif_keystore.h
@@ -18,6 +18,7 @@
#include <base/logging.h>
#include <keystore/keystore_client_impl.h>
+#include <mutex>
#include "osi/include/alarm.h"
#include "osi/include/allocator.h"
@@ -27,13 +28,45 @@
#include "osi/include/osi.h"
#include "osi/include/properties.h"
-using namespace keystore;
-
+namespace bluetooth {
+/**
+ * Client wrapper to access AndroidKeystore.
+ *
+ * <p>Use to encrypt/decrypt data and store to disk.
+ */
class BtifKeystore {
public:
- BtifKeystore();
- ~BtifKeystore();
- int Encrypt(const std::string& hash, const std::string& output_filename,
- int32_t flags);
+ /**
+ * @param keystore_client injected pre-created client object for keystore
+ */
+ BtifKeystore(keystore::KeystoreClient* keystore_client);
+
+ /**
+ * Stores encrypted data to disk.
+ *
+ * <p>Returns true on success.
+ *
+ * @param data to be encrypted
+ * @param output_filename location to write the file
+ * @param flags
+ */
+ bool Encrypt(const std::string& data, const std::string& output_filename,
+ int32_t flags);
+
+ /**
+ * Returns a decrypted string representation of the encrypted data or empty
+ * string on error.
+ *
+ * @param input_filename location of file to read and decrypt
+ */
std::string Decrypt(const std::string& input_filename);
+
+ private:
+ std::unique_ptr<keystore::KeystoreClient> keystore_client_;
+ std::mutex api_mutex_;
+ keystore::KeyStoreNativeReturnCode GenerateKey(const std::string& name,
+ int32_t flags,
+ bool auth_bound);
};
+
+} // namespace bluetooth
diff --git a/system/btif/src/btif_config.cc b/system/btif/src/btif_config.cc
index d9c25a6..9145db4 100644
--- a/system/btif/src/btif_config.cc
+++ b/system/btif/src/btif_config.cc
@@ -56,6 +56,10 @@
#define TIME_STRING_LENGTH sizeof("YYYY-MM-DD HH:MM:SS")
static const char* TIME_STRING_FORMAT = "%Y-%m-%d %H:%M:%S";
+constexpr int kBufferSize = 400 * 10; // initial file is ~400B
+
+using bluetooth::BtifKeystore;
+
// TODO(armansito): Find a better way than searching by a hardcoded path.
#if defined(OS_GENERIC)
static const char* CONFIG_FILE_PATH = "bt_config.conf";
@@ -83,8 +87,6 @@
static std::string hash_file(const char* filename);
static std::string read_checksum_file(const char* filename);
static void write_checksum_file(const char* filename, const std::string& hash);
-static bool verify_hash(const std::string& current_hash,
- const std::string& stored_hash);
static enum ConfigSource {
NOT_LOADED,
@@ -130,7 +132,8 @@
static std::mutex config_lock; // protects operations on |config|.
static std::unique_ptr<config_t> config;
static alarm_t* config_timer;
-static BtifKeystore btifKeystore;
+
+static BtifKeystore btif_keystore(new keystore::KeystoreClientImpl);
// Module lifecycle functions
@@ -226,7 +229,7 @@
}
}
// Compare hashes
- if (!verify_hash(current_hash, stored_hash)) {
+ if (current_hash != stored_hash) {
return nullptr;
}
// END KEY ATTESTATION
@@ -582,18 +585,13 @@
<< "': " << strerror(errno);
return "";
}
- unsigned char hash[SHA256_DIGEST_LENGTH];
+ uint8_t hash[SHA256_DIGEST_LENGTH];
SHA256_CTX sha256;
SHA256_Init(&sha256);
- const int bufSize = 400 * 10; // initial file is ~400B
- std::byte* buffer = (std::byte*) osi_calloc(bufSize);
- int bytesRead = 0;
- if (!buffer) {
- fclose(fp);
- return "";
- }
- while ((bytesRead = fread(buffer, 1, bufSize, fp))) {
- SHA256_Update(&sha256, buffer, bytesRead);
+ std::array<unsigned char, kBufferSize> buffer;
+ int bytes_read = 0;
+ while ((bytes_read = fread(buffer.data(), 1, buffer.size(), fp))) {
+ SHA256_Update(&sha256, buffer.data(), bytes_read);
}
SHA256_Final(hash, &sha256);
std::stringstream ss;
@@ -601,7 +599,6 @@
ss << std::hex << std::setw(2) << std::setfill('0') << (int)hash[i];
}
fclose(fp);
- osi_free(buffer);
return ss.str();
}
@@ -610,19 +607,11 @@
if (access(checksum_filename, R_OK) != 0) {
return "";
}
- std::string output = btifKeystore.Decrypt(checksum_filename);
- return output;
+ return btif_keystore.Decrypt(checksum_filename);
}
static void write_checksum_file(const char* checksum_filename,
const std::string& hash) {
- int result = btifKeystore.Encrypt(hash, checksum_filename, 0);
- if (result != 0) {
- LOG(ERROR) << "Failed writing checksum!";
- }
-}
-
-static bool verify_hash(const std::string& current_hash,
- const std::string& stored_hash) {
- return current_hash.compare(stored_hash) == 0;
+ bool result = btif_keystore.Encrypt(hash, checksum_filename, 0);
+ CHECK(result) << "Failed writing checksum";
}
diff --git a/system/btif/src/btif_keystore.cc b/system/btif/src/btif_keystore.cc
index 0805d21..ac3a803 100644
--- a/system/btif/src/btif_keystore.cc
+++ b/system/btif/src/btif_keystore.cc
@@ -16,10 +16,7 @@
*
******************************************************************************/
-#define LOG_TAG "bt_btif_keystore"
-
#include "btif_keystore.h"
-#include "osi/include/properties.h"
#include <base/files/file_util.h>
#include <base/logging.h>
@@ -30,83 +27,92 @@
#include <sys/stat.h>
using namespace keystore;
+using namespace bluetooth;
-static std::unique_ptr<keystore::KeystoreClient> CreateKeystoreInstance(void);
-static void WriteFile(const std::string& filename, const std::string& content);
-static std::string ReadFile(const std::string& filename);
-static int GenerateKey(const std::string& name, int32_t flags, bool auth_bound);
-static bool DoesKeyExist(const std::string& name);
-
-const std::string FILE_SUFFIX = ".encrypted-checksum";
-const std::string CIPHER_ALGORITHM = "AES/GCM/NoPadding";
-const std::string DIGEST_ALGORITHM = "SHA-256";
-const std::string KEY_STORE = "AndroidKeystore";
-
-std::unique_ptr<KeystoreClient> keystoreClient;
-
-BtifKeystore::BtifKeystore() { keystoreClient = CreateKeystoreInstance(); }
-
-BtifKeystore::~BtifKeystore() {
- // Using a smart pointer, does it delete itself?
- // delete keystoreClient;
-}
-
-int BtifKeystore::Encrypt(const std::string& hash,
- const std::string& output_filename, int32_t flags) {
- std::string output;
- if (!DoesKeyExist(KEY_STORE)) {
- GenerateKey(KEY_STORE, 0, false);
- }
- char is_unittest[PROPERTY_VALUE_MAX] = {0};
- osi_property_get("debug.bluetooth.unittest", is_unittest, "false");
- if (strcmp(is_unittest, "false") == 0) {
- if (!keystoreClient->encryptWithAuthentication(KEY_STORE, hash, flags,
- &output)) {
- LOG(ERROR) << "EncryptWithAuthentication failed.\n";
- return 1;
- }
- }
- WriteFile(output_filename, output);
- return 0;
-}
-
-std::string BtifKeystore::Decrypt(const std::string& input_filename) {
- std::string input = ReadFile(input_filename);
- std::string output;
-
- char is_unittest[PROPERTY_VALUE_MAX] = {0};
- osi_property_get("debug.bluetooth.unittest", is_unittest, "false");
- if (strcmp(is_unittest, "false") == 0) {
- if (!keystoreClient->decryptWithAuthentication(KEY_STORE, input, &output)) {
- LOG(ERROR) << "DecryptWithAuthentication failed.\n";
- }
- }
- return output;
-}
+constexpr char kKeyStore[] = "AndroidKeystore";
static std::string ReadFile(const std::string& filename) {
+ CHECK(!filename.empty()) << __func__ << ": filename should not be empty";
+
std::string content;
- struct stat buffer;
- if (stat(filename.c_str(), &buffer) == 0) {
- base::FilePath path(filename);
- if (!base::ReadFileToString(path, &content)) {
- LOG(ERROR) << "ReadFile failed.\n" << filename.c_str();
- }
+ base::FilePath path(filename);
+ if (!base::PathExists(path)) {
+ // Config file checksum file doesn't exist on first run after OTA.
+ LOG(ERROR) << "file '" << filename.c_str() << "'doesn't exists yet";
+ }
+ if (!base::ReadFileToString(path, &content)) {
+ LOG(ERROR) << "ReadFile failed: " << filename.c_str();
}
return content;
}
static void WriteFile(const std::string& filename, const std::string& content) {
+ CHECK(!filename.empty()) << __func__ << ": filename should not be empty";
+ CHECK(!content.empty()) << __func__ << ": content should not be empty";
+
base::FilePath path(filename);
int size = content.size();
if (base::WriteFile(path, content.data(), size) != size) {
- LOG(ERROR) << "WriteFile failed.\n" << filename.c_str();
+ LOG(FATAL) << "WriteFile failed.\n" << filename.c_str();
}
}
+namespace bluetooth {
+
+BtifKeystore::BtifKeystore(keystore::KeystoreClient* keystore_client)
+ : keystore_client_(keystore_client) {}
+
+bool BtifKeystore::Encrypt(const std::string& data,
+ const std::string& output_filename, int32_t flags) {
+ std::lock_guard<std::mutex> lock(api_mutex_);
+ if (data.empty()) {
+ LOG(ERROR) << __func__ << ": empty data";
+ return false;
+ }
+ if (output_filename.empty()) {
+ LOG(ERROR) << __func__ << ": empty output filename";
+ return false;
+ }
+ std::string output;
+ if (!keystore_client_->doesKeyExist(kKeyStore)) {
+ auto gen_result = GenerateKey(kKeyStore, 0, false);
+ if (!gen_result.isOk()) {
+ LOG(FATAL) << "EncryptWithAuthentication Failed: generateKey response="
+ << gen_result;
+ return false;
+ }
+ }
+ if (!keystore_client_->encryptWithAuthentication(kKeyStore, data, flags,
+ &output)) {
+ LOG(FATAL) << "EncryptWithAuthentication failed.";
+ return false;
+ }
+ WriteFile(output_filename, output);
+ return true;
+}
+
+std::string BtifKeystore::Decrypt(const std::string& input_filename) {
+ std::lock_guard<std::mutex> lock(api_mutex_);
+ std::string output;
+ if (input_filename.empty()) {
+ LOG(ERROR) << __func__ << ": empty input filename";
+ return output;
+ }
+ std::string input = ReadFile(input_filename);
+ if (input.empty()) {
+ LOG(ERROR) << __func__ << ": empty input data";
+ return output;
+ }
+ if (!keystore_client_->decryptWithAuthentication(kKeyStore, input, &output)) {
+ LOG(FATAL) << "DecryptWithAuthentication failed.\n";
+ }
+ return output;
+}
+
// Note: auth_bound keys created with this tool will not be usable.
-static int GenerateKey(const std::string& name, int32_t flags,
- bool auth_bound) {
+KeyStoreNativeReturnCode BtifKeystore::GenerateKey(const std::string& name,
+ int32_t flags,
+ bool auth_bound) {
AuthorizationSetBuilder params;
params.RsaSigningKey(2048, 65537)
.Digest(Digest::SHA_2_224)
@@ -124,23 +130,9 @@
}
AuthorizationSet hardware_enforced_characteristics;
AuthorizationSet software_enforced_characteristics;
-
- char is_unittest[PROPERTY_VALUE_MAX] = {0};
- osi_property_get("debug.bluetooth.unittest", is_unittest, "false");
- if (strcmp(is_unittest, "false") != 0) {
- return -1;
- }
- auto result = keystoreClient->generateKey(name, params, flags,
- &hardware_enforced_characteristics,
- &software_enforced_characteristics);
- return result.getErrorCode();
+ return keystore_client_->generateKey(name, params, flags,
+ &hardware_enforced_characteristics,
+ &software_enforced_characteristics);
}
-static bool DoesKeyExist(const std::string& name) {
- return keystoreClient->doesKeyExist(name) ? true : false;
-}
-
-static std::unique_ptr<KeystoreClient> CreateKeystoreInstance(void) {
- return std::unique_ptr<KeystoreClient>(
- static_cast<KeystoreClient*>(new KeystoreClientImpl));
-}
+} // namespace bluetooth
diff --git a/system/btif/test/btif_keystore_test.cc b/system/btif/test/btif_keystore_test.cc
new file mode 100644
index 0000000..e57c64f
--- /dev/null
+++ b/system/btif/test/btif_keystore_test.cc
@@ -0,0 +1,98 @@
+/******************************************************************************
+ *
+ * Copyright 2019 Google, Inc.
+ *
+ * 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.
+ *
+ ******************************************************************************/
+
+#include <base/files/file_util.h>
+#include <base/logging.h>
+#include <binder/ProcessState.h>
+#include <gtest/gtest.h>
+#include <fstream>
+
+#include "btif/include/btif_keystore.h"
+
+using namespace bluetooth;
+
+constexpr char kFilename[] = "/data/misc/bluedroid/testfile.txt";
+
+class BtifKeystoreTest : public ::testing::Test {
+ protected:
+ std::unique_ptr<BtifKeystore> btif_keystore_;
+ base::FilePath file_path_;
+ BtifKeystoreTest() : file_path_(kFilename) {}
+ void SetUp() override {
+ android::ProcessState::self()->startThreadPool();
+ btif_keystore_ =
+ std::make_unique<BtifKeystore>(static_cast<keystore::KeystoreClient*>(
+ new keystore::KeystoreClientImpl));
+ base::DeleteFile(file_path_, true);
+ };
+ void TearDown() override { btif_keystore_ = nullptr; };
+};
+
+// Encrypt
+TEST_F(BtifKeystoreTest, test_encrypt_decrypt) {
+ std::string hash = "test";
+
+ EXPECT_TRUE(btif_keystore_->Encrypt(hash, kFilename, 0));
+ std::string decrypted_hash = btif_keystore_->Decrypt(kFilename);
+
+ EXPECT_TRUE(base::PathExists(file_path_));
+ EXPECT_EQ(hash, decrypted_hash);
+}
+
+TEST_F(BtifKeystoreTest, test_encrypt_empty_hash) {
+ std::string hash = "";
+
+ EXPECT_FALSE(btif_keystore_->Encrypt(hash, kFilename, 0));
+
+ EXPECT_FALSE(base::PathExists(file_path_));
+}
+
+TEST_F(BtifKeystoreTest, test_encrypt_empty_filename) {
+ std::string hash = "test";
+
+ EXPECT_FALSE(btif_keystore_->Encrypt(hash, "", 0));
+
+ EXPECT_FALSE(base::PathExists(file_path_));
+}
+
+// Decrypt
+TEST_F(BtifKeystoreTest, test_decrypt_empty_hash) {
+ // Only way to get the hash to decrypt is to read it from the file
+ // So make empty file manually
+ std::ofstream outfile(kFilename);
+ outfile.close();
+
+ std::string decrypted_hash = btif_keystore_->Decrypt(kFilename);
+
+ EXPECT_TRUE(decrypted_hash.empty());
+}
+
+TEST_F(BtifKeystoreTest, test_decrypt_file_not_exist) {
+ // Ensure file doesn't exist, then decrypt
+ EXPECT_FALSE(base::PathExists(file_path_));
+
+ std::string decrypted_hash = btif_keystore_->Decrypt(kFilename);
+
+ EXPECT_TRUE(decrypted_hash.empty());
+}
+
+TEST_F(BtifKeystoreTest, test_decrypt_empty_filename) {
+ std::string decrypted_hash = btif_keystore_->Decrypt("");
+
+ EXPECT_TRUE(decrypted_hash.empty());
+}
diff --git a/system/test/suite/Android.bp b/system/test/suite/Android.bp
index 5904cd5..38b4ebd 100644
--- a/system/test/suite/Android.bp
+++ b/system/test/suite/Android.bp
@@ -15,6 +15,8 @@
shared_libs: [
"liblog",
"libcutils",
+ "libbinder",
+ "libutils",
],
static_libs: [
"libbtcore",
@@ -40,6 +42,8 @@
shared_libs: [
"liblog",
"libcutils",
+ "libbinder",
+ "libutils",
],
static_libs: [
"libbtcore",
diff --git a/system/test/suite/adapter/bluetooth_test.cc b/system/test/suite/adapter/bluetooth_test.cc
index 012592f..fdf4a9d 100644
--- a/system/test/suite/adapter/bluetooth_test.cc
+++ b/system/test/suite/adapter/bluetooth_test.cc
@@ -17,6 +17,8 @@
******************************************************************************/
#include "adapter/bluetooth_test.h"
+#include <binder/ProcessState.h>
+#include <stdio.h>
#include <mutex>
#include "btcore/include/property.h"
#include "osi/include/properties.h"
@@ -32,6 +34,7 @@
namespace bttest {
void BluetoothTest::SetUp() {
+ android::ProcessState::self()->startThreadPool();
bt_interface_ = nullptr;
state_ = BT_STATE_OFF;
properties_changed_count_ = 0;
@@ -47,7 +50,8 @@
adapter_state_changed_callback_sem_ = semaphore_new(0);
discovery_state_changed_callback_sem_ = semaphore_new(0);
- osi_property_set("debug.bluetooth.unittest", "true");
+ remove("/data/misc/bluedroid/bt_config.conf.encrypted-checksum");
+ remove("/data/misc/bluedroid/bt_config.bak.encrypted-checksum");
bluetooth::hal::BluetoothInterface::Initialize();
ASSERT_TRUE(bluetooth::hal::BluetoothInterface::IsInitialized());
