From 709896b5f72af6821868ea3e923075605cd9e2c6 Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Fri, 23 Aug 2019 17:38:09 +0200 Subject: [PATCH] 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 --- .../package_manager/ostreemanager.cc | 8 +- .../package_manager/ostreemanager.h | 2 +- .../packagemanagerfake_test.cc | 8 +- .../packagemanagerinterface.cc | 34 +++++++-- .../package_manager/packagemanagerinterface.h | 18 ++++- .../primary/aktualizr_fullostree_test.cc | 7 +- src/libaktualizr/primary/sotauptaneclient.cc | 4 +- src/libaktualizr/primary/sotauptaneclient.h | 2 +- src/libaktualizr/uptane/uptane_test.cc | 73 ++++++++++++++++++- 9 files changed, 134 insertions(+), 22 deletions(-) diff --git a/src/libaktualizr/package_manager/ostreemanager.cc b/src/libaktualizr/package_manager/ostreemanager.cc index 80bb3bb8e2..936401f6b1 100644 --- a/src/libaktualizr/package_manager/ostreemanager.cc +++ b/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 { diff --git a/src/libaktualizr/package_manager/ostreemanager.h b/src/libaktualizr/package_manager/ostreemanager.h index 70df8c0460..84329a1acc 100644 --- a/src/libaktualizr/package_manager/ostreemanager.h +++ b/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 getStagedDeployment() const; static GObjectUniquePtr LoadSysroot(const boost::filesystem::path &path); diff --git a/src/libaktualizr/package_manager/packagemanagerfake_test.cc b/src/libaktualizr/package_manager/packagemanagerfake_test.cc index e06ddc445b..38d525df20 100644 --- a/src/libaktualizr/package_manager/packagemanagerfake_test.cc +++ b/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) { diff --git a/src/libaktualizr/package_manager/packagemanagerinterface.cc b/src/libaktualizr/package_manager/packagemanagerinterface.cc index 7749c94860..94b5d8be85 100644 --- a/src/libaktualizr/package_manager/packagemanagerinterface.cc +++ b/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; } diff --git a/src/libaktualizr/package_manager/packagemanagerinterface.h b/src/libaktualizr/package_manager/packagemanagerinterface.h index 81ddb065bf..cbf4b3c464 100644 --- a/src/libaktualizr/package_manager/packagemanagerinterface.h +++ b/src/libaktualizr/package_manager/packagemanagerinterface.h @@ -15,6 +15,22 @@ using FetcherProgressCb = std::function; +/** + * 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 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 { diff --git a/src/libaktualizr/primary/aktualizr_fullostree_test.cc b/src/libaktualizr/primary/aktualizr_fullostree_test.cc index 731e921d6e..2066e1e8de 100644 --- a/src/libaktualizr/primary/aktualizr_fullostree_test.cc +++ b/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); } } diff --git a/src/libaktualizr/primary/sotauptaneclient.cc b/src/libaktualizr/primary/sotauptaneclient.cc index 7a09ac3d92..698a179969 100644 --- a/src/libaktualizr/primary/sotauptaneclient.cc +++ b/src/libaktualizr/primary/sotauptaneclient.cc @@ -831,9 +831,9 @@ result::Install SotaUptaneClient::uptaneInstall(const std::vectorverifyTarget(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(result); return result; } diff --git a/src/libaktualizr/primary/sotauptaneclient.h b/src/libaktualizr/primary/sotauptaneclient.h index edf3fa2cd6..84ea2c7ca6 100644 --- a/src/libaktualizr/primary/sotauptaneclient.h +++ b/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); diff --git a/src/libaktualizr/uptane/uptane_test.cc b/src/libaktualizr/uptane/uptane_test.cc index 556768488f..e55bea6d88 100644 --- a/src/libaktualizr/uptane/uptane_test.cc +++ b/src/libaktualizr/uptane/uptane_test.cc @@ -334,7 +334,7 @@ void process_events_Install(const std::shared_ptr &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(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(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 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(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: