Commit Graph

13480 Commits

Author SHA1 Message Date
Gilles Peskine
ac0f0860f1 Reformatting to pacify pylint
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-27 10:56:45 +01:00
Manuel Pégourié-Gonnard
b6929891d6 Adjust timeout of tests with "no resend" assertions
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>
2020-03-27 10:50:05 +01:00
Manuel Pégourié-Gonnard
baad2de6d8 Add negative test for hard reconnect cookie check
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>
2020-03-27 10:50:05 +01:00
Manuel Pégourié-Gonnard
824655c837 Fix lack of cookie check on hard reconnect
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>
2020-03-27 10:50:05 +01:00
Gilles Peskine
4457a0a489
Merge pull request #3128 from gilles-peskine-arm/changelog-date
Sort changelog entries by their merge date
2020-03-27 10:42:43 +01:00
Manuel Pégourié-Gonnard
a4aa89b16e Fix leakage of projective coordinates in ECC
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>
2020-03-27 09:43:34 +01:00
Gilles Peskine
42f384c186 Mbed Crypto isn't a separate project anymore
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-27 09:23:38 +01:00
Gilles Peskine
98a53aa399 Follow renames when looking for the creation of a file
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-26 22:50:11 +01:00
Gilles Peskine
13dc634282 Simplify the matching of the last line
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-26 22:49:44 +01:00
Gilles Peskine
7fa3eb7d44 Simplify sorting
Calling sort_key explicitly is marginally faster, but less readable.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-26 22:41:32 +01:00
Gilles Peskine
28af958ea4 Documentation improvements
Document that git is needed.

Be clearer about the entry sort key being an entry sort key, not just
a merge order. Be clearer about what "merge order" means.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-26 22:39:18 +01:00
Guilhem Bryant
82194c888d Fix bracketing style in ssl_tls.c
Signed-off-by: Guilhem Bryant <Guilhem.Bryant@arm.com>
2020-03-26 17:05:59 +00:00
Andrzej Kurek
98911cdd3a
makefile: remove unneeded priority dependency from library makefile
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>
2020-03-26 07:18:42 -04:00
Gilles Peskine
8f46bbf46f Sort changelog entries by their merge date
Changelog entry files were listed in reverse alphabetical order of the
file name, by happenstance. Now, changelog entry files are listed in
the order in which the changes were merged. More precisely: look for
the git commit where the entry file was created, and look where this
commit was merged into the current branch. List older merges first.
List never-merged commits in date order after all the merged ones.
List never-committed files in file timestamp order after all the
committed ones.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-26 11:58:00 +01:00
Gilles Peskine
27a1facd48 Factor out list_files_to_merge
No behavior changes.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-26 11:57:54 +01:00
Manuel Pégourié-Gonnard
2451ac698d
Merge pull request #3126 from gilles-peskine-arm/merge-crypto-stragglers-20200325
Merge mbed-crypto stragglers 2020-03-25
2020-03-25 22:12:30 +01:00
Guilhem Bryant
c5285d8c40 Use internal PSK getter
Signed-off-by: Guilhem Bryant <Guilhem.Bryant@arm.com>
2020-03-25 17:47:25 +00:00
Guilhem Bryant
d511ac3419 Define internal PSK getter
Signed-off-by: Guilhem Bryant <Guilhem.Bryant@arm.com>
2020-03-25 17:47:25 +00:00
Guilhem Bryant
e40a2f7a99 Improve documentation about PSK configuration
Signed-off-by: Guilhem Bryant <Guilhem.Bryant@arm.com>
2020-03-25 17:47:25 +00:00
Gilles Peskine
854770b84e Temporarily remove ChangeLog.md
All the content that is currently in ChangeLog.md is the result of
merging Mbed Crypto, and that content is already present in
ChangeLog. Since we aren't using ChangeLog.md yet, remove it for now.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-25 17:13:19 +01:00
Gilles Peskine
5d324b203a Merge remote-tracking branch 'upstream-crypto/development' into HEAD
Conflicts:
* .github/issue_template.md, .github/pull_request_template.md:
  The changes in Mbed Crypto are not relevant to Mbed TLS. Keep the
  mbedtls versions.
