docknetwork / crypto

Rust crypto library for data privacy tools
Apache License 2.0
78 stars 21 forks source link

Make BBS and BBS+ parameters independent from underlying architecture #16

Closed yamdan closed 10 months ago

yamdan commented 10 months ago

Thank you for publishing such an impressive library!

I've observed that when generating BBS(+) parameters h_i, even with the same label and message_count, the resultant h_i values can differ based on the execution environment: 32-bit (e.g., wasm32) versus 64-bit. This discrepancy seems to stem from the usize type of the parameter i, which varies depending on the environment. To illustrate, in a 32-bit setting, h_1 might be generated as H("label : h_0001 0000 0000 0000"), while in a 64-bit context, it could be H("label : h_0001 0000 0000 0000 0000 0000 0000 0000"). This leads to distinct h_1 values for identical label and message_count.

In this PR, I suggest a straightforward remedy: casting i to u64 just before calculating h_i. I would greatly appreciate your review on this as this could introduce a breaking change for 32-bit environments.

lovesh commented 10 months ago

Excellent catch! Thank you for pointing this out.

Can you please change the cast to u32 rather than u64? This is to prevent breaking the downstream JS libs (using it through WASM). Also the max value of u32 is a reasonable upper bound on the number of generators.

We use this pattern (concatenating a counter with a fixed label) at a few more places in the code like this. Will be fixed.

yamdan commented 10 months ago

Yes, I have no objections to aligning with u32 instead of u64. I understand that this still introduce a breaking change for 64-bit users, but I assume it's more acceptable than the impact on JS/WASM users.

lovesh commented 10 months ago

Yes. Thanks for the change.

yamdan commented 10 months ago

@lovesh In this fix, we simply truncated the MSBs using as u32 for casting, which means that if a message_count greater than $2^{32}$ is provided, repetitions will occur in the generator sequence. Specifically, $hi$​ and $h{2^{32}+i}$​ would be the same, resulting in an invalid BBS(+) generator. However, a message_count greater than $2^{32}$ requires approximately 618GB of memory; so in most practical environments, the operation would stop at Vec::with_capacity(), making this a non-issue. Still, I thought it prudent to note this potential pitfall. If we fix this, I think we can redefine message_count parameter as u32 (which makes impacts ) or use .try_into() instead of as u32 (with appropriate error handling, including change new's output to Result<_, _> .

lovesh commented 10 months ago

@yamdan Yes. We will make this change along with some other changes to keep these architecture-independent. Thanks.