Make MBEDTLS_CHECK_PARAMS usable without defining mbedtls_param_failed #2697
Conversation
Fix the dependency on libmbedcrypto.a, which is now located under crypto. Fix Mbed-TLS#2682
For unit tests and sample programs, CFLAGS=-m32 is enough to get a 32-bit build, because these programs are all compiled directly from *.c to the executable in one shot. But with makefile rules that first build object files and then link them, LDFLAGS=-m32 is also needed.
Don't use the macro name assert. It's technically permitted as long as <assert.h> is not included, but it's fragile, because it means the code and any header that it includes must not include <assert.h>.
Introduce a new configuration option MBEDTLS_CHECK_PARAMS_ASSERT, which is disabled by default. When this option is enabled, MBEDTLS_PARAM_FAILED defaults to assert rather than to a call to mbedtls_param_failed, and <assert.h> is included. This fixes Mbed-TLS#2671 (no easy way to make MBEDTLS_PARAM_FAILED assert) without breaking backward compatibility. With this change, `config.pl full` runs tests with MBEDTLS_PARAM_FAILED set to assert, so the tests will fail if a validation check fails, and programs don't need to provide their own definition of mbedtls_param_failed().
All sample and test programs had a definition of mbedtls_param_failed. This was necessary because we wanted to be able to build them in a configuration with MBEDTLS_CHECK_PARAMS set but without a definition of MBEDTLS_PARAM_FAILED. Now that we activate the sample definition of MBEDTLS_PARAM_FAILED in config.h when testing with MBEDTLS_CHECK_PARAMS set, this boilerplate code is no longer needed.
gilles-peskine-arm
added
bug
mbed TLS team
needs-review
gilles-peskine-arm
added
the
needs-preceding-pr
| @@ -832,9 +832,21 @@ component_test_no_use_psa_crypto_full_cmake_asan() { | |||
| if_build_succeeded env OPENSSL_CMD="$OPENSSL_NEXT" tests/compat.sh -e '^$' -f 'ARIA\|CHACHA' | |||
| } | |||
There was a problem hiding this comment.
In the commit message of commit "Add all.sh component that exercises invalid_param checks ", this sentence doesn't make sense.
"Add a component to all.sh that enables MBEDTLS_CHECK_PARAMS but disables MBEDTLS_CHECK_PARAMS_ASSERT doesn't define MBEDTLS_PARAM_FAILED so that these tests are exercised."
Should this be like so? (I only added an "and".)
"Add a component to all.sh that enables MBEDTLS_CHECK_PARAMS but disables MBEDTLS_CHECK_PARAMS_ASSERT and doesn't define MBEDTLS_PARAM_FAILED so that these tests are exercised."
There was a problem hiding this comment.
I fixed this and reworded the commit message to make it easier to read.
tests/scripts/all.sh
Outdated
| @@ -832,9 +832,21 @@ component_test_no_use_psa_crypto_full_cmake_asan() { | |||
| if_build_succeeded env OPENSSL_CMD="$OPENSSL_NEXT" tests/compat.sh -e '^$' -f 'ARIA\|CHACHA' | |||
| } | |||
|
|
|||
| component_test_check_params_tests () { | |||
There was a problem hiding this comment.
Suggest a more descriptive name, like component_test_check_mbedtls_param_failed(). It's a bit verbose if you also add "tests" into the name to describe that this only runs the tests. I think it more important to emphasize that it is testing the mbedtls_param_failed() function, more so than that this runs only the tests.
There was a problem hiding this comment.
It's the only component that runs the tests that verify that calls to API functions that pass invalid parameters result in a call to MBEDTLS_PARAM_FAILED. I renamed it to test_check_params_functionality.
With the change to the full config, there were no longer any tests that exercise invalid-parameter behavior. The test suite exercises invalid-parameter behavior by calling TEST_INVALID_PARAM and friends, relying on the test suite's mbedtls_check_param function. This function is only enabled if MBEDTLS_CHECK_PARAMS is defined but not MBEDTLS_CHECK_PARAMS_ASSERT. Add a component to all.sh that enables MBEDTLS_CHECK_PARAMS but disables MBEDTLS_CHECK_PARAMS_ASSERT and doesn't define MBEDTLS_PARAM_FAILED. This way, the xxx_invalid_param() tests do run. Since sample programs don't provide a mbedtls_check_param function, this component doesn't build the sample programs.
gilles-peskine-arm
force-pushed
the
check_params-test_without_function-development
branch
from
09586b6 to
adcde5e
Compare
|
I removed the commit with the crypto submodule change. Expect failures on the pr-head job but the pr-merge job should succeed. I also modified the |
|
We still need the commit to update the submodule in this PR (or some other one). It doesn't need to point at a development branch, but should point at the current head of crypto/development. |
Update to the merge of "Make test suites compatible with #include <assert.h>"
|
Oh right. Submodule updated. |
|
ARMmbed/mbed-crypto#148 has been merged. Removing label "needs: preceding PR". |
gilles-peskine-arm
added
needs-backports
|
I believe this should also fix ARMmbed/mbed-os#9193 Please check with on target tests as well |
Patater
added
approved
Tweak
config.handplatform_util.hso thatconfig.pl fullalso enables the definition ofMBEDTLS_PARAM_FAILEDasassert. Adaptall.shso that invalid-parameter tests are executed, but programs don't need to provide anmbedtls_param_failedfunction. Fix #2671I also included two small commits:
-m32, also link with-m32, as discussed in Oss-fuzz integration #1622 (comment)programsgetting rebuilt over and over.The primary goal of this PR is to unblock #1622.
This PR requires the crypto submodule to have the changes from ARMmbed/mbed-crypto#148. To pass CI, I include a commit that updates the submodule, which should be removed once the precursor PR has been merged into the submodule.
Backports:
-m32link option, everything else is not applicable. → 2.7 only: Pass -m32 to the linker as well #2702