Several calls to `fe25519_abs` trigger `memcpy` undefined behavior
niooss-ledger opened this issue · 0 comments
Hello,
In current git master
, several functions are calling function fe25519_abs
with twice the same pointer. For example:
ristretto255_sqrt_ratio_m1
:ristretto255_frombytes
:ristretto255_p3_tobytes
:ristretto255_elligator
:
This call as the consequence of calling memcpy
with the same pointer too:
-
fe25519_abs
callsfe25519_cneg
with the same pointers:libsodium/src/libsodium/crypto_core/ed25519/ref10/ed25519_ref10.c
Lines 185 to 189 in 7978205
-
fe25519_cneg
callsfe25519_copy
with the same pointers:libsodium/src/libsodium/crypto_core/ed25519/ref10/ed25519_ref10.c
Lines 175 to 183 in 7978205
-
And
fe25519_copy
is a wrapper aroundmemcpy
:libsodium/src/libsodium/include/sodium/private/ed25519_ref10_fe_51.h
Lines 231 to 235 in 7978205
libsodium/src/libsodium/include/sodium/private/ed25519_ref10_fe_25_5.h
Lines 287 to 291 in 7978205
So calling fe25519_abs(x, x);
triggers a call to memcpy(x, x, ...)
, which is undefined behavior in C. Indeed, the C99 standard defined in section "7.21.2.1 The memcpy function":
#include <string.h>
void *memcpy(void * restrict s1, const void * restrict s2, size_t n);
Calling memcpy
with s1 == s2
violates the assertions ensured by restrict
.
To fix this, I believe memmove
should be used instead of memcpy
in fe25519_copy
.
For information, this issue was found by running clang's static analyzer with scan-build -analyze-headers -enable-checker alpha.unix.cstring.BufferOverlap make
. It reported:
./include/sodium/private/ed25519_ref10_fe_51.h:194:5: warning: Arguments must not be overlapping buffers [alpha.unix.cstring.BufferOverlap]
memcpy(h, f, 5 * sizeof h[0]);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~