From 7a2e49aaa258a7d4e20d6be5fca7e7a5074ee718 Mon Sep 17 00:00:00 2001 From: Mykhaylo Sul Date: Thu, 14 Nov 2019 23:43:20 +0200 Subject: [PATCH 1/2] OTA-3969: Do the full verification of metadata on the IP Secondary Signed-off-by: Mykhaylo Sul --- src/aktualizr_secondary/CMakeLists.txt | 2 + .../aktualizr_secondary.cc | 108 ++++++++++++------ src/aktualizr_secondary/aktualizr_secondary.h | 12 +- .../aktualizr_secondary_common.h | 7 +- .../aktualizr_secondary_metadata.cc | 50 ++++++++ .../aktualizr_secondary_metadata.h | 25 ++++ src/libaktualizr/primary/sotauptaneclient.cc | 2 +- src/libaktualizr/uptane/directorrepository.cc | 28 ++++- src/libaktualizr/uptane/directorrepository.h | 12 +- src/libaktualizr/uptane/fetcher.cc | 2 +- src/libaktualizr/uptane/fetcher.h | 24 +++- src/libaktualizr/uptane/imagesrepository.cc | 13 ++- src/libaktualizr/uptane/imagesrepository.h | 3 +- src/libaktualizr/uptane/tuf.h | 14 ++- src/libaktualizr/uptane/uptanerepository.h | 3 + .../partialverificationsecondary.cc | 2 +- 16 files changed, 247 insertions(+), 60 deletions(-) create mode 100644 src/aktualizr_secondary/aktualizr_secondary_metadata.cc create mode 100644 src/aktualizr_secondary/aktualizr_secondary_metadata.h diff --git a/src/aktualizr_secondary/CMakeLists.txt b/src/aktualizr_secondary/CMakeLists.txt index 02d5a83b39..5b25ced08e 100644 --- a/src/aktualizr_secondary/CMakeLists.txt +++ b/src/aktualizr_secondary/CMakeLists.txt @@ -4,6 +4,7 @@ set(AKTUALIZR_SECONDARY_LIB_SRC aktualizr_secondary.cc aktualizr_secondary_config.cc aktualizr_secondary_common.cc + aktualizr_secondary_metadata.cc socket_server.cc ) @@ -40,6 +41,7 @@ set(ALL_AKTUALIZR_SECONDARY_HEADERS aktualizr_secondary_interface.h aktualizr_secondary_config.h aktualizr_secondary_common.h + aktualizr_secondary_metadata.h socket_server.h ) diff --git a/src/aktualizr_secondary/aktualizr_secondary.cc b/src/aktualizr_secondary/aktualizr_secondary.cc index 059ec39350..69cda8745a 100644 --- a/src/aktualizr_secondary/aktualizr_secondary.cc +++ b/src/aktualizr_secondary/aktualizr_secondary.cc @@ -19,7 +19,9 @@ class SecondaryAdapter : public Uptane::SecondaryInterface { Uptane::HardwareIdentifier getHwId() override { return secondary.getHwIdResp(); } PublicKey getPublicKey() override { return secondary.getPublicKeyResp(); } Json::Value getManifest() override { return secondary.getManifestResp(); } - bool putMetadata(const Uptane::RawMetaPack& meta_pack) override { return secondary.putMetadataResp(meta_pack); } + bool putMetadata(const Uptane::RawMetaPack& meta_pack) override { + return secondary.putMetadataResp(Metadata(meta_pack)); + } int32_t getRootVersion(bool director) override { return secondary.getRootVersionResp(director); } bool putRoot(const std::string& root, bool director) override { return secondary.putRootResp(root, director); } bool sendFirmware(const std::shared_ptr& data) override { @@ -63,36 +65,7 @@ Json::Value AktualizrSecondary::getManifestResp() const { return keys_.signTuf(manifest); } -bool AktualizrSecondary::putMetadataResp(const Uptane::RawMetaPack& meta_pack) { - TimeStamp now(TimeStamp::Now()); - detected_attack_.clear(); - - // TODO: proper partial verification - root_ = Uptane::Root(Uptane::RepositoryType::Director(), Utils::parseJSON(meta_pack.director_root), root_); - Uptane::Targets targets(Uptane::RepositoryType::Director(), Uptane::Role::Targets(), - Utils::parseJSON(meta_pack.director_targets), std::make_shared(root_)); - if (meta_targets_.version() > targets.version()) { - detected_attack_ = "Rollback attack detected"; - return true; - } - meta_targets_ = targets; - std::vector::const_iterator it; - bool target_found = false; - for (it = meta_targets_.targets.begin(); it != meta_targets_.targets.end(); ++it) { - if (it->IsForSecondary(getSerialResp())) { - if (target_found) { - detected_attack_ = "Duplicate entry for this ECU"; - break; - } - target_found = true; - target_ = std_::make_unique(*it); - } - } - storage_->storeRoot(meta_pack.director_root, Uptane::RepositoryType::Director(), Uptane::Version(root_.version())); - storage_->storeNonRoot(meta_pack.director_targets, Uptane::RepositoryType::Director(), Uptane::Role::Targets()); - - return true; -} +bool AktualizrSecondary::putMetadataResp(const Metadata& metadata) { return doFullVerification(metadata); } int32_t AktualizrSecondary::getRootVersionResp(bool director) const { std::string root_meta; @@ -113,14 +86,17 @@ bool AktualizrSecondary::putRootResp(const std::string& root, bool director) { } bool AktualizrSecondary::sendFirmwareResp(const std::shared_ptr& firmware) { - if (target_ == nullptr) { - LOG_ERROR << "No valid installation target found"; + auto targetsForThisEcu = director_repo_.getTargets(getSerial()); + + if (targetsForThisEcu.size() != 1) { + LOG_ERROR << "Invalid number of targets (should be one): " << targetsForThisEcu.size(); return false; } + auto targetToApply = targetsForThisEcu[0]; std::string treehub_server; - if (target_->IsOstree()) { + if (targetToApply.IsOstree()) { // this is the ostree specific case try { std::string ca, cert, pkey, server_url; @@ -137,9 +113,9 @@ bool AktualizrSecondary::sendFirmwareResp(const std::shared_ptr& fi data::InstallationResult install_res; - if (target_->IsOstree()) { + if (targetToApply.IsOstree()) { #ifdef BUILD_OSTREE - install_res = OstreeManager::pull(config_.pacman.sysroot, treehub_server, keys_, *target_); + install_res = OstreeManager::pull(config_.pacman.sysroot, treehub_server, keys_, targetToApply); if (install_res.result_code.num_code != data::ResultCode::Numeric::kOk) { LOG_ERROR << "Could not pull from OSTree (" << install_res.result_code.toString() @@ -156,12 +132,12 @@ bool AktualizrSecondary::sendFirmwareResp(const std::shared_ptr& fi return false; } - install_res = pacman->install(*target_); + 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; return false; } - storage_->saveInstalledVersion(getSerialResp().ToString(), *target_, InstalledVersionUpdateMode::kCurrent); + storage_->saveInstalledVersion(getSerialResp().ToString(), targetToApply, InstalledVersionUpdateMode::kCurrent); return true; } @@ -200,3 +176,59 @@ void AktualizrSecondary::connectToPrimary() { LOG_INFO << "Failed to connect to Primary"; } } + +bool AktualizrSecondary::doFullVerification(const Metadata& metadata) { + // 5.4.4.2. Full verification https://uptane.github.io/uptane-standard/uptane-standard.html#metadata_verification + + // 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 :) + TimeStamp now(TimeStamp::Now()); + + // 2. Download and check the Root metadata file from the Director repository, following the procedure in + // Section 5.4.4.3. DirectorRepository::updateMeta() method implements this verification step, certain steps are + // missing though. see the method source code for details + + // 3. NOT SUPPORTED: Download and check the Timestamp metadata file from the Director repository, following the + // procedure in Section 5.4.4.4. + // 4. NOT SUPPORTED: Download and check the Snapshot metadata file from the Director repository, following the + // 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() + // 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. + if (!director_repo_.updateMeta(*storage_, metadata)) { + LOG_ERROR << "Failed to update director metadata: " << director_repo_.getLastException().what(); + return false; + } + + auto targetsForThisEcu = director_repo_.getTargets(getSerial()); + + if (targetsForThisEcu.size() != 1) { + LOG_ERROR << "Invalid number of targets (should be one): " << targetsForThisEcu.size(); + return false; + } + + // 6. Download and check the Root metadata file from the Image repository, following the procedure in Section 5.4.4.3. + // 7. Download and check the Timestamp metadata file from the Image repository, following the procedure in + // Section 5.4.4.4. + // 8. Download and check the Snapshot metadata file from the Image repository, following the procedure in + // Section 5.4.4.5. + // 9. Download and check the top-level Targets metadata file from the Image repository, following the procedure in + // Section 5.4.4.6. + if (!image_repo_.updateMeta(*storage_, metadata)) { + LOG_ERROR << "Failed to update image metadata: " << image_repo_.getLastException().what(); + return false; + } + + // 10. Verify that Targets metadata from the Director and Image repositories match. + if (!(director_repo_.getTargets() == *image_repo_.getTargets())) { + LOG_ERROR << "Targets metadata from the Director and Image repositories DOES NOT match "; + return false; + } + + return true; +} diff --git a/src/aktualizr_secondary/aktualizr_secondary.h b/src/aktualizr_secondary/aktualizr_secondary.h index 8ff68889cc..4d62cb068f 100644 --- a/src/aktualizr_secondary/aktualizr_secondary.h +++ b/src/aktualizr_secondary/aktualizr_secondary.h @@ -9,10 +9,15 @@ #include "crypto/keymanager.h" #include "socket_server.h" #include "storage/invstorage.h" -#include "uptane/tuf.h" #include "utilities/types.h" #include "utilities/utils.h" +#include "uptane/directorrepository.h" +#include "uptane/imagesrepository.h" +#include "uptane/tuf.h" + +#include "aktualizr_secondary_metadata.h" + class AktualizrSecondary : public AktualizrSecondaryInterface, private AktualizrSecondaryCommon { public: AktualizrSecondary(const AktualizrSecondaryConfig& config, const std::shared_ptr& storage); @@ -24,7 +29,7 @@ class AktualizrSecondary : public AktualizrSecondaryInterface, private Aktualizr Uptane::HardwareIdentifier getHwIdResp() const; PublicKey getPublicKeyResp() const; Json::Value getManifestResp() const; - bool putMetadataResp(const Uptane::RawMetaPack& meta_pack); + bool putMetadataResp(const Metadata& metadata); int32_t getRootVersionResp(bool director) const; bool putRootResp(const std::string& root, bool director); bool sendFirmwareResp(const std::shared_ptr& firmware); @@ -34,9 +39,12 @@ class AktualizrSecondary : public AktualizrSecondaryInterface, private Aktualizr private: void connectToPrimary(); + bool doFullVerification(const Metadata& metadata); private: SocketServer socket_server_; + Uptane::DirectorRepository director_repo_; + Uptane::ImagesRepository image_repo_; }; #endif // AKTUALIZR_SECONDARY_H diff --git a/src/aktualizr_secondary/aktualizr_secondary_common.h b/src/aktualizr_secondary/aktualizr_secondary_common.h index a1605cbbf7..97fb4db246 100644 --- a/src/aktualizr_secondary/aktualizr_secondary_common.h +++ b/src/aktualizr_secondary/aktualizr_secondary_common.h @@ -16,17 +16,16 @@ class AktualizrSecondaryCommon { bool uptaneInitialize(); - AktualizrSecondaryConfig config_; + const Uptane::EcuSerial& getSerial() const { return ecu_serial_; } + protected: + AktualizrSecondaryConfig config_; std::shared_ptr storage_; KeyManager keys_; Uptane::EcuSerial ecu_serial_; Uptane::HardwareIdentifier hardware_id_; std::shared_ptr pacman; Uptane::Root root_; - Uptane::Targets meta_targets_; - std::string detected_attack_; - std::unique_ptr target_; }; #endif // AKTUALIZR_SECONDARY_COMMON_H_ diff --git a/src/aktualizr_secondary/aktualizr_secondary_metadata.cc b/src/aktualizr_secondary/aktualizr_secondary_metadata.cc new file mode 100644 index 0000000000..8dcb466029 --- /dev/null +++ b/src/aktualizr_secondary/aktualizr_secondary_metadata.cc @@ -0,0 +1,50 @@ +#include "aktualizr_secondary_metadata.h" + +Metadata::Metadata(const Uptane::RawMetaPack& meta_pack) + : _director_metadata{{Uptane::Role::ROOT, meta_pack.director_root}, + {Uptane::Role::TARGETS, meta_pack.director_targets}}, + _image_metadata{ + {Uptane::Role::ROOT, meta_pack.image_root}, + {Uptane::Role::TIMESTAMP, meta_pack.image_timestamp}, + {Uptane::Role::SNAPSHOT, meta_pack.image_snapshot}, + {Uptane::Role::TARGETS, meta_pack.image_targets}, + } {} + +bool Metadata::fetchRole(std::string* result, int64_t maxsize, Uptane::RepositoryType repo, const Uptane::Role& role, + Uptane::Version version) const { + (void)maxsize; + (void)version; + + return getRoleMetadata(result, repo, role); +} + +bool Metadata::fetchLatestRole(std::string* result, int64_t maxsize, Uptane::RepositoryType repo, + const Uptane::Role& role) const { + (void)maxsize; + return getRoleMetadata(result, repo, role); +} + +bool Metadata::getRoleMetadata(std::string* result, const Uptane::RepositoryType& repo, + const Uptane::Role& role) const { + const std::unordered_map* metadata_map = nullptr; + + if (repo == Uptane::RepositoryType::Director()) { + metadata_map = &_director_metadata; + } else if (repo == Uptane::RepositoryType::Image()) { + metadata_map = &_image_metadata; + } + + if (metadata_map == nullptr) { + LOG_ERROR << "There are no any metadata for the given type of repository: " << repo.toString(); + return false; + } + + auto found_meta_it = metadata_map->find(role.ToString()); + if (found_meta_it == metadata_map->end()) { + LOG_ERROR << "There are no any metadata for the given type of role: " << repo.toString() << ": " << role.ToString(); + return false; + } + + *result = found_meta_it->second; + return true; +} diff --git a/src/aktualizr_secondary/aktualizr_secondary_metadata.h b/src/aktualizr_secondary/aktualizr_secondary_metadata.h new file mode 100644 index 0000000000..d3cdb2fa20 --- /dev/null +++ b/src/aktualizr_secondary/aktualizr_secondary_metadata.h @@ -0,0 +1,25 @@ +#ifndef AKTUALIZR_SECONDARY_METADATA_H_ +#define AKTUALIZR_SECONDARY_METADATA_H_ + +#include + +#include "uptane/fetcher.h" + +class Metadata : public Uptane::IMetadataFetcher { + public: + Metadata(const Uptane::RawMetaPack& meta_pack); + + bool fetchRole(std::string* result, int64_t maxsize, Uptane::RepositoryType repo, const Uptane::Role& role, + Uptane::Version version) const override; + bool fetchLatestRole(std::string* result, int64_t maxsize, Uptane::RepositoryType repo, + const Uptane::Role& role) const override; + + private: + bool getRoleMetadata(std::string* result, const Uptane::RepositoryType& repo, const Uptane::Role& role) const; + + private: + const std::unordered_map _director_metadata; + const std::unordered_map _image_metadata; +}; + +#endif // AKTUALIZR_SECONDARY_METADATA_H_ diff --git a/src/libaktualizr/primary/sotauptaneclient.cc b/src/libaktualizr/primary/sotauptaneclient.cc index b9d3664ac5..c3201dd32a 100644 --- a/src/libaktualizr/primary/sotauptaneclient.cc +++ b/src/libaktualizr/primary/sotauptaneclient.cc @@ -382,7 +382,7 @@ void SotaUptaneClient::computeDeviceInstallationResult(data::InstallationResult } bool SotaUptaneClient::getNewTargets(std::vector *new_targets, unsigned int *ecus_count) { - std::vector targets = director_repo.getTargets(); + std::vector targets = director_repo.getTargets().targets; Uptane::EcuSerial primary_ecu_serial = uptane_manifest.getPrimaryEcuSerial(); if (ecus_count != nullptr) { *ecus_count = 0; diff --git a/src/libaktualizr/uptane/directorrepository.cc b/src/libaktualizr/uptane/directorrepository.cc index ff13f14f88..91f2ba1233 100644 --- a/src/libaktualizr/uptane/directorrepository.cc +++ b/src/libaktualizr/uptane/directorrepository.cc @@ -70,7 +70,7 @@ bool DirectorRepository::checkMetaOffline(INvStorage& storage) { return true; } -bool DirectorRepository::updateMeta(INvStorage& storage, Fetcher& fetcher) { +bool DirectorRepository::updateMeta(INvStorage& storage, const IMetadataFetcher& fetcher) { // Uptane step 2 (download time) is not implemented yet. // Uptane step 3 (download metadata) @@ -96,6 +96,10 @@ bool DirectorRepository::updateMeta(INvStorage& storage, Fetcher& fetcher) { // 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 ? std::string director_root; if (!fetcher.fetchLatestRole(&director_root, kMaxRootSize, RepositoryType::Director(), Role::Root())) { return false; @@ -108,6 +112,12 @@ bool DirectorRepository::updateMeta(INvStorage& storage, Fetcher& fetcher) { int local_version = rootVersion(); + // if remote_version <= local_version then the director's root metadata are never verified + // which leads to two issues + // 1. At initial stage (just after provisioning) the root metadata from 1.root.json are not verified + // 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 for (int version = local_version + 1; version <= remote_version; ++version) { if (!fetcher.fetchRole(&director_root, kMaxRootSize, RepositoryType::Director(), Role::Root(), Version(version))) { @@ -121,10 +131,17 @@ bool DirectorRepository::updateMeta(INvStorage& storage, Fetcher& fetcher) { storage.clearNonRootMeta(RepositoryType::Director()); } + // Check that the current (or latest securely attested) time is lower than the expiration timestamp in the latest + // Root metadata file. (Checks for a freeze attack.) if (rootExpired()) { return false; } } + + // Not supported: 3. Download and check the Timestamp metadata file from the Director repository, following the + // procedure in Section 5.4.4.4. Not supported: 4. Download and check the Snapshot metadata file from the Director + // repository, following the procedure in Section 5.4.4.5. + // Update Director Targets Metadata { std::string director_targets; @@ -146,6 +163,15 @@ bool DirectorRepository::updateMeta(INvStorage& storage, Fetcher& fetcher) { local_version = -1; } + // Not supported: If the ECU performing the verification is the Primary ECU, it SHOULD also ensure that the Targets + // metadata from the Director repository doesn’t contain any ECU identifiers for ECUs not actually present in the + // vehicle. + + // Not supported: 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. if (!verifyTargets(director_targets)) { return false; } diff --git a/src/libaktualizr/uptane/directorrepository.h b/src/libaktualizr/uptane/directorrepository.h index 51f6b1ceac..d67b75110f 100644 --- a/src/libaktualizr/uptane/directorrepository.h +++ b/src/libaktualizr/uptane/directorrepository.h @@ -3,7 +3,6 @@ #include "gtest/gtest_prod.h" -#include "fetcher.h" #include "uptanerepository.h" namespace Uptane { @@ -14,18 +13,21 @@ namespace Uptane { class DirectorRepository : public RepositoryCommon { public: DirectorRepository() : RepositoryCommon(RepositoryType::Director()) {} - void resetMeta(); bool verifyTargets(const std::string& targets_raw); - const std::vector& getTargets() const { return targets.targets; } + const Targets& getTargets() const { return targets; } + std::vector getTargets(const Uptane::EcuSerial& ecu_id) const { return targets.getTargets(ecu_id); } const std::string& getCorrelationId() const { return targets.correlation_id(); } bool targetsExpired() const; - bool usePreviousTargets() const; bool checkMetaOffline(INvStorage& storage); void dropTargets(INvStorage& storage); Exception getLastException() const { return last_exception; } - bool updateMeta(INvStorage& storage, Fetcher& fetcher); + bool updateMeta(INvStorage& storage, const IMetadataFetcher& fetcher) override; + + private: + void resetMeta(); + bool usePreviousTargets() const; private: FRIEND_TEST(Director, EmptyTargets); diff --git a/src/libaktualizr/uptane/fetcher.cc b/src/libaktualizr/uptane/fetcher.cc index 749c0250d2..70930cef32 100644 --- a/src/libaktualizr/uptane/fetcher.cc +++ b/src/libaktualizr/uptane/fetcher.cc @@ -3,7 +3,7 @@ namespace Uptane { bool Fetcher::fetchRole(std::string* result, int64_t maxsize, RepositoryType repo, const Uptane::Role& role, - Version version) { + Version version) const { // TODO: chain-loading root.json std::string url = (repo == RepositoryType::Director()) ? config.uptane.director_server : config.uptane.repo_server; if (role.IsDelegation()) { diff --git a/src/libaktualizr/uptane/fetcher.h b/src/libaktualizr/uptane/fetcher.h index 48c8cb025f..8b6c46469b 100644 --- a/src/libaktualizr/uptane/fetcher.h +++ b/src/libaktualizr/uptane/fetcher.h @@ -13,12 +13,30 @@ constexpr int64_t kMaxTimestampSize = 64 * 1024; constexpr int64_t kMaxSnapshotSize = 64 * 1024; constexpr int64_t kMaxImagesTargetsSize = 1024 * 1024; -class Fetcher { +class IMetadataFetcher { + public: + IMetadataFetcher(const IMetadataFetcher&) = delete; + IMetadataFetcher& operator=(const IMetadataFetcher&) = delete; + virtual ~IMetadataFetcher() = default; + + public: + virtual bool fetchRole(std::string* result, int64_t maxsize, RepositoryType repo, const Uptane::Role& role, + Version version) const = 0; + virtual bool fetchLatestRole(std::string* result, int64_t maxsize, RepositoryType repo, + const Uptane::Role& role) const = 0; + + protected: + IMetadataFetcher() = default; +}; + +class Fetcher : public IMetadataFetcher { public: Fetcher(const Config& config_in, std::shared_ptr http_in) : http(std::move(http_in)), config(config_in) {} - bool fetchRole(std::string* result, int64_t maxsize, RepositoryType repo, const Uptane::Role& role, Version version); - bool fetchLatestRole(std::string* result, int64_t maxsize, RepositoryType repo, const Uptane::Role& role) { + bool fetchRole(std::string* result, int64_t maxsize, RepositoryType repo, const Uptane::Role& role, + Version version) const override; + bool fetchLatestRole(std::string* result, int64_t maxsize, RepositoryType repo, + const Uptane::Role& role) const override { return fetchRole(result, maxsize, repo, role, Version()); } diff --git a/src/libaktualizr/uptane/imagesrepository.cc b/src/libaktualizr/uptane/imagesrepository.cc index 6f9d78460a..e68ce3ebf2 100644 --- a/src/libaktualizr/uptane/imagesrepository.cc +++ b/src/libaktualizr/uptane/imagesrepository.cc @@ -135,7 +135,7 @@ std::shared_ptr ImagesRepository::verifyDelegation(const std::s return std::shared_ptr(nullptr); } -bool ImagesRepository::updateMeta(INvStorage& storage, Fetcher& fetcher) { +bool ImagesRepository::updateMeta(INvStorage& storage, const IMetadataFetcher& fetcher) { resetMeta(); // Load Initial Images Root Metadata { @@ -169,6 +169,12 @@ bool ImagesRepository::updateMeta(INvStorage& storage, Fetcher& fetcher) { int local_version = rootVersion(); + // if remote_version <= local_version then the director's root metadata are never verified + // which leads to two issues + // 1. At initial stage (just after provisioning) the root metadata from 1.root.json are not verified + // 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 for (int version = local_version + 1; version <= remote_version; ++version) { if (!fetcher.fetchRole(&images_root, kMaxRootSize, RepositoryType::Image(), Role::Root(), Version(version))) { return false; @@ -235,6 +241,11 @@ bool ImagesRepository::updateMeta(INvStorage& storage, Fetcher& fetcher) { 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.) if (!verifySnapshot(images_snapshot)) { return false; } diff --git a/src/libaktualizr/uptane/imagesrepository.h b/src/libaktualizr/uptane/imagesrepository.h index afcdb9750d..e3a48d3209 100644 --- a/src/libaktualizr/uptane/imagesrepository.h +++ b/src/libaktualizr/uptane/imagesrepository.h @@ -4,7 +4,6 @@ #include #include -#include "fetcher.h" #include "uptanerepository.h" namespace Uptane { @@ -38,7 +37,7 @@ class ImagesRepository : public RepositoryCommon { int64_t getRoleSize(const Uptane::Role& role) const; bool checkMetaOffline(INvStorage& storage); - bool updateMeta(INvStorage& storage, Fetcher& fetcher); + bool updateMeta(INvStorage& storage, const IMetadataFetcher& fetcher) override; private: std::shared_ptr targets; diff --git a/src/libaktualizr/uptane/tuf.h b/src/libaktualizr/uptane/tuf.h index 38e7cf745d..938379f8fb 100644 --- a/src/libaktualizr/uptane/tuf.h +++ b/src/libaktualizr/uptane/tuf.h @@ -249,7 +249,7 @@ class Target { void setUri(std::string uri) { uri_ = std::move(uri); }; bool MatchHash(const Hash &hash) const; - bool IsForSecondary(const EcuSerial &ecuIdentifier) const { + bool IsForEcu(const EcuSerial &ecuIdentifier) const { return (std::find_if(ecus_.cbegin(), ecus_.cend(), [&ecuIdentifier](std::pair pair) { return pair.first == ecuIdentifier; }) != ecus_.cend()); @@ -428,7 +428,9 @@ class Targets : public MetaWithKeys { bool operator==(const Targets &rhs) const { return version_ == rhs.version() && expiry_ == rhs.expiry() && MatchTargetVector(targets, rhs.targets); } + const std::string &correlation_id() const { return correlation_id_; } + void clear() { targets.clear(); delegated_role_names_.clear(); @@ -436,6 +438,16 @@ class Targets : public MetaWithKeys { terminating_role_.clear(); } + std::vector getTargets(const Uptane::EcuSerial &ecu_id) const { + std::vector result; + for (auto it = targets.begin(); it != targets.end(); ++it) { + if (it->ecus().find(ecu_id) != it->ecus().end()) { + result.push_back(*it); + } + } + return result; + } + std::vector targets; std::vector delegated_role_names_; std::map> paths_for_role_; diff --git a/src/libaktualizr/uptane/uptanerepository.h b/src/libaktualizr/uptane/uptanerepository.h index 0176e352c9..118a41fd14 100644 --- a/src/libaktualizr/uptane/uptanerepository.h +++ b/src/libaktualizr/uptane/uptanerepository.h @@ -8,6 +8,7 @@ #include "config/config.h" #include "crypto/crypto.h" #include "crypto/keymanager.h" +#include "fetcher.h" #include "logging/logging.h" #include "storage/invstorage.h" @@ -37,10 +38,12 @@ class Manifest { class RepositoryCommon { public: RepositoryCommon(RepositoryType type_in) : type{type_in} {} + virtual ~RepositoryCommon() = default; bool initRoot(const std::string &root_raw); bool verifyRoot(const std::string &root_raw); int rootVersion() { return root.version(); } bool rootExpired() { return root.isExpired(TimeStamp::Now()); } + virtual bool updateMeta(INvStorage &storage, const IMetadataFetcher &fetcher) = 0; protected: void resetRoot(); diff --git a/src/virtual_secondary/partialverificationsecondary.cc b/src/virtual_secondary/partialverificationsecondary.cc index 17e0b4da39..bfe405426a 100644 --- a/src/virtual_secondary/partialverificationsecondary.cc +++ b/src/virtual_secondary/partialverificationsecondary.cc @@ -46,7 +46,7 @@ bool PartialVerificationSecondary::putMetadata(const RawMetaPack &meta) { std::vector::const_iterator it; bool target_found = false; for (it = meta_targets_.targets.begin(); it != meta_targets_.targets.end(); ++it) { - if (it->IsForSecondary(getSerial())) { + if (it->IsForEcu(getSerial())) { if (target_found) { detected_attack_ = "Duplicate entry for this ECU"; break; From 0fdd4c07cf63e583e79245cab916871d7f8fa15e Mon Sep 17 00:00:00 2001 From: Mykhaylo Sul Date: Mon, 25 Nov 2019 15:20:11 +0200 Subject: [PATCH 2/2] 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; }