Commit Graph

68 Commits

Author SHA1 Message Date
Paul Elliott
3891caf1ce Misc review requested fixes
Style fixes and cast certain defines to size_t

Signed-off-by: Paul Elliott <paul.elliott@arm.com>
2021-03-10 17:00:32 +00:00
Paul Elliott
9f35211774 Fixes for invalid printf format specifiers
Fixes for printf format specifiers, where they have been flagged as
invalid sizes by coverity, and new build flags to enable catching these
errors when building using CMake. Note that this patch uses %zu, which
requires C99 or later.

Signed-off-by: Paul Elliott <paul.elliott@arm.com>
2021-03-10 17:00:32 +00:00
Janos Follath
3aae5d4ed7
Merge pull request #781 from mpg/cipher-auth-crypt-restricted
Fix buffer overflow with NIST-KW in cipher layer
2020-12-07 12:58:36 +00:00
Manuel Pégourié-Gonnard
f5cf71e14a Stop using deprecated functions in the library
all.sh -k '*deprecated*' now passes again

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-12-03 12:25:10 +01:00
Christian von Arnim
883d304785
Fix: Add missing arguments to debug message.
Signed-off-by: Christian von Arnim <christian.von-arnim@isw.uni-stuttgart.de>
2020-12-02 10:13:02 +01:00
Hanno Becker
a817ea449a Check presence of DTLS timers only once
Mbed TLS requires users of DTLS to configure timer callbacks
needed to implement the wait-and-retransmit logic of DTLS.

Previously, the presence of these timer callbacks was checked
at every invocation of `mbedtls_ssl_fetch_input()`, so lowest
layer of the messaging stack interfacing with the underlying
transport.

This commit removes this recurring check and instead checks the
presence of timers once at the beginning of the handshake.

The main rationale for this change is that it is a step towards
separating the various layers of the messaging stack more cleanly:
datagram layer, record layer, message layer, retransmission layer.

Signed-off-by: Hanno Becker <hanno.becker@arm.com>
2020-10-20 15:29:14 +01:00
Manuel Pégourié-Gonnard
6d6f8a4b97 Clarify descriptions of constant-flow helpers
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-09-28 09:51:41 +02:00
Manuel Pégourié-Gonnard
822b3729e7 Remove last use of non-bit operations
According to https://www.bearssl.org/ctmul.html even single-precision
multiplication is not constant-time on some older platforms.

An added benefit of the new code is that it removes the somewhat mysterious
constant 0x1ff - which was selected because at that point the maximum value of
padlen was 256. The new code is perhaps a bit more readable for that reason.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-09-18 12:11:22 +02:00
Manuel Pégourié-Gonnard
2a59fb45b5 Add explicit cast when truncating values
MSVC complains about it otherwise.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-09-18 12:11:22 +02:00
Manuel Pégourié-Gonnard
6e2a9a7faa Factor repeated code in ssl_cf functions
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-09-18 12:11:21 +02:00
Manuel Pégourié-Gonnard
2ddec4306f Use bit operations for constant-flow padding check
The previous code used comparison operators >= and == that are quite likely to
be compiled to branches by some compilers on some architectures (with some
optimisation levels).

For example, take the following function:

void old_update( size_t data_len, size_t *padlen )
{
    *padlen  *= ( data_len >= *padlen + 1 );
}

With Clang 3.8, let's compile it for the Arm v6-M architecture:

% clang --target=arm-none-eabi -march=armv6-m -Os foo.c -S -o - |
    sed -n '/^old_update:$/,/\.size/p'

old_update:
        .fnstart
@ BB#0:
        .save   {r4, lr}
        push    {r4, lr}
        ldr     r2, [r1]
        adds    r4, r2, #1
        movs    r3, #0
        cmp     r4, r0
        bls     .LBB0_2
@ BB#1:
        mov     r2, r3
.LBB0_2:
        str     r2, [r1]
        pop     {r4, pc}
.Lfunc_end0:
        .size   old_update, .Lfunc_end0-old_update

We can see an unbalanced secret-dependant branch, resulting in a total
execution time depends on the value of the secret (here padlen) in a
straightforward way.

The new version, based on bit operations, doesn't have this issue:

new_update:
        .fnstart
@ BB#0:
        ldr     r2, [r1]
        subs    r0, r0, #1
        subs    r0, r0, r2
        asrs    r0, r0, #31
        bics    r2, r0
        str     r2, [r1]
        bx      lr
