Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix overflow bug in Decaf Elligator code #54

Conversation

hdevalence
Copy link
Contributor

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-bit radix_51 implementation, 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_51 implementation, which is written from scratch, includes many debug_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?

@isislovecruft isislovecruft merged commit 714bf3d into dalek-cryptography:develop May 19, 2017
@Manishearth Manishearth mentioned this pull request May 19, 2017
@hdevalence hdevalence deleted the bugfix/overflow_decaf_elligator branch February 8, 2018 21:08
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants