Commit Graph

10247 Commits

Author SHA1 Message Date
Jaeden Amero
58f11d010b Merge branch 'development' into development-restricted
* 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
2019-08-16 10:14:40 +01:00
Jaeden Amero
8813fef228 Merge remote-tracking branch 'origin/pr/2756' into development
* origin/pr/2756:
  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
2019-08-16 10:12:09 +01:00
Jaeden Amero
ec1f91799f Update crypto to a repo with latest crypto
Use a version of Mbed Crypto with 100% up-to-date crypto and tool
changes from Mbed TLS. This is necessary in order for the check params
feature to work in deprecated removed builds and for the arm5vte build
to succeed.
2019-08-15 16:42:21 +01:00
Jaeden Amero
c5ad90a6a7 Update Mbed Crypto
Update Mbed Crypto to a version that supports its headers being used
from a parent project.
2019-08-15 15:44:50 +01:00
Jaeden Amero
922013e46d tls: Remove duplicate psa_util.h include
Don't include psa_util.h twice. It's enough to include it once.
2019-08-15 15:44:50 +01:00
Jaeden Amero
70de9dc052 Remove unused cryptography test files 2019-08-15 15:44:50 +01:00
Jaeden Amero
a0aee30644 Remove crypto C files
Remove unused cryptography C files, as these are sourced from Mbed
Crypto now.
2019-08-15 15:44:50 +01:00
Jaeden Amero
815e9a21a3 Remove files sourced from Mbed Crypto
Remove cryptography related files and a few utility header files that
are shared between Mbed TLS and Mbed Crypto. Mbed TLS will use an Mbed
Crypto sourced version of each of these header files in order to ease
the maintenance burden of both libraries, and to make it easier to keep
Mbed TLS and Mbed Crypto in sync.

As part of removing cryptography related files, tell Doxygen to source
information from the removed the headers, so that it will consider them
for inclusion within Doxygen output.

Later, as part of the Mbed TLS 3.0 (API breaking version), we'll
restructure the organization of the 3 libraries a bit, to move some
things out of Mbed Crypto that don't belong there.

Candidates of not belonging in Mbed Crypto, but are in libmbedcrypto.so
for legacy reasons:
 - asn1.h
 - asn1write.h
 - base64.h
 - memory_buffer_alloc.h
 - platform.h
 - platform_time.h
 - platform_util.h
 - threading.h
 - timing.h
 - version.h
2019-08-15 15:44:50 +01:00
Jaeden Amero
dbe4ff80cf config: Fix Doxygen link to MBEDTLS_PARAM_FAILED
Don't use `#` for linking to function-like macros.
2019-08-15 15:44:50 +01:00
Jaeden Amero
6609aef809 Use mbedtls-based path for includes
To help the build system find the correct include files, paths starting
with "mbedtls/" or "psa/" must be used. Otherwise, you can run into
build failures like the following when building Mbed Crypto as a
submodule.

    In file included from chachapoly.c:31:0:
    ../../include/mbedtls/chachapoly.h:43:10: fatal error: poly1305.h: No such file or directory
     #include "poly1305.h"
              ^~~~~~~~~~~~
    compilation terminated.
2019-08-15 15:44:50 +01:00
Jaeden Amero
78d9d0c1e9 check-names: Consider crypto-sourced header files
Many identifiers come from Mbed Crypto. Teach check-names.sh to look in
the crypto submodule for identifiers, to avoid incorrect test results.
2019-08-15 15:24:26 +01:00
Gilles Peskine
4017d882dd Merge remote-tracking branch 'upstream-restricted/development-proposed' into development-restricted-proposed 2019-08-14 18:38:42 +02:00
Gilles Peskine
60b29d6bfd Merge remote-tracking branch 'upstream-restricted/pr/503' into development-restricted-proposed 2019-08-14 18:37:59 +02:00
Manuel Pégourié-Gonnard
7e821b5bcd Fix possibly-lossy conversion warning from MSVC
ssl_tls.c(4876): warning C4267: '=': conversion from 'size_t' to 'uint8_t', possible loss of data
2019-08-14 15:08:09 +01:00
Hanno Becker
d417cc945c Reintroduce length 0 check for records 2019-08-14 15:08:08 +01:00
Hanno Becker
d0b66d08bb Don't use memcpy() for 2-byte copy operation
Manual copying is slightly shorter here.
2019-08-14 15:08:08 +01:00
Hanno Becker
9eca276768 Remove integer parsing macro
If this is introduced, it should be defined in a prominent place
and put to use throughout the library, but this is left for another
time.
2019-08-14 15:08:08 +01:00
Hanno Becker
f5466258b4 Fix alignment in record header parsing routine 2019-08-14 15:08:08 +01:00
Hanno Becker
b2a86c3e01 Don't disallow 'record from another epoch' log msg in proxy ref test
It happens regularly in test runs that the server example application
shuts down a connection, goes into waiting mode for a new connection,
and then receives the encrypted ClosureAlert from the client. The only
reason why this does currently not trigger the 'record from another epoch'
message is that we handle ClientHello parsing outside of the main record
stack because we want to be able to detect SSLv2 ClientHellos. However,
this is likely to go away, and once it happens, we'll see the log message.
Further, when record checking is used, every record, including the mentioned
closure alert, is passed to the record checking API before being passed to
the rest of the stack, which leads to the log message being printed.

