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.
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.
^ Reverted/removed
In particular, this PR consider FPS-14/15/16/17/18/19/20. Changes are applied for FPS-15/16/20.
| assert_eq!( | ||
| partition_challenges.checked_mul(partitions), | ||
| Some(partition_challenges * partitions) | ||
| ); |
There was a problem hiding this comment.
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.
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.