Actually check downloaded hash during verification (WIP).

checkTargetFile() just checks that the stored metadata is consistent. We
need to go further and actually recheck the hash of the downloaded data.

Still working on the tests.

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

src/libaktualizr/package_manager/ostreemanager.cc

@@ -239,7 +239,7 @@ bool OstreeManager::fetchTarget(const Uptane::Target &target, Uptane::Fetcher &f
   return OstreeManager::pull(config.sysroot, config.ostree_server, keys, target, token, progress_cb).success;
 }
 
-bool OstreeManager::verifyTarget(const Uptane::Target &target) const {
+TargetStatus OstreeManager::verifyTarget(const Uptane::Target &target) const {
   const std::string refhash = target.sha256Hash();
   GError *error = nullptr;
 
@@ -248,7 +248,7 @@ bool OstreeManager::verifyTarget(const Uptane::Target &target) const {
   if (error != nullptr) {
     LOG_ERROR << "Could not get OSTree repo";
     g_error_free(error);
-    return false;
+    return TargetStatus::kNotFound;
   }
 
   GHashTable *ref_list = nullptr;
@@ -257,7 +257,7 @@ bool OstreeManager::verifyTarget(const Uptane::Target &target) const {
     g_hash_table_destroy(ref_list);  // OSTree creates the table with destroy notifiers, so no memory leaks expected
     // should never be greater than 1, but use >= for robustness
     if (length >= 1) {
-      return true;
+      return TargetStatus::kGood;
     }
   }
   if (error != nullptr) {
@@ -266,7 +266,7 @@ bool OstreeManager::verifyTarget(const Uptane::Target &target) const {
   }
 
   LOG_ERROR << "Could not find OSTree commit";
-  return false;
+  return TargetStatus::kNotFound;
 }
 
 Json::Value OstreeManager::getInstalledPackages() const {

src/libaktualizr/package_manager/ostreemanager.h

@@ -51,7 +51,7 @@ class OstreeManager : public PackageManagerInterface {
   data::InstallationResult finalizeInstall(const Uptane::Target &target) const override;
   bool fetchTarget(const Uptane::Target &target, Uptane::Fetcher &fetcher, const KeyManager &keys,
                    FetcherProgressCb progress_cb, const api::FlowControlToken *token) override;
-  bool verifyTarget(const Uptane::Target &target) const override;
+  TargetStatus verifyTarget(const Uptane::Target &target) const override;
 
   GObjectUniquePtr<OstreeDeployment> getStagedDeployment() const;
   static GObjectUniquePtr<OstreeSysroot> LoadSysroot(const boost::filesystem::path &path);

src/libaktualizr/package_manager/packagemanagerfake_test.cc

@@ -17,6 +17,8 @@
 
 /*
  * Verify a stored target.
+ *
+ * TODO: try all status codes?
  */
 TEST(PackageManagerFake, Verify) {
   TemporaryDirectory temp_dir;
@@ -30,7 +32,7 @@ TEST(PackageManagerFake, Verify) {
   Uptane::Target target_bad("some-pkg", primary_ecu, {Uptane::Hash(Uptane::Hash::Type::kSha256, "hash-bad")}, 4, "");
 
   PackageManagerFake fakepm(config.pacman, storage, nullptr, nullptr);
-  EXPECT_FALSE(fakepm.verifyTarget(target_good));
+  EXPECT_EQ(fakepm.verifyTarget(target_good), TargetStatus::kNotFound);
 
   // Write the target with a different hash.
   {
@@ -39,7 +41,7 @@ TEST(PackageManagerFake, Verify) {
     fhandle->wfeed(wb, 4);
     fhandle->wcommit();
   }
-  EXPECT_FALSE(fakepm.verifyTarget(target_good));
+  EXPECT_EQ(fakepm.verifyTarget(target_good), TargetStatus::kIncomplete);
 
   // Write the target with the expected hash.
   storage->removeTargetFile(target_good.filename());
@@ -49,7 +51,7 @@ TEST(PackageManagerFake, Verify) {
     fhandle->wfeed(wb, 4);
     fhandle->wcommit();
   }
-  EXPECT_TRUE(fakepm.verifyTarget(target_good));
+  EXPECT_EQ(fakepm.verifyTarget(target_good), TargetStatus::kGood);
 }
 
 TEST(PackageManagerFake, FinalizeAfterReboot) {

src/libaktualizr/package_manager/packagemanagerinterface.cc

@@ -87,8 +87,8 @@ bool PackageManagerInterface::fetchTarget(const Uptane::Target& target, Uptane::
     if (target.hashes().empty()) {
       throw Uptane::Exception("image", "No hash defined for the target");
     }
-    auto target_exists = storage_->checkTargetFile(target);
-    if (target_exists && (*target_exists).first == target.length()) {
+    TargetStatus exists = PackageManagerInterface::verifyTarget(target);
+    if (exists == TargetStatus::kGood) {
       LOG_INFO << "Image already downloaded; skipping download";
       return true;
     }
@@ -97,13 +97,16 @@ bool PackageManagerInterface::fetchTarget(const Uptane::Target& target, Uptane::
       return true;
     }
     DownloadMetaStruct ds(target, std::move(progress_cb), token);
-    if (target_exists) {
-      ds.downloaded_length = target_exists->first;
+    if (exists == TargetStatus::kIncomplete) {
+      auto target_check = storage_->checkTargetFile(target);
+      ds.downloaded_length = target_check->first;
       auto target_handle = storage_->openTargetFile(target);
       ::restoreHasherState(ds.hasher(), target_handle.get());
       target_handle->rclose();
       ds.fhandle = target_handle->toWriteHandle();
     } else {
+      // If the target was found, but is oversized or the hash doesn't match,
+      // just start over.
       ds.fhandle = storage_->allocateTargetFile(false, target);
     }
 
@@ -145,7 +148,26 @@ bool PackageManagerInterface::fetchTarget(const Uptane::Target& target, Uptane::
   return result;
 }
 
-bool PackageManagerInterface::verifyTarget(const Uptane::Target& target) const {
+TargetStatus PackageManagerInterface::verifyTarget(const Uptane::Target& target) const {
   auto target_exists = storage_->checkTargetFile(target);
-  return !!target_exists;
+  if (!target_exists) {
+    return TargetStatus::kNotFound;
+  } else if (target_exists->first < target.length()) {
+    return TargetStatus::kIncomplete;
+  } else if (target_exists->first > target.length()) {
+    return TargetStatus::kOversized;
+  }
+
+  // Even if the file exists and the length matches, recheck the hash.
+  DownloadMetaStruct ds(target, nullptr, nullptr);
+  ds.downloaded_length = target_exists->first;
+  auto target_handle = storage_->openTargetFile(target);
+  ::restoreHasherState(ds.hasher(), target_handle.get());
+  target_handle->rclose();
+  if (!target.MatchHash(Uptane::Hash(ds.hash_type, ds.hasher().getHexDigest()))) {
+    LOG_ERROR << "Target exists with expected length, but hash does not match metadata! " << target;
+    return TargetStatus::kHashMismatch;
+  }
+
+  return TargetStatus::kGood;
 }

src/libaktualizr/package_manager/packagemanagerinterface.h

@@ -15,6 +15,22 @@
 
 using FetcherProgressCb = std::function<void(const Uptane::Target&, const std::string&, unsigned int)>;
 
+/**
+ * Status of downloaded target.
+ */
+enum class TargetStatus {
+  /* Target has been downloaded and verified. */
+  kGood = 0,
+  /* Target was not found. */
+  kNotFound,
+  /* Target was found, but is incomplete. */
+  kIncomplete,
+  /* Target was found, but is larger than expected. */
+  kOversized,
+  /* Target was found, but hash did not match the metadata. */
+  kHashMismatch,
+};
+
 class PackageManagerInterface {
  public:
   PackageManagerInterface(PackageConfig pconfig, std::shared_ptr<INvStorage> storage,
@@ -33,7 +49,7 @@ class PackageManagerInterface {
   virtual bool imageUpdated() = 0;
   virtual bool fetchTarget(const Uptane::Target& target, Uptane::Fetcher& fetcher, const KeyManager& keys,
                            FetcherProgressCb progress_cb, const api::FlowControlToken* token);
-  virtual bool verifyTarget(const Uptane::Target& target) const;
+  virtual TargetStatus verifyTarget(const Uptane::Target& target) const;
 
   // only returns the version
   Json::Value getManifest(const Uptane::EcuSerial& ecu_serial) const {

src/libaktualizr/primary/aktualizr_fullostree_test.cc

@@ -65,12 +65,13 @@ TEST(Aktualizr, FullOstreeUpdate) {
     result::UpdateCheck update_result = aktualizr.CheckUpdates().get();
     ASSERT_EQ(update_result.status, result::UpdateStatus::kUpdatesAvailable);
     // Verify the target has not yet been downloaded.
-    EXPECT_FALSE(aktualizr.uptane_client()->package_manager_->verifyTarget(update_result.updates[0]));
+    EXPECT_EQ(aktualizr.uptane_client()->package_manager_->verifyTarget(update_result.updates[0]),
+              TargetStatus::kNotFound);
 
     result::Download download_result = aktualizr.Download(update_result.updates).get();
     EXPECT_EQ(download_result.status, result::DownloadStatus::kSuccess);
     // Verify the target has been downloaded.
-    EXPECT_TRUE(aktualizr.uptane_client()->package_manager_->verifyTarget(update_result.updates[0]));
+    EXPECT_EQ(aktualizr.uptane_client()->package_manager_->verifyTarget(update_result.updates[0]), TargetStatus::kGood);
 
     result::Install install_result = aktualizr.Install(update_result.updates).get();
     EXPECT_EQ(install_result.ecu_reports.size(), 1);
@@ -102,7 +103,7 @@ TEST(Aktualizr, FullOstreeUpdate) {
     Uptane::EcuMap primary_ecu{{Uptane::EcuSerial(conf.provision.primary_ecu_serial),
                                 Uptane::HardwareIdentifier(conf.provision.primary_ecu_hardware_id)}};
     Uptane::Target target_bad("some-pkg", primary_ecu, {Uptane::Hash(Uptane::Hash::Type::kSha256, "hash-bad")}, 4, "");
-    EXPECT_FALSE(aktualizr.uptane_client()->package_manager_->verifyTarget(target_bad));
+    EXPECT_EQ(aktualizr.uptane_client()->package_manager_->verifyTarget(target_bad), TargetStatus::kNotFound);
   }
 }
 

src/libaktualizr/primary/sotauptaneclient.cc

@@ -831,9 +831,9 @@ result::Install SotaUptaneClient::uptaneInstall(const std::vector<Uptane::Target
 
   // Recheck the downloaded update hashes.
   for (const auto &update : updates) {
-    if (!package_manager_->verifyTarget(update)) {
+    if (package_manager_->verifyTarget(update) != TargetStatus::kGood) {
       result.dev_report = {false, data::ResultCode::Numeric::kInternalError, ""};
-      storage->storeDeviceInstallationResult(result.dev_report, "Downloaded target's hash is invalid", correlation_id);
+      storage->storeDeviceInstallationResult(result.dev_report, "Downloaded target is invalid", correlation_id);
       sendEvent<event::AllInstallsComplete>(result);
       return result;
     }

src/libaktualizr/primary/sotauptaneclient.h

@@ -82,7 +82,7 @@ class SotaUptaneClient {
   FRIEND_TEST(DockerAppManager, DockerApp_Fetch);
   FRIEND_TEST(Uptane, AssembleManifestGood);
   FRIEND_TEST(Uptane, AssembleManifestBad);
-  FRIEND_TEST(Uptane, InstallFake);
+  FRIEND_TEST(Uptane, InstallFakeGood);
   FRIEND_TEST(Uptane, restoreVerify);
   FRIEND_TEST(Uptane, PutManifest);
   FRIEND_TEST(Uptane, offlineIteration);

src/libaktualizr/uptane/uptane_test.cc

@@ -334,7 +334,7 @@ void process_events_Install(const std::shared_ptr<event::BaseEvent> &event) {
  * Store installation result for device.
  * Check if an update is already installed.
  */
-TEST(Uptane, InstallFake) {
+TEST(Uptane, InstallFakeGood) {
   Config conf("tests/config/basic.toml");
   TemporaryDirectory temp_dir;
   auto http = std::make_shared<HttpFake>(temp_dir.Path(), "hasupdates");
@@ -394,6 +394,77 @@ TEST(Uptane, InstallFake) {
   EXPECT_EQ(installation_report["result"]["success"].asBool(), false);
 }
 
+/*
+ * Verify that installation will fail if the underlying data does not match the
+ * target.
+ *
+ * TODO: oversized and incomplete
+ */
+TEST(Uptane, InstallFakeBad) {
+  Config conf("tests/config/basic.toml");
+  TemporaryDirectory temp_dir;
+  auto http = std::make_shared<HttpFake>(temp_dir.Path(), "hasupdates");
+  conf.uptane.director_server = http->tls_server + "director";
+  conf.uptane.repo_server = http->tls_server + "repo";
+  conf.pacman.type = PackageManager::kNone;
+  conf.provision.primary_ecu_serial = "CA:FE:A6:D2:84:9D";
+  conf.provision.primary_ecu_hardware_id = "primary_hw";
+  conf.storage.path = temp_dir.Path();
+  conf.tls.server = http->tls_server;
+  UptaneTestCommon::addDefaultSecondary(conf, temp_dir, "secondary_ecu_serial", "secondary_hw");
+  conf.postUpdateValues();
+
+  auto storage = INvStorage::newStorage(conf.storage);
+  std::function<void(std::shared_ptr<event::BaseEvent> event)> f_cb = process_events_Install;
+  auto up = UptaneTestCommon::newTestClient(conf, storage, http, nullptr);
+  EXPECT_NO_THROW(up->initialize());
+
+  result::UpdateCheck update_result = up->fetchMeta();
+  EXPECT_EQ(update_result.status, result::UpdateStatus::kUpdatesAvailable);
+  result::Download download_result = up->downloadImages(update_result.updates);
+  EXPECT_EQ(download_result.status, result::DownloadStatus::kSuccess);
+
+  std::string hash = download_result.updates[0].sha256Hash();
+  std::transform(hash.begin(), hash.end(), hash.begin(), ::toupper);
+  boost::filesystem::path image = temp_dir / "images" / hash;
+
+  // Overwrite the file on disk with garbage so that the target verification
+  // fails. First read the existing data so we can re-write it later.
+  auto rhandle = storage->openTargetFile(download_result.updates[0]);
+  const uint64_t length = download_result.updates[0].length();
+  uint8_t content[length];
+  EXPECT_EQ(rhandle->rread(content, length), length);
+  rhandle->rclose();
+  auto whandle = storage->allocateTargetFile(false, download_result.updates[0]);
+  uint8_t content_bad[length];
+  memset(content_bad, 0, length);
+  EXPECT_EQ(whandle->wfeed(content_bad, 3), 3);
+  whandle->wcommit();
+
+  result::Install install_result = up->uptaneInstall(download_result.updates);
+  EXPECT_FALSE(install_result.dev_report.isSuccess());
+  EXPECT_EQ(install_result.dev_report.result_code, data::ResultCode::Numeric::kInternalError);
+
+  // Try again, but this time with equally long data to make sure the hash check
+  // actually gets triggered.
+  whandle = storage->allocateTargetFile(false, download_result.updates[0]);
+  EXPECT_EQ(whandle->wfeed(content_bad, length), length);
+  whandle->wcommit();
+
+  install_result = up->uptaneInstall(download_result.updates);
+  EXPECT_FALSE(install_result.dev_report.isSuccess());
+  EXPECT_EQ(install_result.dev_report.result_code, data::ResultCode::Numeric::kInternalError);
+
+  // Restore the original data to the file so that verification succeeds.
+  whandle = storage->allocateTargetFile(false, download_result.updates[0]);
+  EXPECT_EQ(whandle->wfeed(reinterpret_cast<uint8_t *>(content), length), length);
+  whandle->wcommit();
+
+  install_result = up->uptaneInstall(download_result.updates);
+  EXPECT_TRUE(install_result.dev_report.isSuccess());
+  EXPECT_EQ(install_result.dev_report.result_code, data::ResultCode::Numeric::kOk);
+}
+
 bool EcuInstallationStartedReportGot = false;
 class HttpFakeEvents : public HttpFake {
  public: