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

GCC 14 makes ed25519 not constant-time on x86_64 #4393

Open
mmilata opened this issue Nov 27, 2024 · 1 comment
Open

GCC 14 makes ed25519 not constant-time on x86_64 #4393

mmilata opened this issue Nov 27, 2024 · 1 comment
Labels
ci Continuous Integration (CI) related code Code improvements crypto Stand-alone cryptography library used by both Trezor Core and the Trezor Legacy firmware R&D Research and development team related

Comments

@mmilata
Copy link
Member

mmilata commented Nov 27, 2024

With GCC 14 crypto/tests/test_check under valgrind fails with a bunch of errors like this one:

==35484== Conditional jump or move depends on uninitialised value(s)
==35484==    at 0x475122: ge25519_cmove_stride4b (ed25519-donna-impl-base.c:420)
==35484==    by 0x475122: ge25519_move_conditional_pniels_array (ed25519-donna-impl-base.c:436)
==35484==    by 0x475122: ge25519_scalarmult (ed25519-donna-impl-base.c:489)
==35484==    by 0x476652: ed25519_sign_ext (ed25519.c:121)
==35484==    by 0x4767C2: ed25519_sign (ed25519.c:151)
==35484==    by 0x416276: test_ed25519_fn (test_check.c:7821)
==35484==    by 0x4864530: srunner_run_tagged (in /nix/store/qfqvxjl9fvssn2ps4mh4r5lmfjkxkss6-check-0.15.2/lib/libcheck.so.0.0.0)
==35484==    by 0x40122B: main (test_check.c:11828)

The value is not actually uninitialised, it's a result of a trick we use to check if control flow doesn't depend on the secret key value. Indeed GCC 12 compiles the ge25519_cmove_stride4b to use cmovne while GCC 14 emits je: https://gist.github.com/mmilata/bcacaee840abff5a2ad3e81e838002e7

I assume we don't really care about constant-time signing on x86_64 and we can live with GCC 12 for some more time (probably a good idea to use same version as arm-gcc-embedded which is currently 12). Mostly wanted to document this problem for future reference. Might be a good idea to look at the STM32 ARM machine code too.

See also: #4146

@mmilata mmilata added the code Code improvements label Nov 27, 2024
@mmilata mmilata added crypto Stand-alone cryptography library used by both Trezor Core and the Trezor Legacy firmware ci Continuous Integration (CI) related R&D Research and development team related labels Nov 27, 2024
@matejcik
Copy link
Contributor

cc @onvej-sl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration (CI) related code Code improvements crypto Stand-alone cryptography library used by both Trezor Core and the Trezor Legacy firmware R&D Research and development team related
Projects
Status: No status
Development

No branches or pull requests

2 participants