Merge mbed-crypto/development into psa-api-1.0-beta #198
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
added 30 commits
June 3, 2019 16:07
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.
gilles-peskine-arm
added
enhancement
gilles-peskine-arm
added
needs: work
gilles-peskine-arm
force-pushed
the
psa-api-1.0-beta-merge_development_20190801
branch
from
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. |
gilles-peskine-arm
added
needs: review
k-stachowiak
reviewed
Aug 6, 2019
| @@ -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.
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.
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.
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.
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.
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.
k-stachowiak
approved these changes
Aug 6, 2019
gilles-peskine-arm
removed
the
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.
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.