Commit Graph

10028 Commits

Author SHA1 Message Date
Hanno Becker
f903dc8354 Make mbedtls_ssl_in_hdr_len() CID-unaware
The function mbedtls_ssl_in_hdr_len() is supposed to return the length
of the record header of the current incoming record. With the advent
of the DTLS Connection ID, this length is only known at runtime and
hence so far needed to be derived from the internal in_iv pointer
pointing to the beginning of the payload of the current incooing
record.

By now, however, those uses of mbedtls_ssl_in_hdr_len() where the
presence of a CID would need to be detected have been removed
(specifically, ssl_parse_record_header() doesn't use it anymore
when checking that the current datagram is large enough to hold
the record header, including the CID), and it's sufficient to
statically return the default record header sizes of 5 / 13 Bytes
for TLS / DTLS.
2019-08-01 09:51:53 +02:00
Hanno Becker
05413d9041 Remove duplicate setting of ssl->in_msgtype and ssl->in_msglen 2019-08-01 09:51:53 +02:00
Hanno Becker
bd70c8e771 Move update of in_xxx fields in ssl_get_next_record()
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.
2019-08-01 09:51:53 +02:00
Hanno Becker
bf256cdb0b Move update of in_xxx fields outside of ssl_prepare_record_content()
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.
2019-08-01 09:51:53 +02:00
Hanno Becker
106f3dab57 Reduce dependency of ssl_prepare_record_content() on in_xxx fields 2019-08-01 09:51:53 +02:00
Hanno Becker
68379720b6 Move ssl_update_in_pointers() to after record hdr parsing
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.
2019-08-01 09:51:53 +02:00
Hanno Becker
fc55172c41 Mark DTLS replay check as const on the SSL context 2019-08-01 09:51:53 +02:00
Hanno Becker
6941245852 Move updating the internal rec ptrs to outside of rec hdr parsing
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.
2019-08-01 09:51:53 +02:00
Hanno Becker
40478be987 Mark ssl_decrypt_buf() as `const in the input SSL context
In fact, the SSL context is only used to access the debug callback.
2019-08-01 09:51:52 +02:00
Hanno Becker
a89610aaf2 Adapt ssl_prepare_record_content() to use SSL record structure 2019-08-01 09:51:52 +02:00
Hanno Becker
9babbf7e75 Use record length from record structure when fetching content in TLS 2019-08-01 09:51:52 +02:00
Hanno Becker
2720f4c33c Use record structure when remembering offset of next record in dgram 2019-08-01 09:51:52 +02:00
Hanno Becker
2528ee09ac Use SSL record structure when skipping over unexpected record 2019-08-01 09:51:52 +02:00
Hanno Becker
af5bcfc765 Adapt ssl_buffer_future_record() to work with SSL record structure 2019-08-01 09:51:52 +02:00
Hanno Becker
c6e7c573d9 Setup SSL record structure in ssl_parse_record_header()
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.
2019-08-01 09:51:52 +02:00
Hanno Becker
e84b28cb9d Expand documentation of internal mbedtls_record structure 2019-08-01 09:51:52 +02:00
Hanno Becker
6c0e53ce6f Minor documentation improvements in ssl_parse_record_header() 2019-08-01 09:51:51 +02:00
Hanno Becker
e04527755b Check for sufficient datagram size in ssl_parse_record_header()
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.
2019-08-01 09:51:51 +02:00
Hanno Becker
a61925fa51 Don't send an alert when receiving a record of unknown ContentType
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().
2019-08-01 09:51:49 +02:00
Hanno Becker
dc4d62748c Don't call ssl_fetch_input for record content fetch in DTLS
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.
2019-08-01 09:50:27 +02:00
Hanno Becker
29823466a1 Don't call ssl_fetch_input for record hdr size check in DTLS
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().
2019-08-01 09:50:27 +02:00
Hanno Becker
de7d6d33e5 Move size-check for DTLS record header with CID to DTLS-only branch 2019-08-01 09:50:27 +02:00
Hanno Becker
87b5626d73 Check same-port-reconnect from client outside of record hdr parsing
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.
2019-08-01 09:50:27 +02:00
Hanno Becker
07d420d6ad Remove unnecessary backup of explicit IV in AEAD record decryption
There is no need to hold back the explicit IV for AEAD ciphers.
2019-08-01 09:50:27 +02:00
Hanno Becker
8244cfa8bc Remove redundant minimum length check
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.
2019-08-01 09:50:27 +02:00
Hanno Becker
6d3db0fa25 Improve documentation of mbedtls_ssl_decrypt_buf() 2019-08-01 09:50:26 +02:00
Hanno Becker
9520b31860 Remove misleading comment in mbedtls_ssl_decrypt_buf()
The comment doesn't seem to relate to the code that follows.
2019-08-01 09:50:26 +02:00
Hanno Becker
b603bd34bc Remove assertion in mbedtls_ssl_decrypt_buf()
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.
2019-08-01 09:50:26 +02:00
Hanno Becker
f024285034 Check architectural bound for max record payload len in one place
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().
2019-08-01 09:50:26 +02:00
Hanno Becker
408a2742b3 Remove redundant length-0 checks for incoming unprotected records 2019-08-01 09:50:26 +02:00
Hanno Becker
1c26845777 Remove redundant length check during record header parsing
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.
2019-08-01 09:50:26 +02:00
Ron Eldor
420f3589e3 Fix the license header of hkdf
Change the license header of `hkdf.h` to a format the that script
`apache_to_gpl.pl` knows how to parse.
2019-07-31 14:04:38 +03:00
Manuel Pégourié-Gonnard
6852e95c2a
Merge pull request #618 from hanno-arm/record_checking_api-baremetal
[Baremetal] Record checking: API, Documentation and Stubs
2019-07-30 17:11:46 +02:00
Manuel Pégourié-Gonnard
2b29a37c91 Fix compile bugs in examples with hardcoded I/O callbacks
These were found by `all.sh test_baremetal`, so no need for a new test.
2019-07-30 17:07:38 +02:00
Manuel Pégourié-Gonnard
f1358acdc7 Fix bug in MBEDTLS_X509_CRT_REMOVE_TIME
When looking for a parent, all candidates were considered time-invalid due to
the #ifdef incorrectly including the `parent_valid = 1` line.

When MBEDTLS_HAVE_TIME_DATE is unset the time-validity of certificates is
never checked and always treated as valid. This is usually achieved by proper
usage of mbedtls_x509_time_is_past() and mbedtls_x509_time_is_future() (and
their definition when we don't HAVE_TIME_DATE).

Here the calls to these functions needs to be guarded by
MBEDTLS_X509_CRT_REMOVE_TIME as they access struct members whose presence is
controlled by this option. But the "valid" branch should still always be taken.

(Note: MBEDTLS_X509_CRT_REMOVE_TIME being set forces MBEDTLS_HAVE_TIME_DATE to
be unset, as enforce by check_config.h.)

This bug was found by `all.sh test_baremetal` - no need for a new test.
2019-07-30 16:56:58 +02:00
Manuel Pégourié-Gonnard
80eaddfc36 Clean generated *.su file and gitignore them 2019-07-30 16:56:58 +02:00
Manuel Pégourié-Gonnard
d64a2f72e6 Fix wrong ifdef in ssl_server2 & add test for it
This was found as a warning when running scripts/baremetal.sh --ram
--build-only manually, but it should have been found in a more automated way.
Adding -Werror so that future such issues will be caught by all.sh
(component_test_baremetal already invokes baremetal.sh --ram --build-only).
2019-07-30 16:56:43 +02:00
Manuel Pégourié-Gonnard
0d1db20490 Fix bug in skip_date() (MBEDTLS_X509_CRT_REMOVE_TIME)
Asserting `*p == end` right after setting `end = *p + len` will always fail
unless `len == 0`, which is never the case with properly-formed certificates.

The function x509_skip_dates() is modelled after x509_get_dates() which between
setting `end` and comparing it to `*p` calls mbedtls_x509_get_time() which
advances `*p` to the expected value, which is why this test works in
get_dates().

Since `skip_dates()` has `skip`, not `validate` in its name, and the entire
point of `MBEDTLS_X509_CRT_REMOVE_TIME` is to save code, we don't want to
call the relatively large functions needed to properly parse (and validate)
dates before throwing the parsed dates away, we can just fast-forward to the
end of the sequence.

This makes updating `end` and comparing it to `*p` after the fast-forward
redundant, as the comparison will always be true (unlike the case where we
actually parse the contents of the sequence).

This bug was found by `all.sh test_baremetal` - no need for a new test.
2019-07-30 16:56:25 +02:00
Hanno Becker
62daad3b9a all.sh: Adhere to convention that build_ prefixes build-only tests 2019-07-30 16:56:25 +02:00
Hanno Becker
93de2965d0 Fix rebase slip 2019-07-30 16:56:25 +02:00
Hanno Becker
b7769e4fc0 Remove wrong reference to ARMCC in all.sh test name 2019-07-30 16:56:25 +02:00
Hanno Becker
c4296a3bbb Modify all.sh test names to reflect use of ARMCC 2019-07-30 16:56:25 +02:00
Hanno Becker
28d2a88173 Fix indentation in three example programs 2019-07-30 16:56:25 +02:00
Hanno Becker
e7895aae2c Add all.sh test for baremetal.h runtime test 2019-07-30 16:56:25 +02:00
Hanno Becker
a1f3c521e7 Add --build-only option to baremetal.sh --ram
This option builds the library, tests and example programs
in a minimally modified baremetal.h configuration (modifications
from baremetal_test.h) but doesn't execute any tests.
2019-07-30 16:56:25 +02:00
Hanno Becker
fe1bd781de Add all.sh test for hardcoded elliptic curve 2019-07-30 16:56:25 +02:00
Hanno Becker
7cedd8bed2 Remove overly strict guard in ssl_server2
get_auth_mode() is needed for a change of authmode through SNI,
which is possible even if the original authmode is hardcoded.
2019-07-30 16:56:18 +02:00
Hanno Becker
6dd8e1c54d Add all.sh test for hardcoded miscellaneous SSL config options 2019-07-30 16:50:27 +02:00
Hanno Becker
41e5a6871d Example apps: Don't use runtime IO config API if CBs are hardcoded
Multiple example applications still use mbedtls_ssl_set_bio() even
if the I/O callbacks are hardcoded. This commit fixes this.
2019-07-30 16:50:27 +02:00
Hanno Becker
2a0cd5a031 Add all.sh test for hardcoded IO callbacks 2019-07-30 16:48:20 +02:00