Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.26k stars 2.57k forks source link

PSA macros: initializer element is not constant #4320

Open gilles-peskine-arm opened 3 years ago

gilles-peskine-arm commented 3 years ago

Several PSA macros do case analysis on an algorithm, key type or similar value to determine a size, a condition or similar result. In some cases, some of the arguments of the macro are not used. To avoid unused-argument warnings and reduce (but not eliminate) the variations in case an argument has a side effect, the definition uses (void)maybe_unused_argument and a comma operator, for example

#define PSA_MAC_LENGTH(key_type, key_bits, alg)                                   \
    ((alg) & PSA_ALG_MAC_TRUNCATION_MASK ? PSA_MAC_TRUNCATED_LENGTH(alg) :        \
     PSA_ALG_IS_HMAC(alg) ? PSA_HASH_LENGTH(PSA_ALG_HMAC_GET_HASH(alg)) :         \
     PSA_ALG_IS_BLOCK_CIPHER_MAC(alg) ? PSA_BLOCK_CIPHER_BLOCK_LENGTH(key_type) : \
     ((void)(key_type), (void)(key_bits), 0))

However using the comma operator makes the definition not be a constant expression. Therefore, the following program technically has undefined behavior:

#include <psa/crypto.h>
size_t n = PSA_MAC_LENGTH(PSA_KEY_TYPE_AES, 128, PSA_ALG_CMAC);

In practice, many compilers see that the conditional operator can be evaluated at compile time and ignore the branch that is not taken. However, the following program, where the branch containing the comma operator, fails to build with gcc.

#include <psa/crypto.h>
size_t n = PSA_MAC_LENGTH(PSA_KEY_TYPE_AES, 128, 42);

The error is perfectly reasonable, considering that a comma operator makes the expression non-constant even if both sides are constant:

In file included from include/psa/crypto.h:3807,
                 from c.c:1:
include/psa/crypto_sizes.h:248:5: error: initializer element is not constant
  248 |     ((alg) & PSA_ALG_MAC_TRUNCATION_MASK ? PSA_MAC_TRUNCATED_LENGTH(alg) :        \
      |     ^
c.c:2:12: note: in expansion of macro ‘PSA_MAC_LENGTH’
    2 | size_t n = PSA_MAC_LENGTH(PSA_KEY_TYPE_AES, 128, 42);
      |            ^~~~~~~~~~~~~~

The intent of the API was to allow such programs to build. Sure, this part of the code won't do anything useful since PSA_MAC_LENGTH is passed an invalid MAC algorithm, but this can legitimately happen in a program that can be built in many different configurations and in which some of the code is inoperative in some configurations. Furthermore, even the nominal case is undefined behavior according to the C standard, which is bad even if it tends to work in practice.

Changing the definition of such macros to remove the ((void) unused_argument, actual_result) idiom would obviously solve this problem, but it might cause existing code to fail with unused-arguments error.

A possible fix would be to change evaluate-but-not-take-the-value-into-account from ((void) (unused_argument), actual_result) to something like (0 & (unused_argument) | actual_result).

Goals of this task:

mpg commented 2 years ago

This bug was found again in https://github.com/ARMmbed/mbedtls/pull/5436 where the perfectly reasonable uint8_t buf[PSA_MAC_LENGTH(PSA_KEY_TYPE_HMAC, 256, PSA_ALG_HMAC(PSA_ALG_SHA_256))]; caused CI failures: GCC and some version of Clang reject it due to -Werror=vla in various builds.