Check timestamp/snapshot contains snapshot/targets description #226

erickt · 2022-05-19T04:38:09Z

The timestamp.json states that the timestamp meta section MUST only contain a description of the snapshot.json file. However, updating the timestamp role does not describe when to perform this verification.

Similarly, the snapshot.json states that the snapshot meta section MUST contain a description of the targets.json file, which is also not described in updating the snapshot role.

This patch explicitly states that these checks should be performed, and that the metadata should be rejected if it is missing these entries.

The [timestamp.json] states that the timestamp `meta` section MUST only contain a description of the snapshot.json file. However, [updating the timestamp role] does not describe when to perform this verification. Similarly, the [snapshot.json] states that the snapshot `meta` section MUST contain a description of the targets.json file, which is also not described in [updating the snapshot role]. This patch explicitly states that these checks should be performed, and that the metadata should be rejected if it is missing these entries. [timestamp.json]: https://theupdateframework.github.io/specification/v1.0.30/#file-formats-timestamp [updating the timestamp role]: https://theupdateframework.github.io/specification/v1.0.30/#update-timestamp [snapshot.json]: https://theupdateframework.github.io/specification/v1.0.30/#file-formats-snapshot [updating the snapshot role]: https://theupdateframework.github.io/specification/v1.0.30/#update-snapshot

JustinCappos

Mostly good. Needs a few minor tweaks.

tuf-spec.md

JustinCappos · 2022-05-19T06:53:00Z

tuf-spec.md

+5. **Check for a rollback attack**.
+
+  1. The new snapshot metadata file MUST contain the description of the targets
+     metadata file.  If not, discard the new snapshot metadata file, abort the


Suggested change

metadata file. If not, discard the new snapshot metadata file, abort the

metadata files (the top-level targets.EXT and all delegated roles.) If not, discard the

new snapshot metadata file, abort the

Clarifying that this includes all targets files.

JustinCappos · 2022-05-19T06:56:59Z

tuf-spec.md

-  the update cycle, and report the failure.
+5. **Check for a rollback attack**.
+
+  1. The new snapshot metadata file MUST contain the description of the targets


Suggested change

1. The new snapshot metadata file MUST contain the description of the targets

1. The new snapshot metadata file MUST contain the version numbers of the targets

The listed item is the version numbers, not a description.

I assume that "description" was used as a short-hand for the version plus optional length and hashes (fields in a METAFILES object)? As this terminology is not used elsewhere we should certainly be more exploit here.

Perhaps we could say

The new snapshot metadata file's METAFILES object MUST list all targets metadata (top-level targets.EXT all all delegated roles) including their version numbers, optionally their length and optionally their hashes. If not, discard the new snapshot metadata file, abort the

etc.

I fleshed this out more, with links back to the definitions. How does it read?

joshuagl · 2022-05-19T15:09:51Z

tuf-spec.md

+  2. The new timestamp metadata file MUST only contain the description of the
+     snapshot metadata file.  If not, discard the new snapshot metadata file,
+     abort the cycle, and report the failure.


Just to be clear, here we're checking that timestamp's meta lists only snapshot?

Similar use of the new keyword "description" here, WDYT to changing that to "METAFILES object"?

