-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge mbed-crypto/development into psa-api-1.0-beta #198
Merge mbed-crypto/development into psa-api-1.0-beta #198
Conversation
Currently, the stack silently ignores DTLS frames with an unexpected CID. However, in a system which performs CID-based demultiplexing before passing datagrams to the Mbed TLS stack, unexpected CIDs are a sign of something not working properly, and users might want to know about it. This commit introduces an SSL error code MBEDTLS_ERR_SSL_UNEXPECTED_CID which the stack can return in response to an unexpected CID. It will conditionally be put to use in subsequent commits.
This commit modifies the CID configuration API mbedtls_ssl_conf_cid_len() to allow the configuration of the stack's behaviour when receiving an encrypted DTLS record with unexpected CID.
The implementation is complete now.
The previous implementation of mbedtls_ssl_conf_cid() relied on MBEDTLS_SSL_UNEXPECTED_CID_IGNORE being defined as 1.
Files modified via sed -i 's/MBEDTLS_SSL_CID\([^_]\|$\)/MBEDTLS_SSL_DTLS_CONNECTION_ID\1/g' **/*.c **/*.h **/*.sh **/*.function
This commit modifies mbedtls_ssl_get_peer_cid() to also allow passing NULL pointers in the arguments for the peer's CID value and length, in case this information is needed. For example, some users might only be interested in whether the use of the CID was negotiated, in which case both CID value and length pointers can be set to NULL. Other users might only be interested in confirming that the use of CID was negotiated and the peer chose the empty CID, in which case the CID value pointer only would be set to NULL. It doesn't make sense to pass a NULL pointer for the CID length but a non-NULL pointer for the CID value, as the caller has no way of telling the length of the returned CID - and this case is therefore forbidden.
This commit adds the command line option 'bad_cid' to the UDP proxy `./programs/test/udp_proxy`. It takes a non-negative integral value N, which if not 0 has the effect of duplicating every 1:N CID records and modifying the CID in the first copy sent. This is to exercise the stacks documented behaviour on receipt of unexpected CIDs. It is important to send the record with the unexpected CID first, because otherwise the packet would be dropped already during replay protection (the same holds for the implementation of the existing 'bad_ad' option).
certs.h is not needed in Mbed Crypto. No programs or other library code use it.
There is now a test that ensures all headers are included in the cpp_dummy_build test, so we can't remove compat-1.3.h from the cpp_dummy_build test until we remove compat-1.3.h. This reverts commit 2b725ef.
Make some functions non-static, to avoid Wunused function warnings. Make a function scoped variable block scoped instead, to avoid Wunused variable warnings in some configurations.
Prepare for removing crypto from mbedtls
The following provides more information on this PR: - PSA stands for Platform Security Architecture. - Add support for use of psa_trusted_storage_api internal_trusted_storage.h v1.0.0 as the interface to the psa_trusted_storage_linux backend (i.e. for persistent storage when MBEDTLS_PSA_ITS_FILE_C is not defined). This requires changes to psa_crypto_its.h and psa_crypto_storage.c to migrate to the new API.
PSA Storage: Add psa_trusted_storage_linux persistent storage support for v1.0.0 APIs
…bed#2) option name: LINK_WITH_TRUSTED_STORAGE default value: ON
…e-fix Add CMake option for explicitly link library to trusted_storage
….1.0-release Correct version number in storage format spec
Resolve conflicts by performing the following operations: - Reject changes related to building a crypto submodule, since Mbed Crypto is the crypto submodule. - Reject X.509, NET, and SSL changes. - Reject changes to README, as Mbed Crypto is a different project from Mbed TLS, with a different README. - Avoid adding mention of ssl-opt.sh in a comment near some modified code in include/CMakeLists.txt (around where ENABLE_TESTING as added). - Align config.pl in Mbed TLS with config.pl in Mbed Crypto where PSA options are concerned, to make future merging easier. There is no reason for the two to be different in this regard, now that Mbed TLS always depends on Mbed Crypto. Remaining differences are only the PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER option and the absence of X.509, NET, and SSL related options in Mbed Crypto's config.pl. - Align config.h in Mbed Crypto with Mbed TLS's copy, with a few notable exceptions: - Leave CMAC on by default. - Leave storage on by default (including ITS emulation). - Avoid documenting the PSA Crypto API as is in beta stage in documentation for MBEDTLS_PSA_CRYPTO_C. The only remaining differences are a lack of X.509, NET, and SSL options in Mbed Crypto's config.h, as well as an additional Mbed-Crypto-specific PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER option. Documentation for the check params feature and related macros is also updated to match Mbed TLS's description. - Reject tests/data_files/Makefile changes to generate DER versions of CRTs and keys, as none of those are used by Mbed Crypto tests. - Add the "no PEM and no filesystem" test to all.sh, without ssl-opt.sh run, as Mbed Crypto doesn't have ssl-opt.sh. Also remove use of PSA Crypto storage and ITS emulation, since those depend on filesystem support. - Reject addition of test when no ciphersuites have MAC to all.sh, as the option being tested, MBEDTLS_SSL_SOME_MODES_USE_MAC, is not present in Mbed Crypto. - Use baremetal config in all.sh, as Mbed Crypto's baremetal configuration does exclude the net module (as it doesn't exist in Mbed Crypto) - Reject cmake_subproject_build changes, continuing to link only libmbedcrypto. - Reject changes to visualc and associated templates. Mbed Crypto doesn't need additional logic to handle submodule-sourced headers. - Avoid adding fuzzers from Mbed TLS. The only relevant fuzzers are the privkey and pubkey fuzzers, but non-trivial work would be required to integrate those into Mbed Crypto (more than is comfortable in a merge commit). - Reject addition of Docker wrappers for compat.sh and ssl-opt.sh, as those are not present in Mbed Crypto. - Remove calls to SSL-related scripts from basic-in-docker.sh Fix test errors by performing the following: - Avoid using a link that Doxygen can't seem to resolve in Mbed Crypto, but can resolve in Mbed TLS. In documentation for MBEDTLS_CHECK_PARAMS, don't attempt to link to MBEDTLS_PARAM_FAILED. * origin/development: (339 commits) Do not build fuzz on windows No booleans and import config Removing space before opening parenthesis Style corrections Syntax fix Fixes warnings from MSVC Add a linker flag to enable gcov in basic-build-test.sh Update crypto submodule to a revision with the HAVEGE header changes Test with MBEDTLS_ECP_RESTARTABLE Allow TODO in code Use the docstring in the command line help Split _abi_compliance_command into smaller functions Record the commits that were compared Document how to build the typical argument for -s Allow running /somewhere/else/path/to/abi_check.py tests: Limit each log to 10 GiB Warn if VLAs are used Remove redundant compiler flag Consistently spell -Wextra Fix parsing issue when int parameter is in base 16 ...
Bring in changes from Mbed TLS as of 2019-07-22
…pi-1.0-beta-merge_development_20190801 Conflict resolution: * `scripts/config.pl`: Take the exclusion of `MBEDTLS_PSA_CRYPTO_SE_C` from the API branch. Take the removal of `MBEDTLS_PSA_CRYPTO_STORAGE_ITS_C` (obsolete) from the development branch. * `tests/scripts/all.sh`: Multiple instances of factoring a sequence of `config.pl` calls into a mere `config.pl baremetal` in the development branch, and a change in the composition of `baremetal` in the API branch. In each case, take the version from development. * `tests/suites/test_suite_psa_crypto_slot_management.function`: A function became non-static in development and disappeared in the API branch. Keep the version from the API branch. Functions need to be non-static if they're defined but unused in some configurations, which is not the case for any function in this file at the moment. * `tests/suites/test_suite_psa_crypto.function`: Consecutive changes in the two branches, reconciled.
ebe39aa
to
5386f6b
Compare
In tests of mbedtls_cipher_xxx and mbedtls_pk_xxx with MBEDTLS_USE_PSA_CRYPTO enabled, initialize and deinitialize the PSA subsystem in every function. Before, the tests were only passing because the first function to be called happened to call psa_crypto_init() but not mbedtls_psa_crypto_free(). In some configurations (not tested on CI), psa_crypto_init() was not called so the tests using PSA failed. Call PSA_DONE() at the end of each test function. This ensures that no resources are leaked in the form of PSA crypto slot contents. Incidentally, this also fixes a build error due to test_helper_psa_done() being unused in test_suite_pk: the fact that it wasn't used betrayed the missing calls to PSA_DONE().
Crypto CI ok except for the ABI checking script (known bug: it fails on merges) which we don't care about in this branch anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reproduced the merge locally, which lead me to the question about asn1_skip_integer
being static or not.
I also left a minor suggestion regarding a documenting comment.
Otherwise it looks good.
@@ -712,9 +712,9 @@ exit: | |||
return( ok ); | |||
} | |||
|
|||
static int asn1_skip_integer( unsigned char **p, const unsigned char *end, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change popped up for me during reproduction of the merge. It doesn't seem to be used elsewhere, hence could be left static.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's non-static because it's unused in some build configurations, and the compiler complains that a function is unused. It didn't use to be a problem because we used to compile tests with -Wno-unused
, but since this merge, we've stopped doing that. This is explained in the merge commit message for test_suite_psa_crypto_slot_management.function
. I didn't list it for test_suite_psa_crypto.function
because there it's a change that already happened in the development branch, there was no conflict.
|
||
status = psa_get_se_driver_its_file_uid( driver, &uid ); | ||
if( status != PSA_SUCCESS ) | ||
return( status ); | ||
|
||
/* Read the amount of persistent data that the driver requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the section below explains the code, this one rather describes the function behavior. I find it fine, but instead of placing two such comment sections next to each other, the first one could be placed as a doxygen note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by “placed as a doxygen note”? This implements the behavior documented in crypto_se_driver.h
for the persistent_data
of psa_drv_se_context_t
, which driver->internal.persistent_data
accesses. I think it's useful to explain what's going on at the point of implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have misunderstood the comment to be information that may be interesting to the function's user and as such placed in the doxygen comment for the function. I found it suspicious for a function to have the same amount of comment lines that it has code, so I suggested revising it, which you kindly have. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, @gilles-peskine-arm . LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Take development just after #187 was merged. Update to the 1.0 final ITS interface, allow declarations after statements, and many other changes.
This is one merge commit, plus a follow-up.