An earlier botched backport (d56ca658ab)
had the wrong name for a variable and a missing header inclusion.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The intended logic around MBEDTLS_xxx_ALT is to exclude them from full
because they require the alternative implementation of one or more
library functions, except that MBEDTLS_PLATFORM_xxx_ALT are different:
they're alternative implementations of a platform function and they
have a built-in default, so they should be included in full. Document
this.
Fix a bug whereby MBEDTLS_PLATFORM_xxx_ALT didn't catch symbols where
xxx contains an underscore. As a consequence, MBEDTLS_PLATFORM_NV_SEED_ALT
is now enabled in the full config. Explicitly exclude
MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT because it behaves like the
non-platform ones, requiring an extra build-time dependency.
Explicitly exclude MBEDTLS_PLATFORM_NV_SEED_ALT from baremetal
because it requires MBEDTLS_ENTROPY_NV_SEED, and likewise explicitly
unset it from builds that unset MBEDTLS_ENTROPY_NV_SEED.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Remove the duplicated, and often out-of-date, list in the comments.
Instead explain in a comment, and have a single copy of the list which
is in the code.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Enable MBEDTLS_X509_ALLOW_EXTENSIONS_NON_V3 in the full config. There's
no reason to keep it out. We weren't testing it at all on the CI.
Signed-off-by: Gilles Peskine <Gilles.Peskine@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>
Signed-off-by: Piotr Nowicki <piotr.nowicki@arm.com>
When building with MBEDTLS_MEMORY_DEBUG enabled, and running the ecdh part,
the benchmark program would start writing a very large number of space
characters on stdout, and would have to be killed because it never seemed to
terminate.
This was due to an integer overflow in computing how many space to leave after
the title in order to get memory measurements aligned, which resulted in up
to SIZE_MAX spaces being printed.
This commit just fixes the overflow, the next commit is going to fix the magic
number (12).
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Don't mix Windows and Unix line endings, it's the worst of both worlds.
Update the Visual Studio templates and regenerate the generated files.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Check Windows files for some issues, including permissions. Omit the
checks related to special characters (whitespace, line endings,
encoding) as appropriate.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
To test a file name exactly, prepend a / to the base name.
files_to_check actually checks suffixes, not file names, so rename it
to extensions_to_check.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The identifiers of the unmet dependencies of a test case are
stored in a buffer of fixed size that can be potentially too
small to store all the unmet dependencies. Indicate in test
report if some unmet dependencies are missing.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Fix potential buffer overflow when tracking the unmet dependencies
of a test case. The identifiers of unmet dependencies are stored
in an array of fixed size. Ensure that we don't overrun the array.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Use size_t for some variables that are array indices.
Use unsigned for some variables that are counts of "small" things.
This is a backport of commit 3c1c8ea3e7.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Adding .function was necessary, as otherwise ctags would have no idea what to
do with those files.
Adding .h may not be necessary, as by default ctags considers them C++ which
is probably good enough, but since we're tuning the mapping anyway...
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>
Warn about a gotcha that caused a bug in development.
Ensure that it's at least zeroed out, rather than uninitialized, in
non-verbose mode.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
We keep track of the current epoch and record sequence number in out_ctr,
which was overwritten when writing the record containing the
HelloVerifyRequest starting from out_buf. We can avoid that by only using the
rest of the buffer.
Using MBEDTLS_SSL_MAX_CONTENT_LEN as the buffer size is still correct, as it
was a pretty conservative value when starting from out_buf.
Note: this bug was also fixed unknowingly in 2.13 by introducing a new buffer
that holds the current value of the sequence number (including epoch), while
working on datagram packing: 198594709b
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@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>
There are currently 4 tests in ssl-opt.sh with either -C "resend" or -S
"resend", that is, asserting that no retransmission will occur. They sometimes
fail on loaded CI machines as one side doesn't send a message fast enough,
causing the other side to retransmit, causing the test to fail.
(For the "reconnect" test there was an other issue causing random failures,
fixed in a previous commit, but even after that fix the test would still
sometimes randomly fail, even if much more rarely.)
While it's a hard problem to fix in a general and perfect way, in practice the
probability of failures can be drastically reduced by making the timeout
values much larger.
For some tests, where retransmissions are actually expected, this would have
the negative effect of increasing the average running time of the test, as
each side would wait for longer before it starts retransmission, so we have a
trade-off between average running time and probability of spurious failures.
But for tests where retransmission is not expected, there is no such trade-off
as the expected running time of the test (assuming the code is correct most of
the time) is not impacted by the timeout value. So the only negative effect of
increasing the timeout value is on the worst-case running time on the test,
which is much less important, as test should only fail quite rarely.
This commit addresses the easy case of tests that don't expect retransmission
by increasing the value of their timeout range to 10s-20s. This value
corresponds to the value used for tests that assert `-S "autoreduction"` which
are in the same case and where the current value seems acceptable so far.
It also represents an increase, compared to the values before this commit, of
a factor 20 for the "reconnect" tests which were frequently observed to fail
in the CI, and of a factor 10 for the first two "DTLS proxy" tests, which were
observed to fail much less frequently, so hopefully the new values are enough
to reduce the probability of spurious failures to an acceptable level.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The server must check client reachability (we chose to do that by checking a
cookie) before destroying the existing association (RFC 6347 section 4.2.8).
Let's make sure we do, by having a proxy-in-the-middle inject a ClientHello -
the server should notice, but not destroy the connection.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
