Extend "full" config to non-boolean options and pass Clang+Asan #2684
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gilles-peskine-arm
added
bug
mbed TLS team
needs-review
This was referenced Jun 7, 2019
Merged
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.
Some boolean options had made their way to the "Module configuration options" section, which is otherwise used for non-boolean definitions. Put the system support macros MBEDTLS_PLATFORM_xxx_ALT in the "System support" section, and the default algorithm profile configuration options in the "mbed TLS feature support" section. Moving the options causes "config.pl full" to enable them unless explicitly excluded. * MBEDTLS_PLATFORM_GMTIME_R_ALT remains off due a quirk of how config.pl selects `_ALT` options. * Don't exclude MBEDTLS_PLATFORM_ZEROIZE_ALT from being excluded. * Keep MBEDTLS_TLS_DEFAULT_ALLOW_SHA1_IN_KEY_EXCHANGE off because it's dangerous.
psa_crypto_init() can fail. Do check its return code. Don't call it before initializing local objects that are going to be cleaned up.
Writing 0 instead of PSA_SUCCESS is correct, but bad form.
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>.
If MBEDTLS_CHECK_PARAMS is enabled, include <assert.h>. This allows MBEDTLS_PARAM_FAILED to be defined to assert like our documentation suggests.
In the "full" or "realfull" configuration, also enable non-boolean
options.
Most non-boolean options in config.h are presented with their default
value. Before this commit, we didn't do any testing of the values
presented in config.h. After this commit, by testing a build with the
full config, we now validate that the values we present in config.h
are sensible.
Do not enable the following macros:
* MBEDTLS_CTR_DRBG_ENTROPY_LEN, because its default value and its
permitted range depends on which hash is used for entropy, and the
value that is presented in the file is the default value in the
default configuration but is not in the permitted range if
MBEDTLS_ENTROPY_FORCE_SHA256 is enabled.
* MBEDTLS_PLATFORM_STD_NV_SEED_{READ,WRITE} because those require the
code to implement extra functions.
* MBEDTLS_PLATFORM_xxx_MACRO because those are incompatible with
MBEDTLS_PLATFORM_xxx_ALT.
Update havege.h to the new version in the crypto module. This is technically an API break, since the type mbedtls_havege_state is exposed in a public header. However normal applications should not be affected. Fix Mbed-TLS#2598
Document how to manage the CSR object passed to mbedtls_x509_csr_parse and friends.
Clang+Asan sometimes catches issues that GCC+Asan doesn't, so have at least one job that does this. There was no Asan job with the full config including PSA, so add one with Clang.
In component_test_no_use_psa_crypto_full_cmake_asan, don't set MBEDTLS_PSA_CRYPTO_C, since MBEDTLS_USE_PSA_CRYPTO is not set. Turn off MBEDTLS_MEMORY_BUFFER_ALLOC_C since it makes Asan less effective. Fix a typo in the description.
gilles-peskine-arm
force-pushed
the
fix-config-full
branch
from
23892d1 to
49997f5
Compare
Patater
reviewed
Jun 11, 2019
scripts/config.pl
Outdated
| @@ -129,7 +130,7 @@ | |||
|
|
|||
| # Things that should be enabled in "full" even if they match @excluded | |||
| my @non_excluded = qw( | |||
| PLATFORM_[A-Z0-9]+_ALT | |||
| PLATFORM_(?!ZEROIZE_)[A-Z0-9]+_ALT | |||
There was a problem hiding this comment.
The commit message doesn't adequately explain why we mustn't exclude MBEDTLS_PLATFORM_ZEROIZE_ALT from being excluded. Could you add the reasoning behind the change to the commit message? In particular, why would we want to prevent zeroize from being included in full?
There was a problem hiding this comment.
I added the reasoning in a comment in a subsequent commit. I'm not going to rewrite the old commit because it adhered to the standard for documentation at the time.
Switch "config.pl full" back to skipping the "Module configuration options" section. It's hard to construct almost-full configurations that exclude platform features when all the MBEDTLS_PLATFORM_STD_xxx are enabled, and it isn't particularly useful to uncomment module configuration options in those builds. Add "config.pl veryfull" which does enable options in the "Module configuration options" section. This configuration should be tested at least once and can be the basis of configurations that enable everything except for one non-platform-related feature.
In test components that target a fully-capable platform, start from the "veryfull" configuration. In test components that target a restricted platform, start from the "full" configuration.
Remove options that no longer exist.
It has a poor record of being kept up-to-date, and it has zero clarity advantage over the code below.
Unlike other lines in config.h, this one is an example, not a default. This example reuslts in many of our tests failing for lack of a ciphersuite.
The regex in config.pl to not disable MBEDTLS_PLATFORM_xxx_ALT didn't allow underscores in xxx, but a few options in config.h have more than one underscore. A few options need to stay disabled, but they aren't correlated with having an extra underscore. MBEDTLS_PLATFORM_NV_SEED_ALT now needs to be unset explicitly when MBEDTLS_ENTROPY_NV_SEED is unset.
gilles-peskine-arm
force-pushed
the
fix-config-full
branch
from
b9bda1a to
bf8525e
Compare
It took a few tries but it does now. |
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.
While MBEDTLS_PARAM_FAILED is a better fit for the "Customisation configuration options" section, there are advantages to moving it to the "System support" section. In terms of documentation, for people who are editing config.h rather than looking at hyperlinked documentation, it's better to have the commented-out definition of MBEDTLS_PARAM_FAILED next to MBEDTLS_CHECK_PARAMS. For our testing, moving MBEDTLS_PARAM_FAILED to a section that is included in the "full" or "baremetal" configuration has the major advantage that it makes "config.pl" automatically work with programs that do not provide their own mbedtls_check_params function (which gets used if the MBEDTLS_PARAM_FAILED macro is not set).
gilles-peskine-arm
force-pushed
the
fix-config-full
branch
from
ac1858b to
3dd6da0
Compare
With the config changes, 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, which is only enabled if MBEDTLS_CHECK_PARAMS is not defined. Add a component to all.sh that enables MBEDTLS_CHECK_PARAMS but doesn't define MBEDTLS_PARAM_FAILED so that these tests are exercised. 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
fix-config-full
branch
from
3dd6da0 to
83c1121
Compare
There is one macro in config.h which has an argument: MBEDTLS_PARAM_FAILED. Don't consider it a feature to be listed even though it is now in a feature section, since it is not a boolean setting.
| 'Module configuration options' section. | ||
| This is a good basis for testing the exclusion | ||
| of specific additional options. | ||
| veryfull - Uncomments all #define's in the configuration file |
There was a problem hiding this comment.
veryfull isn't a very descriptive name. Surely we can do better?
There was a problem hiding this comment.
With full and realfull I thought we were following the telescope naming convention... Better suggestions accepted. And let's rename realfull while we're at it.
gilles-peskine-arm
force-pushed
the
fix-config-full
branch
2 times, most recently
from
1c5edfa to
a82e087
Compare
This is an alternative implementation, so it needs to be tested separately. Since all components now use a 64-byte entropy seed, size the seedfile accordingly. Overwrite the seedfile if it exists. It's simpler and doesn't hurt.
gilles-peskine-arm
force-pushed
the
fix-config-full
branch
from
a82e087 to
24ecfba
Compare
gilles-peskine-arm
added
needs-work
and removed
needs-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
config.hso thatconfig.pl fullalso enables the definition ofMBEDTLS_PARAM_FAILEDasassert(without this, all executables must have anmbedtls_param_failedfunction).MBEDTLS_PARAM_FAILEDdefined asassert. Together with the previous point, this fixesconfig.pl fullor just turning on MBEDTLS_CHECK_PARAMS breaks the build #2671.config.pl veryfullthat also sets non-boolean options. This way, we test that the values that we're proposing are sensible.all.shcomponent withconfig.pl fulland Clang+Asan. Clang+Asan catches things that GCC+Asan doesn't.-m32, also link with-m32, as discussed in Oss-fuzz integration #1622 (comment)programsgetting rebuilt over and over.This PR requires the crypto submodule to have the changes from ARMmbed/mbed-crypto#144 . To pass CI, I include a commit that updates the submodule accordingly.
Needs backports:
all.shchanges: except concerning PSA which didn't exist.config.pl fullbehavior: yes, for uniformity between the branches.MBEDTLS_CHECK_PARAMSas assert: to 2.16.