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

clarify bins and bin-n in the table #34

Closed
wants to merge 2 commits into from

Conversation

mnm678
Copy link
Collaborator

@mnm678 mnm678 commented Oct 16, 2019

Fixes #26
Clarifies what happens if bins or bin-n keys are compromised. Please double check for correctness.

Copy link
Collaborator

@trishankatdatadog trishankatdatadog left a comment

Choose a reason for hiding this comment

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

I think the analysis for the case where timestamp, snapshot, and bin-n has been compromised is inaccurate

Freeze and metadata inconsistency attacks are limited by earliest root, targets, and bins (but NOT bin-n) metadata expiration time

@@ -842,9 +843,9 @@ attack, or metadata inconsistency attacks.
+-----------------+-------------------+----------------+--------------------------------+
Copy link
Member

Choose a reason for hiding this comment

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

This should be "targets OR bins OR bin-n" or "targets OR any delegated targets".

Alternatively, if we expect the reader to understand the delegation hierarchy, i.e. that compromising targets allows to compromise bins allows to compromise bin-n, then it might be enough to only mention bin-n, which is what the attacker is after. It seems to me that that's the assumption in the row below.

If we go with one of my suggestions from the top, I'd also change the row below to mention targets, bins and bin-n, and not just bin-n, for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between "NO" and "NOT APPLICABLE" in this context?

Copy link
Member

Choose a reason for hiding this comment

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

If we go with one of my suggestions from the top, I'd also change the row below to mention targets, bins and bin-n, and not just bin-n, for consistency.

I take that back. Only mentioning "bin-n" makes a difference in regards to Freeze Attack and Metadata Inconsistency Attack. As @trishankatdatadog says above targets and bins expiration times can stop them. And shouldn't they also stop Malicious updates?

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'll update to targets or bins or bin-n.

The NO vs NOT APPLICABLE was in the original text. I think they mean the same thing so will change them all to NO for consistency.

@lukpueh
Copy link
Member

lukpueh commented Oct 17, 2019

Freeze and metadata inconsistency attacks are limited by earliest root, targets, and bins (but NOT bin-n) metadata expiration time

I agree with @trishankatdatadog, if bin-n is compromised the attacker has control over their expiration time. Well spotted!

| snapshot | earliest root, | earliest root, | targets, or bins metadata |
| **AND** | targets, or | targets, or | expiry time |
| bin-n | bins expiry | bins expiry | |
| | time | time | |
Copy link

@JustinCappos JustinCappos Oct 17, 2019

Choose a reason for hiding this comment

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

Should we also have a place for bins / targets + snapshot + timestamp compromise? Isn't this missing?

@mnm678
Copy link
Collaborator Author

mnm678 commented Oct 17, 2019

Thank you for all of the feedback. I incorporated some of it in the new table for #43. However, much of the work here assumes different keys for snapshot, timestamp, and bin-n, so I am closing this in favor of #43.

@mnm678 mnm678 closed this Oct 17, 2019
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.

4 participants