.Lfunc_end1:
        .size   new_update, .Lfunc_end1-new_update

(As a bonus, it's smaller and uses less stack.)

While there's no formal guarantee that the version based on bit operations in
C won't be translated using branches by the compiler, experiments tend to show
that's the case [1], and it is commonly accepted knowledge in the practical
crypto community that if we want to sick to C, bit operations are the safest
bet [2].

[1] https://github.com/mpg/ct/blob/master/results
[2] https://github.com/veorq/cryptocoding

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-09-18 12:10:33 +02:00
Janos Follath
d2ce916b58 Merge branch 'development-restricted' 2020-08-26 14:15:34 +01:00
Janos Follath
d4ac4e037b
Merge pull request #736 from mpg/cf-varpos-copy-dev-restricted
Constant-flow copy of HMAC from variable position
2020-08-25 14:35:55 +01:00
Gilles Peskine
ed19762a22
Merge pull request #3574 from makise-homura/e2k_support
Support building on e2k (Elbrus) architecture
2020-08-25 09:46:36 +02:00
makise-homura
af9513bb48 A different approach of signed-to-unsigned comparison
Suggsted by @hanno-arm

Signed-off-by: makise-homura <akemi_homura@kurisa.ch>
2020-08-24 23:42:49 +03:00
Manuel Pégourié-Gonnard
ba6fc9796a Fix a typo in a comment
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-08-24 12:59:55 +02:00
Dan Handley
abccfc1684 Merge development into development-restricted
* development:
  Update copyright notices to use Linux Foundation guidance
  Undef ASSERT before defining it to ensure that no previous definition has sneaked in through included files.
  Add ChangeLog entry for X.509 CN-type vulnerability
  Improve documentation of cn in x509_crt_verify()
  Fix comparison between different name types
  Add test: DNS names should not match IP addresses
  Remove obsolete buildbot reference in compat.sh
  Fix misuse of printf in shell script
  Fix added proxy command when IPv6 is used
  Simplify test syntax
  Fix logic error in setting client port
  ssl-opt.sh: include test name in log files
  ssl-opt.sh: remove old buildbot-specific condition
  ssl-opt.sh: add proxy to all DTLS tests

Signed-off-by: Dan Handley <dan.handley@arm.com>
2020-08-20 11:07:12 +01:00
Manuel Pégourié-Gonnard
de1cf2c5e1 Make mbedtls_ssl_cf_memcpy_offset() constant-flow
all.sh component test_valgrind_constant_flow is now passing.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-08-20 10:22:41 +02:00
Manuel Pégourié-Gonnard
7fe2c5f086 Add mbedtls_ssl_cf_memcpy_offset() with tests
The tests are supposed to be failing now (in all.sh component
test_memsan_constant_flow), but they don't as apparently MemSan doesn't
complain when the src argument of memcpy() is uninitialized, see
https://github.com/google/sanitizers/issues/1296

The next commit will add an option to test constant flow with valgrind, which
will hopefully correctly flag the current non-constant-flow implementation.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-08-19 11:56:02 +02:00
Manuel Pégourié-Gonnard
3c31afaca6 Use temporary buffer to hold the peer's HMAC
This paves the way for a constant-flow implementation of HMAC checking, by
making sure that the comparison happens at a constant address. The missing
step is obviously to copy the HMAC from the secret offset to this temporary
buffer with constant flow, which will be done in the next few commits.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-08-19 11:56:01 +02:00
Bence Szépkúti
1e14827beb Update copyright notices to use Linux Foundation guidance
As a result, the copyright of contributors other than Arm is now
acknowledged, and the years of publishing are no longer tracked in the
source files.

Also remove the now-redundant lines declaring that the files are part of
MbedTLS.

This commit was generated using the following script:

# ========================
#!/bin/sh

# Find files
find '(' -path './.git' -o -path './3rdparty' ')' -prune -o -type f -print | xargs sed -bi '

# Replace copyright attribution line
s/Copyright.*Arm.*/Copyright The Mbed TLS Contributors/I

# Remove redundant declaration and the preceding line
$!N
/This file is part of Mbed TLS/Id
P
D
'
# ========================

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
2020-08-19 10:35:41 +02:00
makise-homura
0be6aa9957 Get back -Wsign-compare and fix sources according to it
Signed-off-by: makise-homura <akemi_homura@kurisa.ch>
2020-08-18 23:52:53 +03:00
Gilles Peskine
e900b59703
Merge pull request #719 from gabor-mezei-arm/689_zeroising_of_plaintext_buffers
Zeroising of plaintext buffers in mbedtls_ssl_read()
2020-08-12 18:51:42 +02:00
Manuel Pégourié-Gonnard
f009542747 Add missing const for consistency
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-07-27 09:34:10 +02:00
Manuel Pégourié-Gonnard
e747843903 Fix a whitespace issue
Co-authored-by: Janos Follath <janos.follath@arm.com>
2020-07-27 09:34:10 +02:00
Manuel Pégourié-Gonnard
e0765f35d5 Use int ret = MBEDTLS_ERROR_CORRUPTION_DETECTED; idiom
Co-authored-by: Gilles Peskine <gilles.peskine@arm.com>
2020-07-27 09:34:10 +02:00
Manuel Pégourié-Gonnard
44c9fdde6e Check errors from the MD layer
Could be out-of-memory for some functions, accelerator issues for others.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-07-22 11:31:20 +02:00
Manuel Pégourié-Gonnard
9713e13e68 Remove unnecessary cast
This is C, not C++, casts between void * and other pointer types are free.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-07-22 11:31:19 +02:00
Manuel Pégourié-Gonnard
baccf803ad Improve some comments and internal documentation
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-07-22 11:31:19 +02:00
Manuel Pégourié-Gonnard
ed0e86428d Factor repeated condition to its own macro
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-07-22 11:31:19 +02:00
Manuel Pégourié-Gonnard
7a8b1e6b71 Implement cf_hmac() actually with constant flow
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-07-22 11:31:18 +02:00
gabor-mezei-arm
a321413807
Zeroising of plaintext buffers to erase unused application data from memory
Signed-off-by: gabor-mezei-arm <gabor.mezei@arm.com>
2020-07-15 14:14:41 +02:00
Manuel Pégourié-Gonnard
65a6fa3e26 Make cf_hmac() STATIC_TESTABLE
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>
2020-07-15 12:26:21 +02:00
Manuel Pégourié-Gonnard
8aa29e382f Use existing implementation of cf_hmac()
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>
2020-07-15 12:26:21 +02:00
Manuel Pégourié-Gonnard
045f094c81 Add dummy constant-flow HMAC function with tests
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>
2020-07-15 12:25:52 +02:00
Manuel Pégourié-Gonnard
2df1f1f16f Factor repeated preprocessor condition to a macro
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>
2020-07-09 12:13:31 +02:00
Manuel Pégourié-Gonnard
527b87890d
Merge pull request #3454 from gilles-peskine-arm/include-common-h-development
Include common.h from all library source files
2020-07-03 09:44:18 +02:00
Gilles Peskine
db09ef6d22 Include common.h instead of config.h in library source files
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>
2020-07-02 11:26:57 +02:00
Manuel Pégourié-Gonnard
f4e3fc9133 Use starts/finish around Lucky 13 dummy compressions
Fixes #3246

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
2020-06-15 11:55:53 +02:00
Hanno Becker
f486e28694 Document precondition of nonce-generating function in ssl_msg.c
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
2020-06-04 13:33:08 +01:00
Hanno Becker
15952814d8 Improve documentation of nonce-generating function in ssl_msg.c
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
2020-06-04 13:31:46 +01:00
Hanno Becker
1cda2667af Spell out check for non-zero'ness
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
2020-06-04 13:28:44 +01:00
Hanno Becker
16bf0e2346 Fix debug print of explicit IV
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>
2020-06-04 13:27:34 +01:00
Hanno Becker
7cca3589cb Fix indentation in debug statement in ssl_msg.c
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
2020-06-04 13:27:22 +01:00
Hanno Becker
ceef848eb6 Rename TLS 1.3 padding granularity macro
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>
2020-06-02 06:16:00 +01:00
Hanno Becker
c3f7b0b16b Fix #endif indicator comment
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
2020-05-31 08:51:29 +01:00
Hanno Becker
67a37db2d2 Add missing configuration guards to SSL record protection helpers
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
2020-05-31 08:51:29 +01:00
Hanno Becker
13996927cb Introduce configuration option for TLS 1.3 padding granularity
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>
2020-05-31 08:51:25 +01:00
Hanno Becker
c0eefa8b92 Introduce helper function to retrieve explicit IV len for transform
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>
2020-05-28 10:32:23 +01:00
Hanno Becker
17263803aa Simplify AEAD nonce derivation
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>
2020-05-28 10:32:23 +01:00