OSDN Git Service

[DO NOT MERGE] btif/osi: move I/O to OSI layer. disable for multi-user.
authorMartin Brabham <optedoblivion@google.com>
Mon, 25 Feb 2019 22:40:33 +0000 (14:40 -0800)
committerMartin Brabham <optedoblivion@google.com>
Mon, 11 Mar 2019 21:33:52 +0000 (21:33 +0000)
Two issues here.
    One, read/write/modify/remove access to the checksum by the secondary user.
    Two, Fail to access keystore with secondary user stack running (stack doesn't run as UID 1002)

Bug: 117993149
Test: atest net_test_btif net_test_bluetooth net_test_osi
Change-Id: I7af452e00a4f342f1c49006e86488b59195b70ce
Merged-In: I7af452e00a4f342f1c49006e86488b59195b70ce

btif/include/btif_keystore.h
btif/src/btif_config.cc
btif/src/btif_keystore.cc
btif/test/btif_keystore_test.cc
osi/include/config.h
osi/src/config.cc
osi/test/config_test.cc

index 01ccd66..4762350 100644 (file)
@@ -42,22 +42,20 @@ class BtifKeystore {
   BtifKeystore(keystore::KeystoreClient* keystore_client);
 
   /**
-   * Stores encrypted data to disk.
+   * Encrypts given data
    *
-   * <p>Returns true on success.
+   * <p>Returns a string representation of the encrypted data
    *
    * @param data to be encrypted
-   * @param output_filename location to write the file
-   * @param flags
+   * @param flags for keystore
    */
-  bool Encrypt(const std::string& data, const std::string& output_filename,
-               int32_t flags);
+  std::string Encrypt(const std::string& data, 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
+   * @param input encrypted data
    */
   std::string Decrypt(const std::string& input_filename);
 
index 9145db4..7c361ae 100644 (file)
 #include <ctype.h>
 #include <openssl/rand.h>
 #include <openssl/sha.h>
+#include <private/android_filesystem_config.h>
 #include <stdio.h>
 #include <string.h>
 #include <time.h>
 #include <unistd.h>
+#include <mutex>
 #include <sstream>
 #include <string>
 
-#include <mutex>
-
 #include "bt_types.h"
 #include "btcore/include/module.h"
 #include "btif_api.h"
 #define FILE_TIMESTAMP "TimeCreated"
 #define FILE_SOURCE "FileSource"
 #define TIME_STRING_LENGTH sizeof("YYYY-MM-DD HH:MM:SS")
+#define DISABLED "disabled"
 static const char* TIME_STRING_FORMAT = "%Y-%m-%d %H:%M:%S";
 
 constexpr int kBufferSize = 400 * 10;  // initial file is ~400B
 
+static bool use_key_attestation() { return getuid() == AID_BLUETOOTH; }
+
 using bluetooth::BtifKeystore;
 
 // TODO(armansito): Find a better way than searching by a hardcoded path.
@@ -579,6 +582,10 @@ static void delete_config_files(void) {
 }
 
 static std::string hash_file(const char* filename) {
+  if (!use_key_attestation()) {
+    LOG(INFO) << __func__ << ": Disabled for multi-user";
+    return DISABLED;
+  }
   FILE* fp = fopen(filename, "rb");
   if (!fp) {
     LOG(ERROR) << __func__ << ": unable to open config file: '" << filename
@@ -603,15 +610,31 @@ static std::string hash_file(const char* filename) {
 }
 
 static std::string read_checksum_file(const char* checksum_filename) {
-  // Ensure file exists
-  if (access(checksum_filename, R_OK) != 0) {
+  if (!use_key_attestation()) {
+    LOG(INFO) << __func__ << ": Disabled for multi-user";
+    return DISABLED;
+  }
+  std::string encrypted_hash = checksum_read(checksum_filename);
+  if (encrypted_hash.empty()) {
+    LOG(INFO) << __func__ << ": read empty hash.";
     return "";
   }
-  return btif_keystore.Decrypt(checksum_filename);
+  return btif_keystore.Decrypt(encrypted_hash);
 }
 
 static void write_checksum_file(const char* checksum_filename,
                                 const std::string& hash) {
-  bool result = btif_keystore.Encrypt(hash, checksum_filename, 0);
-  CHECK(result) << "Failed writing checksum";
+  if (!use_key_attestation()) {
+    LOG(INFO) << __func__
+              << ": Disabled for multi-user, since config changed removing "
+                 "checksums.";
+    remove(CONFIG_FILE_CHECKSUM_PATH);
+    remove(CONFIG_BACKUP_CHECKSUM_PATH);
+    return;
+  }
+  std::string encrypted_checksum = btif_keystore.Encrypt(hash, 0);
+  CHECK(!encrypted_checksum.empty())
+      << __func__ << ": Failed encrypting checksum";
+  CHECK(checksum_save(encrypted_checksum, checksum_filename))
+      << __func__ << ": Failed to save checksum!";
 }
index ac3a803..fe9d3dd 100644 (file)
@@ -31,78 +31,41 @@ using namespace bluetooth;
 
 constexpr char kKeyStore[] = "AndroidKeystore";
 
-static std::string ReadFile(const std::string& filename) {
-  CHECK(!filename.empty()) << __func__ << ": filename should not be empty";
-
-  std::string content;
-  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(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::string BtifKeystore::Encrypt(const std::string& data, int32_t flags) {
   std::lock_guard<std::mutex> lock(api_mutex_);
+  std::string output;
   if (data.empty()) {
     LOG(ERROR) << __func__ << ": empty data";
-    return false;
-  }
-  if (output_filename.empty()) {
-    LOG(ERROR) << __func__ << ": empty output filename";
-    return false;
+    return output;
   }
-  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;
+      return output;
     }
   }
   if (!keystore_client_->encryptWithAuthentication(kKeyStore, data, flags,
                                                    &output)) {
     LOG(FATAL) << "EncryptWithAuthentication failed.";
-    return false;
+    return output;
   }
-  WriteFile(output_filename, output);
-  return true;
+  return output;
 }
 
-std::string BtifKeystore::Decrypt(const std::string& input_filename) {
+std::string BtifKeystore::Decrypt(const std::string& input) {
   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;
+    return "";
   }
+  std::string output;
   if (!keystore_client_->decryptWithAuthentication(kKeyStore, input, &output)) {
     LOG(FATAL) << "DecryptWithAuthentication failed.\n";
   }
index e57c64f..4cabbad 100644 (file)
@@ -16,7 +16,6 @@
  *
  ******************************************************************************/
 
-#include <base/files/file_util.h>
 #include <base/logging.h>
 #include <binder/ProcessState.h>
 #include <gtest/gtest.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);
+  std::string encrypted_hash = btif_keystore_->Encrypt(hash, 0);
+  std::string decrypted_hash = btif_keystore_->Decrypt(encrypted_hash);
 
-  EXPECT_TRUE(base::PathExists(file_path_));
+  EXPECT_FALSE(encrypted_hash.empty());
   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));
+  std::string encrypted_hash = btif_keystore_->Encrypt(hash, 0);
 
-  EXPECT_FALSE(base::PathExists(file_path_));
+  EXPECT_TRUE(encrypted_hash.empty());
 }
 
-// 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());
-}
+  std::string hash = "";
 
-TEST_F(BtifKeystoreTest, test_decrypt_empty_filename) {
-  std::string decrypted_hash = btif_keystore_->Decrypt("");
+  std::string decrypted_hash = btif_keystore_->Decrypt(hash);
 
   EXPECT_TRUE(decrypted_hash.empty());
 }
index 55adecb..47c3a3e 100644 (file)
@@ -49,6 +49,9 @@ std::unique_ptr<config_t> config_new_empty(void);
 // file on the filesystem.
 std::unique_ptr<config_t> config_new(const char* filename);
 
+// Read the checksum from the |filename|
+std::string checksum_read(const char* filename);
+
 // Clones |src|, including all of it's sections, keys, and values.
 // Returns a new config which is a copy and separated from the original;
 // changes to the new config are not reflected in any way in the original.
@@ -133,3 +136,8 @@ bool config_remove_key(config_t* config, const std::string& section,
 // |config_save|, all comments and special formatting in the original file will
 // be lost. Neither |config| nor |filename| may be NULL.
 bool config_save(const config_t& config, const std::string& filename);
+
+// Saves the encrypted |checksum| of config file to a given |filename| Note
+// that this could be a destructive operation: if |filename| already exists,
+// it will be overwritten.
+bool checksum_save(const std::string& checksum, const std::string& filename);
index d4ed2e6..ac9e3cf 100644 (file)
@@ -19,7 +19,7 @@
 #include "osi/include/config.h"
 #include "log/log.h"
 
-#include <base/files/file_path.h>
+#include <base/files/file_util.h>
 #include <base/logging.h>
 #include <ctype.h>
 #include <errno.h>
@@ -84,6 +84,19 @@ std::unique_ptr<config_t> config_new(const char* filename) {
   return config;
 }
 
+std::string checksum_read(const char* filename) {
+  base::FilePath path(filename);
+  if (!base::PathExists(path)) {
+    LOG(ERROR) << __func__ << ": unable to locate file '" << filename << "'";
+    return "";
+  }
+  std::string encrypted_hash;
+  if (!base::ReadFileToString(path, &encrypted_hash)) {
+    LOG(ERROR) << __func__ << ": unable to read file '" << filename << "'";
+  }
+  return encrypted_hash;
+}
+
 std::unique_ptr<config_t> config_new_clone(const config_t& src) {
   std::unique_ptr<config_t> ret = config_new_empty();
 
@@ -324,6 +337,107 @@ error:
   return false;
 }
 
+bool checksum_save(const std::string& checksum, const std::string& filename) {
+  CHECK(!checksum.empty()) << __func__ << ": checksum cannot be empty";
+  CHECK(!filename.empty()) << __func__ << ": filename cannot be empty";
+
+  // Steps to ensure content of config checksum file gets to disk:
+  //
+  // 1) Open and write to temp file (e.g.
+  // bt_config.conf.encrypted-checksum.new). 2) Sync the temp file to disk with
+  // fsync(). 3) Rename temp file to actual config checksum file (e.g.
+  // bt_config.conf.encrypted-checksum).
+  //    This ensures atomic update.
+  // 4) Sync directory that has the conf file with fsync().
+  //    This ensures directory entries are up-to-date.
+  FILE* fp = nullptr;
+  int dir_fd = -1;
+
+  // Build temp config checksum file based on config checksum file (e.g.
+  // bt_config.conf.encrypted-checksum.new).
+  const std::string temp_filename = filename + ".new";
+  base::FilePath path(temp_filename);
+
+  // Extract directory from file path (e.g. /data/misc/bluedroid).
+  const std::string directoryname = base::FilePath(filename).DirName().value();
+  if (directoryname.empty()) {
+    LOG(ERROR) << __func__ << ": error extracting directory from '" << filename
+               << "': " << strerror(errno);
+    goto error2;
+  }
+
+  dir_fd = open(directoryname.c_str(), O_RDONLY);
+  if (dir_fd < 0) {
+    LOG(ERROR) << __func__ << ": unable to open dir '" << directoryname
+               << "': " << strerror(errno);
+    goto error2;
+  }
+
+  if (base::WriteFile(path, checksum.data(), checksum.size()) !=
+      (int)checksum.size()) {
+    LOG(ERROR) << __func__ << ": unable to write file '" << filename.c_str();
+    goto error2;
+  }
+
+  fp = fopen(temp_filename.c_str(), "rb");
+  if (!fp) {
+    LOG(ERROR) << __func__ << ": unable to write to file '" << temp_filename
+               << "': " << strerror(errno);
+    goto error2;
+  }
+
+  // Sync written temp file out to disk. fsync() is blocking until data makes it
+  // to disk.
+  if (fsync(fileno(fp)) < 0) {
+    LOG(WARNING) << __func__ << ": unable to fsync file '" << temp_filename
+                 << "': " << strerror(errno);
+  }
+
+  if (fclose(fp) == EOF) {
+    LOG(ERROR) << __func__ << ": unable to close file '" << temp_filename
+               << "': " << strerror(errno);
+    goto error2;
+  }
+  fp = nullptr;
+
+  // Change the file's permissions to Read/Write by User and Group
+  if (chmod(temp_filename.c_str(), S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP) ==
+      -1) {
+    LOG(ERROR) << __func__ << ": unable to change file permissions '"
+               << filename << "': " << strerror(errno);
+    goto error2;
+  }
+
+  // Rename written temp file to the actual config file.
+  if (rename(temp_filename.c_str(), filename.c_str()) == -1) {
+    LOG(ERROR) << __func__ << ": unable to commit file '" << filename
+               << "': " << strerror(errno);
+    goto error2;
+  }
+
+  // This should ensure the directory is updated as well.
+  if (fsync(dir_fd) < 0) {
+    LOG(WARNING) << __func__ << ": unable to fsync dir '" << directoryname
+                 << "': " << strerror(errno);
+  }
+
+  if (close(dir_fd) < 0) {
+    LOG(ERROR) << __func__ << ": unable to close dir '" << directoryname
+               << "': " << strerror(errno);
+    goto error2;
+  }
+
+  return true;
+
+error2:
+  // This indicates there is a write issue.  Unlink as partial data is not
+  // acceptable.
+  unlink(temp_filename.c_str());
+  if (fp) fclose(fp);
+  if (dir_fd != -1) close(dir_fd);
+  return false;
+}
+
 static char* trim(char* str) {
   while (isspace(*str)) ++str;
 
index 58ce818..7cf4db8 100644 (file)
@@ -1,3 +1,4 @@
+#include <base/files/file_util.h>
 #include <gtest/gtest.h>
 
 #include "AllocationTestHarness.h"
@@ -173,3 +174,24 @@ TEST_F(ConfigTest, config_save_basic) {
   std::unique_ptr<config_t> config = config_new(CONFIG_FILE);
   EXPECT_TRUE(config_save(*config, CONFIG_FILE));
 }
+
+TEST_F(ConfigTest, checksum_read) {
+  std::string filename = "/data/misc/bluedroid/test.checksum";
+  std::string checksum = "0x1234";
+  base::FilePath file_path(filename);
+
+  EXPECT_EQ(base::WriteFile(file_path, checksum.data(), checksum.size()),
+            (int)checksum.size());
+
+  EXPECT_EQ(checksum_read(filename.c_str()), checksum.c_str());
+}
+
+TEST_F(ConfigTest, checksum_save) {
+  std::string filename = "/data/misc/bluedroid/test.checksum";
+  std::string checksum = "0x1234";
+  base::FilePath file_path(filename);
+
+  EXPECT_TRUE(checksum_save(checksum, filename));
+
+  EXPECT_TRUE(base::PathExists(file_path));
+}