Add explicit drop of `toxic_waste` once SRS is generated

[0xAdriaTorralba]

Optimization changes to kzg.rs

Description

The aim of this PR is to improve the kzg.rs file.

In particular, I have added two optimizations:

1. Added MAX_POLYNOMIAL_DEGREE as a constant to determine the maximum degree of polynomial that is allowed to commit.
2. Added explicit drop of memory of the randomly generated toxic_waste once the SRS is generated. Although Rust manages the memory (and in particular, the drops of unused variables) I find particularly important to handle the deletion of this variable explicitly and manually. Since, it is CRUCIAL for the soundness of the protocol for this variable to be DELETED right after its job is done.

Type of change

• [x] Optimization

Checklist

• [ ] Linked to Github Issue
• [x] Unit tests added
• [ ] This change requires new documentation.
  • [ ] Documentation has been added/updated.
• [x] This change is an Optimization
  • [ ] Benchmarks added/run

[Oppen]

IIUC, the goal of this PR is to remove secrets from memory as soon as they stop being needed, right? Can you confirm this?

If it is, I'm afraid mem::drop doesn't do what we need, since drop doesn't clean the values, it just releases the resources and makes the address inaccessible in safe Rust, which an attacker would not restrict to. You would need an implementation of Drop that memsets to 0 (or some poison value) so the value does not linger. There are some crates that do that for you IIRC. But regardless of that, it seems the changes are just to a test routine that only gets built for tests, it sounds unlikely that there is any meaningful secrets there. The code certainly does not seem wrong and the extra tests are appreciated, but it doesn't seem as critical as the description implies. Lastly, the change is labeled as an optimization, but it doesn't seem like one (other than slightly decreasing the lifetime of one value on the stack) and no benchmarks are provided. I would remove that claim unless there is evidence that it improves performance of some important operation.

[0xAdriaTorralba]

I know that this routine is intended only for test (and learning) pruposes. However, any other person (just like me) could be interested on learning more about this protocol, and I find that deleting the toxic waste explicitly after its usage will help and reinforce the importance of this asset being properly handled and disposed.

OK, that makes sense. If this is meant to be an example of usage, then it should follow the correct usage.

Okay, what label do you find more suitable for this PR?

I would say a fix or doc change (in the sense of intent), but I'm not sure the checklist covers those cases. I would just leave it untagged.

[Oppen]

WTF did I touch? I meant to quote you, not edit your comment. Sorry!

[0xAdriaTorralba]

WTF did I touch? I meant to quote you, not edit your comment. Sorry!

Don't worry, I'm going to try to recreate the original message.

---

IIUC, the goal of this PR is to remove secrets from memory as soon as they stop being needed, right? Can you confirm this?

Yes, this is the purpose of this PR.

If it is, I'm afraid mem::drop doesn't do what we need, since drop doesn't clean the values, it just releases the resources and makes the address inaccessible in safe Rust, which an attacker would not restrict to. You would need an implementation of Drop that memsets to 0 (or some poison value) so the value does not linger. There are some crates that do that for you IIRC.

I didn't know about this. I'll take the opportunity to submit a modification taking this into account.

But regardless of that, it seems the changes are just to a test routine that only gets built for tests, it sounds unlikely that there are any meaningful secrets there.

I know that this only affects the test routine and that it should not store any meaningful secret. However, I found this code very self-explanatory and very suitable to have a hands-on experience with KZG commitments. For this reason, I find it important to explicitly and manually remove the toxic waste once it's used, to exemplify good behaviour when implementing a production-ready KZG commitment.

The code certainly does not seem wrong and the extra tests are appreciated, but it doesn't seem as critical as the description implies.

Regarding the extra tests, that's great. On the other hand, I know that this is not a critical affair right now, I'm sorry if it sounded too alarmist.

Lastly, the change is labeled as an optimization, but it doesn't seem like one (other than slightly decreasing the lifetime of one value on the stack) and no benchmarks are provided. I would remove that claim unless there is evidence that it improves the performance of some important operation.

I apologize for that, I did not find any other label in which this would be better fitted. What do you propose?