Merge mbedtls/development-psa, mbedtls/development into development #51
Conversation
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.
Patater
added
enhancement
|
If you are attempting to reproduce the merge yourself, the current base is 1fb011f. |
|
@Patater Thanks for fixing! I'm now unable to find any issues by just inspecting the code, but the CI is still failing in I only looked at a few failures in I really have no idea why this is happening all the time in the |
Yup, look into what's happening today and see if I can't make a fix. |
I suspect this has to do with the way the entropy module and the PSA subsystem are initialized. Each works on its own, but in an application that calls both |
|
One issue, which I think we need to resolve on the TLS side, is the change to main_test.function to make |
Don't unconditionally enable PSA Crypto for all tests. Only enable it in tests that require it. This allows crypto tests to check that psa_crypto_init() fails when it is supposed to fail, since we want to perform some action in a test, and then call psa_crypto_init() and check the result without it having been called previously.
There was a problem hiding this comment.
I made the merge myself (https://github.com/gilles-peskine-arm/mbed-crypto/tree/prc_51-merge-gilles) and found a few differences, sometimes my mistake, sometimes yours, sometimes indifferent.
include/mbedtls/cipher.h: just whitespace, yours is better.library/cipher.c: I missed a bugfix that was made in the crypto branch (remove dead codereturn( 0 );inmbedtls_cipher_write_tag. Please indicate this in the commit message for the merge.tests/scripts/all.sh: I missed removingUSE_CRYPTO_SUBMODULEfromcomponent_test_use_psa_crypto_full_cmake_asan.tests/scripts/run-test-suites.pl: you missed discarding the addition of a crypto submodule thing.tests/suites/helpers.function: you missed some text when merging the two descriptions ofTEST_ASSERT. Also some trivial whitespace differences.
This was a complicated merge and I'd like a more detailed commit message that contains sufficiently precise instructions to do the same merge (or at least up to trivial differences such as whitespace).
The post-merge commits up to 772ce79 (I mistakenly posted the review from a tab that didn't have the latest commit) look good and I can't think of something else that would be missing.
I made this review under the assumption that Mbed-TLS/mbedtls#2395 is correct. Since then you've found one bug (Mbed-TLS/mbedtls@3ea2687) which will need to be fixed here.
When using PSA with MBEDTLS_ENTROPY_NV_SEED, some test suites require the seed file for PSA initialization, which was normally generated later, when entropy tests were run. This change creates an initial seedfile in all.sh.
Patater
added
the
needs: work
Resolve conflicts by performing the following. - Take the upstream Mbed TLS ChangeLog verbatim. - Reject changes to Makefiles and CMake that are related to using Mbed Crypto as a submodule. It doesn't make sense to use Mbed Crypto as a submodule of itself. - Reject README changes, as Mbed Crypto has its own, different README. - Reject PSA-related changes to config.h. We don't want to disable the availability of the PSA Crypto API by default in the Mbed Crypto config.h. - Don't inadvertently revert dead code removal in mbedtls_cipher_write_tag() which was added in f2a7529 ("Fix double return statement in cipher.c") - Where Mbed Crypto already had some MBEDTLS_USE_PSA_CRYPTO code (from past companion PRs) take the latest version from Mbed TLS which includes integration with MBEDTLS_CHECK_PARAMS. - Update the version of the shared library files to match what's currently present in Mbed TLS. - Reject removal of testing with PSA from config full tests. - Resolve conflicts in test tests/suites/helpers.function, where both Mbed Crypto and Mbed TLS both added documentation for TEST_ASSERT. Combine text from both documentation efforts. - Reject adding a submodule of ourselves. - Reject addition of submodule tests in all.sh. - Reject addition of submodule to library path in tests/scripts/run-test-suites.pl. - Avoid using USE_CRYPTO_SUBMODULE=1 in component_test_use_psa_crypto_full_cmake_asan() in all.sh.
config-default.h should always be a verbatim copy of the default configuration (include/mbedtls/config.h) from Mbed TLS.
Copy our include/mbedtls/config.h file, which is our default configuration, to configs/config-psa-crypto.h, updating what was previously there to the latest defaults.
Silence a compiler warning about implicit fallthrough by using a comment
format the compiler understand to mean that the fallthrough is
intentional.
In file included from library/cipher.c:63:0:
include/mbedtls/psa_util.h: In function ‘mbedtls_psa_translate_cipher_mode’:
include/mbedtls/psa_util.h:91:15: error: this statement may fall through [-Werror=implicit-fallthrough=]
if( taglen == 0 )
^
include/mbedtls/psa_util.h:94:9: note: here
default:
^~~~~~~
cc1: all warnings being treated as errors
$ gcc --version
gcc (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Mbed TLS has deprecated a few module specific error codes in favor of more general-purpose or cross-module error codes. Use these new error codes instead of the deprecated error codes.
When MBEDTLS_CHECK_PARAMS is enabled, it's required to have an implementation of mbedtls_param_failed() present. Without it in the PSA examples, building the PSA examples will result in linker errors like the following. ../../library/libmbedcrypto.a(rsa.c.o): In function `mbedtls_rsa_import': rsa.c:(.text+0x9fd): undefined reference to `mbedtls_param_failed' ../../library/libmbedcrypto.a(rsa.c.o): In function `mbedtls_rsa_import_raw': rsa.c:(.text+0xb0b): undefined reference to `mbedtls_param_failed' ../../library/libmbedcrypto.a(rsa.c.o): In function `mbedtls_rsa_complete': rsa.c:(.text+0xe63): undefined reference to `mbedtls_param_failed' ../../library/libmbedcrypto.a(rsa.c.o): In function `mbedtls_rsa_export_raw': rsa.c:(.text+0xfee): undefined reference to `mbedtls_param_failed' ../../library/libmbedcrypto.a(rsa.c.o): In function `mbedtls_rsa_export': rsa.c:(.text+0x116f): undefined reference to `mbedtls_param_failed' ../../library/libmbedcrypto.a(rsa.c.o):rsa.c:(.text+0x1304): more undefined references to `mbedtls_param_failed' follow collect2: error: ld returned 1 exit status programs/psa/CMakeFiles/crypto_examples.dir/build.make:97: recipe for target 'programs/psa/crypto_examples' failed make[2]: *** [programs/psa/crypto_examples] Error 1 Add an implementation of mbedtls_param_failed() to the PSA Crypto examples to avoid getting this error on the PSA examples.
When `MBEDTLS_PLATFORM_C` is not enabled, our PSA Crypto implementation depends on the standard C library for functions like snprintf() and exit(). However, our implementation was not including the proper header files nor redefining all `mbedtls_*` symbols properly to ensure successful builds without MBEDTLS_PLATFORM_C. Add the necessary header files and macro definitions to our PSA Crypto implementation.
Patater
force-pushed
the
update-dev-tls-dev-crypto-merge
branch
from
772ce79
to
db29ab5
Compare
|
Rebased to take in latest TLS PR and address code review feedback. Previous branch at https://github.com/Patater/mbed-crypto/tree/update-dev-tls-dev-crypto-merge-pr-2 Diff against last branch at https://gist.github.com/Patater/5c8847915bd84b96725e6d3b0d322866 |
|
The current base, if you want to reproduce my merge, is still 1fb011f |
Patater
removed
the
needs: work
|
No more rebasing right now. Feel free to review. |
|
The CI passed, except for know-fragile timing selftest on BSD in the Jenkins pr-merge run. |
There was a problem hiding this comment.
I am happy with the new merge commit 67ea2c5 made with the updated development-psa PR Mbed-TLS/mbedtls#2395 and the same mbed-crypto/development base as my previous review. I reviewed the new merge commit by diff with the previous one. I am also happy with the commit message. I am also happy with the additional commits on top of that (rebased from the previous iteration).
|
2 approvals and CI is ok except for a FreeBSD timing test which is a known issue. This is ready for merge. |
gilles-peskine-arm
removed
the
needs: review
Merge Mbed TLS's merge of https://github.com/ARMmbed/mbedtls/tree/development and https://github.com/ARMmbed/mbedtls/tree/development-psa into Mbed Crypto. This finally aligns the crypto implementation that supports parameter validation with the crypto implementation that supports using TLS on PSA with the crypto implementation in Mbed Crypto which supports using null zero-length buffers in RSA. The source for this merge is the current version of Mbed-TLS/mbedtls#2395.
This is a step along the way to aligning the crypto implementation in all three branches
Mbed-TLS/mbedtls#2395 cannot be merged to the Mbed TLS development branch until Mbed Crypto has an up to date crypto implementation, which this PR brings.
After this PR is merged, Mbed TLS will update the submodule pointer in PR Mbed-TLS/mbedtls#2395 and merge it to development. development-psa will no longer have a reason to exist.
See the merge commit description for an explanation of what merge conflicts were resolved and how. Additional commits are added on top to implement new features in Mbed Crypto like parameter validation support in examples and to remove use of recently deprecated constants.