Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fast-forward recovery and rollback prevention #85

Closed
wants to merge 4 commits into from

Conversation

mnm678
Copy link
Collaborator

@mnm678 mnm678 commented Jan 22, 2020

Addresses part of #71

Clarify how and when to delete targets, snapshot, and timestamp metadata to recover from a fast-forward attack. In addition, adds an additional rollback check to prevent a rollback attack in the event of a timestamp compromise.

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, @mnm678? This PR does most of what we agreed on in #71 (comment) ff, i.e. reverting #65, and clarifying, which metadata needs to be deleted/untrusted when, in order to recover from fast forward attack. Please address my inline comments and we can merge this. (unless someone else has reservations, cc @JustinCappos, @trishankatdatadog)

We should also update/review/merge @erickt's #72, which clarifies that version increment checks must also be performed on delegated targets metadata.

tuf-spec.md Outdated
* **1.9**. **Fast-forward attack recovery** A _fast-forward attack_ happens
when attackers arbitrarily increase the version numbers of: (1) the timestamp
metadata, (2) the snapshot metadata, and / or (3) the targets, or a delegated
targets, metadata file in the snapshot metadata. To recover from fast-forward
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to remove "in the snapshot metadata" or clarify. Because attackers can also perform ffwd attacks, if they increase the version number of snapshot in timestamp, or if they increase a version number on any metadata file directly.

@mnm678
Copy link
Collaborator Author

mnm678 commented Jan 23, 2020

@lukpueh Thanks for the review! I addressed your comments.

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @mnm678!

tuf-spec.md Outdated
for more details.

* **1.9.1**. **Targets recovery** If a threshold of targets keys are removed
from the root metadata, delete the old targets, snapshot, and timestamp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fairly clear, but you could make it clearer that you mean the top-level targets file, and not any delegated targets file?

tuf-spec.md Outdated
lower than the expiration timestamp in the new targets metadata file. If so,
the new targets metadata file becomes the trusted targets metadata file. If
the new targets metadata file is expired, discard it, abort the update cycle,
and report the potential freeze attack.

* **4.4**. **Perform a preorder depth-first search for metadata about the
* **4.5**. **Fast-forward attack recovery** If a threshold of delegated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good check, but we can't do this before the subsequent preorder DFS, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, this check should happen during the DFS, maybe as 4.6.2.1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch indeed, @trishankatdatadog. Do you guys think it is clear that the "delegating targets metadata" (as now mentioned in 4.5.2.1) can also be the top-level targets metadata? Or should we add a clarification?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always better to clarify, in my humble experience!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it's better to be clear. I'll add a note

Clarify what files to delete in various situations to recover from
a fast-forward attack.

In addition, this adds additional rollback protection in the event
of a timestamp compromise.
@lukpueh
Copy link
Member

lukpueh commented Jan 24, 2020

@mnm678, I took the liberty to untangle the commit history a bit. It seems like there are a bunch of duplicate commits from this PR and also from prior PRs due to a merge.

So I undid your merge by hard resetting to one of the merge commits' parents (i.e. db9b54b), and cherry-picked the commit that was missing (i.e. c2aa515) onto it. I also did some interactive rebasing to clean up the commit messages and squash in the one UI commit.

You can replace your branch with my cleaned up version with these commands:

# Fetch my version of your branch
git fetch [email protected]:lukpueh/specification.git fast_forward-lp:fast_forward-lp

# Go to your branch (if you're not already on it)
git checkout fast_forward

# Optionally diff it with mine to see that it's indeed the same contents
git diff fast_forward-lp

# Replace yours with mine
git reset --hard fast_forward-lp

# Force push to update the branch on GitHub
# (you might have to change "origin" to whatever points to [email protected]:mnm678/specification.git)
git push origin fast_foward -f

tuf-spec.md Outdated
* **4.5.2.1**. **Fast-forward attack recovery** If a threshold of
delegated targets keys for the current delegation are removed from the
delegating targets metadata, delete the old delegated targets metadata for
the current delegation along with the snapshot and timestamp metadata.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the snapshot and timestamp again here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there was a fast forward attack, the snapshot would contain the fast-forwarded version number of the delegated targets file

@lukpueh
Copy link
Member

lukpueh commented Jan 30, 2020

Superseded by #86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants