CTR-DRBG and HMAC-DRBG may used the seed differently depending on its length.
To avoid leaks, pass them a constant-length seed.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Pass the "certificate policies" extension to the callback supplied to
mbedtls_x509_crt_parse_der_with_ext_cb() if it contains unsupported
policies. This allows the callback to fully replicate the behaviour
of the deprecated MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION
configuration.
Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
These fields might be shifted accordingly in `ssl_parse_record_header()`
when receiving a connection with CID, so they require a manual update
after calling the generic `mbedtls_ssl_reset_in_out_pointers()`.
This commit also adds a regression test which is run by all.sh.
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Also normalize the first line of the copyright headers.
This commit was generated using the following script:
# ========================
#!/bin/sh
# Find scripts
find -path './.git' -prune -o '(' -name '*.c' -o -name '*.cpp' -o -name '*.fmt' -o -name '*.h' ')' -print | xargs sed -i '
# Normalize the first line of the copyright headers (no text on the first line of a block comment)
/^\/\*.*Copyright.*Arm/I {
i\
/*
s/^\// /
}
/Copyright.*Arm/I {
# Print copyright declaration
p
# Read the two lines immediately following the copyright declaration
N
N
# Insert Apache header if it is missing
/SPDX/! i\
* SPDX-License-Identifier: Apache-2.0\
*\
* Licensed under the Apache License, Version 2.0 (the "License"); you may\
* not use this file except in compliance with the License.\
* You may obtain a copy of the License at\
*\
* http://www.apache.org/licenses/LICENSE-2.0\
*\
* Unless required by applicable law or agreed to in writing, software\
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT\
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\
* See the License for the specific language governing permissions and\
* limitations under the License.
# Clear copyright declaration from buffer
D
}
'
# ========================
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
This is basically the same as reading from /dev/urandom on supported
systems, only it has a limit of 256 bytes per call, and does not require
an open file descriptor (so it can be used in chroots, when resource
limits are in place, or are otherwise exhausted).
It's functionally equivalent to the comparable function getentropy(),
but has been around for longer. It's actually used to implement
getentropy in FreeBSD's libc. Discussions about adding getrandom or
getentropy to NetBSD are still ongoing.
It's present in all supported versions of FreeBSD and NetBSD.
It's not present in DragonFly or OpenBSD.
Documentation: https://netbsd.gw.com/cgi-bin/man-cgi?sysctl+7
Comparable code in OpenSSL:
ddec332f32/crypto/rand/rand_unix.c (L208)
Signed-off-by: nia <nia@netbsd.org>
The function mbedtls_mpi_sub_abs first checked that A >= B and then
performed the subtraction, relying on the fact that A >= B to
guarantee that the carry propagation would stop, and not taking
advantage of the fact that the carry when subtracting two numbers can
only be 0 or 1. This made the carry propagation code a little hard to
follow.
Write an ad hoc loop for the carry propagation, checking the size of
the result. This makes termination obvious.
The initial check that A >= B is no longer needed, since the function
now checks that the carry propagation terminates, which is equivalent.
This is a slight performance gain.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
There was some confusion during review about when A->p[n] could be
nonzero. In fact, there is no need to set A->p[n]: only the
intermediate result d might need to extend to n+1 limbs, not the final
result A. So never access A->p[n]. Rework the explanation of the
calculation in a way that should be easier to follow.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The function mpi_sub_hlp had confusing semantics: although it took a
size parameter, it accessed the limb array d beyond this size, to
propagate the carry. This made the function difficult to understand
and analyze, with a potential buffer overflow if misused (not enough
room to propagate the carry).
Change the function so that it only performs the subtraction within
the specified number of limbs, and returns the carry.
Move the carry propagation out of mpi_sub_hlp and into its caller
mbedtls_mpi_sub_abs. This makes the code of subtraction very slightly
less neat, but not significantly different.
In the one other place where mpi_sub_hlp is used, namely mpi_montmul,
this is a net win because the carry is potentially sensitive data and
the function carefully arranges to not have to propagate it.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
mpi_sub_hlp performs a subtraction A - B, but took parameters in the
order (B, A). Swap the parameters so that they match the usual
mathematical syntax.
This has the additional benefit of putting the output parameter (A)
first, which is the normal convention in this module.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Checking the budget only after the randomization is done means sometimes we
were randomizing first, then noticing we ran out of budget, return, come back
and randomize again before we finally normalize.
While this is fine from a correctness and security perspective, it's a minor
inefficiency, and can also be disconcerting while debugging, so we might as
well avoid it.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
It results in smaller code than using CTR_DRBG (64 bytes smaller on ARMv6-M
with arm-none-eabi-gcc 7.3.1), so let's use this by default when both are
available.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Unless MBEDTLS_ECP_NO_INTERNAL_RNG is defined, it's no longer possible for
f_rng to be NULL at the places that randomize coordinates.
Eliminate the NULL check in this case:
- it makes it clearer to reviewers that randomization always happens (unless
the user opted out at compile time)
- a NULL check in a place where it's easy to prove the value is never NULL
might upset or confuse static analyzers (including humans)
- removing the check saves a bit of code size
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Currently we draw pseudo-random numbers at the beginning and end of the main
loop. With ECP_RESTARTABLE, it's possible that between those two occasions we
returned from the multiplication function, hence lost our internal DRBG
context that lives in this function's stack frame. This would result in the
same pseudo-random numbers being used for blinding in multiple places. While
it's not immediately clear that this would give rise to an attack, it's also
absolutely not clear that it doesn't. So let's avoid that by using a DRBG
context that lives inside the restart context and persists across
return/resume cycles. That way the RESTARTABLE case uses exactly the
same pseudo-random numbers as the non-restartable case.
Testing and compile-time options:
- The case ECP_RESTARTABLE && !ECP_NO_INTERNAL_RNG is already tested by
component_test_no_use_psa_crypto_full_cmake_asan.
- The case ECP_RESTARTABLE && ECP_NO_INTERNAL_RNG didn't have a pre-existing
test so a component is added.
Testing and runtime options: when ECP_RESTARTABLE is enabled, the test suites
already contain cases where restart happens and cases where it doesn't
(because the operation is short enough or because restart is disabled (NULL
restart context)).
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
While it seems cleaner and more convenient to set it in the top-level
mbedtls_ecp_mul() function, the existence of the restartable option changes
things - when it's enabled the drbg context needs to be saved in the restart
context (more precisely in the restart_mul sub-context), which can only be
done when it's allocated, which is in the curve-specific mul function.
This commit only internal drbg management from mbedtls_ecp_mul() to
ecp_mul_mxz() and ecp_mul_comb(), without modifying behaviour (even internal),
and a future commit will modify the ecp_mul_comb() version to handle restart
properly.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The case of MBEDTLS_ECP_RESTARTABLE isn't handled correctly yet: in that case
the DRBG instance should persist when resuming the operation. This will be
addressed in the next commit.
When both CTR_DRBG and HMAC_DRBG are available, CTR_DRBG is preferred since
both are suitable but CTR_DRBG tends to be faster and I needed a tie-breaker.
There are currently three possible cases to test:
- NO_INTERNAL_RNG is set -> tested in test_ecp_no_internal_rng
- it's unset and CTR_DRBG is available -> tested in the default config
- it's unset and CTR_DRBG is disabled -> tested in
test_ecp_internal_rng_no_ctr_drbg
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
No effect so far, except on dependency checking, as the feature it's meant to
disable isn't implemented yet (so the descriptions in config.h and the
ChangeLog entry are anticipation for now).
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Let code analyzers know that this is deliberate. For example MSVC
warns about the conversion if it's implicit.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In mpi_montmul, an auxiliary function for modular
exponentiation (mbedtls_mpi_mod_exp) that performs Montgomery
multiplication, the last step is a conditional subtraction to force
the result into the correct range. The current implementation uses a
branch and therefore may leak information about secret data to an
adversary who can observe what branch is taken through a side channel.
Avoid this potential leak by always doing the same subtraction and
doing a contant-trace conditional assignment to set the result.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Separate out a version of mpi_safe_cond_assign that works on
equal-sized limb arrays, without worrying about allocation sizes or
signs.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This reverts commit 2cc69fffcf.
A check was added in mpi_montmul because clang-analyzer warned about a
possibly null pointer. However this was a false positive. Recent
versions of clang-analyzer no longer emit a warning (3.6 does, 6
doesn't).
Incidentally, the size check was wrong: mpi_montmul needs
T->n >= 2 * (N->n + 1), not just T->n >= N->n + 1.
Given that this is an internal function which is only used from one
public function and in a tightly controlled way, remove both the null
check (which is of low value to begin with) and the size check (which
would be slightly more valuable, but was wrong anyway). This allows
the function not to need to return an error, which makes the source
code a little easier to read and makes the object code a little
smaller.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The previous version attempted to write the explicit IV from
the destination buffer before it has been written there.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Invasive testing strategy
Create a new header `common.h`.
Introduce a configuration option `MBEDTLS_TEST_HOOKS` for test-specific code, to be used in accordance with the invasive testing strategy.
This is to avoid confusion with the class of macros
MBEDTLS_SSL_PROTO_TLS1_X
which have an underscore between major and minor version number.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
TLS 1.3 record protection allows the addition of an arbitrary amount
of padding.
This commit introduces a configuration option
```
MBEDTLS_SSL_TLS13_PADDING_GRANULARITY
```
The semantics of this option is that padding is chosen in a minimal
way so that the padded plaintext has a length which is a multiple of
MBEDTLS_SSL_TLS13_PADDING_GRANULARITY.
For example, setting MBEDTLS_SSL_TLS13_PADDING_GRANULARITY to 1024
means that padded plaintexts will have length 1024, 2048, ..., while
setting it to 1 means that no padding will be used.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
This commit adds an error condition for bad user configurations
and updates the number of SSL module errors in error.h.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
This commit uses the previously defined macro to uniformize
bounds checks in several places. It also adds bounds checks to
the ClientHello writing function that were previously missing.
Also, the functions adding extensions to the ClientHello message
can now fail if the buffer is too small or a different error
condition occurs, and moreover they take an additional buffer
end parameter to free them from the assumption that one is
writing to the default output buffer.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
new name: mbedtls_x509_crt_parse_der_with_ext_cb
Co-authored-by: Gilles Peskine <gilles.peskine@arm.com>
Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
The structure `mbedtls_ssl_transform` representing record protection
transformations should ideally be used through a function-based
interface only, as this will ease change of implementation as well
as the addition of new record protection routines in the future.
This commit makes a step in that direction by introducing the
helper function `ssl_transform_get_explicit_iv_len()` which
returns the size of the pre-expansion during record encryption
due to the potential addition of an explicit IV.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
This commit simplifies nonce derivation for AEAD based record protection
routines in the following way.
So far, code distinguished between the cases of GCM+CCM and ChachaPoly:
- In the case of GCM+CCM, the AEAD nonce is the concatentation
of a 4-byte Fixed IV and a dynamically chosen 8-byte IV which is prepended
to the record. In Mbed TLS, this is always chosen to be the record sequence
number, but it need not to.
- In the case of ChaChaPoly, the AEAD nonce is derived as
`( 12-byte Fixed IV ) XOR ( 0 || 8-byte dynamic IV == record seq nr )`
and the dynamically chosen IV is no longer prepended to the record.
This commit removes this distinction by always computing the record nonce
via the formula
`IV == ( Fixed IV || 0 ) XOR ( 0 || Dynamic IV )`
The ChaChaPoly case is recovered in case `Len(Fixed IV) == Len(IV)`, and
GCM+CCM is recovered when `Len(IV) == Len(Fixed IV) + Len(Dynamic IV)`.
Moreover, a getter stub `ssl_transform_aead_dynamic_iv_is_explicit()`
is introduced which infers from a transform whether the dynamically
chosen part of the IV is explicit, which in the current implementation
of `mbedtls_ssl_transform` can be derived from the helper field
`mbedtls_ssl_transform::fixed_ivlen`.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
The computation of the per-record nonce for AEAD record protection
varies with the AEAD algorithm and the TLS version in use.
This commit introduces a helper function for the nonce computation
to ease readability of the quite monolithic record encrytion routine.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
The previous record protection code added the explicit part of the
record nonce prior to encrypting the record. This temporarily leaves
the record structure in the undesireable state that the data outsie
of the interval `rec->data_offset, .., rec->data_offset + rec->data_len`
has already been written.
This commit moves the addition of the explicit IV past record encryption.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
The internal functions
`ssl_cid_{build/parse}_inner_plaintext()`
implement the TLSInnerPlaintext mechanism used by DTLS 1.2 + CID
in order to allow for flexible length padding and to protect the
true content type of a record.
This feature is also present in TLS 1.3 support for which is under
development. As a preparatory step towards sharing the code between
the case of DTLS 1.2 + CID and TLS 1.3, this commit renames
`ssl_cid_{build/parse}_inner_plaintext()`
to
`ssl_{build/parse}_inner_plaintext()`.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Now that lifetimes have structures and secure element drivers handle
all the lifetimes with a certain location, update driver registration
to take a location as argument rather than a lifetime.
This commit updates the Mbed TLS implementation.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The ssl_cli.c:ssl_write_supported_elliptic_curves_ext()
function is compiled only if MBEDTLS_ECDH_C, MBEDTLS_ECDSA_C
or MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED is defined which
implies that MBEDTLS_ECP_C is defined. Thus remove the
precompiler conditions on MBEDTLS_ECP_C in its code.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
On dual world platforms, we want to run the PK module (pk.c) on the NS
side so TLS can use PSA APIs via the PK interface. PK currently has a
hard dependency on mbedtls_ecc_group_to_psa() which is declared in
crypto_extra.h, but only defined in psa_crypto.c, which is only built
for the S side.
Without this change, dual world platforms get error messages like the
following.
[Error] @0,0: L6218E: Undefined symbol mbedtls_ecc_group_to_psa (referred from BUILD/LPC55S69_NS/ARM/mbed-os/features/mbedtls/mbed-crypto/src/pk.o)
Make mbedtls_ecc_group_to_psa() inline within crypto_extra.h so that it
is available to both NS and S world code.
Fixes#3300
Signed-off-by: Darryl Green <darryl.green@arm.com>
Signed-off-by: Jaeden Amero <jaeden.amero@arm.com>
Clear bits in mbedtls_ecc_group_to_psa() to avoid static analyzers and
possibly compilers from warning that bits may be used uninitialized in
certain code paths.
For example, if mbedtls_ecc_group_to_psa() were to be inlined in
crypto_extra.h, the following compiler warning is likely.
In file included from ../include/psa/crypto.h:3774:0,
from ../include/mbedtls/pk.h:49,
from pk.c:29:
pk.c: In function 'mbedtls_pk_wrap_as_opaque':
../include/psa/crypto_struct.h:460:33: error: 'bits' may be used uninitialized in this function [-Werror=maybe-uninitialized]
attributes->core.bits = (psa_key_bits_t) bits;
^~~~~~~~~~~~~~~~~~~~~
pk.c:608:12: note: 'bits' was declared here
size_t bits;
^~~~
Signed-off-by: Jaeden Amero <jaeden.amero@arm.com>
This routine is functionally equivalent to mbedtls_x509_crt_parse_der(),
but it accepts an additional callback function which it calls with
every unsupported certificate extension.
Proposed solution to https://github.com/ARMmbed/mbedtls/issues/3241
Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
getaddrinfo() is not available on win2k. By including wspiapi.h (if
_WIN32_WINNT is defined as value < 0x0501) then a compatibility layer
will be used when running on win2k. For more details, refer to Microsoft
docs for getaddrinfo().
Signed-off-by: opatomic <j@opatomic.com>
ecp_double_add_mxz wrongly does an MPI addition followed by a call to
MOD_MUL instead of MOD_ADD. This is more visible since the
mbedtls_mpi_xxx_mod functions have been added in commit 3b3b34f608
("Replace some macros by functions").
Fix that by using mbedtls_mpi_add_mod instead. The testsuite still
passes after that change.
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
It is sufficient to check for the strongest limit only. Using a smaller
type ensures there is no overflow (assuming size_t is at least 32 bits).
Fixes#2916
Signed-off-by: irwir <irwir@users.noreply.github.com>
1. The functions mbedtls_high_level_strerr and mbedtls_low_level_strerr
accept any error code and extract the high-level and low-level parts
respectively.
2. Documentation updates.
Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>
GCC and Clang accept
```
typedef struct foo foo_t;
typedef struct foo { ... } foo_t;
```
But this is not valid ISO C due to the redefinition of `foo_t`.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
A file generated based on the output of `make list` from programs has been
re-generated.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
When parsing a certificate with the basic constraints extension
the max_pathlen that was read from it was incremented regardless
of its value. However, if the max_pathlen is equal to INT_MAX (which
is highly unlikely), an undefined behaviour would occur.
This commit adds a check to ensure that such value is not accepted
as valid. Relevant tests for INT_MAX and INT_MAX-1 are also introduced.
Certificates added in this commit were generated using the
test_suite_x509write, function test_x509_crt_check. Input data taken
from the "Certificate write check Server1 SHA1" test case, so the generated
files are like the "server1.crt", but with the "is_ca" field set to 1 and
max_pathlen as described by the file name.
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
The presence of these markers in the original code was helpful to me in
figuring out that this portion of the code is auto-generated.
Therefore, I think those are useful and should be present.
Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>
- Use switch case instead of loop to generate faster code
- Add #if defined to address compiler error
Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>
Problem
-------
mbedtls_strerror is a utility function which converts an mbedTLS error code
into a human readable string. It requires the caller to allocate a buffer every
time an error code needs to be converted to a string. It is an overkill and a
waste of RAM for resource constrained microcontrollers - where the most common
use case is to use these strings for logging.
Solution
--------
The proposed commit adds two functions:
* const char * mbedtls_high_level_strerr( int error_code );
* const char * mbedtls_low_level_strerr( int error_code );
The above two functions convert the high level and low level parts of an mbedTLS
error code to human readable strings. They return a const pointer to an
unmodifiable string which is not supposed to be modified by the caller and only
to be used for logging purposes. The caller no longer needs to allocate a
buffer.
Backward Compatibility
----------------------
The proposed change is completely backward compatible as it does not change
the existing mbedtls_strerror function and ensures that it continues to behave
the same way.
Signed-off-by: Gaurav Aggarwal <aggarg@amazon.com>
This commit introduces two changes:
- Add in_msg and out_msg calculations for buffer upsizing. This was previously
considered as unnecessary, but renegotiation using certain ciphersuites needs
this.
- Improving the way out_msg and in_msg pointers are calculated, so that even
if no resizing is introduced, the pointers remain the same;
New tests added:
- various renegotiation schemes with a range of MFL's and ciphersuites;
- an ssl-opt.sh test exercising two things that were problematic: renegotiation
with TLS-ECDHE-ECDSA-WITH-AES-128-CCM-8 and a server MFL that's smaller
than the one negotiated by the client.
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Since the server might want to have a different maximum fragment length
for the outgoing messages than the negotiated one - introduce a new way of
computing it. This commit also adds additional ssl-opt.sh tests ensuring
that the maximum fragment lengths are set as expected.
mbedtls_ssl_get_max_frag_len() is now a deprecated function,
being an alias to mbedtls_ssl_get_output_max_frag_len(). The behaviour
of this function is the same as before.
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
t is never used uninitialized, since the first loop iteration reads 0
bytes of it and then writes hash_len bytes, and subsequent iterations
read and write hash_len bytes. However this is somewhat fragile, and
it would be legitimate for a static analyzer to be unsure.
Initialize t explicitly, to make the code clearer and more robust, at
negligible cost.
Reported by Vasily Evseenko in
https://github.com/ARMmbed/mbedtls/pull/2942
with a slightly different fix.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The current logging was sub-standard, in particular there was no trace
whatsoever of the HelloVerifyRequest being sent. Now it's being logged with
the usual levels: 4 for full content, 2 return of f_send, 1 decision about
sending it (or taking other branches in the same function) because that's the
same level as state changes in the handshake, and also same as the "possible
client reconnect" message" to which it's the logical continuation (what are we
doing about it?).
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
In x509.c, the self-test code is dependent on MBEDTLS_CERTS_C and
MBEDTLS_SHA256_C being enabled. At some point in the recent past that dependency
was on MBEDTLS_SHA1_C but changed to SHA256, but the comment wasn't updated.
This commit updates the comment.
Signed-off-by: Simon Butcher <simon.butcher@arm.com>
Section 4.2.8 of RFC 6347 describes how to handle the case of a DTLS client
establishing a new connection using the same UDP quartet as an already active
connection, which we implement under the compile option
MBEDTLS_SSL_DLTS_CLIENT_PORT_REUSE. Relevant excerpts:
[the server] MUST NOT destroy the existing
association until the client has demonstrated reachability either by
completing a cookie exchange or by completing a complete handshake
including delivering a verifiable Finished message.
[...]
The reachability requirement prevents
off-path/blind attackers from destroying associations merely by
sending forged ClientHellos.
Our code chooses to use a cookie exchange for establishing reachability, but
unfortunately that check was effectively removed in a recent refactoring,
which changed what value ssl_handle_possible_reconnect() needs to return in
order for ssl_get_next_record() (introduced in that refactoring) to take the
proper action. Unfortunately, in addition to changing the value, the
refactoring also changed a return statement to an assignment to the ret
variable, causing the function to reach the code for a valid cookie, which
immediately destroys the existing association, effectively bypassing the
cookie verification.
This commit fixes that by immediately returning after sending a
HelloVerifyRequest when a ClientHello without a valid cookie is found. It also
updates the description of the function to reflect the new return value
convention (the refactoring updated the code but not the documentation).
The commit that changed the return value convention (and introduced the bug)
is 2fddd3765e, whose commit message explains the
change.
Note: this bug also indirectly caused the ssl-opt.sh test case "DTLS client
reconnect from same port: reconnect" to occasionally fail due to a race
condition between the reception of the ClientHello carrying a valid cookie and
the closure of the connection by the server after noticing the ClientHello
didn't carry a valid cookie after it incorrectly destroyed the previous
connection, that could cause that ClientHello to be invisible to the server
(if that message reaches the server just before it does `net_close()`). A
welcome side effect of this commit is to remove that race condition, as the
new connection will immediately start with a ClientHello carrying a valid
cookie in the SSL input buffer, so the server will not call `net_close()` and
not risk discarding a better ClientHello that arrived in the meantime.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
See the comments in the code for how an attack would go, and the ChangeLog
entry for an impact assessment. (For ECDSA, leaking a few bits of the scalar
over several signatures translates to full private key recovery using a
lattice attack.)
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
This change was first introduced in 8af3923 - see this commit for more background.
After the removal of crypto directory, there are no targets that require a
crypto library with the directory prefix, so there's also no need for the priority
dependency to be declared. This commit removes it.
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Define MBEDTLS_STATIC_TESTABLE to mark code that is only exported for
test purposes. Since this is for internal library
use only, define it in a header in library/. Since there is no
suitable header, create one.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
When this option is enabled, the product includes additional
interfaces that enable additional tests. This option should not be
enabled in production, but is included in the "full" build to enable
the extra tests.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Merge the latest state of the target branch (mbedtls/development) into the
pull request to merge mbed-crypto into mbedtls.
Conflicts:
* ChangeLog: add/add conflict. Resolve by using the usual section order.
Rename identifiers containing double-underscore (`__`) to avoid `__`.
The reason to avoid double-underscore is that all identifiers
containing double-underscore are reserved in C++. Rename all such
identifiers that appear in any public header, including ssl_internal.h
which is in principle private but in practice is installed with the
public headers.
This commit makes check-names.sh pass.
```
perl -i -pe 's/\bMBEDTLS_SSL__ECP_RESTARTABLE\b/MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED/g; s/\bMBEDTLS_KEY_EXCHANGE_(_\w+)_(_\w+)\b/MBEDTLS_KEY_EXCHANGE${1}${2}/g' include/mbedtls/*.h library/*.c programs/*/*.c scripts/data_files/rename-1.3-2.0.txt tests/suites/*.function
```
Remove code guarded by `USE_CRYPTO_SUBMODULE`. It's dead now that
crypto can no longer be a submodule.
In `library/Makefile`:
* Replace `$(CRYPTO_INCLUDE)` with the single include directory
`-I../include`.
* Remove references to `$(OBJS_CRYPTO)` when it's in addition to the
local objects (`*.o`) since `$(OBJS_CRYPTO)` is now a subset of the
local objects.
* Merge modules that were duplicated between the mbedtls and the
mbed-crypto repositories back into the single list for `OBJS_CRYPTO`.
Merge `unremove-non-crypto` into `mbedtls/development`. The branch
`unremove-non-crypto` was obtained by starting from `mbed-crypto/development`,
then reverting many commits that removed X.509 and TLS functionality when Mbed
Crypto forked from Mbed TLS (the “unremoval”), then make a few tweaks to
facilitate the merge.
The unremoval step restored old versions of some tls files. If a file doesn't
exist in mbed-crypto, check out the mbedtls version, regardless of what
happened during the unremoval of tls files in the crypto tree. Also
unconditionally take the mbedtls version of a few files where the
modifications are completely project-specific and are not relevant in
mbed-crypto:
* `.github/issue_template.md`: completely different. We may want to reconcile
them independently as a follow-up.
* `.travis.yml`: would only be reverted to an earlier tls version.
* `README.md`: completely different. We may want to reconcile them
independently as a follow-up.
* `doxygen/input/doc_mainpage.h`: the changes in crypto were minimal and not
relevant except as a stopgap as mbed-crypto did not have its own product
versioning in the Doxygen documentation.
* `tests/.jenkins/Jenkinsfile`: completely different.
* `tests/data_files/Makefile`: there were no changes in mbed-crypto,
but the unremoval step restored an old version.
Shell script for everything to do after the merge apart from the conflict
resolution:
```
tls_files=($(comm -23 <(git ls-tree -r --name-only HEAD) <(git ls-tree -r --name-only $(git merge-base upstream-crypto/development MERGE_HEAD))))
tls_files+=($tls_files .github/issue_template.md .travis.yml README.md doxygen/input/doc_mainpage.h tests/.jenkins/Jenkinsfile tests/data_files/Makefile)
git checkout --theirs HEAD -- $tls_files
git add -- $tls_files
```
Resolve the remaining conflicts:
* `library/CMakeLists.txt`:
* Keep the TLS definition of `src_crypto`
* `USE_SHARED_MBEDTLS_LIBRARY`: keep all three libraries, with both
`include` and `crypto/include` in `target_include_directories`, all with
version `2.21.0`.
* `programs/Makefile`:
* Reconcile the APPS lists (add/add from a differently-formatted common
ancestor): insert the `psa/*` from crypto into the tls list.
* Keep the `fuzz` target defined only in tls version.
* Keep the recipe (only in tls version) cleaning `ssl_pthread_server`
stuff for the `clean` target.
* `scripts/config.py`:
* `include_in_full`: add/add conflict. Keep both.
* `tests/scripts/all.sh`:
* `component_test_no_use_psa_crypto_full_cmake_asan`: partially old
version in crypto. Take the tls version.
* `component_test_malloc_0_null` and more: take
`component_test_malloc_0_null` from crypto (with `config.py` rather than
`config.pl`, and with `$ASAN_FLAGS` rather than an explicit list), but
add the call to `ssl-opt.sh` from tls. Take the other components from
crypto.
With this commit, building and running the unit tests with both `make ` and
`cmake` work in the default configuration on Linux. Other platforms, build
systems and configurations are likely not to work, and there is some
regression in test coverage.
There is some loss of functionality because the unremoval step restored older
versions of tls content. This commit contains the latest tls version of
tls-only files, but some changes from the tls side in files that existed on
both sides have regressed. Most problematic changes are hunks that remove some
tls-specific feature and contain either a C preprocessor symbol identifying a
tls-specific module or option, or the name of a tls-specific file. Hunks
that remove a tls-specific preprocessor symbol can be identified with the
regular expression `^-.*MBEDTLS_(ERR_)?(PKCS11|X509|NET|SSL)_`.
Subsequent commits will revert a few parts of the patch from this merge commit
in order to restore the tls functionality that it removes, ensure that the
test coverage includes what was covered in either branch, and fix test
failures.
This reverts commit d832f187f7.
Conflicts:
* CMakeLists.txt:
* USE_PKCS11_HELPER_LIBRARY: there has been a change immediately before
where it was removed. Just re-add what was removed.
* tests/CMakeLists.txt:
* USE_PKCS11_HELPER_LIBRARY: there has been a change immediately before
where it was removed. Just re-add what was removed.
This reverts commit d874a1fd14.
Conflicts:
* CMakeLists.txt:
* ENABLE_ZLIB_SUPPORT: there has been a change immediately after
where it was removed. Just re-add what was removed.
* tests/CMakeLists.txt:
* ENABLE_ZLIB_SUPPORT: there has been a change immediately after
where it was removed. Just re-add what was removed.
This reverts commit 8298d70bee.
Conflicts:
* library/Makefile: removal of SOEXT_X509 and SOEXT_TLS vs change of
value of SOEXT_CRYPTO. Keep all, with the new value of SOEXT_CRYPTO.
This reverts commit 1c66e48670.
Conflicts:
* include/mbedtls/check_config.h:
* MBEDTLS_SSL_PROTO_SSL3: there has been an addition (of
MBEDTLS_SHA512_NO_SHA384) at the place where it was removed. Re-add it
after (alphabetical order).
* MBEDTLS_ENABLE_WEAK_CIPHERSUITES: there has been an addition (of
MBEDTLS_CTR_DRBG_USE_128_BIT_KEY) at the place where it was removed.
Re-add it after (alphabetical order).
* MBEDTLS_SSL_ALL_ALERT_MESSAGES: there has been an addition (of
MBEDTLS_SHA512_SMALLER) at the place where it was removed. Re-add it
after (alphabetical order).
* include/mbedtls/config.h:
* MBEDTLS_ENABLE_WEAK_CIPHERSUITES: there has been an addition (of
MBEDTLS_CTR_DRBG_USE_128_BIT_KEY) at the place where it was removed.
Re-add it after (alphabetical order).
* MBEDTLS_SSL_ALL_ALERT_MESSAGES: there has been an addition (of
MBEDTLS_SHA512_SMALLER) at the place where it was removed. Re-add it
after (alphabetical order).
* library/version_features.c: re-generate by running
scripts/generate_features.pl.
* programs/test/query_config.c: re-generate by running
scripts/generate_query_config.pl.
* scripts/config.pl: this file has been replaced by config.py. Port
the reversed changes to config.py:
* Revert removing three symbols from the list of symbols to
exclude from full.
* Revert removing one symbol (MBEDTLS_NET_C) from the list of symbols
to exclude from baremetal.
* scripts/footprint.sh:
* Re-add the line to unset MBEDTLS_NET_C, but with config.py instead of
config.pl.
* tests/scripts/all.sh:
* component_test_no_platform: re-add the line to unset MBEDTLS_NET_C, but
with config.py instead of config.pl.
* component_build_arm_none_eabi_gcc,
component_build_arm_none_eabi_gcc_no_udbl_division,
component_build_arm_none_eabi_gcc_no_64bit_multiplication,
component_build_armcc: these components now use the baremetal
configuration, so they do not need to turn off MBEDTLS_NET_C explicitly.
This reverts commit bb1f701212.
* include/mbedtls/check_config.h:
* MBEDTLS_X509_RSASSA_PSS_SUPPORT: there has been an addition (of
MBEDTLS_SHA512_NO_SHA384) at the place where it was removed.
Re-add it before MBEDTLS_SHA512_NO_SHA384 to keep it grouped
with MBEDTLS_RSA_C.
Conflicts:
* scripts/config.pl: this file has been replaced by config.py. Port
the reversed changes to config.py:
* Revert removing three symbols from the list of symbols to
exclude from full.
Although the 'flags' variable is not checked or used after a call to
mbedtls_ssl_check_cert_usage, it might be in the future. With this fix, after
each iteration, the flags will apply only to the most recent certificate, not
to any of the previous ones checked. This fix also stops any reads and
writes via a '|=' from/to an uninitialized variable happening.
This commit fixes#2444.
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Add a conditional buffer resizing feature. Introduce tests exercising
it in various setups (serialization, renegotiation, mfl manipulations).
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Some code paths want to access members of the mbedtls_rsa_context structure.
We can only do that when using our own implementation, as otherwise we don't
know anything about that structure.
When parsing a PKCS#1 RSAPrivateKey structure, all parameters are always
present. After importing them, we need to call rsa_complete() for the sake of
alternative implementations. That function interprets zero as a signal for
"this parameter was not provided". As that's never the case, we mustn't pass
any zero value to that function, so we need to explicitly check for it.
This commit is the final step in separating the functionality of
what was originally ssl_tls.c into both ssl_tls.c and ssl_msg.c.
So far, ssl_msg.c has been created as an identical copy of ssl_tls.c.
For each block of code in these files, this commit removes it from
precisely one of the two files, depending on where the respective
functionality belongs.
The splitting separates the following functionalities:
1) An implementation of the TLS and DTLS messaging layer, that is,
the record layer as well as the DTLS retransmission state machine.
This is now contained in ssl_msg.c
2) Handshake parsing and writing functions shared between client and
server (functions specific to either client or server are implemented
in ssl_cli.c and ssl_srv.c, respectively).
This is remains in ssl_tls.c.
This commit adds the newly created copy ssl_msg.c of ssl_tls.c
to the build system but guards its content by an `#if 0 ... #endif`
preprocessor guard in order to avoid compilation failures resulting
from code duplication. This guard will be removed once the contents
of ssl_tls.c and ssl_msg.c have been made disjoint.
This commit is the first in a series of commits aiming to split
the content of ssl_tls.c in two files ssl_tls.c and ssl_msg.c.
As a first step, this commit replaces ssl_tls.c by two identical
copies ssl_tls_old.c and ssl_msg.c. Even though the file
ssl_tls_old.c will subsequently be renamed back into ssl_tls.c,
this approach retains the git history in both files.
This reverts commit c0c92fea3d, reversing
changes made to bfc73bcfd2.
stat() will never return S_IFLNK as the file type, as stat() explicitly
follows symlinks.
Fixes#3005.
Files deleted by us: keep them deleted.
```
git rm $(git status -s | sed -n 's/^DU //p')
```
Individual files with conflicts:
* `README.md`: keep the crypto version.
* `doxygen/input/doc_mainpage.h`: keep the crypto version (with an obsolete Mbed Crypto version number).
* `include/mbedtls/error.h`:
* `ERROR`: similar additions made through parallel commits, with only whitespace differences. Align with the tls version.
* `library/CMakeLists.txt`: keep the crypto version.
* `library/Makefile`: keep the crypto version.
* `scripts/generate_errors.pl`: keep the crypto version (the relevant changes were made through parallel commits).
* `tests/scripts/check-test-cases.py`:
* `Results`: keep the crypto version, which has both the new argument to the constructor (added in crypto only) and the class docstring (added through parallel commits).
* `tests/suites/helpers.function`:
* `ARRAY_LENGTH`, `ASSERT_ALLOC`: additions in the same location. Keep both, in indifferent order.
* `tests/suites/target_test.function`:
* `receive_uint32`: keep the crypto version which has an additional bug fix. The tls changes made in tls are irrelevant after this bug fix.
* `visualc/VS2010/mbedTLS.vcxproj`: run `scripts/generate_visualc_files.pl`.
Review of non-conflicting changes:
* `all.sh`: 1 change.
* zlib test components: don't add them.
* `include/CMakeLists.txt`: 1 change.
* `target_include_directories`: doesn't work as is (different target name). Don't take the change.
* All other non-conflicting changes: take them.
Adapt to the change of encoding of elliptic curve key types in PSA
crypto. Before, an EC key type encoded the TLS curve identifier. Now
the EC key type only includes an ad hoc curve family identifier, and
determining the exact curve requires both the key type and size. This
commit moves from the old encoding and old definitions from
crypto/include/mbedtls/psa_util.h to the new encoding and definitions
from the immediately preceding crypto submodule update.
If psa_key_agreement_ecdh fails, there may be output that leaks
sensitive information in the output buffer. Zeroize it.
If this is due to an underlying failure in the ECDH implementation, it
is currently not an issue since both the traditional Mbed TLS/Crypto
implementation and Everest only write to the output buffer once every
intermediate step has succeeded, but zeroizing is more robust. If this
is because the recently added key size check fails, a leak could be a
serious issue.
All key types now have an encoding on 32 bits where the bottom 16 bits
are zero. Change to using 16 bits only.
Keep 32 bits for key types in storage, but move the significant
half-word from the top to the bottom.
Likewise, change EC curve and DH group families from 32 bits out of
which the top 8 and bottom 16 bits are zero, to 8 bits only.
Reorder psa_core_key_attributes_t to avoid padding.
Remove the values of curve encodings that are based on the TLS registry
and include the curve size, keeping only the new encoding that merely
encodes a curve family in 8 bits.
Keep the old constant names as aliases for the new values and
deprecate the old names.
Define constants for ECC curve families and DH group families. These
constants have 0x0000 in the lower 16 bits of the key type.
Support these constants in the implementation and in the PSA metadata
tests.
Switch the slot management and secure element driver HAL tests to the
new curve encodings. This requires SE driver code to become slightly
more clever when figuring out the bit-size of an imported EC key since
it now needs to take the data size into account.
Switch some documentation to the new encodings.
Remove the macro PSA_ECC_CURVE_BITS which can no longer be implemented.
Change the representation of psa_ecc_curve_t and psa_dh_group_t from
the IETF 16-bit encoding to a custom 24-bit encoding where the upper 8
bits represent a curve family and the lower 16 bits are the key size
in bits. Families are based on naming and mathematical similarity,
with sufficiently precise families that no two curves in a family have
the same bit size (for example SECP-R1 and SECP-R2 are two different
families).
As a consequence, the lower 16 bits of a key type value are always
either the key size or 0.
Internally, use the corresponding function from psa_crypto.c instead.
Externally, this function is not used in Mbed TLS and is documented as
"may change at any time".
Don't rely on the bit size encoded in the PSA curve identifier, in
preparation for removing that.
For some inputs, the error code on EC key creation changes from
PSA_ERROR_INVALID_ARGUMENT to PSA_ERROR_NOT_SUPPORTED or vice versa.
There will be further such changes in subsequent commits.
When mbedtls_x509_crt_parse_path() checks each object in the supplied path, it only processes regular files. This change makes it also accept a symlink to a file. Fixes#3005.
This was observed to be a problem on Fedora/CentOS/RHEL systems, where the ca-bundle in the default location is actually a symlink.
ssl_decompress_buf() was operating on data from the ssl context, but called at
a point where this data is actually in the rec structure. Call it later so
that the data is back to the ssl structure.
Otherwise these values are recomputed in mbedtls_rsa_deduce_crt, which
currently suffers from side channel issues in the computation of QP (see
https://eprint.iacr.org/2020/055). By loading the pre-computed values not
only is the side channel avoided, but runtime overhead of loading RSA keys
is reduced.
Discussion in https://github.com/ARMmbed/mbed-crypto/issues/347
If Y was constructed through functions in this module, then Y->n == 0
iff Y->p == NULL. However we do not prevent filling mpi structures
manually, and zero may be represented with n=0 and p a valid pointer.
Most of the code can cope with such a representation, but for the
source of mbedtls_mpi_copy, this would cause an integer underflow.
Changing the test for zero from Y->p==NULL to Y->n==0 causes this case
to work at no extra cost.
If psa_mac_finish_internal fails (which can only happen due to bad
parameters or hardware problem), the error code was converted to
PSA_ERROR_INVALID_SIGNATURE if the uninitialized stack variable
actual_mac happened to contain the expected MAC. This is a minor bug
but it may be possible to leverage it as part of a longer attack path
in some scenarios.
Reported externally. Found by static analysis.
The library style is to start with the includes corresponding to the
current module and then the rest in alphabetical order. Some modules
have several header files (eg. ssl_internal.h).
The recently added error.h includes did not respect this convention and
this commit restores it. In some cases this is not possible just by
moving the error.h declarations. This commit fixes the pre-existing
order in these instances too.
Now that the Error module has error codes as well and is processed by
the generate_errors script like any other module, we don't need to
include the header manually.
One of the error codes was already reserved, this commit just makes it
explicit. The other one is a new error code for initializing return
values in the library: `MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED` should
not be returned by the library. If it is returned, then it is surely a
bug in the library or somebody is tampering with the device.
One of the error codes was already reserved, this commit just makes it
explicit. The other one is a new error code for initializing return
values in the library: `MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED` should
not be returned by the library. If it is returned, then it is surely a
bug in the library or somebody is tampering with the device.
The functions mbedtls_ctr_drbg_random() and
mbedtls_ctr_drbg_random_with_add() could return 0 if an AES function
failed. This could only happen with alternative AES
implementations (the built-in implementation of the AES functions
involved never fail), typically due to a failure in a hardware
accelerator.
Bug reported and fix proposed by Johan Uppman Bruce and Christoffer
Lauri, Sectra.
Rename some macros and functions related to signature which are
changing as part of the addition of psa_sign_message and
psa_verify_message.
perl -i -pe '%t = (
PSA_KEY_USAGE_SIGN => PSA_KEY_USAGE_SIGN_HASH,
PSA_KEY_USAGE_VERIFY => PSA_KEY_USAGE_VERIFY_HASH,
PSA_ASYMMETRIC_SIGNATURE_MAX_SIZE => PSA_SIGNATURE_MAX_SIZE,
PSA_ASYMMETRIC_SIGN_OUTPUT_SIZE => PSA_SIGN_OUTPUT_SIZE,
psa_asymmetric_sign => psa_sign_hash,
psa_asymmetric_verify => psa_verify_hash,
); s/\b(@{[join("|", keys %t)]})\b/$t{$1}/ge' $(git ls-files . ':!:**/crypto_compat.h')
* origin/pr/2854:
Shorter version of mbedtls_ssl_send_fatal_handshake_failure
Resolve#2801 - remove repetitive assignment to ssl->in_msg (the first value was never used)
Resolve#2800 - move declaration to avoid unused variable warning in case MBEDTLS_SSL_PROTO_DTLS was undefined
Resolve#2717 - remove erroneous sizeof (the operator was applied to constant integer number)
In the CTR_DRBG module, add selftest data for when
MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled.
I generated the test data by running our own code. This is ok because
we have other tests that ensure that the algorithm is implemented
correctly.
This makes programs/self/selftest pass when
MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled.
First deal with deleted files.
* Files deleted by us: keep them deleted.
* Files deleted by them, whether modified by us or not: keep our version.
```
git rm $(git status -s | sed -n 's/^DU //p')
git reset -- $(git status -s | sed -n 's/^D //p')
git checkout -- $(git status -s | sed -n 's/^ D //p')
git add -- $(git status -s | sed -n 's/^UD //p')
```
Individual files with conflicts:
* `3rdparty/everest/library/Hacl_Curve25519_joined.c`: spurious conflict because git mistakenly identified this file as a rename. Keep our version.
* `README.md`: conflict due to their change in a paragraph that doesn't exist in our version. Keep our version of this paragraph.
* `docs/architecture/Makefile`: near-identical additions. Adapt the definition of `all_markdown` and include the clean target.
* `doxygen/input/docs_mainpage.h`: conflict in the version number. Keep our version number.
* `include/mbedtls/config.h`: two delete/modify conflicts. Keep the removed chunks out.
* `library/CMakeLists.txt`: discard all their changes as they are not relevant.
* `library/Makefile`:
* Discard the added chunk about the crypto submodule starting with `INCLUDING_FROM_MBEDTLS:=1`.
* delete/modify: keep the removed chunk out.
* library build: This is almost delete/modify. Their changes are mostly not applicable. Do keep the `libmbedcrypto.$(DLEXT): | libmbedcrypto.a` order dependency.
* `.c.o`: `-o` was added on both sides but in a different place. Change to their place.
* `library/error.c`: to be regenerated.
* `library/version_features.c`: to be regenerated.
* `programs/Makefile`: Most of the changes are not relevant. The one relevant change is in the `clean` target for Windows; adapt it by removing `/S` from our version.
* `programs/test/query_config.c`: to be regenerated.
* `scripts/config.py`: added in parallel on both sides. Keep our version.
* `scripts/footprint.sh`: parallel changes. Keep our version.
* `scripts/generate_visualc_files.pl`: one delete/modify conflict. Keep the removed chunks out.
* `tests/Makefile`: discard all of their changes.
* `tests/scripts/all.sh`:
* `pre_initialize_variables` add `append_outcome`: add it.
* `pre_initialize_variables` add `ASAN_CFLAGS`: already there, keep our version.
* `pre_parse_command_line` add `--no-append-outcome`: add it.
* `pre_parse_command_line` add `--outcome-file`: add it.
* `pre_print_configuration`: add `MBEDTLS_TEST_OUTCOME_FILE`.
* Several changes in SSL-specific components: keep our version without them.
* Several changes where `config.pl` was changed to `config.py` and there was an adjacent difference: keep our version.
* Changes regarding the inclusion of `MBEDTLS_MEMORY_xxx`: ignore them here, they will be normalized in a subsequent commit.
* `component_test_full_cmake_gcc_asan`: add it without the TLS tests.
* `component_test_no_use_psa_crypto_full_cmake_asan`: keep the fixed `msg`, discard other changes.
* `component_test_memory_buffer_allocator_backtrace`, `component_test_memory_buffer_allocator`: add them without the TLS tests.
* `component_test_m32_everest`: added in parallel on both sides. Keep our version.
* `tests/scripts/check-names.sh`, `tests/scripts/list-enum-consts.pl`, `tests/scripts/list-identifiers.sh`, ``tests/scripts/list-macros.sh`: discard all of their changes.
* `tests/scripts/test-ref-configs.pl`: the change in the conflict is not relevant, so keep our version there.
* `visualc/VS2010/*.vcxproj`: to be regenerated.
Regenerate files:
```
scripts/generate_visualc_files.pl
git add visualc/VS2010/*.vcxproj
scripts/generate_errors.pl
git add library/error.c
scripts/generate_features.pl
git add library/version_features.c
scripts/generate_query_config.pl
git add programs/test/query_config.c
```
Rejected changes in non-conflicting files:
* `CMakeLists.txt`: discard their addition which has already been side-ported.
* `doxygen/mbedtls.doxyfile`: keep the version number change. Discard the changes related to `../crypto` paths.
Keep the following changes after examination:
* `.travis.yml`: all of their changes are relevant.
* `include/mbedtls/error.h`: do keep their changes. Even though Crypto doesn't use TLS errors, it must not encroach on TLS's allocated numbers.
* `tests/scripts/check-test-cases.py`: keep the code dealing with `ssl-opt.sh`. It works correctly when the file is not present.
Using 4096 bytes of stack for the temporary buffer used for holding a
throw-away DER-formatted CSR limits the portability of generating
certificate signing requests to only devices with lots of stack space.
To increase portability, use the mbedtls_pem_write_buffer() in-place
capability instead, using the same buffer for input and output. This
works since the DER encoding for some given data is always smaller than
that same data PEM-encoded.
PEM format is desirable to use even on stack-constrained devices as the
format is easy to work with (for example, copy-pasting from a tiny
device's serial console output, for CSRs generated on tiny devices
without the private key leaving said tiny device).
mbedtls_pk_sign does not take the size of its output buffer as a
parameter. We guarantee that MBEDTLS_PK_SIGNATURE_MAX_SIZE is enough.
For RSA and ECDSA signatures made in software, this is ensured by the
way MBEDTLS_PK_SIGNATURE_MAX_SIZE is defined at compile time. For
signatures made through RSA-alt and PSA, this is not guaranteed
robustly at compile time, but we can test it at runtime, so do that.
This issue has been reported by Tuba Yavuz, Farhaan Fowze, Ken (Yihang) Bai,
Grant Hernandez, and Kevin Butler (University of Florida) and
Dave Tian (Purdue University).
In AES encrypt and decrypt some variables were left on the stack. The value
of these variables can be used to recover the last round key. To follow best
practice and to limit the impact of buffer overread vulnerabilities (like
Heartbleed) we need to zeroize them before exiting the function.
When writing a private EC key, use a constant size for the private
value, as specified in RFC 5915. Previously, the value was written
as an ASN.1 INTEGER, which caused the size of the key to leak
about 1 bit of information on average, and could cause the value to be
1 byte too large for the output buffer.
In the case of *ret we might need to preserve a 0 value throughout the
loop and therefore we need an extra condition to protect it from being
overwritten.
The value of done is always 1 after *ret has been set and does not need
to be protected from overwriting. Therefore in this case the extra
condition can be removed.
The code relied on the assumptions that CHAR_BIT is 8 and that unsigned
does not have padding bits.
In the Bignum module we already assume that the sign of an MPI is either
-1 or 1. Using this, we eliminate the above mentioned dependency.
The signature of mbedtls_mpi_cmp_mpi_ct() meant to support using it in
place of mbedtls_mpi_cmp_mpi(). This meant full comparison functionality
and a signed result.
To make the function more universal and friendly to constant time
coding, we change the result type to unsigned. Theoretically, we could
encode the comparison result in an unsigned value, but it would be less
intuitive.
Therefore we won't be able to represent the result as unsigned anymore
and the functionality will be constrained to checking if the first
operand is less than the second. This is sufficient to support the
current use case and to check any relationship between MPIs.
The only drawback is that we need to call the function twice when
checking for equality, but this can be optimised later if an when it is
needed.
Multiplication is known to have measurable timing variations based on
the operands. For example it typically is much faster if one of the
operands is zero. Remove them from constant time code.
1. variable name accoriding to the Mbed TLS coding style;
2. add a comment explaining safety of the optimization;
3. safer T2 initialization and memory zeroing on the function exit;
Record checking fails if mbedtls_ssl_check_record() is called with
external buffer. Received record sequence number is available in the
incoming record but it is not available in the ssl contexts `in_ctr`-
variable that is used when decoding the sequence number.
To fix the problem, temporarily update ssl context `in_ctr` to
point to the received record header and restore value later.
We want to explicitly disallow creating new transactions when a
transaction is already in progress. However, we were incorrectly
checking for the existence of the injected entropy file before
continuing with creating a transaction. This meant we could have a
transaction already in progress and would be able to still create a new
transaction. It also meant we couldn't start a new transaction if any
entropy had been injected. Check the transaction file instead of the
injected entropy file in order to prevent multiple concurrent
transactions.
Change the default entropy nonce length to be nonzero in some cases.
Specifically, the default nonce length is now set in such a way that
the entropy input during the initial seeding always contains enough
entropy to achieve the maximum possible security strength per
NIST SP 800-90A given the key size and entropy length.
If MBEDTLS_CTR_DRBG_ENTROPY_LEN is kept to its default value,
mbedtls_ctr_drbg_seed() now grabs extra entropy for a nonce if
MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is disabled and either
MBEDTLS_ENTROPY_FORCE_SHA256 is enabled or MBEDTLS_SHA512_C is
disabled. If MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled, or if
the entropy module uses SHA-512, then the default value of
MBEDTLS_CTR_DRBG_ENTROPY_LEN does not require a second call to the
entropy function to achieve the maximum security strength.
This choice of default nonce size guarantees NIST compliance with the
maximum security strength while keeping backward compatibility and
performance high: in configurations that do not require grabbing more
entropy, the code will not grab more entropy than before.
Add a new function mbedtls_ctr_drbg_set_nonce_len() which configures
the DRBG instance to call f_entropy a second time during the initial
seeding to grab a nonce.
The default nonce length is 0, so there is no behavior change unless
the user calls the new function.
Add a new function mbedtls_ctr_drbg_set_nonce_len() which configures
the DRBG instance to call f_entropy a second time during the initial
seeding to grab a nonce.
The default nonce length is 0, so there is no behavior change unless
the user calls the new function.
The blinding applied to the scalar before modular inversion is
inadequate. Bignum is not constant time/constant trace, side channel
attacks can retrieve the blinded value, factor it (it is smaller than
RSA keys and not guaranteed to have only large prime factors). Then the
key can be recovered by brute force.
Reducing the blinded value makes factoring useless because the adversary
can only recover pk*t+z*N instead of pk*t.
mbedtls_ctr_drbg_seed() always set the entropy length to the default,
so a call to mbedtls_ctr_drbg_set_entropy_len() before seed() had no
effect. Change this to the more intuitive behavior that
set_entropy_len() sets the entropy length and seed() respects that and
only uses the default entropy length if there was no call to
set_entropy_len().
This removes the need for the test-only function
mbedtls_ctr_drbg_seed_entropy_len(). Just call
mbedtls_ctr_drbg_set_entropy_len() followed by
mbedtls_ctr_drbg_seed(), it works now.
Move the definitions of mbedtls_ctr_drbg_seed_entropy_len() and
mbedtls_ctr_drbg_seed() to after they are used. This makes the code
easier to read and to maintain.
mbedtls_hmac_drbg_seed() always set the entropy length to the default,
so a call to mbedtls_hmac_drbg_set_entropy_len() before seed() had no
effect. Change this to the more intuitive behavior that
set_entropy_len() sets the entropy length and seed() respects that and
only uses the default entropy length if there was no call to
set_entropy_len().
Fix a signed int overflow in mbedtls_asn1_get_int() for numbers
between INT_MAX+1 and UINT_MAX (typically 0x80000000..0xffffffff).
This was undefined behavior which in practice would typically have
resulted in an incorrect value, but which may plausibly also have
caused the postcondition (*p == initial<*p> + len) to be violated.
Credit to OSS-Fuzz.
mbedtls_entropy_func returns up to MBEDTLS_ENTROPY_BLOCK_SIZE bytes.
This is the output of a hash function and does not indicate how many
bytes of entropy went into the hash computation.
Enforce that mbedtls_entropy_func gathers a total of
MBEDTLS_ENTROPY_BLOCK_SIZE bytes or more from strong sources. Weak
sources don't count for this calculation. This is complementary to the
per-source threshold mechanism.
In particular, we define system sources with a threshold of 32. But
when using SHA-512 for the entropy accumulator,
MBEDTLS_ENTROPY_BLOCK_SIZE = 64, so users can expect 64 bytes' worth
of entropy. Before, you only got 64 bytes of entropy if there were two
sources. Now you get 64 bytes of entropy even with a single source
with a threshold of 32.
This caused problems when running multiple jobs at once, since
there was no target matching libmbedcrypto.so with the path
prefix. It only worked if it was built first, since such file was found.
Additionally, building of libmbedcrypto.so now waits for the static .a version.
Previously, recipes for both libmbedcrypto.a and libmbedcrypto.so could run
independently when running parallel jobs, which resulted in the .o files
being built twice. It could sometimes be a problem, since linking would start
when building one of the object files was still in progress (the previous one
existed). This in turn resulted in reading (and trying to link) a malformed file.
The "|" character is followed by "order-only-prerequisites", and in this case,
makes linking of the shared version of the library wait for the .a file.
Since it's guaranteed to be always built in the "all" target, it's fine to do that.
All of the .o files are only built once thanks to this change.
Add a parameter to the p_validate_slot_number method to allow the
driver to modify the persistent data.
With the current structure of the core, the persistent data is already
updated. All it took was adding a way to modify it.
When registering a key in a secure element, go through the transaction
mechanism. This makes the code simpler, at the expense of a few extra
storage operations. Given that registering a key is typically very
rare over the lifetime of a device, this is an acceptable loss.
Drivers must now have a p_validate_slot_number method, otherwise
registering a key is not possible. This reduces the risk that due to a
mistake during the integration of a device, an application might claim
a slot in a way that is not supported by the driver.
In ssl_parse_hello_verify_request, we read 3 bytes (version and cookie
length) without checking that there are that many bytes left in
ssl->in_msg. This could potentially read from memory outside of the
ssl->receive buffer (which would be a remotely exploitable
crash).
In ssl_parse_hello_verify_request, we print cookie_len bytes without
checking that there are that many bytes left in ssl->in_msg. This
could potentially log data outside the received message (not a big
deal) and could potentially read from memory outside of the receive
buffer (which would be a remotely exploitable crash).
If none of the inputs to a key derivation is a
PSA_KEY_DERIVATION_INPUT_SECRET passed with
psa_key_derivation_input_key(), forbid
psa_key_derivation_output_key(). It usually doesn't make sense to
derive a key object if the secret isn't itself a proper key.
Allow a direct input as the SECRET input step in a key derivation, in
addition to allowing DERIVE keys. This makes it easier for
applications to run a key derivation where the "secret" input is
obtained from somewhere else. This makes it possible for the "secret"
input to be empty (keys cannot be empty), which some protocols do (for
example the IV derivation in EAP-TLS).
Conversely, allow a RAW_DATA key as the INFO/LABEL/SALT/SEED input to a key
derivation, in addition to allowing direct inputs. This doesn't
improve security, but removes a step when a personalization parameter
is stored in the key store, and allows this personalization parameter
to remain opaque.
Add test cases that explore step/key-type-and-keyhood combinations.
In TLS, the master secret is always a key. But EAP-TLS uses the TLS
PRF to derive an IV with an empty string for the "secret" input. The
code always stored the secret into a key slot before calling the TLS
PRF, but this doesn't work when the secret is empty, since PSA Crypto
no longer supports empty keys. Add a special case for an empty secret.
The signature must have exactly the same length as the key, it can't
be longer. Fix#258
If the signature doesn't have the correct size, that's an invalid
signature, not a problem with an output buffer size. Fix the error code.
Add test cases.
In psa_asymmetric_sign, immediately reject an empty signature buffer.
This can never be right.
Add test cases (one RSA and one ECDSA).
Change the SE HAL mock tests not to use an empty signature buffer.
Zero-length keys are rejected at creation time, so we don't need any
special handling internally.
When exporting a key, we do need to take care of the case where the
output buffer is empty, but this is easy: an empty output buffer is
never valid.
Document how mbedtls_asn1_store_named_data allocates val.p in the new
or modified entry.
Change the behavior to be more regular, always setting the new length
to val_len. This does not affect the previous documented behavior
since this aspect was not documented. This does not affect current
usage in Mbed TLS's X.509 module where calls with the same OID always
use the same size for the associated value.
At the end of `psa_hmac_setup_internal()`, the ipad is cleared.
However, the size that was given to clear was `key_len` which is larger
than the size of `ipad`.
* crypto/development: (77 commits)
all.sh: disable MEMORY_BUFFER_ALLOC in cmake asan build
Unify gcc and clang cmake flags to test with UBsan
Add an input check in psa_its_set
Remove storage errors from psa_generate_random
Update getting_started.md
Update based on Jaeden's comments.
Update getting_started.md
Fix return code warnings
Update getting_started.md
Fix warnings
Add PSA_ERROR_STORAGE_FAILURE to psa_cipher_generate_iv
Remove errorneous insert
Add STORAGE_FAILURE everywhere + add missing codes
Add storage failure to psa_mac_verify_finish
Add storage failure to psa_mac_sign_finish
Add PSA_ERROR_STORAGE_FAILURE to psa_aead_*_setup functions
Added PSA_ERROR_BAD_STATE to functions with operations
Added extra bad state case to psa_hash_setup
Add missing return codes to psa_generate_key
Add PSA_ERROR_BUFFER_TOO_SMALL to psa_mac_compute
...
We were still reusing the internal HMAC-DRBG of the deterministic ECDSA
for blinding. This meant that with cryptographically low likelyhood the
result was not the same signature as the one the deterministic ECDSA
algorithm has to produce (however it is still a valid ECDSA signature).
To correct this we seed a second HMAC-DRBG with the same seed to restore
correct behavior. We also apply a label to avoid reusing the bits of the
ephemeral key for a different purpose and reduce the chance that they
leak.
This workaround can't be implemented in the restartable case without
penalising the case where external RNG is available or completely
defeating the purpose of the restartable feature, therefore in this case
the small chance of incorrect behavior remains.
The current interface does not allow passing an RNG, which is needed for
blinding. Using the scheme's internal HMAC-DRBG results the same
blinding values for the same key and message, diminishing the
effectiveness of the countermeasure. A new function
`mbedtls_ecdsa_det_ext` is available to address this problem.
`mbedtls_ecdsa_sign_det` reuses the internal HMAC-DRBG instance to
implement blinding. The advantage of this is that the algorithm is
deterministic too, not just the resulting signature. The drawback is
that the blinding is always the same for the same key and message.
This diminishes the efficiency of blinding and leaks information about
the private key.
A function that takes external randomness fixes this weakness.
* crypto/development: (863 commits)
crypto_platform: Fix typo
des: Reduce number of self-test iterations
Fix -O0 build for Aarch64 bignum multiplication.
Make GNUC-compatible compilers use the right mbedtls_t_udbl again on Aarch64 builds.
Add optimized bignum multiplication for Aarch64.
Enable 64-bit limbs for all Aarch64 builds.
HMAC DRBG: Split entropy-gathering requests to reduce request sizes
psa: Use application key ID where necessary
psa: Adapt set_key_id() for when owner is included
psa: Add PSA_KEY_ID_INIT
psa: Don't duplicate policy initializer
crypto_extra: Use const seed for entropy injection
getting_started: Update for PSA Crypto API 1.0b3
Editorial fixes.
Cross reference 'key handles' from INVALID_HANDLE
Update documentation for psa_destroy_key
Update documentation for psa_close_key
Update psa_open_key documentation
Remove duplicated information in psa_open_key
Initialize key bits to max size + 1 in psa_import_key
...
* origin/development:
Fix uninitialized variable in x509_crt
Add a ChangeLog entry for mbedtls_net_close()
Added mbedtls_net_close and use it in ssl_fork_server to correctly disassociate the client socket from the parent process and the server socket from the child process.
Add ChangeLog entry
fix memory leak in mpi_miller_rabin()
* origin/pr/2803:
Add a ChangeLog entry for mbedtls_net_close()
Added mbedtls_net_close and use it in ssl_fork_server to correctly disassociate the client socket from the parent process and the server socket from the child process.
* origin/development: (42 commits)
Handle deleting non-existant files on Windows
Update submodule
Use 3rdparty headers from the submodule
Add Everest components to all.sh
3rdparty: Add config checks for Everest
Fix macros in benchmark.c
Update generated files
3rdparty: Fix inclusion order of CMakeLists.txt
Fix trailing whitespace
ECDH: Fix inclusion of platform.h for proper use of MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED
ECDH: Fix use of ECDH API in full handshake benchmark
ECDH: Removed unnecessary calls to mbedtls_ecp_group_load in ECDH benchmark
ECDH: Fix Everest x25519 make_public
Fix file permissions
3rdparty: Rename THIRDPARTY_OBJECTS
3rdparty: Update description of MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED
3rdparty: Fix Makefile coding conventions
ECDSA: Refactor return value checks for mbedtls_ecdsa_can_do
Add a changelog entry for Everest ECDH (X25519)
Document that curve lists can include partially-supported curves
...
Manually edit ChangeLog to ensure correct placement of ChangeLog notes.
* origin/pr/2799: (42 commits)
Handle deleting non-existant files on Windows
Update submodule
Use 3rdparty headers from the submodule
Add Everest components to all.sh
3rdparty: Add config checks for Everest
Fix macros in benchmark.c
Update generated files
3rdparty: Fix inclusion order of CMakeLists.txt
Fix trailing whitespace
ECDH: Fix inclusion of platform.h for proper use of MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED
ECDH: Fix use of ECDH API in full handshake benchmark
ECDH: Removed unnecessary calls to mbedtls_ecp_group_load in ECDH benchmark
ECDH: Fix Everest x25519 make_public
Fix file permissions
3rdparty: Rename THIRDPARTY_OBJECTS
3rdparty: Update description of MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED
3rdparty: Fix Makefile coding conventions
ECDSA: Refactor return value checks for mbedtls_ecdsa_can_do
Add a changelog entry for Everest ECDH (X25519)
Document that curve lists can include partially-supported curves
...
If we try to delete a non-existant file using del on Windows, as
can happen when running make clean, del will throw an error. Make
the Makefiles more robust by only deleting files if they exist.
This patch fixes an issue we encountered with more stringent compiler
warnings. The signature_is_good variable has a possibility of being
used uninitialized. This patch moves the use of the variable to a
place where it cannot be used while uninitialized.
Signed-off-by: Andy Gross <andy.gross@linaro.org>
The SSL context maintains a set of 'out pointers' indicating the
address at which to write the header fields of the next outgoing
record. Some of these addresses have a static offset from the
beginning of the record header, while other offsets can vary
depending on the active record encryption mechanism: For example,
if an explicit IV is in use, there's an offset between the end
of the record header and the beginning of the encrypted data to
allow the explicit IV to be placed in between; also, if the DTLS
Connection ID (CID) feature is in use, the CID is part of the
record header, shifting all subsequent information (length, IV, data)
to the back.
When setting up an SSL context, the out pointers are initialized
according to the identity transform + no CID, and it is important
to keep them up to date whenever the record encryption mechanism
changes, which is done by the helper function ssl_update_out_pointers().
During context deserialization, updating the out pointers according
to the deserialized record transform went missing, leaving the out
pointers the initial state. When attemping to encrypt a record in
this state, this lead to failure if either a CID or an explicit IV
was in use. This wasn't caught in the tests by the bad luck that
they didn't use CID, _and_ used the default ciphersuite based on
ChaChaPoly, which doesn't have an explicit IV. Changing either of
this would have made the existing tests fail.
This commit fixes the bug by adding a call to ssl_update_out_pointers()
to ssl_context_load() implementing context deserialization.
Extending test coverage is left for a separate commit.
According to SP800-90A, the DRBG seeding process should use a nonce
of length `security_strength / 2` bits as part of the DRBG seed. It
further notes that this nonce may be drawn from the same source of
entropy that is used for the first `security_strength` bits of the
DRBG seed. The present HMAC DRBG implementation does that, requesting
`security_strength * 3 / 2` bits of entropy from the configured entropy
source in total to form the initial part of the DRBG seed.
However, some entropy sources may have thresholds in terms of how much
entropy they can provide in a single call to their entropy gathering
function which may be exceeded by the present HMAC DRBG implementation
even if the threshold is not smaller than `security_strength` bits.
Specifically, this is the case for our own entropy module implementation
which only allows requesting at most 32 Bytes of entropy at a time
in configurations disabling SHA-512, and this leads to runtime failure
of HMAC DRBG when used with Mbed Crypto' own entropy callbacks in such
configurations.
This commit fixes this by splitting the seed entropy acquisition into
two calls, one requesting `security_strength` bits first, and another
one requesting `security_strength / 2` bits for the nonce.
Fixes#237.
* origin/development:
Update the crypto submodule
Use multipart PSA key derivation API
platform: Include stdarg.h where needed
Update Mbed Crypto to contain mbed-crypto#152
CMake: Add a subdirectory build regression test
README: Enable builds as a CMake subproject
ChangeLog: Enable builds as a CMake subproject
Remove use of CMAKE_SOURCE_DIR
Update library version to 2.18.0
* origin/pr/2807:
platform: Include stdarg.h where needed
Update Mbed Crypto to contain mbed-crypto#152
CMake: Add a subdirectory build regression test
README: Enable builds as a CMake subproject
ChangeLog: Enable builds as a CMake subproject
Remove use of CMAKE_SOURCE_DIR
Update library version to 2.18.0
Avoid compiler errors when MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER
is set by using the application ID type.
[Error] psa_crypto_slot_management.c@175,9: used type 'psa_key_id_t' (aka 'psa_key_file_id_t') where arithmetic or pointer type is required
Bring Mbed TLS 2.18.0 and 2.18.1 release changes back into the
development branch. We had branched to release 2.18.0 and 2.18.1 in
order to allow those releases to go out without having to block work on
the `development` branch.
Manually resolve conflicts in the Changelog by moving all freshly addded
changes to a new, unreleased version entry.
Reject changes to include/mbedtls/platform.h made in the mbedtls-2.18
branch, as that file is now sourced from Mbed Crypto.
* mbedtls-2.18:
platform: Include stdarg.h where needed
Update Mbed Crypto to contain mbed-crypto#152
CMake: Add a subdirectory build regression test
README: Enable builds as a CMake subproject
ChangeLog: Enable builds as a CMake subproject
Remove use of CMAKE_SOURCE_DIR
Update library version to 2.18.0
* origin/development: (114 commits)
Don't redefine calloc and free
Add changelog entry to record checking
Fix compiler warning
Add debug messages
Remove duplicate entries from ChangeLog
Fix parameter name in doxygen
Add missing guards for mac usage
Improve reability and debugability of large if
Fix a typo in a comment
Fix MSVC warning
Fix compile error in reduced configurations
Avoid duplication of session format header
Implement config-checking header to context s11n
Provide serialisation API only if it's enabled
Fix compiler warning: comparing signed to unsigned
Actually reset the context on save as advertised
Re-use buffer allocated by handshake_init()
Enable serialisation tests in ssl-opt.sh
Change requirements for setting timer callback
Add setting of forced fields when deserializing
...
Breaking into a series of statements makes things easier when stepping through
the code in a debugger.
Previous comments we stating the opposite or what the code tested for (what we
want vs what we're erroring out on) which was confusing.
Also expand a bit on the reasons for these restrictions.
Modelled after the config-checking header from session s11n.
The list of relevant config flags was established by manually checking the
fields serialized in the format, and which config.h flags they depend on.
This probably deserves double-checking by reviewers.
Since the type of cid_len is unsigned but shorter than int, it gets
"promoted" to int (which is also the type of the result), unless we make the
other operand an unsigned int which then forces the expression to unsigned int
as well.
The number of meaning of the flags will be determined later, when handling the
relevant struct members. For now three bytes are reserved as an example, but
this number may change later.
This mainly follows the design document (saving all fields marked "saved" in
the main structure and the transform sub-structure) with two exceptions:
- things related to renegotiation are excluded here (there weren't quite in
the design document as the possibility of allowing renegotiation was still
on the table, which is no longer is) - also, ssl.secure_renegotiation (which
is not guarded by MBEDTLS_SSL_RENEGOTIATION because it's used in initial
handshakes even with renegotiation disabled) is still excluded, as we don't
need it after the handshake.
- things related to Connection ID are added, as they weren't present at the
time the design document was written.
The exact format of the header (value of the bitflag indicating compile-time
options, whether and how to merge it with the serialized session header) will
be determined later.
Enforce restrictions indicated in the documentation.
This allows to make some simplifying assumptions (no need to worry about
saving IVs for CBC in TLS < 1.1, nor about saving handshake data) and
guarantees that all values marked as "forced" in the design document have the
intended values and can be skipped when serialising.
Some of the "forced" values are not checked because their value is a
consequence of other checks (for example, session_negotiated == NULL outside
handshakes). We do however check that session and transform are not NULL (even
if that's also a consequence of the initial handshake being over) as we're
going to dereference them and static analyzers may appreciate the info.
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.
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.
This bug was present since cert digest had been introduced, which highlights
the need for testing.
While at it, fix a bug in the comment explaining the format - this was
introduced by me copy-pasting to hastily from current baremetal, that has a
different format (see next PR in the series for the same in development).
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).
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.
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.
Configs with no DEBUG_C are used 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.
* origin/development: (51 commits)
Fix possibly-lossy conversion warning from MSVC
Reintroduce length 0 check for records
Don't use memcpy() for 2-byte copy operation
Remove integer parsing macro
Fix alignment in record header parsing routine
Don't disallow 'record from another epoch' log msg in proxy ref test
Make sure 'record from another epoch' is displayed for next epoch
Implement record checking API
Mark ssl_parse_record_header() as `const` in SSL context
Make mbedtls_ssl_in_hdr_len() CID-unaware
Remove duplicate setting of ssl->in_msgtype and ssl->in_msglen
Move update of in_xxx fields in ssl_get_next_record()
Move update of in_xxx fields outside of ssl_prepare_record_content()
Reduce dependency of ssl_prepare_record_content() on in_xxx fields
Move ssl_update_in_pointers() to after record hdr parsing
Mark DTLS replay check as `const` on the SSL context
Move updating the internal rec ptrs to outside of rec hdr parsing
Mark ssl_decrypt_buf() as `const in the input SSL context
Adapt ssl_prepare_record_content() to use SSL record structure
Use record length from record structure when fetching content in TLS
...