(I've also changes snapshot -> timestamp in the "discard ..." statement)

Suggested change

2. The new timestamp metadata file MUST only contain the description of the

snapshot metadata file. If not, discard the new snapshot metadata file,

abort the cycle, and report the failure.

2. The new timestamp metadata file's `METAFILES` object MUST only contain the

snapshot metadata file. If not, discard the new timestamp metadata file,

abort the cycle, and report the failure.

I used the term description from https://theupdateframework.github.io/specification/latest/#file-formats-timestamp, but I agree it's a little vague. I like yours better.

joshuagl · 2022-05-19T15:17:46Z

tuf-spec.md

-  the update cycle, and report the failure.
+5. **Check for a rollback attack**.
+
+  1. The new snapshot metadata file MUST contain the description of the targets


I assume that "description" was used as a short-hand for the version plus optional length and hashes (fields in a METAFILES object)? As this terminology is not used elsewhere we should certainly be more exploit here.

erickt

I did another pass to add explicit instructions on how to handle validating the version, and optional length and optional hashes. How does it read?

erickt · 2022-05-19T16:22:35Z

tuf-spec.md

+  2. The new timestamp metadata file MUST only contain the description of the
+     snapshot metadata file.  If not, discard the new snapshot metadata file,
+     abort the cycle, and report the failure.


I used the term description from https://theupdateframework.github.io/specification/latest/#file-formats-timestamp, but I agree it's a little vague. I like yours better.

tuf-spec.md

erickt · 2022-05-19T19:02:05Z

tuf-spec.md

-  the update cycle, and report the failure.
+5. **Check for a rollback attack**.
+
+  1. The new snapshot metadata file MUST contain the description of the targets


I fleshed this out more, with links back to the definitions. How does it read?

erickt · 2022-05-20T18:03:41Z

I just read through README.rst, and it looks like I was supposed to submit this against the draft branch. However that branch hasn't been touched since 2019. Should I change this to merge into that branch?

JustinCappos

I'd like to understand better why we would discard the new snapshot / timestamp file if there is a mismatch.

JustinCappos · 2022-05-21T16:37:02Z

tuf-spec.md

+
+    1. The [=metapath/LENGTH=] in the new timestamp metadata file, if any, MUST
+      be equal to the [=metapath/LENGTH=] in the trusted timestamp file, if any.
+      If not, discard the new timestamp metadata file, abort the cycle, and


Why do we discard the new timestamp metadata? Could an attacker later roll back the timestamp file?

I'm not sure if it's necessarily resisting an attack, but more about enabling the optimization where we can skip downloading metadata if we already have it locally without putting our local trusted metadata in an inconsistent state (such as in #114 and #227).

It might be helpful if we work with an example. Say we have locally trusted metadata, with Timestamp T1 and Snapshot S1, which is at version N. The remote repository is at Timestamp T1 and Snapshot S2, which also is at version N.

When we perform a client update, we will:

Download T2

Verify it was signed correctly

Verify that T2's snapshot description's version >= the trusted snapshot S1's version

At this point we could decide that we we already have the trusted Snapshot S1, so we could abort the update workflow. However this would be incorrect, because it's possible that Snapshot S2 could have different content than Snapshot S1. If Timestamp T2 contains the optional length and/or hashes of Snapshot S2. This means that if we create a brand new client based off the local trusted metadata, we cannot use the length/hashes from Timestamp T2 to verify Snapshot S1. This would then force us to throw away the Snapshot S1 and download the new S2 version.

This patch tries to avoid this issue by enforcing that we cannot get into this state by treating Timestamp T2 as invalid, since it doesn't match what we already trust.

This would be especially handy if we implement transactional downloading of metadata in #69 to make sure the local metadata doesn't get into an incomplete state.

The remote repository is at Timestamp T1 and Snapshot S2, which also is at version N.

Did you mean T2? You do say the client downloads T2 below. I assume this is a typo here?

Just to make sure, do I understand correctly that in your example:

T1 describes S1

T2 describes S2

T2 has a higher version than T1

S2 has the same version as S1

S2 and S1 have different contents

This implies that the repo has incorrectly updated snapshot and timestamp, without bumping snapshot's version number, right?

Following you proposal, the client that already trusts T1 and S1 can only update, if the repo issues a new T2 (or higher) describing a new S2, with a version number higher than S1. Is that correct? If so, I think it's fair.

JustinCappos · 2022-05-21T16:37:12Z

tuf-spec.md

+
+    2. For each entry in the new timestamp metadata file's [=metapath/HASHES=]
+      dictionary, if the key is present in the trusted timestamp metadata file,
+      the values MUST be equal.  If not, discard the new timestamp metadata


Same comment as above here.

This was covered in the above comment, so lets track it there.

JustinCappos · 2022-05-21T16:38:00Z

tuf-spec.md

+  1. The new snapshot metadata file's [=METAFILES=] object MUST contain a
+    [=snapshot/METAPATH=] entry for the targets metadata file, and all delegated targets
+    metadata files, if any, in the trusted snapshot metadata file.  If not,
+    discard the new snapshot metadata file, abort the cycle, and report the


Do we need to discard the snapshot file in this case? Why?

This was covered in the above comment, so lets track it there.

lukpueh · 2022-05-24T12:12:38Z

I just read through README.rst, and it looks like I was supposed to submit this against the draft branch.

The README says:

For patch-type changes, pull requests may be submitted directly against the 'master' branch."

So this should be fine.

lukpueh · 2022-05-24T12:15:57Z

However that branch hasn't been touched since 2019.

See #228 for an update and #229 for a request to prevent a stale draft branch in the future.

erickt changed the title ~~Check timestamp/snapshot contains snapshot/targets version~~ Check timestamp/snapshot contains snapshot/targets description May 19, 2022

JustinCappos requested changes May 19, 2022

View reviewed changes

joshuagl requested changes May 19, 2022

View reviewed changes

Flesh out timestamp/snapshot rollback attack resistance

440e099

erickt commented May 19, 2022

View reviewed changes

Bump date and version

cc41726

erickt requested review from joshuagl and JustinCappos May 20, 2022 18:03

JustinCappos reviewed May 21, 2022

View reviewed changes

erickt mentioned this pull request May 23, 2022

Do we always need to download snapshot and targets? #227

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check timestamp/snapshot contains snapshot/targets description #226

Check timestamp/snapshot contains snapshot/targets description #226

erickt commented May 19, 2022

JustinCappos left a comment

JustinCappos May 19, 2022

JustinCappos May 19, 2022

joshuagl May 19, 2022

joshuagl May 19, 2022 •

edited

Loading

erickt May 19, 2022

joshuagl May 19, 2022 •

edited

Loading

erickt May 19, 2022

joshuagl May 19, 2022

erickt left a comment

erickt May 19, 2022

erickt May 19, 2022

erickt commented May 20, 2022

JustinCappos left a comment

JustinCappos May 21, 2022

erickt May 23, 2022

lukpueh Jun 2, 2022

JustinCappos May 21, 2022

erickt May 23, 2022

JustinCappos May 21, 2022

erickt May 23, 2022

lukpueh commented May 24, 2022

lukpueh commented May 24, 2022

	metadata file. If not, discard the new snapshot metadata file, abort the
	metadata files (the top-level targets.EXT and all delegated roles.) If not, discard the
	new snapshot metadata file, abort the

	1. The new snapshot metadata file MUST contain the description of the targets
	1. The new snapshot metadata file MUST contain the version numbers of the targets

Check timestamp/snapshot contains snapshot/targets description #226

Are you sure you want to change the base?

Check timestamp/snapshot contains snapshot/targets description #226

Conversation

erickt commented May 19, 2022

JustinCappos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuagl May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuagl May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erickt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erickt commented May 20, 2022

JustinCappos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented May 24, 2022

lukpueh commented May 24, 2022

joshuagl May 19, 2022 •

edited

Loading

joshuagl May 19, 2022 •

edited

Loading