jedisct1 / libsodium

A modern, portable, easy to use crypto library.
https://libsodium.org
Other
12.06k stars 1.72k forks source link

Calculations not strictly conforming #1184

Closed rgambord closed 2 years ago

rgambord commented 2 years ago

Here is an example line,

https://github.com/jedisct1/libsodium/blob/68564326e1e9dc57ef03746f85734232d20ca6fb/src/libsodium/sodium/utils.c#L168

The arguments to the subtraction operation are implicitly converted to type int and so the result is also signed int with a potentially negative value, resulting in implementation defined behavior when the right shift is performed. Calculations like this must all include explicit casts to unsigned int of both arguments, before right shift is performed in order to be strictly conforming.

From the C99 standards:

6.3.1.1

If an int can represent all values of the original type, the value is converted to an int; otherwise, it is converted to an unsigned int. These are called the integer promotions. All other types are unchanged by the integer promotions.

6.5.7

The result of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a nonnegative value, the value of the result is the integral part of the quotient of E1 divided by the quantity, 2 raised to the power E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined.

You can see the difference with the following example. On the explicit cast to intmax_t, values are preserved. This means negative values are sign-extended, and positive values are not. You can see, without any explicit casting, that the result is a negative value.

#include <stdio.h>
#include <stdint.h>

int
main(void)
{
  unsigned char a = 0;
  unsigned char b = 1;
  printf("%jx\n", (intmax_t) (a - b)); // The result of a - b is negative int, so it's sign extended on cast to intmax_t
  printf("%jx\n", (intmax_t) ((unsigned) a - (unsigned) b)); // Result is unsigned int, so it's not sign extended on cast to intmax_t
  printf("%jx\n", (intmax_t) ((unsigned) (a - b))); // Result is negative int, then cast to unsigned int, so it's not sign extended on cast to intmax_t
}