-
Notifications
You must be signed in to change notification settings - Fork 314
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
fix: apply security audit results #1196
Conversation
@@ -70,7 +70,7 @@ pub fn create_label_exp<H: Hasher>( | |||
// hash parents for all non 0 nodes | |||
let hash = if node > 0 { | |||
// prefetch previous node, which is always a parent | |||
let prev = &layer_labels[(node - 1) * NODE_SIZE..node * NODE_SIZE]; | |||
let prev = &exp_parents_data[(node - 1) * NODE_SIZE..node * NODE_SIZE]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not directly from the audit, but related to FPS-19. Review is needed if the data should be explicitly pulled from the parents, which the comment indicates, or if we can grab it from the layer_labels. They are both identical slices at this point, so it's a cosmetic update for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Reverted/removed
In particular, this PR consider FPS-14/15/16/17/18/19/20. Changes are applied for FPS-15/16/20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. My only question is whether you intend to address the last item from the SNARK audit:
net2, used inrust-fil-proofs, to be replaced withsocket2
assert_eq!( | ||
partition_challenges.checked_mul(partitions), | ||
Some(partition_challenges * partitions) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, it would be okay if this overflows because the result would be conservative and, if anything, lead to requirements being judged unsatisfied when they 'should' be satisfied. Of course this cannot happen for correct protocol values anyway, and I agree a check is helpful for development.
That said, if this 'error' were hit in reality, it would probably mean we need to revise the code. Technically, values which would overflow could be 'correct'. None of this is a practical issue. I'm just thinking through the significance of the assertion.
I don't recommend or request it, but we could 'technically' return true
if the assertion fails (even though this would undoubtedly be papering over a usage error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I'll leave in the assert as-is, but realize they should not trigger in normal circumstances and the downside is that we're calculating values more than needed. If we return true if the check fails, there's probably no reason to do it since I think even logging it would likely be lost/ignored.
No, as far as I can tell, we don't use that directly, but it's pulled in from some dependencies (that we don't manage). If you find that incorrect, let me know, maybe I'm not looking at it right. |
Got it. I don't have contrary information — just didn't see it in the diff and wondered why. Thanks for the explanation. |
In particular, this PR consider FPS-14/15/16/17/18/19/20. Changes are applied for FPS-15/16/20.