Skip to content

Merge development-psa from mbedTLS into development #42

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

Conversation

AndrzejKurek
Copy link
Contributor

This PR supersedes #40.
It merges development_psa from mbedtls repository (commit 409fdae) into development.
The aim is to have a sibling PR to the big merge happening here: Mbed-TLS/mbedtls#2395 , to have the tests pass on the TLS side. Then, an updated PR with development -> mbed-crypto merge will be created.
I have performed the following tests:

  • make test (passed);
  • ssl-opt.sh (passed).

Following conflicts have been resolved:

  • CMakeFiles.txt: used PSA - added "include_directories(include/)" and "include_directories(library/)";
  • include/mbedtls/check_config.h: used PSA - added crypto_spm, crypto_storage_c, crypto_storage_file_c and crypto_storage_its_c prerequisites;
  • cipher.h:
    • used mbedtls: formatting style of mbedtls_cipher_set_iv;
    • used mbedtls: documentation of mbedtls_cipher_update_ad;
    • used mbedtls: documentation of mbedtls_cipher_auth_encrypt;
    • used mbedtls: documentation of mbedtls_cipher_auth_decrypt;
  • pk.h: used mbedtls - documentation of mbedtls_pk_free;
  • library/version_features.c: used PSA - definition of additional features like psa_crypto_storage_c, storage_file_c, etc.;
  • scripts/config.pl: used PSA - added "psa_crypto_spm", "psa_has_its_io", "psa_crypto_storage_its_c", "use_psa_crypto" to @excluded;
  • Readme.md - used PSA - used the bare version of readme that PSA repository has (cut off at "the test suites need Perl [...]"). Not sure about it though.
  • include/mbedtls/config.h - used PSA - used the less elaborate descriptions of PSA defines from PSA repository, with PSA_CRYPTO_C defined on default and with added new storage-related defines.
  • library/ssl_srv.c: used PSA - used mbedtls - MBEDTLS_SSL_DEBUG_ECDH instead of DEBUG_ECP in line ~3925;
  • library/pk.c: used mbedtls - parameter validation in mbedtls_pk_check_pair (makes checks for pub == NULL and pr == NULL conditional);
  • library/rsa.c:
    • used both - rsa_rsaes_pkcs1_v15_encrypt: additional null checks from PSA and mode & padding checks from mbedtls;
    • used mbedtls - mbedtls_rsa_rsaes_pkcs1_v15_decrypt with mbedtls logic of handling padding - not sure about that;
  • library/pk_wrap.c: used psa - handling public key format in the new way (even though the PR will have to be reworked), since it has already been merged in the crypto repository;
  • tests/suites/helpers.function - used both - parameter validation handling via new test macros and the new PSA_ASSERT and other definitions.
  • tests/scripts/all.sh - used both - removed a check for crypto/Makefile (crypto submodule initialization), removed tests of crypto submodule, added parameter validation tests according to the even newer development head - unfortunately the commit that is merged into development-psa is a commit just before adding component checks to all.sh, but it's already present in mbed-crypto repository, so it is added here as a component check, but in development-psa - it's the old way.
  • library/CMakeLists.txt - used both - PSA version of the file with mbedTLS SOVERSION's.
  • library/cipher.c:
    • return parentheses added at line ~416 - used mbedtls
    • mbedtls_cipher_set_iv - used mbedtls - added parameter validation and removed the "ctx->iv_size = 0;" setting if ( iv == NULL && iv_len == 0 );
    • mbedtls_cipher_crypt - used mbedtls - added parameter validation;
    • mbedtls_cipher_auth_encrypt - used mbedtls - added parameter validation;
    • mbedtls_cipher_auth_decrypt - used mbedtls - added parameter validation;
    • mbedtls_cipher_setkey - used mbedtls - added parameter validation;

Manual changes:

  • removed .gitmodules file and crypto submodule
  • removed submodule option from Makefile, programs/Makefile, library/Makefile, tests/Makefile, and CMakeLists.txt

Hanno Becker and others added 30 commits December 19, 2018 12:47
The acceptance of NULL should be tested regardless of the
setting of MBEDTLS_CHECK_PARAMS.
The test that mbedtls_aria_free() accepts NULL parameters
can be performed even if MBEDTLS_CHECK_PARAMS is unset, but
was previously included in the test case aria_invalid_params()
which is only executed if MBEDTLS_CHECK_PARAMS is set.
We allow a NULL input buffer if the input length is zero,
but we don't test it. As long as that's the case, we shouldn't
promise to support it.
It should be tested regardless of the setting of MBEDTLS_CHECK_PARAMS.
It seems to work, but we don't test it currently,
so we shouldn't promise it.
free() functions are documented as no-ops on NULL. Implement and test
this correctly.
A 0-length buffer for the key is a legitimate edge case. Ensure that
it works, even with buf=NULL. Document the key and keylen parameters.

There are already test cases for parsing an empty buffer. A subsequent
commit will add tests for writing to an empty buffer.
This needs a real key to test properly.
@AndrzejKurek AndrzejKurek added needs: design review DO NOT MERGE The PR is not intended to be merged (yet). Usually used for a review of worked in progress. needs: ci Needs a passing full CI run labels Feb 4, 2019
@AndrzejKurek
Copy link
Contributor Author

I've performed a merge of development into this PR (without conflicts) to include Hanno's changes to public key format adaption code.

Adjust crypto submodule version to use new, forked crypto version accordingly.
Manually removed submodule-related changes
@AndrzejKurek
Copy link
Contributor Author

I've merged the Mbed-TLS/mbedtls#2395 branch into the branch used in this PR, to have the tests passing on MbedTLS side.

@mpg
Copy link
Contributor

mpg commented Feb 12, 2019

Jaeden has created #51 which supersedes this.

@mpg mpg closed this Feb 12, 2019
gilles-peskine-arm pushed a commit to gilles-peskine-arm/mbed-crypto that referenced this pull request May 22, 2019
The fact that self-signed end-entity certs can be explicitly trusted by
putting them in the CA list even if they don't have the CA bit was not
documented though it's intentional, and tested by "Certificate verification ARMmbed#73
(selfsigned trusted without CA bit)" in test_suite_x509parse.data

It is unclear to me whether the restriction that explicitly trusted end-entity
certs must be self-signed is a good one. However, it seems intentional as it is
tested in tests ARMmbed#42 and ARMmbed#43, so I'm not touching it for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE The PR is not intended to be merged (yet). Usually used for a review of worked in progress. enhancement New feature or request needs: ci Needs a passing full CI run needs: design review needs: review The pull request is ready for review. This generally means that it has no known issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.