[sdk] Add a length restriction on poseidon hash input

[samkim-crypto]

Problem

The newly added poseidon hash syscall (#32680) performs a length check on the number of slice inputs to the function (here) but not on the length of the slice. The assigned compute units for the hash function is also independent of the length of the slice inputs.

Consider invoking the poseidon hash syscall on a very long input. If the function is invoked in the LittleEndian setting, then this is fine since the slices will immediately fail to be converted to a valid field element (here).

However, if the function is invoked in the BigEndian setting, then the bytes are reversed (here) before they are converted into a field element, which takes O(n) time. This creates a risk for potential ddos since the compute units do not account for this linear time operation.

Summary of Changes

Added a length check on the input slices before the hash function is invoked.

Fixes #

[codecov]

Codecov Report

Merging #33363 (b84d6af) into master (997aa0a) will increase coverage by 0.0%.
The diff coverage is 50.0%.

@@           Coverage Diff           @@
##           master   #33363   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         798      798           
  Lines      216579   216581    +2     
=======================================
+ Hits       177481   177513   +32     
+ Misses      39098    39068   -30     

[samkim-crypto]

@vadorovsky, can you double check if my reasoning is correct here? Thanks!

[vadorovsky]

I think it's an unnecessary change? Do you have an actual reproducer which hijacks the syscall (some input which is too large and is considered as valid)?

light-poseidon already has a necessary check for each input, which also instead of hard coding a specific value (like 32), it counts the byte limit for the modulus of the prime field, so it is able to handle other elliptic curves or prime fields without having to hard code any new values:

https://github.com/Lightprotocol/light-poseidon/blob/4cee12395fb04ad5da888c21ca85dc9a30dac684/light-poseidon/src/lib.rs#L451-L481

We even have tests with too large inputs:

https://github.com/Lightprotocol/light-poseidon/blob/4cee12395fb04ad5da888c21ca85dc9a30dac684/light-poseidon/tests/bn254_fq_x5.rs#L233-L308

In case you have any reproducer, then I think we should rather fix light-poseidon - the library itself should be secure and handle all the checks, instead offloading them to the callers.

The only reason why I did an explicit check on the number of slice inputs in Solana is to be able to calculate the compute cost.

[samkim-crypto]

This is not an issue about whether the bytes are correctly parsed as valid field elements or not, but whether the assigned compute units correctly capture the compute costs to evaluate the hash function.

Correct me if I am misunderstanding the flow of the code, but if we evaluate the hash function with BigEndian here, this will invoke hash_bytes_be(vals). The hash_bytes_be(vals) will first reverse the bytes before checking whether the bytes are valid. The reverse operation is a linear time operation: the cost will grow with the size of the input bytes.

So imagine a really long input like in your test. The computation could be correct in that it returns PoseidonError::InputLargerThanModulus, but if it takes a really long time (due to the reverse operation) to output PoseidonError::InputLargerThanModulus, then this would be a problem.

The bench code that was used to assign CU's fixes the inputs at 32 bytes. We should make sure that the CU's are still accurate for inputs that are much larger than 32 bytes.

Let me know what you think. It could be that I am missing something, but we really want to be sure for these type of things that could potentially lead to ddos.

[vadorovsky]

@samkim-crypto OK, I understand now. Thanks for clear explanation!
This PR looks good to me, thanks for spotting the issue! I think it makes sense to merge this PR.

At the same time, I submitted a fix: Lightprotocol/light-poseidon#32, which performs the validation before the big-endian array is inverted. In the long-term, we might also want to bump light-poseidon once the fix lands there too and we make a new release.

[vadorovsky]

@samkim-crypto #33923 is merged, should we close this PR?

[samkim-crypto]

Yep, closing. Thanks!