Bring in changes from Mbed TLS as of 2019-07-22 #187
Merged
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
added 30 commits
June 3, 2019 14:47
- Don't use Doxygen style comments - Document CID and CID length fields.
Quoting the CID draft 04:
- Block Ciphers:
MAC(MAC_write_key, seq_num +
tls12_cid + // New input
DTLSPlaintext.version +
cid + // New input
cid_length + // New input
length_of_DTLSInnerPlaintext + // New input
DTLSInnerPlaintext.content + // New input
DTLSInnerPlaintext.real_type + // New input
DTLSInnerPlaintext.zeros // New input
)
And similar for AEAD and Encrypt-then-MAC.
In contrast to other aspects of the Connection ID extension, the CID-based additional data for MAC computations differs from the non-CID case even if the CID length is 0, because it includes the CID length.
Changes generated via:
% sed -i '/.*CID [0-9]+[0-9]/{n;s/depends_on:/depends_on:MBEDTLS_SSL_CID:/}' test_suite_ssl.data
If a record exhibits an invalid feature only after successful authenticated decryption, this is a protocol violation by the peer and should hence lead to connection failure. The previous code, however, would silently ignore such records. This commit fixes this. So far, the only case to which this applies is the non-acceptance of empty non-AD records in TLS 1.2. With the present commit, such records lead to connection failure, while previously, they were silently ignored. With the introduction of the Connection ID extension (or TLS 1.3), this will also apply to records whose real content type -- which is only revealed during authenticated decryption -- is invalid.
The SSL context structure mbedtls_ssl_context contains several pointers ssl->in_hdr, ssl->in_len, ssl->in_iv, ssl->in_msg pointing to various parts of the record header in an incoming record, and they are setup in the static function ssl_update_in_pointers() based on the _expected_ transform for the next incoming record. In particular, the pointer ssl->in_msg is set to where the record plaintext should reside after record decryption, and an assertion double-checks this after each call to ssl_decrypt_buf(). This commit removes the dependency of ssl_update_in_pointers() on the expected incoming transform by setting ssl->in_msg to ssl->in_iv -- the beginning of the record content (potentially including the IV) -- and adjusting ssl->in_msg after calling ssl_decrypt_buf() on a protected record. Care has to be taken to not load ssl->in_msg before calling mbedtls_ssl_read_record(), then, which was previously the case in ssl_parse_server_hello(); the commit fixes that.
With the introduction of the CID extension, the record content type may change during decryption; we must therefore re-consider every record content type check that happens before decryption, and either move or duplicate it to ensure it also applies to records whose real content type is only revealed during decryption. This commit does this for the silent dropping of unexpected ApplicationData records in DTLS. Previously, this was caught in ssl_parse_record_header(), returning MBEDTLS_ERR_SSL_UNEXPECTED_RECORD which in ssl_get_next_record() would lead to silent skipping of the record. When using CID, this check wouldn't trigger e.g. when delayed encrypted ApplicationData records come on a CID-based connection during a renegotiation. This commit moves the check to mbedtls_ssl_handle_message_type() and returns MBEDTLS_ERR_SSL_NON_FATAL if it triggers, which leads so silent skipover in the caller mbedtls_ssl_read_record().
With the introduction of the CID feature, the stack needs to be able to handle a change of record content type during record protection, which in particular means that the record content type check will need to move or be duplicated. This commit introduces a tiny static helper function which checks the validity of record content types, which hopefully makes it easier to subsequently move or duplicate this check.
The function mbedtls_ssl_hdr_len() returns the length of the record
header (so far: always 13 Bytes for DTLS, and always 5 Bytes for TLS).
With the introduction of the CID extension, the lengths of record
headers depends on whether the records are incoming or outgoing,
and also on the current transform.
Preparing for this, this commit splits mbedtls_ssl_hdr_len() in two
-- so far unmodified -- functions mbedtls_ssl_in_hdr_len() and
mbedtls_ssl_out_hdr_len() and replaces the uses of mbedtls_ssl_hdr_len()
according to whether they are about incoming or outgoing records.
There is no need to change the signature of mbedtls_ssl_{in/out}_hdr_len()
in preparation for its dependency on the currently active transform,
since the SSL context is passed as an argument, and the currently
active transform is referenced from that.
Context: The CID draft does not require that the length of CIDs used for incoming records must not change in the course of a connection. Since the record header does not contain a length field for the CID, this means that if CIDs of varying lengths are used, the CID length must be inferred from other aspects of the record header (such as the epoch) and/or by means outside of the protocol, e.g. by coding its length in the CID itself. Inferring the CID length from the record's epoch is theoretically possible in DTLS 1.2, but it requires the information about the epoch to be present even if the epoch is no longer used: That's because one should silently drop records from old epochs, but not the entire datagrams to which they belong (there might be entire flights in a single datagram, including a change of epoch); however, in order to do so, one needs to parse the record's content length, the position of which is only known once the CID length for the epoch is known. In conclusion, it puts a significant burden on the implementation to infer the CID length from the record epoch, which moreover mangles record processing with the high-level logic of the protocol (determining which epochs are in use in which flights, when they are changed, etc. -- this would normally determine when we drop epochs). Moreover, with DTLS 1.3, CIDs are no longer uniquely associated to epochs, but every epoch may use a set of CIDs of varying lengths -- in that case, it's even theoretically impossible to do record header parsing based on the epoch configuration only. We must therefore seek a way for standalone record header parsing, which means that we must either (a) fix the CID lengths for incoming records, or (b) allow the application-code to configure a callback to implement an application-specific CID parsing which would somehow infer the length of the CID from the CID itself. Supporting multiple lengths for incoming CIDs significantly increases complexity while, on the other hand, the restriction to a fixed CID length for incoming CIDs (which the application controls - in contrast to the lengths of the CIDs used when writing messages to the peer) doesn't appear to severely limit the usefulness of the CID extension. Therefore, the initial implementation of the CID feature will require a fixed length for incoming CIDs, which is what this commit enforces, in the following way: In order to avoid a change of API in case support for variable lengths CIDs shall be added at some point, we keep mbedtls_ssl_set_cid(), which includes a CID length parameter, but add a new API mbedtls_ssl_conf_cid_len() which applies to an SSL configuration, and which fixes the CID length that any call to mbetls_ssl_set_cid() which applies to an SSL context that is bound to the given SSL configuration must use. While this creates a slight redundancy of parameters, it allows to potentially add an API like mbedtls_ssl_conf_cid_len_cb() later which could allow users to register a callback which dynamically infers the length of a CID at record header parsing time, without changing the rest of the API.
Using the Connection ID extension increases the maximum record expansion because - the real record content type is added to the plaintext - the plaintext may be padded with an arbitrary number of zero bytes, in order to prevent leakage of information through package length analysis. Currently, we always pad the plaintext in a minimal way so that its length is a multiple of 16 Bytes. This commit adapts the various parts of the library to account for that additional source of record expansion.
mbedtls_ssl_context contains pointers in_buf, in_hdr, in_len, ... which point to various parts of the header of an incoming TLS or DTLS record; similarly, there are pointers out_buf, ... for outgoing records. This commit adds fields in_cid and out_cid which point to where the CID of incoming/outgoing records should reside, if present, namely prior to where the record length resides. Quoting https://tools.ietf.org/html/draft-ietf-tls-dtls-connection-id-04: The DTLSInnerPlaintext value is then encrypted and the CID added to produce the final DTLSCiphertext. struct { ContentType special_type = tls12_cid; /* 25 */ ProtocolVersion version; uint16 epoch; uint48 sequence_number; opaque cid[cid_length]; // New field uint16 length; opaque enc_content[DTLSCiphertext.length]; } DTLSCiphertext; For outgoing records, out_cid is set in ssl_update_out_pointers() based on the settings in the current outgoing transform. For incoming records, ssl_update_in_pointers() sets in_cid as if no CID was present, and it is the responsibility of ssl_parse_record_header() to update the field (as well as in_len, in_msg and in_iv) when parsing records that do contain a CID. This will be done in a subsequent commit. Finally, the code around the invocations of ssl_decrypt_buf() and ssl_encrypt_buf() is adapted to transfer the CID from the input/output buffer to the CID field in the internal record structure (which is what ssl_{encrypt/decrypt}_buf() uses). Note that mbedtls_ssl_in_hdr_len() doesn't need change because it infers the header length as in_iv - in_hdr, which will account for the CID for records using such.
This commit modifies the code surrounding the invocations of ssl_decrypt_buf() and ssl_encrypt_buf() to deal with a change of record content type during CID-based record encryption/decryption.
Previously, ssl_get_next_record() would fetch 13 Bytes for the record header and hand over to ssl_parse_record_header() to parse and validate these. With the introduction of CID-based records, the record length is not known in advance, and parsing and validating must happen at the same time. ssl_parse_record_header() is therefore rewritten in the following way: 1. Fetch and validate record content type and version. 2. If the record content type indicates a record including a CID, adjust the record header pointers accordingly; here, we use the statically configured length of incoming CIDs, avoiding any elaborate CID parsing mechanism or dependency on the record epoch, as explained in the previous commit. 3. Fetch the rest of the record header (note: this doesn't actually fetch anything, but makes sure that the datagram fetched in the earlier call to ssl_fetch_input() contains enough data). 4. Parse and validate the rest of the record header as before.
This commit changes the stack's behaviour when facing a record with a non-matching CID. Previously, the stack failed in this case, while now we silently skip over the current record.
And add tests for various CID configuration changes during renegotiation to ssl-opt.sh.
* origin/pr/1622: (29 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 checks MBEDTLS_PEM_PARSE_C Restore programs/fuzz/Makefile after in-tree cmake Move fuzz directory to programs Documentation for corpus generation Restore tests/fuzz/Makefile after in-tree cmake Adding ifdefs to avoid warnings for unused globals Adds LDFLAGS fsanitize=address Ignore compiled object files and executables Also clean the fuzz subdirectory copyediting README.md Protecting client/server fuzz targts with ifdefs Makefile support 1 Fuzz README and direct compilation ...
* origin/pr/2660: Fix parsing issue when int parameter is in base 16 Refactor receive_uint32() Refactor get_byte function Make the script portable to both pythons Update the test encoding to support python3 update the test script
* origin/pr/2727: tests: Limit each log to 10 GiB
Patater
added
bug
Patater
force-pushed
the
tls-development-20190722
branch
2 times, most recently
from
9095468 to
ff4f094
Compare
|
CI failure is known issue with ABI checker script. Fix for ABI checker is here. Ran locally and got the following output, which is a pass: |
gilles-peskine-arm
requested changes
Jul 30, 2019
There was a problem hiding this comment.
Looks almost good to me.
I did the locally merge using some cached resolutions from my previous merge at #158 (review) and ended up with some differences:
- Some errors on my part.
- I did not review the changes in
tests/data_filesextensively. If you just runmake -C tests/data_files, you get a complaint aboutmbedtls_cert_req: not found, but it was broken before (#194). When we have a baseline that this works, we can review exactly what we have in the crypto repository. - This time I made the choice to take on the include directory parametrization in the visualc files this time but you didn't. As we discussed last time, both approaches have their benefits and drawbacks.
- See individual comments and questions.
2 tasks
I am a bit confused here about what is being claimed both on your end and my end. Last time, I made it so we could use the same visualc template file style as Mbed TLS, even if the visualc script in Mbed Crypto is not identical. This time, I did not change the visualc generation system within Mbed Crypto. Even when generating visualc from the top level Mbed TLS project, it isn't necessary for the template styles to match (e.g. use of INCLUDE_DIRECTORIES), as Mbed TLS will use its versions of the templates, not Crypto's. Crypto's are used only when Crypto generates its visualc files. Which approach did you go with in your trial merge? |
Patater
force-pushed
the
tls-development-20190722
branch
from
ff4f094 to
cc582c8
Compare
|
Force pushed to fix review feedback:
|
|
About visualc include parametrization: there was a spurious “not” in my sentence, sorry. We both exchanged approaches. But as I said, it doesn't matter: both approaches are acceptable. |
yanesca
previously approved these changes
Jul 31, 2019
There was a problem hiding this comment.
I have reviewed the diff. I couldn't find anything worth mentioning. Looks good to me (given that @gilles-peskine-arm 's feedback has been addressed).
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
...
Patater
force-pushed
the
tls-development-20190722
branch
from
cc582c8 to
8dd1690
Compare
|
Force pushed to address review feedback:
|
gilles-peskine-arm
approved these changes
Jul 31, 2019
|
CI failure is known issue with ABI checker script. Fix for ABI checker is here. Ran locally and got the following output, which is a pass: |
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.
Suggested ways of reviewing:
developmentfiles to the files changed in this PR