Use `( x >> y ) & z` instead of `x >> y & z`. Both are equivalent
by operator precedence, but the former is more readable and the
commonly used idiom in the library.
stdio.h was being included both conditionally if MBEDTLS_FS_IO was
defined, and also unconditionally, which made at least one of them
redundant.
This change removes the unconditional inclusion of stdio.h and makes it
conditional on MBEDTLS_PLATFORM_C.
Context: This commit makes a change to mbedtls_pk_parse_key() which
is responsible for parsing of private keys. The function doesn't know
the key format in advance (PEM vs. DER, encrypted vs. unencrypted) and
tries them one by one, resetting the PK context in between.
Issue: The previous code resets the PK context through a call to
mbedtls_pk_free() along, lacking the accompanying mbedtls_pk_init()
call. Practically, this is not an issue because functionally
mbedtls_pk_free() + mbedtls_pk_init() is equivalent to mbedtls_pk_free()
with the current implementation of these functions, but strictly
speaking it's nonetheless a violation of the API semantics according
to which xxx_free() functions leave a context in uninitialized state.
(yet not entirely random, because xxx_free() functions must be idempotent,
so they cannot just fill the context they operate on with garbage).
Change: The commit adds calls to mbedtls_pk_init() after those calls
to mbedtls_pk_free() within mbedtls_pk_parse_key() after which the
PK context might still be used.
When a random number is generated for the Miller-Rabin primality test,
if the bit length of the random number is larger than the number being
tested, the random number is shifted right to have the same bit length.
This introduces bias, as the random number is now guaranteed to be
larger than 2^(bit length-1).
Changing this to instead zero all bits higher than the tested numbers
bit length will remove this bias and keep the random number being
uniformly generated.
This commit changes the behavior of the record decryption routine
`ssl_decrypt_buf()` in the following situation:
1. A CBC ciphersuite with Encrypt-then-MAC is used.
2. A record with valid MAC but invalid CBC padding is received.
In this situation, the previous code would not raise and error but
instead forward the decrypted packet, including the wrong padding,
to the user.
This commit changes this behavior to return the error
MBEDTLS_ERR_SSL_INVALID_MAC instead.
While erroneous, the previous behavior does not constitute a
security flaw since it can only happen for properly authenticated
records, that is, if the peer makes a mistake while preparing the
padded plaintext.
This commit duplicates the public function mbedtls_asn1_find_named_data()
defined in library/asn1parse.c within library/asn1write.c in order to
avoid a dependency of the ASN.1 writing module on the ASN.1 parsing module.
The duplication is unproblematic from a semantic and an efficiency
perspective becasue it is just a short list traversal that doesn't
actually do any ASN.1 parsing.
Previously, mbedtls_pkcs5_pbes2() was unconditionally declared
in `pkcs5.h` but defined as a stub returning
`MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE` in case
MBEDTLS_ASN1_PARSE_C was not defined.
In line with the previous commits, this commit removes declaration
and definition from both `pkcs5.h` and `pkcs5.c` in case
MBEDTLS_ASN1_PARSE_C is not defined.
This commit ensures that buffers holding fragmented or
handshake messages get zeroized before they are freed
when the respective handshake message is no longer needed.
Previously, the handshake message content would leak on
the heap.
The code assumed that `int x = - (unsigned) u` with 0 <= u < INT_MAX
sets `x` to the negative of u, but actually this calculates
(UINT_MAX - u) and then converts this value to int, which overflows.
Cast to int before applying the unary minus operator to guarantee the
desired behavior.
The code was making two unsequenced reads from volatile locations.
This is undefined behavior. It was probably harmless because we didn't
care in what order the reads happened and the reads were from ordinary
memory, but UB is UB and IAR8 complained.
The input distribution to primality testing functions is completely
different when used for generating primes and when for validating
primes. The constants used in the library are geared towards the prime
generation use case and are weak when used for validation. (Maliciously
constructed composite numbers can pass the test with high probability)
The mbedtls_mpi_is_prime() function is in the public API and although it
is not documented, it is reasonable to assume that the primary use case
is validating primes. The RSA module too uses it for validating key
material.
This commit removes the definition of the API function
`mbedtls_platform_set_calloc_free()`
from `library/platform.c` in case the macros
`MBEDTLS_PLATFORM_CALLOC_MACRO`
`MBEDTLS_PLATFORM_FREE_MACRO`
for compile time configuration of calloc/free are set.
This is in line with the corresponding header `mbedtls/platform.h`
which declares `mbedtls_platform_set_calloc_free()` only if
`MBEDTLS_PLATFORM_{CALLOC/FREE}_MACRO` are not defined.
Fixes#1642.
The previous code triggered a compiler warning because of a comparison
of a signed and an unsigned integer.
The conversion is safe because `len` is representable by 16-bits,
hence smaller than the maximum integer.
In the previous bounds check `(*p) > end - len`, the computation
of `end - len` might underflow if `end` is within the first 64KB
of the address space (note that the length `len` is controlled by
the peer). In this case, the bounds check will be bypassed, leading
to `*p` exceed the message bounds by up to 64KB when leaving
`ssl_parse_server_psk_hint()`. In a pure PSK-based handshake,
this doesn't seem to have any consequences, as `*p*` is not accessed
afterwards. In a PSK-(EC)DHE handshake, however, `*p` is read from
in `ssl_parse_server_ecdh_params()` and `ssl_parse_server_dh_params()`
which might lead to an application crash of information leakage.
Get rid of the variable p. This makes it more apparent where the code
accesses the buffer at an offset whose value is sensitive.
No intended behavior change in this commit.
Rather than doing the quadratic-time constant-memory-trace on the
whole working buffer, do it on the section of the buffer where the
data to copy has to lie, which can be significantly smaller if the
output buffer is significantly smaller than the working buffer, e.g.
for TLS RSA ciphersuites (48 bytes vs MBEDTLS_MPI_MAX_SIZE).
In mbedtls_rsa_rsaes_pkcs1_v15_decrypt, use size_greater_than (which
is based on bitwise operations) instead of the < operator to compare
sizes when the values being compared must not leak. Some compilers
compile < to a branch at least under some circumstances (observed with
gcc 5.4 for arm-gnueabi -O9 on a toy program).
Replace memmove(to, to + offset, length) by a functionally equivalent
function that strives to make the same memory access patterns
regardless of the value of length. This fixes an information leak
through timing (especially timing of memory accesses via cache probes)
that leads to a Bleichenbacher-style attack on PKCS#1 v1.5 decryption
using the plaintext length as the observable.
mbedtls_rsa_rsaes_pkcs1_v15_decrypt takes care not to reveal whether
the padding is valid or not, even through timing or memory access
patterns. This is a defense against an attack published by
Bleichenbacher. The attacker can also obtain the same information by
observing the length of the plaintext. The current implementation
leaks the length of the plaintext through timing and memory access
patterns.
This commit is a first step towards fixing this leak. It reduces the
leak to a single memmove call inside the working buffer.
Make the function more robust by taking an arbitrary zero/nonzero
argument instead of insisting on zero/all-bits-one. Update and fix its
documentation.
mbedtls_rsa_rsaes_pkcs1_v15_decrypt took care of calculating the
padding length without leaking the amount of padding or the validity
of the padding. However it then skipped the copying of the data if the
padding was invalid, which could allow an adversary to find out
whether the padding was valid through precise timing measurements,
especially if for a local attacker who could observe memory access via
cache timings.
Avoid this leak by always copying from the decryption buffer to the
output buffer, even when the padding is invalid. With invalid padding,
copy the same amount of data as what is expected on valid padding: the
minimum valid padding size if this fits in the output buffer,
otherwise the output buffer size. To avoid leaking payload data from
an unsuccessful decryption, zero the decryption buffer before copying
if the padding was invalid.
Deprecate mbedtls_hmac_drbg_update (which returns void) in favor of a
new function mbedtls_hmac_drbg_update_ret which reports error. The old
function is not officially marked as deprecated in this branch because
this is a stable maintenance branch.
Deprecate mbedtls_ctr_drbg_update (which returns void) in favor of a
new function mbedtls_ctr_drbg_update_ret which reports error. The old
function is not officially marked as deprecated in this branch because
this is a stable maintenance branch.
The debugging functions
- mbedtls_debug_print_ret,
- mbedtls_debug_print_buf,
- mbedtls_debug_print_mpi, and
- mbedtls_debug_print_crt
return immediately if the SSL configuration bound to the
passed SSL context is NULL, has no debugging functions
configured, or if the debug threshold is below the debugging
level.
However, they do not check whether the provided SSL context
is not NULL before accessing the SSL configuration bound to it,
therefore leading to a segmentation fault if it is.
In contrast, the debugging function
- mbedtls_debug_print_msg
does check for ssl != NULL before accessing ssl->conf.
This commit unifies the checks by always returning immediately
if ssl == NULL.
`mbedtls_ssl_get_record_expansion()` is supposed to return the maximum
difference between the size of a protected record and the size of the
encapsulated plaintext.
Previously, it did not correctly estimate the maximum record expansion
in case of CBC ciphersuites in (D)TLS versions 1.1 and higher, in which
case the ciphertext is prefixed by an explicit IV.
This commit fixes this bug. Fixes#1914.
In `mbedtls_ccm_self_test()`, enforce input and output
buffers sent to the ccm API to be contigous and aligned,
by copying the test vectors to buffers on the stack.
- in x509_profile_check_pk_alg
- in x509_profile_check_md_alg
- in x509_profile_check_key
and in ssl_cli.c : unsigned char gets promoted to signed integer
In ecp_mul_comb(), if (!p_eq_g && grp->T == NULL) and then ecp_precompute_comb() fails (which can
happen due to OOM), then the new array of points T will be leaked (as it's newly allocated, but
hasn't been asigned to grp->T yet).
Symptom was a memory leak in ECDHE key exchange under low memory conditions.
The length to the debug message could conceivably leak through the time it
takes to print it, and that length would in turn reveal whether padding was
correct or not.
The basis for the Lucky 13 family of attacks is for an attacker to be able to
distinguish between (long) valid TLS-CBC padding and invalid TLS-CBC padding.
Since our code sets padlen = 0 for invalid padding, the length of the input to
the HMAC function, and the location where we read the MAC, give information
about that.
A local attacker could gain information about that by observing via a
cache attack whether the bytes at the end of the record (at the location of
would-be padding) have been read during MAC verification (computation +
comparison).
Let's make sure they're always read.
The basis for the Lucky 13 family of attacks is for an attacker to be able to
distinguish between (long) valid TLS-CBC padding and invalid TLS-CBC padding.
Since our code sets padlen = 0 for invalid padding, the length of the input to
the HMAC function gives information about that.
Information about this length (modulo the MD/SHA block size) can be deduced
from how much MD/SHA padding (this is distinct from TLS-CBC padding) is used.
If MD/SHA padding is read from a (static) buffer, a local attacker could get
information about how much is used via a cache attack targeting that buffer.
Let's get rid of this buffer. Now the only buffer used is the internal MD/SHA
one, which is always read fully by the process() function.
Move definition of `MBEDTLS_CIPHER_MODE_STREAM` to header file
(`mbedtls_cipher_internal.h`), because it is used by more than
one file. Raised by TrinityTonic in #1719
Address review comments:
1. add `mbedtls_cipher_init()` after freeing context, in test code
2. style comments
3. set `ctx->iv_size = 0` in case `IV == NULL && iv_len == 0`
Fix compilation warnings with IAR toolchain, on 32 bit platform.
Reported by rahmanih in #683
This is based on work by Ron Eldor in PR #750, some of which was independently
fixed by Azim Khan and already merged in PR #1655.
This PR fixes multiple issues in the source code to address issues raised by
tests/scripts/check-files.py. Specifically:
* incorrect file permissions
* missing newline at the end of files
* trailing whitespace
* Tabs present
* TODOs in the souce code
As a protection against the Lucky Thirteen attack, the TLS code for
CBC decryption in encrypt-then-MAC mode performs extra MAC
calculations to compensate for variations in message size due to
padding. The amount of extra MAC calculation to perform was based on
the assumption that the bulk of the time is spent in processing
64-byte blocks, which was correct for most supported hashes but not for
SHA-384. Adapt the formula to 128-byte blocks for SHA-384.
Fix IAR compiler warnings
Two warnings have been fixed:
1. code 'if( len <= 0xFFFFFFFF )' gave warning 'pointless integer comparison'.
This was fixed by wraping the condition in '#if SIZE_MAX > 0xFFFFFFFF'.
2. code 'diff |= A[i] ^ B[i];' gave warning 'the order of volatile accesses is undefined in'.
This was fixed by read the volatile data in temporary variables before the computation.
Explain IAR warning on volatile access
Consistent use of CMAKE_C_COMPILER_ID