2020-03-25 15:47:43 +01:00
Gilles Peskine
8bad3ab45f
Merge pull request #300 from gilles-peskine-arm/assemble-changelog
Changelog assembly script
2020-03-25 15:17:04 +01:00
Jaeden Amero
dfd5ea239d
Merge pull request #3117 from AndrzejKurek/null_buffer_tests
Increase test coverage by adding AES and CAMELLIA empty buffer tests
2020-03-25 18:04:48 +04:00
Manuel Pégourié-Gonnard
bca8aa07b0 Update references from drafts to RFCs in config.h
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-03-25 10:34:39 +01:00
Manuel Pégourié-Gonnard
8b0187e6f4
Merge pull request #392 from mpg/no-new-issue-or-pr
Have issue and PR templates redirect to Mbed TLS
2020-03-25 09:55:33 +01:00
Manuel Pégourié-Gonnard
cdcb187eb2 Have issue and PR templates redirect to Mbed TLS
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-03-25 09:31:23 +01:00
Gilles Peskine
c4672fdb81 New header common.h; define MBEDTLS_STATIC_TESTABLE
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>
2020-03-24 22:57:11 +01:00
Gilles Peskine
fea6eaf5e3 Declare MBEDTLS_TEST_HOOKS in config.h
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>
2020-03-24 22:56:45 +01:00
Gilles Peskine
fa51820e39 Expand the rule for internal functions exposed for tests only
Clarify that using a header in library/ rather than include/ for
internal functions is a rule, not just a possibility.

As suggested by Manuel, state a rule for functions that need to be
static for best optimization but that we want to unit-test.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-24 22:56:38 +01:00
Gilles Peskine
4b7279e5d9 Write up some solutions
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-24 22:56:31 +01:00
Gilles Peskine
dff10c773b Add a note that TLS requires further consideration
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-24 22:56:22 +01:00
Gilles Peskine
ab4b9b4165 New test strategy document: invasive testing
Evaluate possible approaches for invasive testing.

State some rules.

This commit was originally written for Mbed Crypto only.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-24 22:56:15 +01:00
Gilles Peskine
3b310c69a5 Add a note to some invasive tests
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-24 22:55:48 +01:00
Gilles Peskine
d414c32a16
Merge pull request #3113 from mpg/ssl-opt-names
Improve some test names in ssl-opt.sh
2020-03-24 19:48:09 +01:00
Gilles Peskine
46c54c0a52 Pylint: disable logging-format-interpolation warning
Pylint warns about things like ``log.info('...'.format(...))``.
It insists on ``log.info('...', ...)``.
This is of minor utility (mainly a performance gain when there are
many messages that use formatting and are below the log level).
Some versions of Pylint (including 1.8, which is the version on
Ubuntu 18.04) only recognize old-style format strings using '%',
and complain about something like ``log.info('{}', foo)`` with
logging-too-many-args (Pylint supports new-style formatting if
declared globally with logging_format_style under [LOGGING] but
this requires Pylint >=2.2).

Disable this warning to remain compatible with Pylint 1.8 and not have
to change abi_check.py to use %-formats instead of {}-formats when
logging.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-24 19:37:01 +01:00
Gilles Peskine
49f467903f Pylint: allow if-return-else-return
Allow the perfectly reasonable idiom
    if condition1:
        return value1
    else:
        return value2

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-24 19:37:01 +01:00
Gilles Peskine
e22a4dacf7 Explicit return value from main
Rather than sometimes returning an integer, sometimes a boolean and
sometimes implicitly returning None, always return 0 for success and 1
for failure.

