Update Mbed Crypto with latest Mbed TLS changes as of 2019-11-15 #322
Conversation
This file contains hidden or 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
This is enabled by default as we generally enable things by default unless there's a reason not to (experimental, deprecated, security risk). We need a compile-time option because, even though the functions themselves can be easily garbage-collected by the linker, implementing them will require saving 64 bytes of Client/ServerHello.random values after the handshake, that would otherwise not be needed, and people who don't need this feature shouldn't have to pay the price of increased RAM usage.
Also introduce stub definitions so that things compile and link.
Enforce restrictions indicated in the documentation. This allows to make some simplifying assumptions (no need to worry about saving IVs for CBC in TLS < 1.1, nor about saving handshake data) and guarantees that all values marked as "forced" in the design document have the intended values and can be skipped when serialising. Some of the "forced" values are not checked because their value is a consequence of other checks (for example, session_negotiated == NULL outside handshakes). We do however check that session and transform are not NULL (even if that's also a consequence of the initial handshake being over) as we're going to dereference them and static analyzers may appreciate the info.
This mainly follows the design document (saving all fields marked "saved" in the main structure and the transform sub-structure) with two exceptions: - things related to renegotiation are excluded here (there weren't quite in the design document as the possibility of allowing renegotiation was still on the table, which is no longer is) - also, ssl.secure_renegotiation (which is not guarded by MBEDTLS_SSL_RENEGOTIATION because it's used in initial handshakes even with renegotiation disabled) is still excluded, as we don't need it after the handshake. - things related to Connection ID are added, as they weren't present at the time the design document was written. The exact format of the header (value of the bitflag indicating compile-time options, whether and how to merge it with the serialized session header) will be determined later.
The number of meaning of the flags will be determined later, when handling the relevant struct members. For now three bytes are reserved as an example, but this number may change later.
For now, the header (version+format bytes) is duplicated. This might be optimized later.
Previously it was missing reset in case 1, and in case 2 the code was never executed as the option value was reset to 0. Tighten checking of return values of save(NULL, 0) now that it works. Also, improve the printed output as well as the comments. I checked manually that everything now works and fail in the expected way: save, reset-or-reinit and load all succeed, but the subsequent read or write fails.
Update crypto submodule
…ake-jobs-2 Fix parallel make jobs for shared target
Use a common set of options when building with Asan without CMake.
When building with make with the address sanitizer enabled, also enable the undefined behavior sanitizer.
Some sanitizers default to displaying an error message and recovering. This could result in a test being recorded as passing despite a complaint from the sanitizer. Turn off sanitizer recovery to avoid this risk.
When running 'make test' with GNU make, if a test suite program displays "PASSED", this was automatically counted as a pass. This would in particular count as passing: * A test suite with the substring "PASSED" in a test description. * A test suite where all the test cases succeeded, but the final cleanup failed, in particular if a sanitizer reported a memory leak. Use the test executable's return status instead to determine whether the test suite passed. It's always 0 on PASSED unless the executable's cleanup code fails, and it's never 0 on any failure. Fix ARMmbed#303
…opment Make sure Asan failures are detected in 'make test'
Record checking fails if mbedtls_ssl_check_record() is called with external buffer. Received record sequence number is available in the incoming record but it is not available in the ssl contexts `in_ctr`- variable that is used when decoding the sequence number. To fix the problem, temporarily update ssl context `in_ctr` to point to the received record header and restore value later.
Fix mbedtls_ssl_check_record usage with ext buf
Using 4096 bytes of stack for the temporary buffer used for holding a throw-away DER-formatted CSR limits the portability of generating certificate signing requests to only devices with lots of stack space. To increase portability, use the mbedtls_pem_write_buffer() in-place capability instead, using the same buffer for input and output. This works since the DER encoding for some given data is always smaller than that same data PEM-encoded. PEM format is desirable to use even on stack-constrained devices as the format is easy to work with (for example, copy-pasting from a tiny device's serial console output, for CSRs generated on tiny devices without the private key leaving said tiny device).
* ARMmbed#292: Make psa_close_key(0) and psa_destroy_key(0) succeed * ARMmbed#299: Allow xxx_drbg_set_entropy_len before xxx_drbg_seed * ARMmbed#259: Check `len` against buffers size upper bound in PSA tests * ARMmbed#288: Add ECDSA tests with hash and key of different lengths * ARMmbed#305: CTR_DRBG: grab a nonce from the entropy source if needed * ARMmbed#316: Stop transactions from being reentrant * ARMmbed#317: getting_started: Make it clear that keys are passed in * ARMmbed#314: Fix pk_write with EC key to use a constant size for the private value * ARMmbed#298: Test a build without any asymmetric cryptography * ARMmbed#284: Fix some possibly-undefined variable warnings * ARMmbed#315: Define MBEDTLS_PK_SIGNATURE_MAX_SIZE * ARMmbed#318: Finish side-porting commits from mbedtls-restricted that missed the split
Use the constant that is now provided by the crypto submodule instead of rolling our own definition which is not correct in all cases.
Use the constant that is now provided by the crypto submodule instead of rolling our own definition which is not correct in all cases.
MBEDTLS_PK_SIGNATURE_MAX_SIZE is tested in Mbed Crypto. Its effect on Mbed TLS is also tested via the X.509 tests. The case of MBEDTLS_MPI_MAX_SIZE < MBEDTLS_ECDSA_MAX_LEN, for which this component was added as a regression test, is covered by config-suite-b.h which is tested via test-ref-configs.pl.
…e-tls Use MBEDTLS_PK_SIGNATURE_MAX_SIZE
x509write_csr: Reduce stack usage of mbedtls_x509write_csr_pem()
Since "Remove component designed to test MAX_SIGNATURE_SIZE", secp521r1_prv.der is no longer used. ec_521_prv.pem can be used for the same purpose.
…e-tls-rm_521 Remove unused test data file
…rypto-development-20191115
First deal with deleted files.
* Files deleted by us: keep them deleted.
* Files deleted by them, whether modified by us or not: keep our version.
```
git rm $(git status -s | sed -n 's/^DU //p')
git reset -- $(git status -s | sed -n 's/^D //p')
git checkout -- $(git status -s | sed -n 's/^ D //p')
git add -- $(git status -s | sed -n 's/^UD //p')
```
Individual files with conflicts:
* `3rdparty/everest/library/Hacl_Curve25519_joined.c`: spurious conflict because git mistakenly identified this file as a rename. Keep our version.
* `README.md`: conflict due to their change in a paragraph that doesn't exist in our version. Keep our version of this paragraph.
* `docs/architecture/Makefile`: near-identical additions. Adapt the definition of `all_markdown` and include the clean target.
* `doxygen/input/docs_mainpage.h`: conflict in the version number. Keep our version number.
* `include/mbedtls/config.h`: two delete/modify conflicts. Keep the removed chunks out.
* `library/CMakeLists.txt`: discard all their changes as they are not relevant.
* `library/Makefile`:
* Discard the added chunk about the crypto submodule starting with `INCLUDING_FROM_MBEDTLS:=1`.
* delete/modify: keep the removed chunk out.
* library build: This is almost delete/modify. Their changes are mostly not applicable. Do keep the `libmbedcrypto.$(DLEXT): | libmbedcrypto.a` order dependency.
* `.c.o`: `-o` was added on both sides but in a different place. Change to their place.
* `library/error.c`: to be regenerated.
* `library/version_features.c`: to be regenerated.
* `programs/Makefile`: Most of the changes are not relevant. The one relevant change is in the `clean` target for Windows; adapt it by removing `/S` from our version.
* `programs/test/query_config.c`: to be regenerated.
* `scripts/config.py`: added in parallel on both sides. Keep our version.
* `scripts/footprint.sh`: parallel changes. Keep our version.
* `scripts/generate_visualc_files.pl`: one delete/modify conflict. Keep the removed chunks out.
* `tests/Makefile`: discard all of their changes.
* `tests/scripts/all.sh`:
* `pre_initialize_variables` add `append_outcome`: add it.
* `pre_initialize_variables` add `ASAN_CFLAGS`: already there, keep our version.
* `pre_parse_command_line` add `--no-append-outcome`: add it.
* `pre_parse_command_line` add `--outcome-file`: add it.
* `pre_print_configuration`: add `MBEDTLS_TEST_OUTCOME_FILE`.
* Several changes in SSL-specific components: keep our version without them.
* Several changes where `config.pl` was changed to `config.py` and there was an adjacent difference: keep our version.
* Changes regarding the inclusion of `MBEDTLS_MEMORY_xxx`: ignore them here, they will be normalized in a subsequent commit.
* `component_test_full_cmake_gcc_asan`: add it without the TLS tests.
* `component_test_no_use_psa_crypto_full_cmake_asan`: keep the fixed `msg`, discard other changes.
* `component_test_memory_buffer_allocator_backtrace`, `component_test_memory_buffer_allocator`: add them without the TLS tests.
* `component_test_m32_everest`: added in parallel on both sides. Keep our version.
* `tests/scripts/check-names.sh`, `tests/scripts/list-enum-consts.pl`, `tests/scripts/list-identifiers.sh`, ``tests/scripts/list-macros.sh`: discard all of their changes.
* `tests/scripts/test-ref-configs.pl`: the change in the conflict is not relevant, so keep our version there.
* `visualc/VS2010/*.vcxproj`: to be regenerated.
Regenerate files:
```
scripts/generate_visualc_files.pl
git add visualc/VS2010/*.vcxproj
scripts/generate_errors.pl
git add library/error.c
scripts/generate_features.pl
git add library/version_features.c
scripts/generate_query_config.pl
git add programs/test/query_config.c
```
Rejected changes in non-conflicting files:
* `CMakeLists.txt`: discard their addition which has already been side-ported.
* `doxygen/mbedtls.doxyfile`: keep the version number change. Discard the changes related to `../crypto` paths.
Keep the following changes after examination:
* `.travis.yml`: all of their changes are relevant.
* `include/mbedtls/error.h`: do keep their changes. Even though Crypto doesn't use TLS errors, it must not encroach on TLS's allocated numbers.
* `tests/scripts/check-test-cases.py`: keep the code dealing with `ssl-opt.sh`. It works correctly when the file is not present.
Enabling MBEDTLS_MEMORY_BUFFER_ALLOC_C module together with MBEDTLS_PLATFORM_MEMORY causes the library to use its own malloc replacement. This makes memory management analyzers such as ASan largely ineffective. We now test MBEDTLS_MEMORY_BUFFER_ALLOC_C separately. Disable it in the "full" config. This mirrors a change that was made in Mbed TLS on config.pl and had not been ported to Mbed Crypto yet. With this commit, config.py is aligned in Mbed Crypto and Mbed TLS.
gilles-peskine-arm
added
needs: ci
dgreen-arm
approved these changes
Nov 18, 2019
Patater
reviewed
Nov 18, 2019
There was a problem hiding this comment.
I did the merge myself and ran into a few meaningful differences, listed below. I ran into other differences as well, but those are stylistic and I don't mind going with another style.
None of these are blockers.
The diffs are - for your PR version, and + for my merge.
- I kept the
/Sinprograms/Makefile. This provides consistency with what came before in the crypto branch, as/Swas already present there. No TLS branch removes/Sintentionally, so we shouldn't remove/Sfrom Crypto by this merge.
diff --git a/programs/Makefile b/programs/Makefile
index f56df5f97fae..add1a8649b49 100644
--- a/programs/Makefile
+++ b/programs/Makefile
@@ -249,8 +249,8 @@ ifndef WINDOWS
rm -f $(APPS) $(EXTRA_GENERATED)
-rm -f test/cpp_dummy_build$(EXEXT)
else
- if exist *.o del /Q /F *.o
- if exist *.exe del /Q /F *.exe
+ if exist *.o del /S /Q /F *.o
+ if exist *.exe del /S /Q /F *.exe
if exist $(EXTRA_GENERATED) del /S /Q /F $(EXTRA_GENERATED)
endif- In
component_test_no_use_psa_crypto_full_cmake_asan()inall.sh, I aligned with TLS by unsettingMBEDTLS_PSA_CRYPTO_C. We shouldn't make this test test more different configurations than it currently does: the merge should align the configuration used by this test where applicable.
Patater
removed
the
needs: review
|
Given approving reviews and no blocking issues, merging. We can further align the TLS and Crypto branches as desired in follow up PRs. |
gilles-peskine-arm
added a commit
to gilles-peskine-arm/mbed-crypto
that referenced
this pull request
Feb 3, 2020
* ARMmbed#321: Replace config.pl by config.py * ARMmbed#322: Update Mbed Crypto with latest Mbed TLS changes as of 2019-11-15 * ARMmbed#308: Small performance improvement of mbedtls_mpi_div_mpi() * ARMmbed#324: test_psa_constant_names: support key agreement, better code structure * ARMmbed#320: Link to the PSA crypto portal page from README.md * ARMmbed#293: Always gather MBEDTLS_ENTROPY_BLOCK_SIZE bytes of entropy * ARMmbed#310: Clarify test descriptions in test_suite_memory_buffer_alloc * ARMmbed#307: Add ASN.1 ENUMERATED tag support * ARMmbed#328: Remove dependency of crypto_values.h on crypto_extra.h * ARMmbed#325: Rename psa_asymmetric_{sign_verify} to psa_{sign,verify}_hash Missed listing in the previous submodule update: * ARMmbed#304: Make sure Asan failures are detected in 'make test'
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.
The previous update was #223. Salient changes:
config.py, in particular withMBEDTLS_MEMORY_BUFFER_ALLOC_Cdisabled inconfig.py full.This is a merge commit (9afbfdc) and its follow-up.
CI passed and produced the expected outcome file.
Internal ref: IOTCRYPT-974