ngclient: improve rollback protection

[jku]

I believe ngclient implements the spec as its written... but fails to follow the spirit of the spec in one aspect: as the spec does not explicitly say that expired snapshot and timestamp can be used to do the rollback checks of new snapshot and timestamp (while it does explicitly allow this for root), ngclient does not use expired snapshot and timestamp and instead considers those expired versions invalid. This means a rollback attack (giving the client an older version of the metadata file) is easier as long as the local metadata versions have had time to expire.

Spec (client workflow) could be improved:

• specify when local cached metadata should be used to maximize the effectiveness of the rollback checks -- currently spec talks about "trusted timestamp" and "trusted snapshot" but never explains how they become trusted or when they're loaded
• specify when using expired metadata is allowed
  • root: when used to do rollback checks for newer root (this is already in spec)
  • timestamp: when used to do rollback checks for newer timestamp
  • snapshot: when used to do rollback checks for newer snapshot
• specify when using a snapshot version that does not match the timestamp meta version is allowed: when used to do rollback checks for newer snapshot

I believe it makes sense to work on the ngclient improvements at the same time. I think this should do it:

• for all of [root, timestamp, snapshot], do all validity checks in TrustedMetadataSet.update_*() as now, except do not check expiry: this allows updating TrustedMetadataSet first with a local (possibly expired) file and then updating with a new downloaded one
• instead check the expiry when the next top-level type is updated (this is safe because of the strong checks we do: TrustedMetadataSet guarantees that top-level updates can only be done in strict order):
  • when timestamp is updated, first check root expiry
  • when snapshot is updated, first check timestamp expiry
  • when targets is updated, first check snapshot expiry and meta version
• deprecate root_update_finished() as the expiry check is now done consistently as described above

[mnm678]

This sounds good with the caveat that expired timestamp or snapshot should not be used to check for rollback attacks if the snapshot or timestamp keys have been replaced (for fast forward attack recovery).

The spec changes might fit as part of theupdateframework/specification#150, or maybe as a separate pr once we finalize that one.

[jku]

This sounds good with the caveat that expired timestamp or snapshot should not be used to check for rollback attacks if the snapshot or timestamp keys have been replaced (for fast forward attack recovery).

Yes, as I said all validity checks should be done except expiry. If metadata is not signed correctly it does not get used for anything.

[jku]

There's one more rollback case we handle worse than we should:

• timestamp gets updated and now contains snapshot version N+1
• now local snapshot version N < N+1: TrustedMetadataSet considers that untrusted (IMO correctly since spec does not explicitly say the wrong version can be trusted) and does not load it
• when we update new snapshot, the rollback checks in 5.5.5 are not done because there is no trusted snapshot to compare to

I think the solution here is the same as with expiry: consider the specs intention that old snapshot can be used for rollback checks, but not for anything else. In practice for ngclient:

• snapshot is considered initially verified/trusted even if version < meta version (to allow it to be used in 5.5.5 to verify a newer snapshot)
• when we have a final snapshot (in practice when targets is first updated), check that snapshot version == meta version

[jku]

• now local snapshot version N < N+1: TrustedMetadataSet considers that untrusted (IMO correctly since spec does not explicitly say the wrong version can be trusted) and does not load it

If hashes/length are included for metadata, then we can't check those either for the purpose of rollback checks -- we can check for them once we have the "final" snapshot but not before.

EDIT: this is actually a slightly (but for ngclient meaningfully) different case: hashes/length check is a security measure to prevent parsing data we don't trust, while the various rollback cases are for data that is "correct" (signed). For these signed cases there is effectively no difference between data downloaded from the network and data loaded from local disk but in the hashes/length case the difference is real... I'll file a different issue for this: #1523