From d018279e21e520ddbba5e2d56cc1128528c20f11 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Thu, 2 Sep 2021 18:24:50 +0300 Subject: [PATCH] ngclient: Fix rollback checks The rollback checks themselves work, but they create a situation where Updater does not realize that it needs to download e.g. a new snapshot because the local snapshot is valid as _intermediate_ snapshot (that can be used for rollback protection but nothing else), but is not valid as final snapshot. Raise in the end of update_snapshot and update_timestamp if the files are not valid final metadata: this way the intermediate metadata does get loaded but Updater also knows it is not the final metadata. This modifies the existing tests but does not yet test the situation described in the first paragraph. Fixes #1563 Signed-off-by: Jussi Kukkonen --- tests/test_trusted_metadata_set.py | 27 +++++---- .../_internal/trusted_metadata_set.py | 56 ++++++++++++------- tuf/ngclient/updater.py | 4 +- 3 files changed, 54 insertions(+), 33 deletions(-) diff --git a/tests/test_trusted_metadata_set.py b/tests/test_trusted_metadata_set.py index 4f848d5f8c..b097e350a1 100644 --- a/tests/test_trusted_metadata_set.py +++ b/tests/test_trusted_metadata_set.py @@ -260,11 +260,12 @@ def test_update_timestamp_expired(self): def timestamp_expired_modifier(timestamp: Timestamp) -> None: timestamp.expires = datetime(1970, 1, 1) - # intermediate timestamp is allowed to be expired + # expired intermediate timestamp is loaded but raises timestamp = self.modify_metadata("timestamp", timestamp_expired_modifier) - self.trusted_set.update_timestamp(timestamp) + with self.assertRaises(exceptions.ExpiredMetadataError): + self.trusted_set.update_timestamp(timestamp) - # update snapshot to trigger final timestamp expiry check + # snapshot update does start but fails because timestamp is expired with self.assertRaises(exceptions.ExpiredMetadataError): self.trusted_set.update_snapshot(self.metadata["snapshot"]) @@ -293,10 +294,11 @@ def timestamp_version_modifier(timestamp: Timestamp) -> None: timestamp = self.modify_metadata("timestamp", timestamp_version_modifier) self.trusted_set.update_timestamp(timestamp) - #intermediate snapshot is allowed to not match meta version - self.trusted_set.update_snapshot(self.metadata["snapshot"]) + # if intermediate snapshot version is incorrect, load it but also raise + with self.assertRaises(exceptions.BadVersionNumberError): + self.trusted_set.update_snapshot(self.metadata["snapshot"]) - # final snapshot must match meta version + # targets update starts but fails if snapshot version does not match with self.assertRaises(exceptions.BadVersionNumberError): self.trusted_set.update_targets(self.metadata["targets"]) @@ -328,11 +330,12 @@ def test_update_snapshot_expired_new_snapshot(self): def snapshot_expired_modifier(snapshot: Snapshot) -> None: snapshot.expires = datetime(1970, 1, 1) - # intermediate snapshot is allowed to be expired + # expired intermediate snapshot is loaded but will raise snapshot = self.modify_metadata("snapshot", snapshot_expired_modifier) - self.trusted_set.update_snapshot(snapshot) + with self.assertRaises(exceptions.ExpiredMetadataError): + self.trusted_set.update_snapshot(snapshot) - # update targets to trigger final snapshot expiry check + # targets update does start but fails because snapshot is expired with self.assertRaises(exceptions.ExpiredMetadataError): self.trusted_set.update_targets(self.metadata["targets"]) @@ -348,8 +351,10 @@ def version_bump(snapshot: Snapshot) -> None: new_timestamp = self.modify_metadata("timestamp", meta_version_bump) self.trusted_set.update_timestamp(new_timestamp) - # load a "local" snapshot, then update to newer one: - self.trusted_set.update_snapshot(self.metadata["snapshot"]) + # load a "local" snapshot with mismatching version (loading happens but + # BadVersionNumberError is raised), then update to newer one: + with self.assertRaises(exceptions.BadVersionNumberError): + self.trusted_set.update_snapshot(self.metadata["snapshot"]) new_snapshot = self.modify_metadata("snapshot", version_bump) self.trusted_set.update_snapshot(new_snapshot) diff --git a/tuf/ngclient/_internal/trusted_metadata_set.py b/tuf/ngclient/_internal/trusted_metadata_set.py index be1b0b44ed..336c45c075 100644 --- a/tuf/ngclient/_internal/trusted_metadata_set.py +++ b/tuf/ngclient/_internal/trusted_metadata_set.py @@ -178,16 +178,20 @@ def update_root(self, data: bytes) -> None: def update_timestamp(self, data: bytes) -> None: """Verifies and loads 'data' as new timestamp metadata. - Note that an expired intermediate timestamp is considered valid so it - can be used for rollback checks on newer, final timestamp. Expiry is - only checked for the final timestamp in update_snapshot(). + Note that an intermediate timestamp is allowed to be expired: + TrustedMetadataSet will throw an ExpiredMetadataError in this case + but the intermediate timestamp will be loaded. This way a newer + timestamp can still be loaded (and the intermediate timestamp will + be used for rollback protection). Expired timestamp will prevent + loading snapshot metadata. Args: data: unverified new timestamp metadata as bytes Raises: - RepositoryError: Metadata failed to load or verify. The actual - error type and content will contain more details. + RepositoryError: Metadata failed to load or verify as final + timestamp. The actual error type and content will contain + more details. """ if self.snapshot is not None: raise RuntimeError("Cannot update timestamp after snapshot") @@ -237,21 +241,33 @@ def update_timestamp(self, data: bytes) -> None: self._trusted_set["timestamp"] = new_timestamp logger.debug("Updated timestamp") + # timestamp is loaded: raise if it is not valid _final_ timestamp + self._check_final_timestamp() + + def _check_final_timestamp(self) -> None: + """Raise if timestamp is expired""" + + assert self.timestamp is not None # nosec + if self.timestamp.signed.is_expired(self.reference_time): + raise exceptions.ExpiredMetadataError("timestamp.json is expired") + def update_snapshot(self, data: bytes) -> None: """Verifies and loads 'data' as new snapshot metadata. - Note that intermediate snapshot is considered valid even if it is - expired or the version does not match the timestamp meta version. This - means the intermediate snapshot can be used for rollback checks on - newer, final snapshot. Expiry and meta version are only checked for - the final snapshot in update_delegated_targets(). + Note that an intermediate snapshot is allowed to be expired and version + is allowed to not match timestamp meta version: TrustedMetadataSet will + throw an ExpiredMetadataError/BadVersionNumberError in these cases + but the intermediate snapshot will be loaded. This way a newer + snapshot can still be loaded (and the intermediate snapshot will + be used for rollback protection). Expired snapshot or snapshot that + does not match timestamp meta version will prevent loading targets. Args: data: unverified new snapshot metadata as bytes Raises: - RepositoryError: Metadata failed to load or verify. The actual - error type and content will contain more details. + RepositoryError: data failed to load or verify as final snapshot. + The actual error type and content will contain more details. """ if self.timestamp is None: @@ -260,10 +276,8 @@ def update_snapshot(self, data: bytes) -> None: raise RuntimeError("Cannot update snapshot after targets") logger.debug("Updating snapshot") - # Local timestamp was allowed to be expired to allow for rollback - # checks on new timestamp but now timestamp must not be expired - if self.timestamp.signed.is_expired(self.reference_time): - raise exceptions.ExpiredMetadataError("timestamp.json is expired") + # Snapshot cannot be loaded if final timestamp is expired + self._check_final_timestamp() meta = self.timestamp.signed.meta["snapshot.json"] @@ -314,8 +328,11 @@ def update_snapshot(self, data: bytes) -> None: self._trusted_set["snapshot"] = new_snapshot logger.debug("Updated snapshot") + # snapshot is loaded, but we raise if it's not valid _final_ snapshot + self._check_final_snapshot() + def _check_final_snapshot(self) -> None: - """Check snapshot expiry and version before targets is updated""" + """Raise if snapshot is expired or meta version does not match""" assert self.snapshot is not None # nosec assert self.timestamp is not None # nosec @@ -361,9 +378,8 @@ def update_delegated_targets( if self.snapshot is None: raise RuntimeError("Cannot load targets before snapshot") - # Local snapshot was allowed to be expired and to not match meta - # version to allow for rollback checks on new snapshot but now - # snapshot must not be expired and must match meta version + # Targets cannot be loaded if final snapshot is expired or its version + # does not match meta version in timestamp self._check_final_snapshot() delegator: Optional[Metadata] = self.get(delegator_name) diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py index a3c7189d75..318cb85f4b 100644 --- a/tuf/ngclient/updater.py +++ b/tuf/ngclient/updater.py @@ -322,7 +322,7 @@ def _load_timestamp(self) -> None: self._trusted_set.update_timestamp(data) except (OSError, exceptions.RepositoryError) as e: # Local timestamp does not exist or is invalid - logger.debug("Failed to load local timestamp %s", e) + logger.debug("Local timestamp not valid as final: %s", e) # Load from remote (whether local load succeeded or not) data = self._download_metadata( @@ -339,7 +339,7 @@ def _load_snapshot(self) -> None: logger.debug("Local snapshot is valid: not downloading new one") except (OSError, exceptions.RepositoryError) as e: # Local snapshot does not exist or is invalid: update from remote - logger.debug("Failed to load local snapshot %s", e) + logger.debug("Local snapshot not valid as final: %s", e) assert self._trusted_set.timestamp is not None # nosec metainfo = self._trusted_set.timestamp.signed.meta["snapshot.json"]