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().
compat.sh used to skip OpenSSL altogether for DTLS 1.2, because older
versions of OpenSSL didn't support it. But these days it is supported.
We don't want to use DTLS 1.2 with OpenSSL unconditionally, because we
still use legacy versions of OpenSSL to test with legacy ciphers. So
check whether the version we're using supports it.
Without any -O option, the default is -O0, and then the assembly code
is not used, so this would not be a non-regression test for the
assembly code that doesn't build.
Commit 16b1bd8932 "bn_mul.h: add ARM DSP optimized MULADDC code"
added some ARM DSP instructions that was assumed to always be available
when __ARM_FEATURE_DSP is defined to 1. Unfortunately it appears that
the ARMv5TE architecture (GCC flag -march=armv5te) supports the DSP
instructions, but only in Thumb mode and not in ARM mode, despite
defining __ARM_FEATURE_DSP in both cases.
This patch fixes the build issue by requiring at least ARMv6 in addition
to the DSP feature.
Due to how the checking script is run in docker, worktree_rev is
ambiguous when running rev-parse. We're running it in the checked
out worktree, so we can use HEAD instead, which is unambiguous.
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
