From 0fdd4c07cf63e583e79245cab916871d7f8fa15e Mon Sep 17 00:00:00 2001 From: Mykhaylo Sul Date: Mon, 25 Nov 2019 15:20:11 +0200 Subject: [PATCH] OTA-3969: Uptane verification tests for IP secondary Signed-off-by: Mykhaylo Sul --- src/aktualizr_secondary/CMakeLists.txt | 5 + .../aktualizr_secondary.cc | 14 +- .../uptane_verification_test.cc | 198 ++++++++++++++++++ src/libaktualizr/uptane/directorrepository.cc | 11 +- src/libaktualizr/uptane/imagesrepository.cc | 5 +- 5 files changed, 224 insertions(+), 9 deletions(-) create mode 100644 src/aktualizr_secondary/uptane_verification_test.cc diff --git a/src/aktualizr_secondary/CMakeLists.txt b/src/aktualizr_secondary/CMakeLists.txt index 5b25ced08e..5a93f169fe 100644 --- a/src/aktualizr_secondary/CMakeLists.txt +++ b/src/aktualizr_secondary/CMakeLists.txt @@ -53,6 +53,11 @@ list(INSERT TEST_LIBS 0 aktualizr_secondary_static_lib) add_aktualizr_test(NAME aktualizr_secondary_config SOURCES aktualizr_secondary_config_test.cc PROJECT_WORKING_DIRECTORY) +add_aktualizr_test(NAME aktualizr_secondary_uptane_verification + SOURCES uptane_verification_test.cc + ARGS "$" + PROJECT_WORKING_DIRECTORY) + add_aktualizr_test(NAME aktualizr_secondary_update SOURCES update_test.cc ARGS ${PROJECT_BINARY_DIR}/ostree_repo PROJECT_WORKING_DIRECTORY) diff --git a/src/aktualizr_secondary/aktualizr_secondary.cc b/src/aktualizr_secondary/aktualizr_secondary.cc index 69cda8745a..f9a4e18a45 100644 --- a/src/aktualizr_secondary/aktualizr_secondary.cc +++ b/src/aktualizr_secondary/aktualizr_secondary.cc @@ -132,6 +132,13 @@ bool AktualizrSecondary::sendFirmwareResp(const std::shared_ptr& fi return false; } + // This check is not applicable for the ostree case, just for the binary file case(s) + if (targetToApply.length() != firmware->length()) { + LOG_ERROR << "The target image size specified in metadata " << targetToApply.length() + << " does not match actual size " << firmware->length(); + return false; + } + install_res = pacman->install(targetToApply); if (install_res.result_code.num_code != data::ResultCode::Numeric::kOk) { LOG_ERROR << "Could not install target (" << install_res.result_code.toString() << "): " << install_res.description; @@ -182,7 +189,7 @@ bool AktualizrSecondary::doFullVerification(const Metadata& metadata) { // 1. Load and verify the current time or the most recent securely attested time. // - // We trust the time that the given system/OS/ECU provides, In this ECU we trust :) + // We trust the time that the given system/OS/ECU provides, In ECU We Trust :) TimeStamp now(TimeStamp::Now()); // 2. Download and check the Root metadata file from the Director repository, following the procedure in @@ -195,8 +202,9 @@ bool AktualizrSecondary::doFullVerification(const Metadata& metadata) { // procedure in Section 5.4.4.5. // // 5. Download and check the Targets metadata file from the Director repository, following the procedure in - // Section 5.4.4.6. DirectorRepository::updateMeta() method implements this verification step The followin steps of - // the Director's target metadata verification are missing in DirectorRepository::updateMeta() + // Section 5.4.4.6. DirectorRepository::updateMeta() method implements this verification step + // + // The followin steps of the Director's target metadata verification are missing in DirectorRepository::updateMeta() // 6. If checking Targets metadata from the Director repository, verify that there are no delegations. // 7. If checking Targets metadata from the Director repository, check that no ECU identifier is represented more // than once. diff --git a/src/aktualizr_secondary/uptane_verification_test.cc b/src/aktualizr_secondary/uptane_verification_test.cc new file mode 100644 index 0000000000..b5dff9f619 --- /dev/null +++ b/src/aktualizr_secondary/uptane_verification_test.cc @@ -0,0 +1,198 @@ +#include + +#include "aktualizr_secondary.h" +#include "test_utils.h" + +static boost::filesystem::path UPTANE_METADATA_GENERATOR; + +class AktualizrSecondaryPtr { + public: + AktualizrSecondaryPtr() { + AktualizrSecondaryConfig config; + + config.pacman.type = PackageManager::kNone; + config.storage.path = _storage_dir.Path(); + config.storage.type = StorageType::kSqlite; + + auto storage = INvStorage::newStorage(config.storage); + _secondary = std::make_shared(config, storage); + } + + public: + std::shared_ptr& operator->() { return _secondary; } + + private: + TemporaryDirectory _storage_dir; + std::shared_ptr _secondary; +}; + +class UptaneRepo { + public: + UptaneRepo() { + _uptane_gen.run({"generate", "--path", _root_dir.PathString()}); + EXPECT_EQ(_uptane_gen.lastExitCode(), 0) << _uptane_gen.lastStdOut(); + } + + Uptane::RawMetaPack addImageFile(const std::string& targetname, const std::string& hardware_id, + const std::string& serial, const std::string& expires = "") { + const auto image_file_path = _root_dir / targetname; + boost::filesystem::ofstream(image_file_path) << "some data"; + + _uptane_gen.run({"image", "--path", _root_dir.PathString(), "--targetname", targetname, "--filename", + image_file_path.c_str(), "--hwid", hardware_id, "--expires", expires}); + + EXPECT_EQ(_uptane_gen.lastExitCode(), 0) + << _uptane_gen.lastStdOut(); /* uptane_gen output message into stdout in case of an error */ + + _uptane_gen.run({"addtarget", "--path", _root_dir.PathString(), "--targetname", targetname, "--hwid", hardware_id, + "--serial", serial, "--expires", expires}); + EXPECT_EQ(_uptane_gen.lastExitCode(), 0) << _uptane_gen.lastStdOut(); + + _uptane_gen.run({"signtargets", "--path", _root_dir.PathString()}); + EXPECT_EQ(_uptane_gen.lastExitCode(), 0) << _uptane_gen.lastStdOut(); + + return getCurrentMetadata(); + } + + Uptane::RawMetaPack getCurrentMetadata() const { + Uptane::RawMetaPack metadata; + + boost::filesystem::load_string_file(_director_dir / "root.json", metadata.director_root); + boost::filesystem::load_string_file(_director_dir / "targets.json", metadata.director_targets); + + boost::filesystem::load_string_file(_imagerepo_dir / "root.json", metadata.image_root); + boost::filesystem::load_string_file(_imagerepo_dir / "timestamp.json", metadata.image_timestamp); + boost::filesystem::load_string_file(_imagerepo_dir / "snapshot.json", metadata.image_snapshot); + boost::filesystem::load_string_file(_imagerepo_dir / "targets.json", metadata.image_targets); + + return metadata; + } + + std::shared_ptr getImageData(const std::string& targetname) const { + auto image_data = std::make_shared(); + boost::filesystem::load_string_file(_root_dir / targetname, *image_data); + return image_data; + } + + private: + TemporaryDirectory _root_dir; + TemporaryFile _default_image_file; + boost::filesystem::path _director_dir{_root_dir / "repo/director"}; + boost::filesystem::path _imagerepo_dir{_root_dir / "repo/repo"}; + Process _uptane_gen{UPTANE_METADATA_GENERATOR.string()}; +}; + +class SecondaryUptaneVerificationTest : public ::testing::Test { + protected: + SecondaryUptaneVerificationTest() { + _uptane_repo.addImageFile(_default_target, _secondary->getHwIdResp().ToString(), + _secondary->getSerialResp().ToString()); + } + + std::shared_ptr getImageData(const std::string& targetname = _default_target) const { + return _uptane_repo.getImageData(targetname); + } + + protected: + static constexpr const char* const _default_target{"defaulttarget"}; + AktualizrSecondaryPtr _secondary; + UptaneRepo _uptane_repo; +}; + +TEST_F(SecondaryUptaneVerificationTest, fullUptaneVerificationPositive) { + EXPECT_TRUE(_secondary->putMetadataResp(Metadata(_uptane_repo.getCurrentMetadata()))); + EXPECT_TRUE(_secondary->sendFirmwareResp(getImageData())); +} + +TEST_F(SecondaryUptaneVerificationTest, MalformedMetadaJson) { + { + auto malformed_metadata = _uptane_repo.getCurrentMetadata(); + malformed_metadata.director_root[4] = 'f'; + + EXPECT_FALSE(_secondary->putMetadataResp(Metadata(malformed_metadata))); + } + + { + auto malformed_metadata = _uptane_repo.getCurrentMetadata(); + malformed_metadata.director_targets[4] = 'f'; + + EXPECT_FALSE(_secondary->putMetadataResp(Metadata(malformed_metadata))); + } + + { + auto malformed_metadata = _uptane_repo.getCurrentMetadata(); + malformed_metadata.image_root[4] = 'f'; + + EXPECT_FALSE(_secondary->putMetadataResp(Metadata(malformed_metadata))); + } + + { + auto malformed_metadata = _uptane_repo.getCurrentMetadata(); + malformed_metadata.image_timestamp[4] = 'f'; + + EXPECT_FALSE(_secondary->putMetadataResp(Metadata(malformed_metadata))); + } + + { + auto malformed_metadata = _uptane_repo.getCurrentMetadata(); + malformed_metadata.image_snapshot[4] = 'f'; + + EXPECT_FALSE(_secondary->putMetadataResp(Metadata(malformed_metadata))); + } + + { + auto malformed_metadata = _uptane_repo.getCurrentMetadata(); + malformed_metadata.image_targets[4] = 'f'; + + EXPECT_FALSE(_secondary->putMetadataResp(Metadata(malformed_metadata))); + } +} + +TEST_F(SecondaryUptaneVerificationTest, IncorrectTargetQuantity) { + { + // two targets for the same ECU + _uptane_repo.addImageFile("second_target", _secondary->getHwIdResp().ToString(), + _secondary->getSerialResp().ToString()); + + EXPECT_FALSE(_secondary->putMetadataResp(Metadata(_uptane_repo.getCurrentMetadata()))); + } + + { + // zero targets for the ECU being tested + auto metadata = UptaneRepo().addImageFile("mytarget", _secondary->getHwIdResp().ToString(), "non-existing-serial"); + + EXPECT_FALSE(_secondary->putMetadataResp(Metadata(metadata))); + } + + { + // zero targets for the ECU being tested + auto metadata = UptaneRepo().addImageFile("mytarget", "non-existig-hwid", _secondary->getSerialResp().ToString()); + + EXPECT_FALSE(_secondary->putMetadataResp(Metadata(metadata))); + } +} + +TEST_F(SecondaryUptaneVerificationTest, InvalidImageFileSize) { + EXPECT_TRUE(_secondary->putMetadataResp(Metadata(_uptane_repo.getCurrentMetadata()))); + auto image_data = getImageData(); + image_data->append("\n"); + EXPECT_FALSE(_secondary->sendFirmwareResp(image_data)); +} + +#ifndef __NO_MAIN__ +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + + if (argc != 2) { + std::cerr << "Error: " << argv[0] << " requires the path to the uptane-generator utility\n"; + return EXIT_FAILURE; + } + + UPTANE_METADATA_GENERATOR = argv[1]; + + logger_init(); + logger_set_threshold(boost::log::trivial::error); + + return RUN_ALL_TESTS(); +} +#endif diff --git a/src/libaktualizr/uptane/directorrepository.cc b/src/libaktualizr/uptane/directorrepository.cc index 91f2ba1233..d083b5d6a4 100644 --- a/src/libaktualizr/uptane/directorrepository.cc +++ b/src/libaktualizr/uptane/directorrepository.cc @@ -96,10 +96,11 @@ bool DirectorRepository::updateMeta(INvStorage& storage, const IMetadataFetcher& // Update Director Root Metadata { - // This procedure doesn't exactly follow the spec: - // https://uptane.github.io/uptane-standard/uptane-standard.html#check_root At first, it should try to download N+1 - // version of the Root metadata file instead of downloading the "latest" one how do we know which version is the - // latest and why just root.json is downloaded ? + // According to the current design root.json without a number is guaranteed to be the latest version. + // Therefore we fetch the latest (root.json), and + // if it matches what we already have stored, we're good. + // If not, then we have to go fetch the missing ones by name/number until we catch up. + std::string director_root; if (!fetcher.fetchLatestRole(&director_root, kMaxRootSize, RepositoryType::Director(), Role::Root())) { return false; @@ -118,6 +119,7 @@ bool DirectorRepository::updateMeta(INvStorage& storage, const IMetadataFetcher& // 2. If local_version becomes higher than 1, e.g. 2 than a rollback attack is possible since the business logic // here won't return any error as suggested in #4 of // https://uptane.github.io/uptane-standard/uptane-standard.html#check_root + // TODO: https://saeljira.it.here.com/browse/OTA-4119 for (int version = local_version + 1; version <= remote_version; ++version) { if (!fetcher.fetchRole(&director_root, kMaxRootSize, RepositoryType::Director(), Role::Root(), Version(version))) { @@ -172,6 +174,7 @@ bool DirectorRepository::updateMeta(INvStorage& storage, const IMetadataFetcher& // 6. If checking Targets metadata from the Director repository, verify that there are no delegations. // 7. If checking Targets metadata from the Director repository, check that no ECU identifier is represented more // than once. + // TODO: https://saeljira.it.here.com/browse/OTA-4118 if (!verifyTargets(director_targets)) { return false; } diff --git a/src/libaktualizr/uptane/imagesrepository.cc b/src/libaktualizr/uptane/imagesrepository.cc index e68ce3ebf2..d8fc3d8919 100644 --- a/src/libaktualizr/uptane/imagesrepository.cc +++ b/src/libaktualizr/uptane/imagesrepository.cc @@ -175,6 +175,7 @@ bool ImagesRepository::updateMeta(INvStorage& storage, const IMetadataFetcher& f // 2. If local_version becomes higher than 1, e.g. 2 than a rollback attack is possible since the business logic // here won't return any error as suggested in #4 of // https://uptane.github.io/uptane-standard/uptane-standard.html#check_root + // TODO: https://saeljira.it.here.com/browse/OTA-4119 for (int version = local_version + 1; version <= remote_version; ++version) { if (!fetcher.fetchRole(&images_root, kMaxRootSize, RepositoryType::Image(), Role::Root(), Version(version))) { return false; @@ -241,11 +242,11 @@ bool ImagesRepository::updateMeta(INvStorage& storage, const IMetadataFetcher& f local_version = -1; } - // I am not sure that #6 of https://uptane.github.io/uptane-standard/uptane-standard.html#check_snapshot is - // performed // 6. Check that each Targets metadata filename listed in the previous Snapshot metadata file is also listed in this // Snapshot metadata file. If this condition is not met, discard the new Snapshot metadata file, abort the update // cycle, and report the failure. (Checks for a rollback attack.) + // Let's wait for this ticket resolution https://github.com/uptane/uptane-standard/issues/149 + // https://saeljira.it.here.com/browse/OTA-4121 if (!verifySnapshot(images_snapshot)) { return false; }