Update to Mbed TLS 2.16.0 #12
Closed
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 commit introduces macros * MBEDTLS_DEPRECATED_STRING_CONSTANT * MBEDTLS_DEPRECATED_NUMERIC_CONSTANT to platform_util.h which can be used to deprecate public macro constants. Their definition is essentially taken from dhm.h where the MBEDTLS_DEPRECATED_STRING_CONSTANT was used to deprecate insecure hardcoded DHM primes.
Deprecate the old specific error codes * MBEDTLS_ERR_ARIA_INVALID_KEY_LENGTH * MBEDTLS_ERR_ARIA_INVALID_INPUT_LENGTH
Deprecate the old specific error codes * MBEDTLS_ERR_CAMELLIA_INVALID_KEY_LENGTH * MBEDTLS_ERR_CAMELLIA_INVALID_INPUT_LENGTH
Deprecate the old specific error codes * MBEDTLS_ERR_BLOWFISH_INVALID_KEY_LENGTH * MBEDTLS_ERR_BLOWFISH_INVALID_INPUT_LENGTH
The previous introduction of constant deprecation macros
in platform_util.h lead to failure of tests/scrips/check-names.sh
because the regular expressions in the latter choked on the brackets
in the part `__attribute__((deprecated))` of the definition of the
helper type `mbedtls_deprecated_{numeric|string}_constant_t`.
Postponing any further study and potential robustness improvements
in check-names.sh to another time, this commit circumvents this
problem by temporarily abbreviating `__attribute__((deprecated))`
as `MBEDTLS_DEPRECATED`, which doesn't lead to problems with
check-names.sh.
Merging MBEDTLS_ERR_CAMELLIA_INVALID_INPUT_LENGTH and MBEDTLS_ERR_CAMELLIA_INVALID_KEY_LENGTH is an API break.
The check was already done later when calling ECB, (as evidenced by the tests passing, which have a call with data_unit set to NULL), but it's more readable to have it here too, and more helpful when debugging.
The check is mandatory as skipping it results in buffer overread of arbitrary size.
Clarified and made more coherent the parameter validation feature, it's scope and what has changed. Added version 2.14.1 to the history which was released on a branch.
Update the version of the library to 2.16.0
Resolve conflicts in the following areas:
- Resolve conflicts in library/CMakeLists.txt around changes that
enable use of Mbed Crypto as a submodule and the version bump to
Mbed TLS 2.16.0.
- Resolve conflicts in the cipher and pk modules, where parameter
validation and changes to enable use of PSA collide. Added parameter
validation to mbedtls_cipher_setup_psa().
- Resolve incompatibilties in the RSA module where changes made for
parameter validation prevent our PSA Crypto implementation from
working. The PSA Crypto implementation depends on being able to pass
zero-length buffers that are NULL to RSA encryption functions. This
de-facto reverts 2f660d0 ("Forbid passing NULL input buffers to
RSA encryption routines").
- Resolve conflicts in test tests/suites/helpers.function, where both
Mbed Crypto and Mbed TLS both added documentation for TEST_ASSERT.
Go with the Mbed TLS version, de-facto reverting 8954d0c
("Write documentation for TEST_ASSERT").
- Resolve conflicts in ChangeLog, due to extra entries that shipped
with 2.15 and Mbed Crypto 1.0.0b2 that didn't make it into Mbed TLS
2.16.0.
- Resolve conflicts in library/ssl_srv.c, where parameter validation
and changes to the ECDH alternate implementation interface clashed.
Patater
added
bug
|
The merge conflict required a series of nontrivial design decisions, such as all the places where the objectives of “parameter validation” conflict with changes made for the PSA crypto API or for TLS-over-PSA. I would strongly prefer to have separate commits on one or both sides to make these design changes, and have a merge conflict which only takes obvious decisions. |
gilles-peskine-arm
requested changes
Jan 11, 2019
There was a problem hiding this comment.
The design choices made in the merge commit are too hard to follow. Additionally there are mistakes. I started to do a merge at https://github.com/gilles-peskine-arm/mbed-crypto/tree/feature-psa+2.16.0-gilles-1 but gave up for include/mbedtls/cipher.h and library/cipher.c.
- The changes in
cipher.care too messy to figure out. Please make one or more commit on either or both sides to bring them closer so that readers can see what's going on. - We both got
rsa.hwrong: you removed the ability to pass empty input to encryption functions but kept it documented as allowed, I did the opposite. - In
rsa.c, inmbedtls_rsa_rsaes_pkcs1_v15_decrypt, the caseoutput_max_len == 0is not handled correctly. You need so skip the finalmemcpyintooutput, and you need to do it right to avoid breaking the constant-time property of the code. Constant-time code is delicate, please avoid touching it as part of a bigger commit. - In
cipher.handcipher.c, there are whitespace-only changes that conflict with semantic changes. This isn't an absolute blocker, but it's far from ideal. Since the merge needs rework anyway, please make whitespace changes in a separate commit before the merge to avoid a whitespace/semantic conflict. - In
helpers.function, forTEST_ASSERT, you kept the copy of the macro definition with the wrong indentation, and you removed useful documentation from one of the branches.
Please make pre-merge commits on one or both sides to bring them closer in a controlled, comprehensible way, so that the merge commit has no conflict or only easily-understood conflicts. An add-add conflict is ok, reconciling documentation improvements is ok, but doing non-cosmetic changes to constant-time code is not ok and the conflicts in cipher.c are too complex.
|
I don't like giant merges any more than the next guy. I would suggest we break this up instead, using multiple merges to get us to 2.16.0. As such, I'm closing this PR. Regarding the review feedback:
|
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.
This brings our crypto implementation up to date with the latest Mbed TLS crypto. This primarily brings us the parameter validation feature and new ECDH alternative implementation interface, but also other security and bug fixes.
All code has been reviewed already, except the merge commit, which is pretty hairy. Please review the merge commit (probably best done with command line tools, not GitHub at least.)