* development:
Update crypto to a repo with latest crypto
Update Mbed Crypto
tls: Remove duplicate psa_util.h include
Remove unused cryptography test files
Remove crypto C files
Remove files sourced from Mbed Crypto
config: Fix Doxygen link to MBEDTLS_PARAM_FAILED
Use mbedtls-based path for includes
check-names: Consider crypto-sourced header files
This commit implements the record checking API
mbedtls_ssl_check_record()
on top of the restructured incoming record stack.
Specifically, it makes use of the fact that the core processing routines
ssl_parse_record_header()
mbedtls_ssl_decrypt_buf()
now operate on instances of the SSL record structure mbedtls_record
instead of the previous mbedtls_ssl_context::in_xxx fields.
ssl_get_next_record() updates the legacy in_xxx fields in two places,
once before record decryption and once after. Now that record decryption
doesn't use or affect the in_xxx fields anymore, setting up the these
legacy fields can entirely be moved to the end of ssl_get_next_record(),
which is what this comit does.
This commit solely moves existing code, but doesn't yet simplify the
now partially redundant settings of the in_xxx fields. This will be
done in a separate commit.
Multiple record attributes such as content type and payload length
may change during record decryption, and the legacy in_xxx fields
in the SSL context therefore need to be updated after the record
decryption routine ssl_decrypt_buf() has been called.
After the previous commit has made ssl_prepare_record_content()
independent of the in_xxx fields, setting them can be moved
outside of ssl_prepare_record_content(), which is what this
commit does.
Previously, ssl_update_in_pointers() ensured that the in_xxx pointers
in the SSL context are set to their default state so that the record
header parsing function ssl_parse_record_header() could make use of them.
By now, the latter is independent of these pointers, so they don't need
to be setup before calling ssl_parse_record_header() anymore.
However, other parts of the messaging stack might still depend on it
(to be studied), and hence this commit does not yet reomve
ssl_update_in_pointers() entirely.
The stack maintains pointers mbedtls_ssl_context::in_xxx pointing to
various parts of the [D]TLS record header. Originally, these fields
were determined and set in ssl_parse_record_header(). By now,
ssl_parse_record_header() has been modularized to setup an instance
of the internal SSL record structure mbedtls_record, and to derive
the old in_xxx fields from that.
This commit takes a further step towards removing the in_xxx fields
by deriving them from the established record structure _outside_ of
ssl_parse_record_header() after the latter has succeeded.
One exception is the handling of possible client reconnects,
which happens in the case then ssl_parse_record_header() returns
MBEDTLS_ERR_SSL_UNEXPECTED_RECORD; since ssl_check_client_reconnect()
so far uses the in_xxx fields, they need to be derived from the
record structure beforehand.
This commit makes a first step towards modularizing the incoming record
processing by having it operate on instances of the structure mbedtls_record
representing SSL records.
So far, only record encryption/decryption operate in terms of record
instances, but the rest of the parsing doesn't. In particular,
ssl_parse_record_header() operates directly on the fixed input buffer,
setting the various ssl->in_xxx pointers and fields, and only directly
before/after calling ssl_decrypt_buf() these fields a converted to/from
mbedtls_record instances.
This commit does not yet remove the ssl->in_xxx fields, but makes a step
towards extending the lifetime of mbedtls_record structure representing
incoming records, by modifying ssl_parse_record_header() to setup an
instance of mbedtls_record, and setting the ssl->in_xxx fields from that
instance. The instance so-constructed isn't used further so far, and in
particular it is not yet consolidated with the instance set up for use
in ssl_decrypt_record(). That's for a later commit.
Previously, ssl_parse_record_header() did not check whether the current
datagram is large enough to hold a record of the advertised size. This
could lead to records being silently skipped over or backed up on the
basis of an invalid record length. Concretely, the following would happen:
1) In the case of a record from an old epoch, the record would be
'skipped over' by setting next_record_offset according to the advertised
but non-validated length, and only in the subsequent mbedtls_ssl_fetch_input()
it would be noticed in an assertion failure if the record length is too
large for the current incoming datagram.
While not critical, this is fragile, and also contrary to the intend
that MBEDTLS_ERR_SSL_INTERNAL_ERROR should never be trigger-able by
external input.
2) In the case of a future record being buffered, it might be that we
backup a record before we have validated its length, hence copying
parts of the input buffer that don't belong to the current record.
This is a bug, and it's by luck that it doesn't seem to have critical
consequences.
This commit fixes this by modifying ssl_parse_record_header() to check that
the current incoming datagram is large enough to hold a record of the
advertised length, returning MBEDTLS_ERR_SSL_INVALID_RECORD otherwise.
We don't send alerts on other instances of ill-formed records,
so why should we do it here? If we want to keep it, the alerts
should rather be sent ssl_get_next_record().
As explained in the previous commit, if mbedtls_ssl_fetch_input()
is called multiple times, all but the first call are equivalent to
bounds checks in the incoming datagram.
In DTLS, if mbedtls_ssl_fetch_input() is called multiple times without
resetting the input buffer in between, the non-initial calls are functionally
equivalent to mere bounds checks ensuring that the incoming datagram is
large enough to hold the requested data. In the interest of code-size
and modularity (removing a call to a non-const function which is logically
const in this instance), this commit replaces such a call to
mbedtls_ssl_fetch_input() by an explicit bounds check in
ssl_parse_record_header().
Previously, `ssl_handle_possible_reconnect()` was part of
`ssl_parse_record_header()`, which was required to return a non-zero error
code to indicate a record which should not be further processed because it
was invalid, unexpected, duplicate, .... In this case, some error codes
would lead to some actions to be taken, e.g. `MBEDTLS_ERR_SSL_EARLY_MESSAGE`
to potential buffering of the record, but eventually, the record would be
dropped regardless of the precise value of the error code. The error code
`MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED` returned from
`ssl_handle_possible_reconnect()` did not receive any special treatment and
lead to silent dopping of the record - in particular, it was never returned
to the user.
In the new logic this commit introduces, `ssl_handle_possible_reconnect()` is
part of `ssl_check_client_reconnect()` which is triggered _after_
`ssl_parse_record_header()` found an unexpected record, which is already in
the code-path eventually dropping the record; we want to leave this code-path
only if a valid cookie has been found and we want to reset, but do nothing
otherwise. That's why `ssl_handle_possible_reconnect()` now returns `0` unless
a valid cookie has been found or a fatal error occurred.
Availability of sufficient incoming data should be checked when
it is needed, which is in mbedtls_ssl_fetch_input(), and this
function has the necessary bounds checks in place.
The check is in terms of the internal input buffer length and is
hence likely to be originally intended to protect against overflow
of the input buffer when fetching data from the underlying
transport in mbedtls_ssl_fetch_input(). For locality of reasoning,
it's better to perform such a check close to where it's needed,
and in fact, mbedtls_ssl_fetch_input() _does_ contain an equivalent
bounds check, too, rendering the bounds check in question redundant.
mbedtls_ssl_decrypt_buf() asserts that the passed transform is not NULL,
but the function is only invoked in a single place, and this invocation
is clearly visible to be within a branch ensuring that the incoming
transform isn't NULL. Remove the assertion for the benefit of code-size.
The previous code performed architectural maximum record length checks
both before and after record decryption. Since MBEDTLS_SSL_IN_CONTENT_LEN
bounds the maximum length of the record plaintext, it suffices to check
only once after (potential) decryption.
This must not be confused with the internal check that the record
length is small enough to make the record fit into the internal input
buffer; this is done in mbedtls_ssl_fetch_input().
All modules using restartable ECC operations support passing `NULL`
as the restart context as a means to not use the feature.
The restart contexts for ECDSA and ECP are nested, and when calling
restartable ECP operations from restartable ECDSA operations, the
address of the ECP restart context to use is calculated by adding
the to the address of the ECDSA restart context the offset the of
the ECP restart context.
If the ECP restart context happens to not reside at offset `0`, this
leads to a non-`NULL` pointer being passed to restartable ECP
operations from restartable ECDSA-operations; those ECP operations
will hence assume that the pointer points to a valid ECP restart
address and likely run into a segmentation fault when trying to
dereference the non-NULL but close-to-NULL address.
The problem doesn't arise currently because luckily the ECP restart
context has offset 0 within the ECDSA restart context, but we should
not rely on it.
This commit fixes the passage from restartable ECDSA to restartable ECP
operations by propagating NULL as the restart context pointer.
Apart from being fragile, the previous version could also lead to
NULL pointer dereference failures in ASanDbg builds which dereferenced
the ECDSA restart context even though it's not needed to calculate the
address of the offset'ed ECP restart context.
dummy
Remove use of CMAKE_SOURCE_DIR in case mbedtls is built from within
another CMake project. Define MBEDTLS_DIR to ${CMAKE_CURRENT_SOURCE_DIR}
in the main CMakeLists.txt file and refer to that when defining target
include paths to enable mbedtls to be built as a sub project.
Fixes#2609
Signed-off-by: Ashley Duncan <ashes.man@gmail.com>
Signed-off-by: Jaeden Amero <jaeden.amero@arm.com>
* origin/development: (33 commits)
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
Warn if VLAs are used
Remove redundant compiler flag
Consistently spell -Wextra
Update Mbed Crypto to contain mbed-crypto#152
Improve compatibility with firewalled networks
Dockerfile: apt -> apt-get
Change Docker container to bionic
Clean up file prologue comments
Add docker-based test scripts
ChangeLog: Add ChangeLog entry for #2681
Allow declarations after statements
CMake: Add a subdirectory build regression test
README: Enable builds as a CMake subproject
...
* origin/pr/2706:
Update Mbed Crypto to contain mbed-crypto#152
CMake: Add a subdirectory build regression test
README: Enable builds as a CMake subproject
ChangeLog: Enable builds as a CMake subproject
Remove use of CMAKE_SOURCE_DIR
* origin/pr/2632:
Adapt ChangeLog
Avoid use of large stack buffers in mbedtls_x509_write_crt_pem()
Improve documentation of mbedtls_pem_write_buffer()
Perform CRT writing in-place on the output buffer
Adapt x509write_crt.c to coding style
Remove use of CMAKE_SOURCE_DIR in case mbedtls is built from within
another CMake project. Define MBEDTLS_DIR to ${CMAKE_CURRENT_SOURCE_DIR}
in the main CMakeLists.txt file and refer to that when defining target
include paths to enable mbedtls to be built as a sub project.
Fixes#2609
Signed-off-by: Ashley Duncan <ashes.man@gmail.com>
Signed-off-by: Jaeden Amero <jaeden.amero@arm.com>
* restricted/pr/573:
Remove redundant config.pl call
Add a test for signing content with a long ECDSA key
Add documentation notes about the required size of the signature buffers
Add missing MBEDTLS_ECP_C dependencies in check_config.h
Change size of preallocated buffer for pk_sign() calls
* origin/pr/2711:
programs: Make `make clean` clean all programs always
ssl_tls: Enable Suite B with subset of ECP curves
windows: Fix Release x64 configuration
platform: Include stdarg.h where needed
timing: Remove redundant include file
net_sockets: Fix typo in net_would_block()
* origin/pr/2697:
Update crypto submodule
Add all.sh component that exercises invalid_param checks
Remove mbedtls_param_failed from programs
Make it easier to define MBEDTLS_PARAM_FAILED as assert
Make test suites compatible with #include <assert.h>
Pass -m32 to the linker as well
Don't systematically rebuild programs
* origin/pr/2260:
Update crypto submodule
Remove heading spaces in tests/data_files/Makefile
Re-generate library/certs.c from script
Add new line at the end of test-ca2.key.enc
Use strict syntax to annotate origin of test data in certs.c
Add run to all.sh exercising !MBEDTLS_PEM_PARSE_C + !MBEDTLS_FS_IO
Allow DHM self test to run without MBEDTLS_PEM_PARSE_C
ssl-opt.sh: Auto-skip tests that use files if MBEDTLS_FS_IO unset
Document origin of hardcoded certificates in library/certs.c
Adapt ChangeLog
Rename server1.der to server1.crt.der
Add DER encoded files to git tree
Add build instructions to generate DER versions of CRTs and keys
Document "none" value for ca_path/ca_file in ssl_client2/ssl_server2
ssl_server2: Skip CA setup if `ca_path` or `ca_file` argument "none"
ssl_client2: Skip CA setup if `ca_path` or `ca_file` argument "none"
Correct white spaces in ssl_server2 and ssl_client2
Adapt ssl_client2 to parse DER encoded test CRTs if PEM is disabled
Adapt ssl_server2 to parse DER encoded test CRTs if PEM is disabled
Introduce a new configuration option MBEDTLS_CHECK_PARAMS_ASSERT,
which is disabled by default. When this option is enabled,
MBEDTLS_PARAM_FAILED defaults to assert rather than to a call to
mbedtls_param_failed, and <assert.h> is included.
This fixes#2671 (no easy way to make MBEDTLS_PARAM_FAILED assert)
without breaking backward compatibility. With this change,
`config.pl full` runs tests with MBEDTLS_PARAM_FAILED set to assert,
so the tests will fail if a validation check fails, and programs don't
need to provide their own definition of mbedtls_param_failed().
Increase the SO versions of libmbedx509 and libmbedtls due to the
addition of fields in publicly visible (non-opaque) structs:
- mbedtls_ssl_config
- mbedtls_ssl_context
- mbedtls_x509_crt
When MBEDTLS_SSL_ENCRYPT_THEN_MAC is enabled, but not
MBEDTLS_SSL_SOME_MODES_USE_MAC, mbedtls_ssl_derive_keys() and
build_transforms() will attempt to use a non-existent `encrypt_then_mac`
field in the ssl_transform.
Compile [ 93.7%]: ssl_tls.c
[Error] ssl_tls.c@865,14: 'mbedtls_ssl_transform {aka struct mbedtls_ssl_transform}' ha
s no member named 'encrypt_then_mac'
[ERROR] ./mbed-os/features/mbedtls/src/ssl_tls.c: In function 'mbedtls_ssl_derive_keys'
:
./mbed-os/features/mbedtls/src/ssl_tls.c:865:14: error: 'mbedtls_ssl_transform {aka str
uct mbedtls_ssl_transform}' has no member named 'encrypt_then_mac'
transform->encrypt_then_mac = session->encrypt_then_mac;
^~
Change mbedtls_ssl_derive_keys() and build_transforms() to only access
`encrypt_then_mac` if `encrypt_then_mac` is actually present.
Add a regression test to detect when we have regressions with
configurations that do not include any MAC ciphersuites.
Fixes d56ed2491b ("Reduce size of `ssl_transform` if no MAC ciphersuite is enabled")
- Explain the use of explicit ASN.1 tagging for the extensions structuree
- Remove misleading comment which suggests that mbedtls_x509_get_ext()
also parsed the header of the first extension, which is not the case.
Some functions within the X.509 module return an ASN.1 low level
error code where instead this error code should be wrapped by a
high-level X.509 error code as in the bulk of the module.
Specifically, the following functions are affected:
- mbedtls_x509_get_ext()
- x509_get_version()
- x509_get_uid()
This commit modifies these functions to always return an
X.509 high level error code.
Care has to be taken when adapting `mbetls_x509_get_ext()`:
Currently, the callers `mbedtls_x509_crt_ext()` treat the
return code `MBEDTLS_ERR_ASN1_UNEXPECTED_TAG` specially to
gracefully detect and continue if the extension structure is not
present. Wrapping the ASN.1 error with
`MBEDTLS_ERR_X509_INVALID_EXTENSIONS` and adapting the check
accordingly would mean that an unexpected tag somewhere
down the extension parsing would be ignored by the caller.
The way out of this is the following: Luckily, the extension
structure is always the last field in the surrounding structure,
so if there is some data remaining, it must be an Extension
structure, so we don't need to deal with a tag mismatch gracefully
in the first place.
We may therefore wrap the return code from the initial call to
`mbedtls_asn1_get_tag()` in `mbedtls_x509_get_ext()` by
`MBEDTLS_ERR_X509_INVALID_EXTENSIONS` and simply remove
the special treatment of `MBEDTLS_ERR_ASN1_UNEXPECTED_TAG`
in the callers `x509_crl_get_ext()` and `x509_crt_get_ext()`.
This renders `mbedtls_x509_get_ext()` unsuitable if it ever
happened that an Extension structure is optional and does not
occur at the end of its surrounding structure, but for CRTs
and CRLs, it's fine.
The following tests need to be adapted:
- "TBSCertificate v3, issuerID wrong tag"
The issuerID is optional, so if we look for its presence
but find a different tag, we silently continue and try
parsing the subjectID, and then the extensions. The tag '00'
used in this test doesn't match either of these, and the
previous code would hence return LENGTH_MISMATCH after
unsucessfully trying issuerID, subjectID and Extensions.
With the new code, any data remaining after issuerID and
subjectID _must_ be Extension data, so we fail with
UNEXPECTED_TAG when trying to parse the Extension data.
- "TBSCertificate v3, UIDs, invalid length"
The test hardcodes the expectation of
MBEDTLS_ERR_ASN1_INVALID_LENGTH, which needs to be
wrapped in MBEDTLS_ERR_X509_INVALID_FORMAT now.
Fixes#2431.
When parsing a substructure of an ASN.1 structure, no field within
the substructure must exceed the bounds of the substructure.
Concretely, the `end` pointer passed to the ASN.1 parsing routines
must be updated to point to the end of the substructure while parsing
the latter.
This was previously not the case for the routines
- x509_get_attr_type_and_value(),
- mbedtls_x509_get_crt_ext(),
- mbedtls_x509_get_crl_ext().
These functions kept using the end of the parent structure as the
`end` pointer and would hence allow substructure fields to cross
the substructure boundary. This could lead to successful parsing
of ill-formed X.509 CRTs.
This commit fixes this.
Care has to be taken when adapting `mbedtls_x509_get_crt_ext()`
and `mbedtls_x509_get_crl_ext()`, as the underlying function
`mbedtls_x509_get_ext()` returns `0` if no extensions are present
but doesn't set the variable which holds the bounds of the Extensions
structure in case the latter is present. This commit addresses
this by returning early from `mbedtls_x509_get_crt_ext()` and
`mbedtls_x509_get_crl_ext()` if parsing has reached the end of
the input buffer.
The following X.509 parsing tests need to be adapted:
- "TBSCertificate, issuer two inner set datas"
This test exercises the X.509 CRT parser with a Subject name
which has two empty `AttributeTypeAndValue` structures.
This is supposed to fail with `MBEDTLS_ERR_ASN1_OUT_OF_DATA`
because the parser should attempt to parse the first structure
and fail because of a lack of data. Previously, it failed to
obey the (0-length) bounds of the first AttributeTypeAndValue
structure and would try to interpret the beginning of the second
AttributeTypeAndValue structure as the first field of the first
AttributeTypeAndValue structure, returning an UNEXPECTED_TAG error.
- "TBSCertificate, issuer, no full following string"
This test exercises the parser's behaviour on an AttributeTypeAndValue
structure which contains more data than expected; it should therefore
fail with MBEDTLS_ERR_ASN1_LENGTH_MISMATCH. Because of the missing bounds
check, it previously failed with UNEXPECTED_TAG because it interpreted
the remaining byte in the first AttributeTypeAndValue structure as the
first byte in the second AttributeTypeAndValue structure.
- "SubjectAltName repeated"
This test should exercise two SubjectAltNames extensions in succession,
but a wrong length values makes the second SubjectAltNames extension appear
outside of the Extensions structure. With the new bounds in place, this
therefore fails with a LENGTH_MISMATCH error. This commit adapts the test
data to put the 2nd SubjectAltNames extension inside the Extensions
structure, too.
All of them are copied from (former) CRT and key files in `tests/data_files`.
For files which have been regenerated since they've been copied to `certs.c`,
update the copy.
Add declarations for DER encoded test CRTs to certs.h
Add DER encoded versions of CRTs to certs.c
fix comment in certs.c
Don't use (signed) char for DER encoded certificates
Consistently use `const char *` for test CRTs regardless of encoding
Remove non-sensical and unused PW variable for DER encoded key
Provide test CRTs in PEM and DER fmt, + pick suitable per config
This commit modifies `certs.h` and `certs.c` to start following the
following pattern for the provided test certificates and files:
- Raw test data is named `NAME_ATTR1_ATTR2_..._ATTRn`
For example, there are
`TEST_CA_CRT_{RSA|EC}_{PEM|DER}_{SHA1|SHA256}`.
- Derived test data with fewer attributes, iteratively defined as one
of the raw test data instances which suits the current configuration.
For example,
`TEST_CA_CRT_RSA_PEM`
is one of `TEST_CA_CRT_RSA_PEM_SHA1` or `TEST_CA_CRT_RSA_PEM_SHA256`,
depending on whether SHA-1 and/or SHA-256 are defined in the current
config.
Add missing public declaration of test key password
Fix signedness and naming mismatches
Further improve structure of certs.h and certs.c
Fix definition of mbedtls_test_cas test CRTs depending on config
Remove semicolon after macro string constant in certs.c
This commit modifies mbedtls_ssl_get_peer_cid() to also allow passing
NULL pointers in the arguments for the peer's CID value and length, in
case this information is needed.
For example, some users might only be interested in whether the use of
the CID was negotiated, in which case both CID value and length pointers
can be set to NULL. Other users might only be interested in confirming
that the use of CID was negotiated and the peer chose the empty CID,
in which case the CID value pointer only would be set to NULL.
It doesn't make sense to pass a NULL pointer for the CID length but a
non-NULL pointer for the CID value, as the caller has no way of telling
the length of the returned CID - and this case is therefore forbidden.
This commit modifies the CID configuration API mbedtls_ssl_conf_cid_len()
to allow the configuration of the stack's behaviour when receiving an
encrypted DTLS record with unexpected CID.
Currently, the stack silently ignores DTLS frames with an unexpected CID.
However, in a system which performs CID-based demultiplexing before passing
datagrams to the Mbed TLS stack, unexpected CIDs are a sign of something not
working properly, and users might want to know about it.
This commit introduces an SSL error code MBEDTLS_ERR_SSL_UNEXPECTED_CID
which the stack can return in response to an unexpected CID. It will
conditionally be put to use in subsequent commits.
There are two options:
1. Don't set it, and don't use it during record protection,
guarding the respective paths by a check whether TLS or
DTLS is used.
2. Set it to the default value even for TLS, and avoid the
protocol-dependent branch during record protection.
This commit picks option 2.
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.
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 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.
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.
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.
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.
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.
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.
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().
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.
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.
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.
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.
This commit temporarily comments the copying of the negotiated CIDs
into the established ::mbedtls_ssl_transform in mbedtls_ssl_derive_keys()
until the CID feature has been fully implemented.
While mbedtls_ssl_decrypt_buf() and mbedtls_ssl_encrypt_buf() do
support CID-based record protection by now and can be unit tested,
the following two changes in the rest of the stack are still missing
before CID-based record protection can be integrated:
- Parsing of CIDs in incoming records.
- Allowing the new CID record content type for incoming records.
- Dealing with a change of record content type during record
decryption.
Further, since mbedtls_ssl_get_peer_cid() judges the use of CIDs by
the CID fields in the currently transforms, this change also requires
temporarily disabling some grepping for ssl_client2 / ssl_server2
debug output in ssl-opt.sh.
This commit modifies ssl_decrypt_buf() and ssl_encrypt_buf()
to include the CID into authentication data during record
protection.
It does not yet implement the new DTLSInnerPlaintext format
from https://tools.ietf.org/html/draft-ietf-tls-dtls-connection-id-04
