Remove MBEDTLS_CHECK_PARAMS

[gilles-peskine-arm]

Context

The option MBEDTLS_CHECK_PARAMS (disabled by default) enables certain kinds of “parameter validation”. It covers two kinds of validations:

• In some functions that require a valid pointer, “parameter validation” checks that the pointer is non-null. With the feature disabled, a null pointer is not treated differently from any other invalid pointer, and typically leads to a runtime crash. 90% of the uses of the feature are of this kind.
• In some functions that take an enum-like argument, “parameter validation” checks that the value is a valid one. With the feature disabled, an invalid value causes a silent default to one of the valid values.

The default reaction to a failed check is to call a function mbedtls_param_failed which the application must provide. If this function returns, its caller returns an error MBEDTLS_ERR_xxx_BAD_INPUT_DATA.

This feature is only used in some classic (non-PSA) cryptography modules. It is not used in X.509, TLS or in PSA crypto, and it has not been implemented in all classic crypto modules.

Proposal

Remove MBEDTLS_CHECK_PARAMS and all dependent features. Keep the validation of enum values, but remove the null pointer checks. This means changing code that does something like this:

#if MBEDTLS_CHECK_PARAMS
#define VALIDATE(cond) do {if(cond) return BAD_INPUT_DATA;} while (0)
#else
#define VALIDATE(cond) do {} while (0)
#endif
…
VALIDATE(coin == HEADS || coin == TAILS);
VALIDATE(data != NULL);
if (coin == HEADS) heads();
else tails();

to something like this:

if (coin == HEADS) heads();
else if (coin == TAILS) tails();
else return BAD_INPUT_DATA;

Rationale

Maintenance burden: Every compilation option has a testing cost. Beyond this general observation, MBEDTLS_CHECK_PARAMS has a major impact on the design of our test framework and scripts because it requires applications to provide their own implementation of mbedtls_check_params (unlike other platform functions for which the library provides a sensible default which applications can override).

Limited benefits:

• Null pointer checks prevent runtime errors from badly written applications, but null pointers usually have a relatively benign effect: on most platforms, a null pointer dereference is a hard crash which can't be exploited as information disclosure or data injection. The presence of these null pointer checks can be counter productive because they give the appearance that the function accepts a null pointer, which makes it harder to find places where calling code is passing a potentially null pointer (both humans and some static analyzers have heuristics about which pointers are nullable, and extra null checks tend to confuse these heuristics).
• Validation of enum-like values is somewhat useful, but not extremely important, because the parameters concerned are usually constants in applications. This could be made systematic at little cost in effort and code size (see statistics below).

Misleading documentation: In some modules, the documentation mentions “parameter validation” (or “parameter-verification failure”), which should mean that the function validates all of its parameters, but this in fact refers specifically to null pointer check and enum-like value checks. Some of these functions omit checks that are more important such as buffer sizes.

Inconsistent use: not all modules have this form of “parameter validation”, and this is solely for historical reasons.

Statistics

(From d520037, which is development as of a few days ago.)

• Total quasi-hits: 837

  grep _VALIDATE library/*.c
  

• Without false positives: 738
  False positives: definitions of auxiliary macros; MPS.

  grep _VALIDATE library/*.c |grep -v -F -e ':#' -e MBEDTLS_INTERNAL_VALIDATE -e MBEDTLS_MPS_STATE_VALIDATE_RAW
  

• Null checks: 688
  Allowing for whitespace variations, a cast of the pointer, and
  allowing null for a length of 0.

  grep _VALIDATE library/*.c |grep -E '_VALIDATE(_RET)?\( *(\w+ *== *0 *\|\| *)?(\([^()]+\* *\))? *\*?\w+ *!= *NULL *(\|\| *\w+ *== *0)? *\);'
  

• Without null checks and false positives: 51

  grep _VALIDATE library/*.c |grep -v -E '_VALIDATE(_RET)?\( *(\w+ *== *0 *\|\| *)?(\([^()]+\* *\))? *\*?\w+ *!= *NULL *(\|\| *\w+ *== *0)? *\);' |grep -v -F -e ':#' -e MBEDTLS_INTERNAL_VALIDATE -e MBEDTLS_MPS_STATE_VALIDATE_RAW
  

  • RSA mode validation (not applicable in 3.0): 13

  grep -F 'RSA_VALIDATE_RET( mode ==' library/*.c
  

• Remaining calls that are not _RET and not false positives: 2

  grep _VALIDATE library/*.c |grep -v -E '_VALIDATE(_RET)?\( *(\w+ == 0 *\|\| *)?(\([^()]+\* *\))? *\w+ *!= *NULL *(\|\| \w+ == 0)? *\);' |grep -v -e _RET -e '#define' -e MBEDTLS_INTERNAL_VALIDATE -e MBEDTLS_MPS_STATE_VALIDATE_RAW
  

  Both RSA padding mode validation (in mbedtls_rsa_init, mbedtls_rsa_set_padding).

Work items for 3.0

(size:S)

• Remove MBEDTLS_CHECK_PARAMS, MBEDTLS_CHECK_PARAMS_ASSERT and MBEDTLS_PARAM_FAILED from config.h.
• Make it a compile-time error to enable MBEDTLS_CHECK_PARAMS.
• Remove component_test_check_params* from all.sh.
• A minimum amount of changes so that the CI passes.
• Tweak the API for RSA context initialization to have better handling for an invalid padding mode. This is best reviewed separately, so it is not part of the current issue and filed separately.

Work items for 3.x

(size:L)

• Remove all uses of the feature for null pointer checks.
• Change all uses of the feature for enum value checks to code that is executed unconditionally. In 3.0 phase, unit test cases related to invalid enum values have been kept but deactivated with a NOT_DEFINED dependency. When enum values checks are re-enabled in the library code, do not forget to re-activate the corresponding test cases.
• Remove all related auxiliary macros.
• Remove all related tests.
• Remove support code from the test framework.
• Remove mentions of “parameter validation” or “parameter-validation failure” or any related wording from the documentation, or change them to correctly reflect what the code actually does.
• In 2.25+, MBEDTLS_CHECK_PARAMS feature was used in PSA crypto (functions psa_wipe_key_slot() and psa_unlock_key_slot()) to reduce the risk of missing invalid key slot unlock or wiping when running Mbed TLS tests. In 3.0 due to the removal of the MBEDTLS_CHECK_PARAMS feature and as not critical, those risk mitigations have been removed. This has to be revisited in 3.x: restore the risk mitigations using MBEDTLS_TEST_HOOKS ? do not restore them ?

Mailing list discussion

https://lists.trustedfirmware.org/pipermail/mbed-tls/2021-April/000326.html

[Christian-Sander]

I would just like to add for consideration that reading from 0x0 is a valid operation on (at least some) ARM systems. Writing however is not., so at some places this might lead to garbage data instead of crashes.

[mpg]

There has been no objection on the list or elsewhere, so I'm marking this as approved.

[mpg]

Note: add to the 3.x follow-ups: find a better solution for ronald-cron-arm@fdf6fc2