Fix overflow bug in Decaf Elligator code #54
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
pinkforest
pushed a commit
to pinkforest/curve25519-dalek
that referenced
this pull request
Jun 27, 2023
…25519-dalek-1.0.0-pre.0 Update curve25519-dalek dependency to 1.0.0-pre.0.
pinkforest
pushed a commit
to pinkforest/curve25519-dalek
that referenced
this pull request
Jun 27, 2023
README.md: Add "See also" section with link to `crypto_box` crate
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The implementation of Elligator-flavoured Decaf I wrote had an overflow bug, caused by a too-long sequence of additions of
FieldElements. This is a problem for the default 32-bit implementation, but not for the 64-bitradix_51implementation, which I was using when I wrote the code.Because Rust traps overflows in debug builds, this bug was caught by the unit test. However, it would be good to try to prevent the same mistake in the future.
Static checking
I think it's possible to do this in a hacky way now, but it would be extremely ugly and coarse, creating a new type name for each interval.
Const generics, as in Rust RFC 2000, would allow clean but coarse-grained (e.g., "number of bits of headroom) static analysis. I think waiting for this is the best option.
Pi types 2, as in Rust RFC 1932, would allow clean and fine-grained (exact bounds) static analysis. Unfortunately, it's postponed indefinitely in favour of RFC 2000.
Some other notes on static analysis are in this comment; I am not sure whether that could be implemented with RFC 2000, because it might require unification of types that wouldn't be unified in that proposal. On unification, see also this comment.
Dynamic checking
Rust traps overflows in debug builds, allowing dynamic checking/fuzzing, which caught this bug. The 64-bit
radix_51implementation, which is written from scratch, includes manydebug_assert!s to detect bounds failures. Of course, in release builds, all of this code is omitted to produce constant-time branch-free code.The 32-bit implementation, which was ported from ed25519.go (which was ported from ref10), is considerably less clean, and has far fewer checks. I think it would be good to rewrite it to match the quality level of the 64-bit code.
We already do dynamic checking here, which does 1,000 scalar multiplications (causing about 1M each of field multiplications and squarings).
The initial test for Decaf-Elligator used only 100 samples, which wasn't sufficient to reliably detect the overflow, which I think is why it wasn't detected by Travis CI.
@Manishearth suggested using cargo-fuzz; maybe it would be better to move dynamic checking into a fuzzing module, so that it can be run seperately from the unit tests?