In the entries (mbedtls_x509_crl_entry values) on the list constructed
by mbedtls_x509_crl_parse_der(), set entry->raw.tag to
(SEQUENCE | CONSTRUCTED) rather than to the tag of the first ASN.1
element of the entry (which happens to be the tag of the serial
number, so INTEGER or INTEGER | CONTEXT_SPECIFIC). This is doesn't
really matter in practice (and in particular the value is never used
in Mbed TLS itself), and isn't documented, but at least it's
consistent with how mbedtls_x509_buf is normally used.
The primary importance of this change is that the old code tried to
access the tag of the first element of the entry even when the entry
happened to be empty. If the entry was empty and not followed by
anything else in the CRL, this could cause a read 1 byte after the end
of the buffer containing the CRL.
The test case "X509 CRL ASN1 (TBSCertList, single empty entry at end)"
hit the problematic buffer overflow, which is detected with ASan.
Credit to OSS-Fuzz for detecting the problem.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This is an LTS branch, C99 isn't allowed yet, it breaks versions of MSVC that
we still support for this branch.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
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>
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 and 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>
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>
- it's 2020, there shouldn't be too many systems out there where SHA-1 is the
only available hash option, so its usefulness is limited
- OTOH testing configurations without SHA-2 reveal bugs that are not easy to
fix in a fully compatible way
So overall, the benefit/cost ratio is not good enough to justify keeping SHA-1
as a fallback option here.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The previous commit introduced a potential memory overread by reading
secret_len bytes from secret->p, while the is no guarantee that secret has
enough limbs for that.
Fix that by using an intermediate buffer and mpi_write_binary().
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The dependency on a DRBG module was perhaps a bit strict for LTS branches, so
let's have an option that works with no DRBG when at least one SHA module is
present.
This changes the internal API of ecp_drbg_seed() by adding the size of the
MPI as a parameter. Re-computing the size from the number of limbs doesn't
work too well here as we're writing out to a fixed-size buffer and for some
curves (P-521) that would round up too much. Using mbedtls_mpi_get_len() is
not entirely satisfactory either as it would mean using a variable-length
encoding, with could open side channels.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@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>
This will allow us to ship the LTS branches in a single archive
This commit was generated using the following script:
# ========================
#!/bin/sh
header1='\ * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later\
*\
* This file is provided under the Apache License 2.0, or the\
* GNU General Public License v2.0 or later.\
*\
* **********\
* Apache License 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.\
*\
* **********\
*\
* **********\
* GNU General Public License v2.0 or later:\
*\
* This program is free software; you can redistribute it and/or modify\
* it under the terms of the GNU General Public License as published by\
* the Free Software Foundation; either version 2 of the License, or\
* (at your option) any later version.\
*\
* This program is distributed in the hope that it will be useful,\
* but WITHOUT ANY WARRANTY; without even the implied warranty of\
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\
* GNU General Public License for more details.\
*\
* You should have received a copy of the GNU General Public License along\
* with this program; if not, write to the Free Software Foundation, Inc.,\
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.\
*\
* **********'
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 s/\/\*/&\n */
# Insert new copyright header
/SPDX-License-Identifier/ i\
$header1
# Delete old copyright header
/SPDX-License-Identifier/,$ {
# Delete lines until the one preceding the mbedtls declaration
N
1,/This file is part of/ {
/This file is part of/! D
}
}
"
# Format copyright header for inclusion into scripts
header2=$(echo "$header1" | sed 's/^\\\? \* \?/#/')
find -path './.git' -prune -o '(' -name '*.gdb' -o -name '*.pl' -o -name '*.py' -o -name '*.sh' ')' -print | xargs sed -i "
# Insert new copyright header
/SPDX-License-Identifier/ i\
$header2
# Delete old copyright header
/SPDX-License-Identifier/,$ {
# Delete lines until the one preceding the mbedtls declaration
N
1,/This file is part of/ {
/This file is part of/! D
}
}
"
# ========================
Signed-off-by: Bence Szépkúti <bence.szepkuti@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>
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>
