If `MBEDTLS_SSL_KEEP_PEER_CERTIFICATE` is not set, `mbedtls_ssl_session`
contains the digest of the peer's certificate for the sole purpose of
detecting a CRT change on renegotiation. Hence, it is not needed if
renegotiation is disabled.
This commit removes the `peer_cert_digest` fields (and friends) from
`mbedtls_ssl_session` if
`!MBEDTLS_SSL_KEEP_PEER_CERTIFICATE + !MBEDTLS_SSL_RENEGOTIATION`,
which is a sensible configuration for constrained devices.
Apart from straightforward replacements of
`if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)`
by
`if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) && \
defined(MBEDTLS_SSL_RENEGOTIATION)`,
there's one notable change: On the server-side, the CertificateVerify
parsing function is a no-op if the client hasn't sent a certificate.
So far, this was determined by either looking at the peer CRT or the
peer CRT digest in the SSL session structure (depending on the setting
of `MBEDTLS_SSL_KEEP_PEER_CERTIFICATE`), which now no longer works if
`MBEDTLS_SSL_KEEP_PEER_CERTIFICATE` is unset. Instead, this function
now checks whether the temporary copy of the peer's public key within
the handshake structure is initialized or not (which is also a
beneficial simplification in its own right, because the pubkey is
all the function needs anyway).
The previous placing of the return statement made it look like there
are configurations for which no return statement is emitted; while
that's not true (if this function is used, at least some version of
TLS must be enabled), it's still clearer to move the failing return
statement to outside of all preprocessor guards.
If we don't need to store the peer's CRT chain permanently, we may
free it immediately after verifying it. Moreover, since we parse the
CRT chain in-place from the input buffer in this case, pointers from
the CRT structure remain valid after freeing the structure, and we
use that to extract the digest and pubkey from the CRT after freeing
the structure.
It is used in `mbedtls_ssl_session_free()` under
`MBEDTLS_X509_CRT_PARSE_C`, but defined only if
`MBEDTLS_KEY_EXCHANGE__WITH_CERT__ENABLED`.
Issue #2422 tracks the use of
`MBEDTLS_KEY_EXCHANGE__WITH_CERT_ENABLED` instead of
`MBEDTLS_X509_CRT_PARSE_C` for code and fields
related to CRT-based ciphersuites.
The server expects a CertificateVerify message only if it has
previously received a Certificate from the client.
So far, this was detected by looking at the `peer_cert` field
in the current session. Preparing to remove the latter, this
commit changes this to instead determine the presence of a peer
certificate by checking the new `peer_cert_digest` pointer.
We must dispatch between the peer's public key stored as part of
the peer's CRT in the current session structure (situation until
now, and future behaviour if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is
enabled), and the sole public key stored in the handshake structure
(new, if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled).
We must dispatch between the peer's public key stored as part of
the peer's CRT in the current session structure (situation until
now, and future behaviour if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is
enabled), and the sole public key stored in the handshake structure
(new, if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled).
We must dispatch between the peer's public key stored as part of
the peer's CRT in the current session structure (situation until
now, and future behaviour if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is
enabled), and the sole public key stored in the handshake structure
(new, if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled).
We must dispatch between the peer's public key stored as part of
the peer's CRT in the current session structure (situation until
now, and future behaviour if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is
enabled), and the sole public key stored in the handshake structure
(new, if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled).
This commit modifies `mbedtls_ssl_parse_certificate()` to store a
copy of the peer's public key after parsing and verifying the peer's
CRT chain.
So far, this leads to heavy memory duplication: We have the CRT chain
in the I/O buffer, then parse (and, thereby, copy) it to a
`mbedtls_x509_crt` structure, and then make another copy of the
peer's public key, plus the overhead from the MPI and ECP structures.
This inefficiency will soon go away to a significant extend, because:
- Another PR adds functionality to parse CRTs without taking
ownership of the input buffers. Applying this here will allow
parsing and verifying the peer's chain without making an additional
raw copy. The overhead reduces to the size of `mbedtls_x509_crt`,
the public key, and the DN structures referenced in the CRT.
- Once copyless parsing is in place and the removal of the peer CRT
is fully implemented, we can extract the public key bounds from
the parsed certificate and then free the entire chain before
parsing the public key again. This means that we never store
the parsed public key twice at the same time.
When removing the (session-local) copy of the peer's CRT chain, we must
keep a handshake-local copy of the peer's public key, as (naturally) every
key exchange will make use of that public key at some point to verify that
the peer actually owns the corresponding private key (e.g., verify signatures
from ServerKeyExchange or CertificateVerify, or encrypt a PMS in a RSA-based
exchange, or extract static (EC)DH parameters).
This commit adds a PK context field `peer_pubkey` to the handshake parameter
structure `mbedtls_handshake_params_init()` and adapts the init and free
functions accordingly. It does not yet make actual use of the new field.
This commit adds an ASN.1 buffer field `pk_raw` to `mbedtls_x509_crt`
which stores the bounds of the raw public key data within an X.509 CRT.
This will be useful in subsequent commits to extract the peer's public
key from its certificate chain.
This commit changes the format of session tickets to include
the digest of the peer's CRT if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE
is disabled.
This commit does not yet remove the peer CRT itself.
`mbedtls_ssl_parse_certificate()` parses the peer's certificate chain
directly into the `peer_cert` field of the `mbedtls_ssl_session`
structure being established. To allow to optionally remove this field
from the session structure, this commit changes this to parse the peer's
chain into a local variable instead first, which can then either be freed
after CRT verification - in case the chain should not be stored - or
mapped to the `peer_cert` if it should be kept. For now, only the latter
is implemented.
mbedtls_ssl_parse_certificate() will fail if a ciphersuite requires
a certificate, but none is provided. While it is sensible to double-
check this, failure should be reported as an internal error and not
as an unexpected message.
A subsequent commit will need this function in the session ticket
and session cache implementations. As the latter are server-side,
this commit also removes the MBEDTLS_SSL_CLI_C guard.
For now, the function is declared in ssl_internal.h and hence not
part of the public API.
This commit modifies the helper `ssl_parse_certificate_chain()` to
accep any target X.509 CRT chain instead of hardcoding it to
`session_negotiate->peer_cert`. This increases modularity and paves
the way towards removing `mbedtls_ssl_session::peer_cert`.
This commit simplifies the client-side code for outgoing CertificateVerify
messages, and server-side code for outgoing CertificateRequest messages and
incoming CertificateVerify messages, through the use of the macro
`MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED`
indicating whether a ciphersuite allowing CertificateRequest messages
is enabled in the configuration, as well as the helper function
`mbedtls_ssl_ciphersuite_cert_req_allowed()`
indicating whether a particular ciphersuite allows CertificateRequest
messages.
These were already used in the client-side code to simplify the
parsing functions for CertificateRequest messages.
This commit adds a helper function `ssl_parse_certificate_coordinate()`
which checks whether a `Certificate` message is expected from the peer.
The logic is the following:
- For ciphersuites which don't use server-side CRTs, no Certificate
message is expected (neither for the server, nor the client).
- On the server, no client certificate is expected in the following cases:
* The server server didn't request a Certificate, which is controlled
by the `authmode` setting.
* A RSA-PSK suite is used; this is the only suite using server CRTs
but not allowing client-side authentication.
This commit introduces a static helper function
`mbedtls_ssl_ciphersuite_uses_srv_cert()`
which determines whether a ciphersuite may make use of server-side CRTs.
This function is in turn uses in `mbedtls_ssl_parse_certificate()` to
skip certificate parsing for ciphersuites which don't involve CRTs.
Note: Ciphersuites not using server-side CRTs don't allow client-side CRTs
either, so it is safe to guard `mbedtls_ssl_{parse/write}_certificate()`
this way.
Note: Previously, the code uses a positive check over the suites
- MBEDTLS_KEY_EXCHANGE_PSK
- MBEDTLS_KEY_EXCHANGE_DHE_PSK
- MBEDTLS_KEY_EXCHANGE_ECDHE_PSK
- MBEDTLS_KEY_EXCHANGE_ECJPAKE,
while now, it uses a negative check over `mbedtls_ssl_ciphersuite_uses_srv_cert()`,
which checks for the suites
- MBEDTLS_KEY_EXCHANGE_RSA
- MBEDTLS_KEY_EXCHANGE_RSA_PSK
- MBEDTLS_KEY_EXCHANGE_DHE_RSA
- MBEDTLS_KEY_EXCHANGE_ECDH_RSA
- MBEDTLS_KEY_EXCHANGE_ECDHE_RSA
- MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA
- MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA
This is equivalent since, together, those are all ciphersuites.
Quoting ssl_ciphersuites.h:
```
typedef enum {
MBEDTLS_KEY_EXCHANGE_NONE = 0,
MBEDTLS_KEY_EXCHANGE_RSA,
MBEDTLS_KEY_EXCHANGE_DHE_RSA,
MBEDTLS_KEY_EXCHANGE_ECDHE_RSA,
MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA,
MBEDTLS_KEY_EXCHANGE_PSK,
MBEDTLS_KEY_EXCHANGE_DHE_PSK,
MBEDTLS_KEY_EXCHANGE_RSA_PSK,
MBEDTLS_KEY_EXCHANGE_ECDHE_PSK,
MBEDTLS_KEY_EXCHANGE_ECDH_RSA,
MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA,
MBEDTLS_KEY_EXCHANGE_ECJPAKE,
} mbedtls_key_exchange_type_t;
```
The handler `mbedtls_ssl_parse_certificate()` for incoming `Certificate`
messages contains many branches updating the handshake state. For easier
reasoning about state evolution, this commit introduces a single code-path
updating the state machine at the end of `mbedtls_ssl_parse_certificate()`.
If an attempt for session resumption fails, the `session_negotiate` structure
might be partially filled, and in particular already contain a peer certificate
structure. This certificate structure needs to be freed before parsing the
certificate sent in the `Certificate` message.
This commit moves the code-path taking care of this from the helper
function `ssl_parse_certificate_chain()`, whose purpose should be parsing
only, to the top-level handler `mbedtls_ssl_parse_certificate()`.
The fact that we don't know the state of `ssl->session_negotiate` after
a failed attempt for session resumption is undesirable, and a separate
issue #2414 has been opened to improve on this.
This commit introduces a server-side static helper function
`ssl_srv_check_client_no_crt_notification()`, which checks if
the message we received during the incoming certificate state
notifies the server of the lack of certificate on the client.
For SSLv3, such a notification comes as a specific alert,
while for all other TLS versions, it comes as a `Certificate`
handshake message with an empty CRT list.
So far, we've used the `peer_cert` pointer to detect whether
we're parsing the first CRT, but that will soon be removed
if `MBEDTLS_SSL_KEEP_PEER_CERTIFICATE` is unset.
This commit introduces a helper function `ssl_clear_peer_cert()`
which frees all data related to the peer's certificate from an
`mbedtls_ssl_session` structure. Currently, this is the peer's
certificate itself, while eventually, it'll be its digest only.
After mitigating the 'triple handshake attack' by checking that
the peer's end-CRT didn't change during renegotation, the current
code avoids re-parsing the CRT by moving the CRT-pointer from the
old session to the new one. While efficient, this will no longer
work once only the hash of the peer's CRT is stored beyond the
handshake.
This commit removes the code-path moving the old CRT, and instead
frees the entire peer CRT chain from the initial handshake as soon
as the 'triple handshake attack' protection has completed.
Introduce MBEDTLS_X509_INFO to indicate the availability of the
mbedtls_x509_*_info() function and closely related APIs. When this is
not defined, also omit name and description from
mbedtls_oid_descriptor_t, and omit OID arrays, macros, and types that
are entirely unused. This saves several KB of code space.
Change-Id: I056312613379890e0d70e1d08c34171287c0aa17
In a reduced configuration without PEM, PKCS5 or PKCS12, armc5 found that ret
was set but not used. Fixing that lead to a new warning about the variable not
being used at all. Now the variable is only declared when it's needed.
Some TLS-only code paths were not protected by an #ifdef and while some
compiler are happy to just silently remove them, armc5 complains:
Warning: #111-D: statement is unreachable
Let's make armc5 happy.
This is enabled by default as we generally enable things by default unless
there's a reason not to (experimental, deprecated, security risk).
We need a compile-time option because, even though the functions themselves
can be easily garbage-collected by the linker, implementing them will require
saving 64 bytes of Client/ServerHello.random values after the handshake, that
would otherwise not be needed, and people who don't need this feature
shouldn't have to pay the price of increased RAM usage.
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().
A positive option looks better, but comes with the following compatibility
issue: people using a custom config.h that is not based on the default
config.h and need TLS support would need to manually change their config in
order to still get TLS.
Work around that by making the public option negative. Internally the positive
option is used, though.
In the future (when preparing the next major version), we might want to switch
back to a positive option as this would be more consistent with other options
we have.
If int is not capable of storing as many values as unsigned, the code
may generate a trap value. If signed int and unsigned int aren't
32-bit types, the code may calculate meaningless values.
The elements of the HAVEGE state are manipulated with bitwise
operations, with the expectations that the elements are 32-bit
unsigned integers (or larger). But they are declared as int, and so
the code has undefined behavior. Clang with Asan correctly points out
some shifts that reach the sign bit.
Use unsigned int internally. This is technically an aliasing violation
since we're accessing an array of `int` via a pointer to `unsigned
int`, but since we don't access the array directly inside the same
function, it's very unlikely to be compiled in an unintended manner.
This commit handles occurrences of case 2 and 3 in the following list:
1. Some DTLS-specific code with no TLS-specific code (most frequent)
2. Some specific code for each protocol
3. Some TLS-specific code with no DTLS-specific code (least frequent)
Case 3 previously had a weird structure in that the TLS-specific code was
always present, but the if structure was conditional on DTLS being enabled.
This is changed by this commit to a more logical structure where both the code
and the test are conditional on TLS being enabled.
Case 2 doesn't require any change in the code structure in general. However,
there is one occurrence where the if/else structure is simplified to assigning
the result of a boolean operation, and one occurrence where I also noticed a
useless use of `ssl_ep_len()` in a TLS-specific branch, that I turned to the
constant 0 as it makes more sense.
Case 1 will be handled in the next commit, as it can easily be handled in an
automated way - only cases 2 and 3 (sometimes) required manual intervention.
The list of occurrences for cases 2 and 3 was established manually by looking
for occurrences of '= MBEDTLS_SSL_TRANSPORT_' in the code and manually
checking if there was a TLS-specific branch.
New sizes (see previous commit for the measuring script):
```
both
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
17156 0 0 17156 4304 ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
17649 0 0 17649 44f1 ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
39286 60 0 39346 99b2 ssl_tls.o (ex library/libmbedtls.a)
88874 60 600 89534 15dbe (TOTALS)
DTLS-only
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
17068 0 0 17068 42ac ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
17553 0 0 17553 4491 ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
38499 60 0 38559 969f ssl_tls.o (ex library/libmbedtls.a)
87903 60 600 88563 159f3 (TOTALS)
TLS-only
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
14912 0 0 14912 3a40 ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
15868 0 0 15868 3dfc ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
27619 60 0 27679 6c1f ssl_tls.o (ex library/libmbedtls.a)
73182 60 600 73842 12072 (TOTALS)
```
And use those tools in a few places. For now the purpose is just to validate
those tools before using them in all occurrences of transport-specific code.
The effect of these changes was measured with the following script:
```
set -eu
build() {
printf "\n$1\n"
CC=arm-none-eabi-gcc CFLAGS='-Werror -Os -march=armv6-m -mthumb' \
AR=arm-none-eabi-ar LD=arm-none-eabi-ld make clean lib >/dev/null
arm-none-eabi-size -t library/libmbedtls.a
}
git checkout -- include/mbedtls/config.h
scripts/config.pl unset MBEDTLS_NET_C
scripts/config.pl unset MBEDTLS_TIMING_C
scripts/config.pl unset MBEDTLS_FS_IO
scripts/config.pl unset MBEDTLS_ENTROPY_NV_SEED
scripts/config.pl set MBEDTLS_NO_PLATFORM_ENTROPY
build "both"
scripts/config.pl unset MBEDTLS_SSL_PROTO_TLS
build "DTLS-only"
scripts/config.pl set MBEDTLS_SSL_PROTO_TLS
scripts/config.pl unset MBEDTLS_SSL_PROTO_DTLS
scripts/config.pl unset MBEDTLS_SSL_DTLS_HELLO_VERIFY
scripts/config.pl unset MBEDTLS_SSL_DTLS_ANTI_REPLAY
scripts/config.pl unset MBEDTLS_SSL_DTLS_BADMAC_LIMIT
scripts/config.pl unset MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE
build "TLS-only"
git checkout -- include/mbedtls/config.h
```
The output of the script is as follows:
```
both
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
17160 0 0 17160 4308 ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
17637 0 0 17637 44e5 ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
39322 60 0 39382 99d6 ssl_tls.o (ex library/libmbedtls.a)
88902 60 600 89562 15dda (TOTALS)
DTLS-only
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
17072 0 0 17072 42b0 ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
17565 0 0 17565 449d ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
38953 60 0 39013 9865 ssl_tls.o (ex library/libmbedtls.a)
88373 60 600 89033 15bc9 (TOTALS)
TLS-only
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
14916 0 0 14916 3a44 ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
15852 0 0 15852 3dec ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
27623 60 0 27683 6c23 ssl_tls.o (ex library/libmbedtls.a)
73174 60 600 73834 1206a (TOTALS)
```
It can be seen that a DTLS-only build is now starting to be a bit smaller than
a dual-mode build, which is the purpose of the new build option.
For now the option has no effect.
Adapted existing example config files. The fact that I needed to do this
highlights that this is a slightly incompatible change: existing users need to
update their existing custom configs (if standalone as opposed to based on the
default config) in order to still get the same behaviour.
The alternative would be to have a negative config option (eg NO_TLS or
DTLS_ONLY) but this doesn't fit as nicely with the existing options, so
hopefully the minor incompatibility is acceptable.
I don't think it's worth adding a new component to all.sh:
- builds with both DTLS and TLS are done in the default (and full) config
- TLS-only builds are done with eg config-suite-b.h in test-ref-configs
- a DTLS-only build is done with config-thread.h in test-ref-configs
- builds with none of them (and SSL_TLS_C enabled) are forbidden
Context: During a handshake, the SSL/TLS handshake logic constructs
an instance of ::mbedtls_ssl_session representing the SSL session
being established. This structure contains information such as the
session's master secret, the peer certificate, or the session ticket
issues by the server (if applicable).
During a renegotiation, the new session is constructed aside the existing
one and destroys and replaces the latter only when the renegotiation is
complete. While conceptually clear, this means that during the renegotiation,
large pieces of information such as the peer's CRT or the session ticket
exist twice in memory, even though the original versions are removed
eventually.
This commit removes the simultaneous presence of two peer CRT chains
in memory during renegotiation, in the following way:
- Unlike in the case of SessionTickets handled in the previous commit,
we cannot simply free the peer's CRT chain from the previous handshake
before parsing the new one, as we need to verify that the peer's end-CRT
hasn't changed to mitigate the 'Triple Handshake Attack'.
- Instead, we perform a binary comparison of the original peer end-CRT
with the one presented during renegotiation, and if it succeeds, we
avoid re-parsing CRT by moving the corresponding CRT pointer from the
old to the new session structure.
- The remaining CRTs in the peer's chain are not affected by the triple
handshake attack protection, and for them we may employ the canonical
approach of freeing them before parsing the remainder of the new chain.
Note that this commit intends to not change any observable behavior
of the stack. In particular:
- The peer's CRT chain is still verified during renegotiation.
- The tail of the peer's CRT chain may change during renegotiation.
Context: During a handshake, the SSL/TLS handshake logic constructs
an instance of ::mbedtls_ssl_session representing the SSL session
being established. This structure contains information such as the
session's master secret, the peer certificate, or the session ticket
issues by the server (if applicable).
During a renegotiation, the new session is constructed aside the existing
one and destroys and replaces the latter only when the renegotiation is
complete. While conceptually clear, this means that during the renegotiation,
large pieces of information such as the peer's CRT or the session ticket
exist twice in memory, even though the original versions are removed
eventually.
This commit starts removing this memory inefficiency by freeing the old
session's SessionTicket before the one for the new session is allocated.
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. Fix any
unused variable warnings along the way, by additionally wrapping
function parameters with MBEDTLS_SSL_SOME_MODES_USE_MAC.
Add a regression test to detect when we have regressions with
configurations that do not include any MAC ciphersuites.
Fixes 92231325a7 ("Reduce size of `ssl_transform` if no MAC ciphersuite is enabled")
Context:
The existing API `mbedtls_x509_parse_crt_der()` for parsing DER
encoded X.509 CRTs unconditionally makes creates a copy of the
input buffer in RAM. While this comes at the benefit of easy use,
-- specifically: allowing the user to free or re-use the input
buffer right after the call -- it creates a significant memory
overhead, as the CRT is duplicated in memory (at least temporarily).
This might not be tolerable a resource constrained device.
As a remedy, this commit adds a new X.509 API call
`mbedtls_x509_parse_crt_der_nocopy()`
which has the same signature as `mbedtls_x509_parse_crt_der()`
and almost the same semantics, with one difference: The input
buffer must persist and be unmodified for the lifetime of the
established instance of `mbedtls_x509_crt`, that is, until
`mbedtls_x509_crt_free()` is called.
- 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.
This commit introduces a new SSL error code
`MBEDTLS_ERR_SSL_VERSION_MISMATCH`
which can be used to indicate operation failure due to a
mismatch of version or configuration.
It is put to use in the implementation of `mbedtls_ssl_session_load()`
to signal the attempt to de-serialize a session which has been serialized
in a build of Mbed TLS using a different version or configuration.
This commit makes use of the added space in the session header to
encode the state of those parts of the compile-time configuration
which influence the structure of the serialized session in the
present version of Mbed TLS. Specifically, these are
- the options which influence the presence/omission of fields
from mbedtls_ssl_session (which is currently shallow-copied
into the serialized session)
- the setting of MBEDTLS_X509_CRT_PARSE_C, which determines whether
the serialized session contains a CRT-length + CRT-value pair after
the shallow-copied mbedtls_ssl_session instance.
- the setting of MBEDTLS_SSL_SESSION_TICKETS, which determines whether
the serialized session contains a session ticket.
This commit adds space for two bytes in the header of serizlied
SSL sessions which can be used to determine the structure of the
remaining serialized session in the respective version of Mbed TLS.
Specifically, if parts of the session depend on whether specific
compile-time options are set or not, the setting of these options
can be encoded in the added space.
This commit doesn't yet make use of the fields.
The format of serialized SSL sessions depends on the version and the
configuration of Mbed TLS; attempts to restore sessions established
in different versions and/or configurations lead to undefined behaviour.
This commit adds an 3-byte version header to the serialized session
generated and cleanly fails ticket parsing in case a session from a
non-matching version of Mbed TLS is presented.
We have explicit recommendations to use US spelling for technical writing, so
let's apply this to code as well for uniformity. (My fingers tend to prefer UK
spelling, so this needs to be fixed in many places.)
sed -i 's/\([Ss]eriali\)s/\1z/g' **/*.[ch] **/*.function **/*.data ChangeLog
This uncovered a bug that led to a double-free (in practice, in general could
be free() on any invalid value): initially the session structure is loaded
with `memcpy()` which copies the previous values of pointers peer_cert and
ticket to heap-allocated buffers (or any other value if the input is
attacker-controlled). Now if we exit before we got a chance to replace those
invalid values with valid ones (for example because the input buffer is too
small, or because the second malloc() failed), then the next call to
session_free() is going to call free() on invalid pointers.
This bug is fixed in this commit by always setting the pointers to NULL right
after they've been read from the serialised state, so that the invalid values
can never be used.
(An alternative would be to NULL-ify them when writing, which was rejected
mostly because we need to do it when reading anyway (as the consequences of
free(invalid) are too severe to take any risk), so doing it when writing as
well is redundant and a waste of code size.)
Also, while thinking about what happens in case of errors, it became apparent
to me that it was bad practice to leave the session structure in an
half-initialised state and rely on the caller to call session_free(), so this
commit also ensures we always clear the structure when loading failed.
This allows callers to discover what an appropriate size is. Otherwise they'd
have to either try repeatedly, or allocate an overly large buffer (or some
combination of those).
Adapt documentation an example usage in ssl_client2.
Avoid useless copy with mbedtls_ssl_get_session() before serialising.
Used in ssl_client2 for testing and demonstrating usage, but unfortunately
that means mbedtls_ssl_get_session() is no longer tested, which will be fixed
in the next commit.
On client side, this is required for the main use case where of serialising a
session for later resumption, in case tickets are used.
On server side, this doesn't change much as ticket_len will always be 0.
This unblocks testing the functions by using them in ssl_client2, which will
be done in the next commit.
This finishes making these functions public. Next step is to get them tested,
but there's currently a blocker for that, see next commit (and the commit
after it for tests).
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.
While 'session hash' is currently unique, so suitable to prove that the
intended code path has been taken, it's a generic enough phrase that in the
future we might add other debug messages containing it in completely unrelated
code paths. In order to future-proof the accuracy of the test, let's use a
more specific string.
The previous comment used "TLS" as a shortcut for "TLS 1.0/1.1" which was
confusing. This partially reflected the names of the calc_verify/finished that
go ssl, tls (for 1.0/1.1) tls_shaxxx (for 1.2), but still it's clearer to be
explicit in the comment - and perhaps in the long term the function names
could be clarified instead.
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
When using this function to deserialize, it's not a problem to have a session
structure as input as we'll have one around anyway (most probably freshly
deserialised).
However for tests it's convenient to be able to build a transform without
having a session structure around.
Also, removing this structure from parameters makes the function signature
more uniform, the only exception left being the ssl param at the end that's
hard to avoid for now.
We called in tinycrypt in the file names, but uecc in config.h, all.sh and
other places, which could be confusing. Just use tinycrypt everywhere because
that's the name of the project and repo where we took the files.
The changes were made using the following commands (with GNU sed and zsh):
sed -i 's/uecc/tinycrypt/g' **/*.[ch] tests/scripts/all.sh
sed -i 's/MBEDTLS_USE_UECC/MBEDTLS_USE_TINYCRYPT/g' **/*.[ch] tests/scripts/all.sh scripts/config.pl
Configs with no DEBUG_C are use for example in test-ref-configs.pl, which also
runs parts of compat.sh or ssl-opt.sh on them, so the added 'ssl = NULL'
statements will be exercised in those tests at least.
Make it more explicit what's used. Unfortunately, we still need ssl as a
parameter for debugging, and because calc_verify wants it as a parameter (for
all TLS versions except SSL3 it would actually only need handshake, but SSL3
also accesses session_negotiate).
It's also because of calc_verify that we can't make it const yet, but see next
commit.
This commit adds tests exercising mutually inverse pairs of
record encryption and decryption transformations for the various
transformation types allowed in TLS: Stream, CBC, and AEAD.
The hash contexts `ssl_transform->md_ctx_{enc/dec}` are not used if
only AEAD ciphersuites are enabled. This commit removes them from the
`ssl_transform` struct in this case, saving a few bytes.
This commit guards code specific to AEAD, CBC and stream cipher modes
in `ssl_derive_keys` by the respective configuration flags, analogous
to the guards that are already in place in the record decryption and
encryption functions `ssl_decrypt_buf` resp. `ssl_decrypt_buf`.
Analogous to the previous commit, but concerning the record decryption
routine `ssl_decrypt_buf`.
An important change regards the checking of CBC padding:
Prior to this commit, the CBC padding check always read 256 bytes at
the end of the internal record buffer, almost always going past the
boundaries of the record under consideration. In order to stay within
the bounds of the given record, this commit changes this behavior by
always reading the last min(256, plaintext_len) bytes of the record
plaintext buffer and taking into consideration the last `padlen` of
these for the padding check. With this change, the memory access
pattern and runtime of the padding check is entirely determined by
the size of the encrypted record, in particular not giving away
any information on the validity of the padding.
The following depicts the different behaviors:
1) Previous CBC padding check
1.a) Claimed padding length <= plaintext length
+----------------------------------------+----+
| Record plaintext buffer | | PL |
+----------------------------------------+----+
\__ PL __/
+------------------------------------...
| read for padding check ...
+------------------------------------...
|
contents discarded
from here
1.b) Claimed padding length > plaintext length
+----------------------------------------+----+
| Record plaintext buffer | PL |
+----------------------------------------+----+
+-------------------------...
| read for padding check ...
+-------------------------...
|
contents discarded
from here
2) New CBC padding check
+----------------------------------------+----+
| Record plaintext buffer | | PL |
+----------------------------------------+----+
\__ PL __/
+---------------------------------------+
| read for padding check |
+---------------------------------------+
|
contents discarded
until here
The previous version of the record encryption function
`ssl_encrypt_buf` takes the entire SSL context as an argument,
while intuitively, it should only depend on the current security
parameters and the record buffer.
Analyzing the exact dependencies, it turned out that in addition
to the currently active `ssl_transform` instance and the record
information, the encryption function needs access to
- the negotiated protocol version, and
- the status of the encrypt-then-MAC extension.
This commit moves these two fields into `ssl_transform` and
changes the signature of `ssl_encrypt_buf` to only use an instance
of `ssl_transform` and an instance of the new `ssl_record` type.
The `ssl_context` instance is *solely* kept for the debugging macros
which need an SSL context instance.
The benefit of the change is twofold:
1) It avoids the need of the MPS to deal with instances of
`ssl_context`. The MPS should only work with records and
opaque security parameters, which is what the change in
this commit makes progress towards.
2) It significantly eases testing of the encryption function:
independent of any SSL context, the encryption function can
be passed some record buffer to encrypt alongside some arbitrary
choice of parameters, and e.g. be checked to not overflow the
provided memory.
The macro constant `MBEDTLS_SSL_MAC_ADD` defined in `ssl_internal.h`
defines an upper bound for the amount of space needed for the record
authentication tag. Its definition distinguishes between the
presence of an ARC4 or CBC ciphersuite suite, in which case the maximum
size of an enabled SHA digest is used; otherwise, `MBEDTLS_SSL_MAC_ADD`
is set to 16 to accomodate AEAD authentication tags.
This assignment has a flaw in the situation where confidentiality is
not needed and the NULL cipher is in use. In this case, the
authentication tag also uses a SHA digest, but the definition of
`MBEDTLS_SSL_MAC_ADD` doesn't guarantee enough space.
The present commit fixes this by distinguishing between the presence
of *some* ciphersuite using a MAC, including those using a NULL cipher.
For that, the previously internal macro `SSL_SOME_MODES_USE_MAC` from
`ssl_tls.c` is renamed and moved to the public macro
`MBEDTLS_SOME_MODES_USE_MAC` defined in `ssl_internal.h`.
Prior to this commit, the security parameter struct `ssl_transform`
contained a `ciphersuite_info` field pointing to the information
structure for the negotiated ciphersuite. However, the only
information extracted from that structure that was used in the core
encryption and decryption functions `ssl_encrypt_buf`/`ssl_decrypt_buf`
was the authentication tag length in case of an AEAD cipher.
The present commit removes the `ciphersuite_info` field from the
`ssl_transform` structure and adds an explicit `taglen` field
for AEAD authentication tag length.
This is in accordance with the principle that the `ssl_transform`
structure should contain the raw parameters needed for the record
encryption and decryption functions to work, but not the higher-level
information that gave rise to them. For example, the `ssl_transform`
structure implicitly contains the encryption/decryption keys within
their cipher contexts, but it doesn't contain the SSL master or
premaster secrets. Likewise, it contains an explicit `maclen`, while
the status of the 'Truncated HMAC' extension -- which determines the
value of `maclen` when the `ssl_transform` structure is created in
`ssl_derive_keys` -- is not contained in `ssl_transform`.
The `ciphersuite_info` pointer was used in other places outside
the encryption/decryption functions during the handshake, and for
these functions to work, this commit adds a `ciphersuite_info` pointer
field to the handshake-local `ssl_handshake_params` structure.
The `ssl_transform` security parameter structure contains opaque
cipher contexts for use by the record encryption/decryption functions
`ssl_decrypt_buf`/`ssl_encrypt_buf`, while the underlying key material
is configured once in `ssl_derive_keys` and is not explicitly dealt with
anymore afterwards. In particular, the key length is not needed
explicitly by the encryption/decryption functions but is nonetheless
stored in an explicit yet superfluous `keylen` field in `ssl_transform`.
This commit removes this field.
This commit improves hygiene and formatting of macro definitions
throughout the library. Specifically:
- It adds brackets around parameters to avoid unintended
interpretation of arguments, e.g. due to operator precedence.
- It adds uses of the `do { ... } while( 0 )` idiom for macros that
can be used as commands.
* restricted/pr/553:
Fix mbedtls_ecdh_get_params with new ECDH context
Add changelog entry for mbedtls_ecdh_get_params robustness
Fix ecdh_get_params with mismatching group
Add test case for ecdh_get_params with mismatching group
Add test case for ecdh_calc_secret
Fix typo in documentation
* origin/pr/2436:
Use certificates from data_files and refer them
Specify server certificate to use in SHA-1 test
refactor CA and SRV certificates into separate blocks
refactor SHA-1 certificate defintions and assignment
refactor server SHA-1 certificate definition into a new block
define TEST_SRV_CRT_RSA_SOME in similar logic to TEST_CA_CRT_RSA_SOME
server SHA-256 certificate now follows the same logic as CA SHA-256 certificate
add entry to ChangeLog
* restricted/pr/550:
Update query_config.c
Fix failure in SSLv3 per-version suites test
Adjust DES exclude lists in test scripts
Clarify 3DES changes in ChangeLog
Fix documentation for 3DES removal
Exclude 3DES tests in test scripts
Fix wording of ChangeLog and 3DES_REMOVE docs
Reduce priority of 3DES ciphersuites
If mbedtls_ecdh_get_params is called with keys belonging to
different groups, make it return an error the second time, rather than
silently interpret the first key as being on the second curve.
This makes the non-regression test added by the previous commit pass.
Refactor the function mbedtls_asn1_write_bitstring() that removes
trailing 0s at the end of DER encoded bitstrings. The function is
implemented according to Hanno Becker's suggestions.
This commit also changes the functions x509write_crt_set_ns_cert_type
and crt_set_key_usage to call the new function as the use named
bitstrings instead of the regular bitstrings.
In mbedtls_mpi_exp_mod(), the limit check on wsize is never true when
MBEDTLS_MPI_WINDOW_SIZE is at least 6. Wrap in a preprocessor guard
to remove the dead code and resolve a Coverity finding from the
DEADCODE checker.
Change-Id: Ice7739031a9e8249283a04de11150565b613ae89
Return the error code if failed, instead of returning value `1`.
If not failed, return the call of the underlying function,
in `mbedtls_ecdsa_genkey()`.
Use `cmake -D CMAKE_BUILD_TYPE=Asan` rather than manually setting
`-fsanitize=address`. This lets cmake determine the necessary compiler
and linker flags.
With UNSAFE_BUILD on, force -Wno-error. This is necessary to build
with MBEDTLS_TEST_NULL_ENTROPY.
mbedtls_mpi_read_binary() calls memcpy() with the source pointer being
the source pointer passed to mbedtls_mpi_read_binary(), the latter may
be NULL if the buffer length is 0 (and this happens e.g. in the ECJPAKE
test suite). The behavior of memcpy(), in contrast, is undefined when
called with NULL source buffer, even if the length of the copy operation
is 0.
This commit fixes this by explicitly checking that the source pointer is
not NULL before calling memcpy(), and skipping the call otherwise.
Context: The function `mbedtls_mpi_fill_random()` uses a temporary stack
buffer to hold the random data before reading it into the target MPI.
Problem: This is inefficient both computationally and memory-wise.
Memory-wise, it may lead to a stack overflow on constrained devices with
limited stack.
Fix: This commit introduces the following changes to get rid of the
temporary stack buffer entirely:
1. It modifies the call to the PRNG to output the random data directly
into the target MPI's data buffer.
This alone, however, constitutes a change of observable behaviour:
The previous implementation guaranteed to interpret the bytes emitted by
the PRNG in a big-endian fashion, while rerouting the PRNG output into the
target MPI's limb array leads to an interpretation that depends on the
endianness of the host machine.
As a remedy, the following change is applied, too:
2. Reorder the bytes emitted from the PRNG within the target MPI's
data buffer to ensure big-endian semantics.
Luckily, the byte reordering was already implemented as part of
`mbedtls_mpi_read_binary()`, so:
3. Extract bigendian-to-host byte reordering from
`mbedtls_mpi_read_binary()` to a separate internal function
`mpi_bigendian_to_host()` to be used by `mbedtls_mpi_read_binary()`
and `mbedtls_mpi_fill_random()`.
The calls to cipher_finish didn't actually do anything:
- the cipher mode is always ECB
- in that case cipher_finish() only sets *olen to zero, and returns either 0
or an error depending on whether there was pending data
- olen is a local variable in the caller, so setting it to zero right before
returning is not essential
- the return value of cipher_finis() was not checked by the caller so that's
not useful either
- the cipher layer does not have ALT implementations so the behaviour
described above is unconditional on ALT implementations (in particular,
cipher_finish() can't be useful to hardware as (with ECB) it doesn't call any
functions from lower-level modules that could release resources for example)
Since the calls are causing issues with parameter validation, and were no
serving any functional purpose, it's simpler to just remove them.
Somehow, mbedtls_sha256_ret() is defined even if MBEDTLS_SHA256_ALT
is set, and it is using SHA256_VALIDATE_RET. The documentation should
be enhanced to indicate that MBEDTLS_SHA256_ALT does _not_ replace
the entire module, but only the core SHA-256 functions.
Somehow, mbedtls_sha512_ret() is defined even if MBEDTLS_SHA512_ALT
is set, and it is using SHA512_VALIDATE_RET. The documentation should
be enhanced to indicate that MBEDTLS_SHA512_ALT does _not_ replace
the entire module, but only the core SHA-512 functions.
Somehow, mbedtls_sha1_ret() is defined even if MBEDTLS_SHA1_ALT
is set, and it is using SHA1_VALIDATE_RET. The documentation should
be enhanced to indicate that MBEDTLS_SHA1_ALT does _not_ replace
the entire module, but only the core SHA-1 functions.
Document when a context must be initialized or not, when it must be
set up or not, and whether it needs a private key or a public key will
do.
The implementation is sometimes more liberal than the documentation,
accepting a non-set-up context as a context that can't perform the
requested information. This preserves backward compatibility.
The MPI_VALIDATE_RET() macro cannot be used for parameter
validation of mbedtls_mpi_lsb() because this function returns
a size_t.
Use the underlying MBEDTLS_INTERNAL_VALIDATE_RET() insteaed,
returning 0 on failure.
Also, add a test for this behaviour.
A 0-length buffer for the key is a legitimate edge case. Ensure that
it works, even with buf=NULL. Document the key and keylen parameters.
There are already test cases for parsing an empty buffer. A subsequent
commit will add tests for writing to an empty buffer.
Add checks for null pointers under MBEDTLS_CHECK_PARAMS.
In functions that perform operations with a context, only check if the
context pointer is non-null under MBEDTLS_CHECK_PARAMS. In the default
configuration, unconditionally dereference the context pointer.
In functions that query a context, support NULL as a
pointer-to-context argument, and return the same value as for a
context which has been initialized but not set up.
- The validity of the input and output parameters is checked by
parameter validation.
- A PRNG is required in public mode only (even though it's also
recommended in private mode), so move the check to the
corresponding branch.
The check was already done later when calling ECB, (as evidenced by the tests
passing, which have a call with data_unit set to NULL), but it's more readable
to have it here too, and more helpful when debugging.
The function called through the macro MBEDTLS_PARAM_FAILED() must be supplied by
users and makes no sense as a library function, apart from debug and test.
Function calls to alternative implementations have to follow certain
rules in order to preserve correct functionality. To avoid accidentally
breaking these rules we state them explicitly in the ECP module for
ourselves and every contributor to see.
We initialized the ECC hardware before calling
mbedtls_ecp_mul_shortcuts(). This in turn calls
mbedtls_ecp_mul_restartable(), which initializes and frees the hardware
too. This issue has been introduced by recent changes and caused some
accelerators to hang.
We move the initialization after the mbedtle_ecp_mul_shortcuts() calls
to avoid double initialization.
The SSL module accesses ECDH context members directly. This can't work
with the new context, where we can't make any assumption about the
implementation of the context.
This commit makes use of the new functions to avoid accessing ECDH
members directly. The only members that are still accessed directly are
the group ID and the point format and they are independent from the
implementation.
The SSL module accesses ECDH context members directly to print debug
information. This can't work with the new context, where we can't make
assumptions about the implementation of the context. This commit adds
new debug functions to complete the encapsulation of the ECDH context
and work around the problem.
The functionality from public API functions are moved to
`xxx_internal()` functions. The public API functions are modified to do
basic parameter validation and dispatch the call to the right
implementation.
There is no intended change in behaviour when
`MBEDTLS_ECDH_LEGACY_CONTEXT` is enabled.
In the future we want to support alternative ECDH implementations. We
can't make assumptions about the structure of the context they might
use, and therefore shouldn't access the members of
`mbedtls_ecdh_context`.
Currently the lifecycle of the context can't be done without direct
manipulation. This commit adds `mbedtls_ecdh_setup()` to complete
covering the context lifecycle with functions.
`mbedtls_ecp_tls_read_group()` both parses the group ID and loads the
group into the structure provided. We want to support alternative
implementations of ECDH in the future and for that we need to parse the
group ID without populating an `mbedtls_ecp_group` structure (because
alternative implementations might not use that).
This commit moves the part that parses the group ID to a new function.
There is no need to test the new function directly, because the tests
for `mbedtls_ecp_tls_read_group()` are already implicitly testing it.
There is no intended change in behaviour in this commit.
