This option allows to test the constant-flow nature of selected code, using
MemSan and the fundamental observation behind ctgrind that the set of
operations allowed on undefined memory by dynamic analysers is the same as the
set of operations allowed on secret data to avoid leaking it to a local
attacker via side channels, namely, any operation except branching and
dereferencing.
(This isn't the full story, as on some CPUs some instructions have variable
execution depending on the inputs, most notably division and on some cores
multiplication. However, testing that no branch or memory access depends on
secret data is already a good start.)
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The test function now depends on MBEDTLS_TEST_HOOKS, which is enabled by
config.py full, and since there are already components in all.sh exercising
the full config, this test function is sill exercised even with this new
dependency.
Since this is the first time a test function depends on MBEDTLS_TEST_HOOKS,
fix a bug in check-names.sh that wasn't apparent so far: headers from
library/*.h were not considered when looking for macro definitions. This
became apparent because MBEDTLS_STATIC_TESTABLE is defined in library/common.h
and started being used in library/ssl_msg.c, so was flagged as a likely typo.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Just move code from ssl_decrypt_buf() to the new cf_hmac() function and then
call cf_hmac() from there.
This makes the new cf_hmac() function used, opening the door for making it
static in the next commit. It also validates that its interface works for
using it in ssl_decrypt_buf().
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The dummy implementation is not constant-flow at all for now, it's just
here as a starting point and a support for developing the tests and putting
the infrastructure in place.
Depending on the implementation strategy, there might be various corner cases
depending on where the lengths fall relative to block boundaries. So it seems
safer to just test all possible lengths in a given range than to use only a
few randomly-chosen values.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The condition is a complex and repeated a few times. There were already some
inconsistencies in the repetitions as some of them forgot about DES.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Address remaining PR comments for #2118
- Add ChangeLog.d/x509write_csr_heap_alloc.txt.
- Fix parameter alignment per Gille's recommendation.
- Update comments to more explicitly describe the manipulation of buf.
- Replace use of `MBEDTLS_MPI_MAX_SIZE` as `sig` buffer size for
call to `x509write_csr_der_internal()` with more intuitive
`MBEDTLS_PK_SIGNATURE_MAX_SIZE`.
- Update `mbedtls_x509write_csr_der()` to return
`MBEDTLS_ERR_X509_ALLOC_FAILED` on mbedtls_calloc error.
Signed-off-by: Simon Leet <simon.leet@microsoft.com>
Using a stack-buffer with a size > 2K could easily produce a stack
overflow for an embedded device which has a limited stack size.
This commit dynamically allocates the large CSR buffer.
This commit avoids using a temporary buffer for storing the OIDs.
A single buffer is used:
a) OIDs are written backwards starting with the end of the buffer;
b) OIDs are memmove'd to the beginning of the buffer;
c) signature over this OIDs is computed and written backwards from the
end of the buffer;
d) the two memory regions are compacted.
Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com>
Changed PSA core (and PKWrite) from reaching into MPI to using the proper
ecp function to fetch a private key.
Added changelog.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
mbedtls_ecp_write_key is a mirror function to mbedtls_ecp_read_key, which
writes a private key back into a byte buffer in the correct format.
This is a helpful convenience function, since the byte order is defined
differently between Montgomery and Weierstrass curves. Since this difference
is accounted for in mbedtls_ecp_read_key, it made sense to add
mbedtls_ecp_write_key for the purpose of abstracting this away such that
psa_export_key doesn't need to take byte order into account.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Rename PSA_ECC_CURVE_xxx to PSA_ECC_FAMILY_xxx, also rename
PSA_KEY_TYPE_GET_CURVE to PSA_KEY_TYPE_ECC_GET_FAMILY and rename
psa_ecc_curve_t to psa_ecc_family_t. Old defines are provided in
include/crypto_compat.h for backward compatibility.
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
"Include the library directory for the sake of 3rdparty" did the job
for Make and Visual Studio. This commit does the job for CMake.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
All libraries (should) rely on the same directory structure. Instead of
repeating the same clauses 6 times (3 libraries times 2 build modes), set
the include paths, compile definitions and install instructions with a
single piece of code.
Include the 3rdparty directory for all libraries, not just crypto. It's
currently only needed for crypto, but that's just happenstance.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In library source files, include "common.h", which takes care of
including "mbedtls/config.h" (or the alternative MBEDTLS_CONFIG_FILE)
and other things that are used throughout the library.
FROM=$'#if !defined(MBEDTLS_CONFIG_FILE)\n#include "mbedtls/config.h"\n#else\n#include MBEDTLS_CONFIG_FILE\n#endif' perl -i -0777 -pe 's~\Q$ENV{FROM}~#include "common.h"~' library/*.c 3rdparty/*/library/*.c scripts/data_files/error.fmt scripts/data_files/version_features.fmt
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
When compiling library files under `3rdparty/`, the directory containing
the `.c` file that is being compiled is not the current directory, so
headers from the `library/` directory are not found. Fix this by
adding `.` to the include path.
This was not detected until now because as of this commit, no 3rdparty
source file requires a header under `library/`.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Executed "./scripts/bump_version.sh --version 2.23.0 --so-crypto 5"
A symbol has been removed from the mbedcrypto library since the last
release:
mbedtls_ecc_group_to_psa ( enum mbedtls_ecp_group_id grpid,
size_t* bits )
This is an ABI break and we need to increase the SO version.
Signed-off-by: Janos Follath <janos.follath@arm.com>
inv_mod() already returns a specific error code if the value is not
invertible, so no need to check in advance that it is. Also, this is a
preparation for blinding the call to inv_mod(), which is made easier by
avoiding the redundancy (otherwise the call to gcd() would need to be blinded
too).
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
In the next commit, we'll need to draw a second random value, in order to
blind modular inversion. Having a function for that will avoid repetition.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
* development: (87 commits)
entropy: Adjust parameter type of internal function to avoid a cast
entropy: Avoid arithmetic on void pointer
add comment about potential future extension
Adjust comments about SEED synchronisation
entropy: Rename sysctl_wrapper to sysctl_arnd_wrapper
test_suite_x509parse.function improvement
Pass "certificate policies" extension to callback
Update iv and len context pointers manually when reallocating buffers
Add Apache-2.0 headers to all source files
Remove Dangerous Parameter Passing
Add Apache-2.0 headers to all scripts
Add missing copyright dates to scripts and sources
Show failure in ssl-opts.sh when key export fails
Add changelog entry
tests: Reformating due to rnd_* renaming
tests: Add mbedtls_test_ prefix to rnd_* symbols
tests: Reformating due to hexcmp() renaming
tests: Add mbedtls_test_ prefix to hexcmp()
tests: Reformating due to unhexify_alloc() renaming
tests: Add mbedtls_test_ prefix to unhexify_alloc()
...
Don't define anymore globally third party include
directories and compile definitions. Declare them within the
scope of the crypto library target as per the third party
source files.
Note that targets linking to the crypto library inherit from
the third party public include directories.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
While this is a static function, so right now we know we don't need the check,
things may change in the future, so better be on the safe side.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
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>
