crypto: Variable-time implementation of VRF verify #3925
Conversation
Current implementation is constant-time.
Constant-time is most likely not needed for most cases, as VRF inputs
are usually public.
There is quite a lot of copy-pasting in ed25519_ref10.c.
I usually strongly advocate against copy-pasting
but unfortunately the libsodium code seems to itself contain a lot of
copy-pasting so I just kept it as is.
But it may make sense to introduce functions.
There is unit testing at 2 levels:
1. In Go, in `crypto/vrf_test.go` but it is quite limited
2. In libsodium itself, which I am not sure is run by CI/CD
but this can be run manually:
in `crypto/libsodium-fork`, run:
```
./autogen.sh
./configure --disable-shared
make
cd test/default
make vrf.log
```
(or just `make check`)
Note that the `make` in top-level of `go-algorand` copies the full
source tree of `libsodium-fork` inside
`crypto/copies/darwin/amd64/libsodium-fork`
(on macOS)
Regarding benchmarking, on my computer (single run), I get:
```
$ cd crypto
$ go test -bench '.*Verify.*' .
slice *crypto.OneTimeSignatureSecrets/OneTimeSignatureSecretsPersistent/Offsets have an unbounded allocbound defined
slice *crypto.OneTimeSignatureSecrets/OneTimeSignatureSecretsPersistent/Batches have an unbounded allocbound defined
slice *crypto.OneTimeSignatureSecretsPersistent/Offsets have an unbounded allocbound defined
slice *crypto.OneTimeSignatureSecretsPersistent/Batches have an unbounded allocbound defined
WARN[0004] tried to sign {[29 218 75 201 184 250 48 100 132 68 111 121 174 181 244 52 145 176 162 72]} with out-of-range one-time identifier {199 147} (firstbatch 201, len(batches) 799, firstoffset 147, len(offsets) 109) file=onetimesig.go function="github.com/algorand/go-algorand/crypto.(*OneTimeSignatureSecrets).Sign" line=302
goos: darwin
goarch: amd64
pkg: github.com/algorand/go-algorand/crypto
cpu: Intel(R) Core(TM) i7-8559U CPU @ 2.70GHz
BenchmarkSignVerify-8 14564 81885 ns/op
BenchmarkVerify-8 18763 64014 ns/op
BenchmarkVrfVerify-8 5508 215581 ns/op
BenchmarkVrfVerifyVarTime-8 7802 152587 ns/op
PASS
ok github.com/algorand/go-algorand/crypto 13.787s
```
So essentially VRF verify is 2.5x slower than ed25519 verify
which is not too surprising as it requires two double scalar multiplications
(one very similar to ed25519 verify, with one of the element being the
pre-computed base; and one slightly slower where it is not the case).
This is about 30% faster than the current implementation.
cce
changed the title
crypto/libsodium-fork/src/libsodium/crypto_core/ed25519/ref10/ed25519_ref10.c
Show resolved
Hide resolved
| // we use add instead of sub | ||
| ge25519_add(&t, &u, &Ai[(-aslide[i]) / 2]); | ||
| } | ||
|
|
There was a problem hiding this comment.
what you propose sounds logically sound, in that you have replaced the 25519 arithmetic with the additive inverse in the area of code that deals with aslide in particular, to achieve the -a result. caveat, I can't render an opinion on the libsodium implementation as a whole.
| and b = b[0]+256*b[1]+...+256^31 b[31]. | ||
| where contrary to | ||
| ge25519_double_sub_scalarmult_vartime | ||
| B is not necessarily the base |
There was a problem hiding this comment.
in the prior function, B came from a header file, either fe_51/base2.h or fe_25_5/base2.h, here is it calculated in the same style as A
| } | ||
|
|
||
| /* Same as _vrf_ietfdraft03_hash_points but for ge25519_p2 points instead | ||
| * for the last two points */ |
There was a problem hiding this comment.
this function changes the functional signature from p3 to p2 and replaces point_to_string with the relevant new call to point_p2_to_string, otherwise it's as above
| ge25519_frombytes(&H_point, h_string); | ||
|
|
||
| /* calculate U = s*B - c*Y = -c*Y + s*B */ | ||
| ge25519_double_sub_scalarmult_vartime(&U_point, c_scalar, Y_point, s_scalar); |
|
Really cool! Also, kind of a mystery. I guess my only concern is where this logic is located. From an implementation perspective there is a lot of cut and paste from above so colocation makes sense, but the reference nature of the library makes it not so great. |
|
I'm not sure what is the best option. We need to access to static C function that are only available in this file like |
|
A standard node would run this function once per round? and maybe whenever the VRF opcode is used? |
|
You need to run it for every block proposals and every vote. The "worst case" is when you have as many votes as seats in the committees (that is the stake is distributed relatively uniformly over a few thousands participation nodes). In that case, assuming we only need 1 period and 2 steps like we do right now almost all the time (e.g., if the block proposer is honest and the network is not partitioned), you would need to run VRF about 10+2267+1112 per round (I'm assuming the code stop evaluating votes when threshold has passed and that you saw on average 10 block proposals before finding the smallest one. If you have malicious participation nodes, they may force using more periods/steps, they may also submit a lot of incorrect VRF values, which may need to be verified. On top of that, there are all the opcode vrf_verify calls. |
|
I feel like this PR is in a bind because the person most qualified to review it also wrote it. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3925 +/- ##
==========================================
+ Coverage 49.96% 49.98% +0.02%
==========================================
Files 400 400
Lines 68633 68640 +7
==========================================
+ Hits 34292 34311 +19
+ Misses 30636 30632 -4
+ Partials 3705 3697 -8 ☔ View full report in Codecov by Sentry. |
3 participants
Summary
Current implementation is constant-time.
Constant-time is most likely not needed for most cases, as VRF inputs
are usually public.
There is quite a lot of copy-pasting in ed25519_ref10.c.
I usually strongly advocate against copy-pasting
but unfortunately the libsodium code seems to itself contain a lot of
copy-pasting so I just kept it as is.
But it may make sense to introduce functions.
Importantly: Note that this code has not been audited and the effect
of using non-constant-time VRF verification has not been studied in details.
Test Plan
There is unit testing at 2 levels:
crypto/vrf_test.gobut it is quite limitedbut this can be run manually:
in
crypto/libsodium-fork, run:(or just
make check)Note that the
makein top-level ofgo-algorandcopies the fullsource tree of
libsodium-forkinsidecrypto/copies/darwin/amd64/libsodium-fork(on macOS)
Regarding benchmarking, on my computer (single run), I get:
So essentially VRF verify is 2.5x slower than ed25519 verify
which is not too surprising as it requires two double scalar multiplications
(one very similar to ed25519 verify, with one of the element being the
pre-computed base; and one slightly slower where it is not the case).
This is about 30% faster than the current implementation.