config.pl full or just turning on MBEDTLS_CHECK_PARAMS breaks the build

[gilles-peskine-arm]

Description

• Type: Bug
• Priority: Major

The full configuration breaks normal programs. We don't explicitly document this configuration, and it's intended for testing mbedtls rather than for production. I certainly wouldn't want to assert that this configuration is good for security, for example. Nonetheless, I would not expect this configuration to break perfectly valid programs so much that they don't compile.

Specifically, the problem is that config.pl full enables MBEDTLS_CHECK_PARAMS, but with the default setting of MBEDTLS_PARAM_FAILED, which calls a function (mbedtls_param_failed) that the user must supply. This is not a sensible default.

Justification for severity: breaks perfectly sensible code such as 3cd7a32 (CI run [internal link]: https://jenkins-internal.mbed.com/job/mbedtls-pr-multibranch/job/PR-1622-head/4/execution/node/819/log).

Steps to reproduce (from an mbedtls checkout, e.g. at 7be9b4e which is the head of development right now):

git submodule update
make clean
scripts/config.pl full
make
cat <<'EOF' >|a.c
#include <mbedtls/bignum.h>
int main(void)
{
    mbedtls_mpi x;
    mbedtls_mpi_init(&x);
    if (mbedtls_mpi_lset(&x, 42)) return 1;
    return 0;
}
EOF
gcc -O -Wall -I include -L library -L crypto/library a.c -lmbedcrypto

Expected behavior: successful build.

Actual behavior:

crypto/library/libmbedcrypto.a(bignum.o): In function `mbedtls_mpi_grow':
bignum.c:(.text+0x597): undefined reference to `mbedtls_param_failed'
crypto/library/libmbedcrypto.a(bignum.o): In function `mbedtls_mpi_shrink':
bignum.c:(.text+0x698): undefined reference to `mbedtls_param_failed'
crypto/library/libmbedcrypto.a(bignum.o): In function `mbedtls_mpi_copy':
bignum.c:(.text+0x7c6): undefined reference to `mbedtls_param_failed'
bignum.c:(.text+0x7e1): undefined reference to `mbedtls_param_failed'
crypto/library/libmbedcrypto.a(bignum.o): In function `mbedtls_mpi_safe_cond_assign':
bignum.c:(.text+0x991): undefined reference to `mbedtls_param_failed'
crypto/library/libmbedcrypto.a(bignum.o):bignum.c:(.text+0x9ac): more undefined references to `mbedtls_param_failed' follow
collect2: error: ld returned 1 exit status

I don't know what the best remedy is. We could simply keep MBEDTLS_CHECK_PARAMS off in the full config. I think MBEDTLS_CHECK_PARAMS is not useful, so this solution is attractive to me, but doing this breaks the expectation that config.pl full turns out everything that the library can do and that isn't contradictory with something else. Furthermore, beyond config.pl full, there is the problem that the default behavior of just turning on the option isn't sensible: I think the default implementation of MBEDTLS_PARAM_FAILED, if you just turn on MBEDTLS_CHECK_PARAMS without any other related configuration changes, should be assert.

[Patater]

What reasons might we not want to provide a default implementation of MBEDTLS_PARAM_FAILED that is assert? Seems sensible to me so far.

[yanesca]

I agree, it makes perfect sense to me to provide a default implementation that is an assert.

[Patater]

I'm a little bit confused. The default definition of MBEDTLS_PARAM_FAILED seems to be assert() already. https://github.com/ARMmbed/mbedtls/blob/development/include/mbedtls/config.h#L3285

Is config.pl not setting this properly?

[gilles-peskine-arm]

That's not a default definition, that's a suggested definition that a human can enable. config.pl full only enables boolean options.

By default, since MBEDTLS_PARAM_FAILED is not defined in config.h, platform_util.h sets it to mbedtls_param_failed. The library does not provide a function by this name: it's up to applications to provide it.

My preferred resolution is to uncomment the line #define MBEDTLS_PARAM_FAILED assert… in config.h. This has no effect if MBEDTLS_CHECK_PARAMS is not defined. But now that I think about it, it changes the behavior of existing applications that do provide mbedtls_param_failed and that expect to configure the library by starting from the default config.h (always a good idea) and enabling MBEDTLS_CHECK_PARAMS, and don't do anything about MBEDTLS_PARAM_FAILED. Now it stays undefined so mbedtls_param_failed is used. This change would cause assert to be used instead. To preserve the existing behavior, we would need to detect whether the application provides a symbol or a macro mbedtls_param_failed, but detecting symbols is not portable.

[yanesca]

Yes, preserving the current behaviour would be normally the only responsible choice. However there are two circumstances that might change this and makes worth considering to choose the easier and cleaner way:

• It is a fairly recent feature, a couple of months old, it is unlikely to have very wide adoption.
• The next public release is allowed to break API.

[Patater]

Defining a weakly linked mbedtls_param_failed() that called assert() would work, but as you said, not universally portable. It may be portable enough to platforms we care about, though, given that all the major toolchains support weak symbols.

Another alternative would be to extend config.pl full to work with non-boolean options, treating the commented-out bits in config.h as not merely suggestions for humans, but default values for when config.pl sets those options.

[gilles-peskine-arm]

In #2684, I went with Jaeden's second solution: extend config.pl full to turn on (most) non-boolean options. This gives us better testing. With that PR, merely enabling MBEDTLS_CHECK_PARAMS does not work, but enabling MBEDTLS_CHECK_PARAMS and uncommenting the suggested definition of MBEDTLS_PARAM_FAILED works.