No behavior change for the program as a whole, since the None/True/False
values were implicitly converted to the desired numerical value.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-24 19:36:17 +01:00
Gilles Peskine
ce674a90c5 Clearer code to search for config.h
Don't use a function argument as a for loop variable. It worked (mostly) but
Pylint frowns on it (redefined-argument-from-local) and I think Pylint has a
point.

If the configuration file is not found, raise an exception mentioning the
search path rather than just its last element.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-24 19:25:34 +01:00
Gilles Peskine
13c95c4d74 Make check_python_files non-optional in all.sh
check_python_files was optional in all.sh because we used to have CI
machines where pylint wasn't available. But this had the downside that
check_python_files kept breaking because it wasn't checked in the CI.
Now our CI has pylint and check_python_files should not be optional.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-24 19:25:34 +01:00
Gilles Peskine
56e99d623d Make sure to use a Python 3 pylint
On some systems, such as Ubuntu up to 19.04, `pylint` is for Python 2
and `pylint3` is for Python 3, so we should not use `pylint` even if
it's available.

Use the Python module instead of the trivial shell wrapper. This way
we can make sure to use the correct Python version.

Fix #3111

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
2020-03-24 19:23:41 +01:00
Andrzej Kurek
1b7a780e65
Increase test coverage by adding AES and CAMELLIA empty buffer tests
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
2020-03-24 13:18:58 -04:00
Gilles Peskine
7a054d1def
Merge pull request #3094 from gilles-peskine-arm/merge-crypto-docs
Merge documentation from Mbed Crypto
2020-03-24 14:19:34 +01:00
Gilles Peskine
37b61814cb
Merge pull request #3085 from gilles-peskine-arm/merge-crypto-unremoved-20200304
Merge of Mbed Crypto

Merge Mbed Crypto back into Mbed TLS. See https://github.com/ARMmbed/mbed-crypto/issues/374 for more details.
2020-03-24 13:56:15 +01:00
Manuel Pégourié-Gonnard
9e2c80fa24 Improve some test names in ssl-opt.sh
- "Default" should only be used for tests that actually use the defaults (ie,
  not passing options on the command line, except maybe debug/dtls)
- All tests in the "Encrypt then MAC" group should start with that string as a
  common prefix

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-03-24 10:53:39 +01:00
Gilles Peskine
3ed2de9f54 Refresh generated files 2020-03-23 21:54:07 +01:00
Gilles Peskine
f03bd81be0 Place MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH in alphabetical order
The automatic merge of include/mbedtls/config.h had placed
MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH before
MBEDTLS_SSL_TRUNCATED_HMAC_COMPAT.
2020-03-23 18:13:58 +01:00
Gilles Peskine
5e7d6fd240 Merge 'mbedtls/development' into merge-crypto-unremoved-20200304
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.
2020-03-23 18:02:07 +01:00
Gilles Peskine
5ec51f923a Future-proof make clean under Windows
In Windows cmd, `del foo` succeeds if `foo` doesn't exist, but
`del *.ext` fails if `*.ext` doesn't match any files. Therefore we use
the idiom `if exist *.ext del *.ext` to delete files matching
wildcards.

We've accidentally used `if exist $(some_list) del $(some_list)`, and
that's wrong, because it's only syntactically correct if
`$(some_list)` contains exactly one element. As long as `$(some_list)`
contains actual file names and not wildcards, just use `del $(some_list)`.
2020-03-23 17:55:32 +01:00
Gilles Peskine
a647f1296e Delete third-party objects in cleanup 2020-03-23 17:55:32 +01:00
Gilles Peskine
049fefff90 Add dependencies on !MBEDTLS_SHA512_NO_SHA384 to x509/ssl unit tests
Add dependencies on !MBEDTLS_SHA512_NO_SHA384 to X.509 and SSL unit
tests that use SHA-384 (identified based on having a description that
contains "SHA384" or "SHA-384").
2020-03-23 17:55:32 +01:00