In summary, grepping for 'record from another epoch' is a fragile way
of checking whether a reordered message has arrived. A more reliable
way is to grep for 'Buffer record from epoch' which is printed when
a record from a future epoch is actually buffered, and 'ssl_buffer_message'
which is the function buffering a future handshake message.
2019-08-14 15:08:08 +01:00
Hanno Becker
552f747216 Make sure 'record from another epoch' is displayed for next epoch
The test 'DTLS proxy: delay ChangeCipherSpec' from ssl-opt.sh
relies on this.
2019-08-14 15:08:08 +01:00
Hanno Becker
5422981052 Implement record checking API
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.
2019-08-14 15:08:08 +01:00
Hanno Becker
331de3df9a Mark ssl_parse_record_header() as const in SSL context 2019-08-14 15:08:08 +01:00
Hanno Becker
47be7686ab 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-14 15:08:07 +01:00
Hanno Becker
b0fe0eedce Remove duplicate setting of ssl->in_msgtype and ssl->in_msglen 2019-08-14 15:06:44 +01:00
Hanno Becker
44d89b2d53 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-14 15:06:44 +01:00
Hanno Becker
8685c822c1 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-14 15:06:44 +01:00
Hanno Becker
58ef0bf19f Reduce dependency of ssl_prepare_record_content() on in_xxx fields 2019-08-14 15:06:44 +01:00
Hanno Becker
d8bf8ceeb4 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-14 15:06:06 +01:00
Hanno Becker
0183d699bf Mark DTLS replay check as const on the SSL context 2019-08-14 15:06:06 +01:00
Hanno Becker
7ae20e0f4c 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-14 15:06:06 +01:00
Hanno Becker
605949f84c 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-14 15:06:06 +01:00
Hanno Becker
fdf660426d Adapt ssl_prepare_record_content() to use SSL record structure 2019-08-14 15:06:06 +01:00
Hanno Becker
a31756619c Use record length from record structure when fetching content in TLS 2019-08-14 15:06:06 +01:00
Hanno Becker
f50da50c04 Use record structure when remembering offset of next record in dgram 2019-08-14 15:06:06 +01:00
Hanno Becker
4acada35f5 Use SSL record structure when skipping over unexpected record 2019-08-14 15:06:06 +01:00
Hanno Becker
519f15dbba Adapt ssl_buffer_future_record() to work with SSL record structure 2019-08-14 15:06:05 +01:00
Hanno Becker
e5e7e7833c 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-14 15:06:04 +01:00
Gilles Peskine
61fc108d25 Merge remote-tracking branch 'upstream-public/pr/2728' into development 2019-08-14 16:00:58 +02:00
Gilles Peskine
1435767d2a Merge remote-tracking branch 'upstream-public/pr/2753' into development 2019-08-14 16:00:11 +02:00
Gilles Peskine
681edbeaa6 Merge remote-tracking branch 'upstream-public/pr/2777' into development 2019-08-14 15:59:01 +02:00
Gilles Peskine
787d1515eb Merge remote-tracking branch 'upstream-public/pr/2779' into development 2019-08-14 15:58:07 +02:00
Hanno Becker
d840cea4a1 Expand documentation of internal mbedtls_record structure 2019-08-14 14:45:37 +01:00
Hanno Becker
37cfe73c92 Minor documentation improvements in ssl_parse_record_header() 2019-08-14 14:45:20 +01:00
Hanno Becker
955a5c98df 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-14 14:44:55 +01:00
Hanno Becker
d5c0f826e6 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-14 14:44:36 +01:00
Hanno Becker
a8814794e9 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-14 14:43:46 +01:00
Hanno Becker
59be60e98b 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-14 14:41:57 +01:00
Hanno Becker
e538d8287e Move size-check for DTLS record header with CID to DTLS-only branch 2019-08-14 14:41:37 +01:00
Hanno Becker
2fddd3765e 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-14 14:41:06 +01:00
Hanno Becker
4894873b92 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-14 14:34:49 +01:00